Tails

#1 Updated by bertagaz 2017-03-19 08:33:20

• related to Bug #12369: Write a wrapper for KeePassX to open the old database if there is one but no new database added

#2 Updated by intrigeri 2017-03-19 09:22:51

• related to deleted (Bug #12369: Write a wrapper for KeePassX to open the old database if there is one but no new database)

#3 Updated by intrigeri 2017-03-19 09:25:22

• blocked by Bug #12369: Write a wrapper for KeePassX to open the old database if there is one but no new database added

#4 Updated by intrigeri 2017-03-19 09:27:45

• Subject changed from Ask whether to move the keepassx database if not using the supported path to If a custom-named KeePassX database is found in the Persistent folder, propose renaming it

#5 Updated by intrigeri 2017-03-19 09:28:00

• Priority changed from Normal to Elevated

#6 Updated by bertagaz 2017-03-19 20:48:26

• Status changed from Confirmed to In Progress
• Assignee changed from bertagaz to sajolida
• % Done changed from 0 to 20
• QA Check set to Info Needed

There are gettext strings in commit:d65da0da27afb2a016d9448e33b8d8ffd6d336e5 that needs to be reviewed UX wise, so reassigning to sajolida.

#7 Updated by bertagaz 2017-03-20 09:58:10

• Assignee changed from sajolida to bertagaz

bertagaz wrote:
> There are gettext strings in commit:d65da0da27afb2a016d9448e33b8d8ffd6d336e5 that needs to be reviewed UX wise, so reassigning to sajolida.

Wait, I’m doing some rephrasing.

#8 Updated by bertagaz 2017-03-20 13:13:37

• File keepassx_renaming_prompt.png added
• Assignee changed from bertagaz to sajolida

bertagaz wrote:
> Wait, I’m doing some rephrasing.

Ok, the gettext strings in config/chroot_local-includes/usr/local/bin/keepassx are ready for a review UX wise before the code review.

Basically, the user gets prompted with this zenity window, asking whether to rename their new database file when it’s not using the default supported filename in Tails. If they click “Rename”, the file gets renamed and opened in keepassx, if they click “Open anyway”, keepassx open the database without renaming.

Does the wording sounds good?

#12 Updated by intrigeri 2017-04-15 16:21:29

• Assignee changed from sajolida to intrigeri
• QA Check changed from Info Needed to Ready for QA

I would like to see this usability problem fixed in 3.0~beta4, and doing it with suboptimal strings is better than not doing it at all. I have no clue what’s sajolida’s ETA on this one, so I’ll review the branch, will merge if happy, and will add a note on Bug #12368 about reviewing these strings.

#13 Updated by intrigeri 2017-04-15 16:53:25

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Ready for QA to Dev Needed
• Feature Branch set to bugfix/12375-keepassx-prompt-for-moving-database

I did a code review:

• The message for commit:d65da0da27afb2a016d9448e33b8d8ffd6d336e5 (“move the default database if the user renamed the database”) doesn’t seem to match what the code does, so I wonder if I get your intent right (the code sounds OK, but when the corresponding description says something else I can’t be sure what you’re trying to do). Just to clarify, this branch is meant to address the case when a user had an old DB with the default name, and then the first time they run KeePassX on 3.0 it gets imported, but they save it with a name that’s not New database. But we don’t care about the case when the user’s old DB had a non-default name. Correct?
• Please keep the indentation style consistent: you used tabs in the initial script, and now I see spaces in prompt_for_database_renaming.
• Please quote variables consistently (this branch introduces some unquoted vars that we quote everywhere else in this script).
• I don’t think we should be looking for a DB in ~/Persistent unless it’s a mountpoint.
• I’m not very happy with the find | wc -l pattern, that is not robust wrt. weirdly named files. I won’t block on it though as I have no obvious solution to this in shell (which might hint us that shell was perhaps not the best language for this job). See below for some mitigation measures to limit the impact.
• When I see [ "$(find $PERSISTENT_DATA_DIR -maxdepth 1 -name 'keepassx.kdbx' 2>/dev/null | wc -l)" = "0" ], I think ! [ -e "$NEW_DB" ]. Any reason not to do the latter?
• I suggest doing this 2nd test (the one discussed in the previous bullet point) before the find|wc one, so we avoid running this code, that we know lacks robustness, in most cases.

If you can’t handle this by May 15, please let me know and I’ll take care of it myself.

#14 Updated by bertagaz 2017-04-18 12:31:43

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> I would like to see this usability problem fixed in 3.0~beta4, and doing it with suboptimal strings is better than not doing it at all. I have no clue what’s sajolida’s ETA on this one, so I’ll review the branch, will merge if happy, and will add a note on Bug #12368 about reviewing these strings.

Agreed. I don’t think it’s that bad actually anyway.

> I did a code review:

Thanks!

> * The message for commit:d65da0da27afb2a016d9448e33b8d8ffd6d336e5 (“move the default database if the user renamed the database”) doesn’t seem to match what the code does, so I wonder if I get your intent right (the code sounds OK, but when the corresponding description says something else I can’t be sure what you’re trying to do). Just to clarify, this branch is meant to address the case when a user had an old DB with the default name, and then the first time they run KeePassX on 3.0 it gets imported, but they save it with a name that’s not New database. But we don’t care about the case when the user’s old DB had a non-default name. Correct?

Yes, sorry for the lame commit message…

> * Please keep the indentation style consistent: you used tabs in the initial script, and now I see spaces in prompt_for_database_renaming.

commit:4559096874174e89d941b29588fbabb935b7c165

> * Please quote variables consistently (this branch introduces some unquoted vars that we quote everywhere else in this script).

commit:acd5c16fac8904ab962320901b73a2ff2c11125a

> * I don’t think we should be looking for a DB in ~/Persistent unless it’s a mountpoint.

commit:3d31afa486917a351ba7ed3b93bd2a80a73af952

> * I’m not very happy with the find | wc -l pattern, that is not robust wrt. weirdly named files. I won’t block on it though as I have no obvious solution to this in shell (which might hint us that shell was perhaps not the best language for this job). See below for some mitigation measures to limit the impact.

Did not find another way to do that either.

> * When I see [ "$(find $PERSISTENT_DATA_DIR -maxdepth 1 -name 'keepassx.kdbx' 2>/dev/null | wc -l)" = "0" ], I think ! [ -e "$NEW_DB" ]. Any reason not to do the latter?

Oooch, no idea why I implemented it that way…

commit:db68910ef931753966b62adcf5e8db4efbbc96c4

> * I suggest doing this 2nd test (the one discussed in the previous bullet point) before the find|wc one, so we avoid running this code, that we know lacks robustness, in most cases.

commit:76aef3665c1db18c09fdf72200051c4d7be1c956

#15 Updated by intrigeri 2017-04-18 15:24:02

• Target version changed from Tails_3.0 to Tails_3.0~rc1

#16 Updated by sajolida 2017-04-26 13:57:41

• related to Bug #12368: Design UX for migrating databases to KeePassX 2 added

#17 Updated by sajolida 2017-04-26 15:15:50

• File rephrased.png added

Cool! I’ve rephrased the prompt with 4eaa0ba619. Please have a look and tell me what you think. It also contains tiny bits of bash that I probably go wrong as usual :)

Now my main concern is that if the user chooses to keep the current file name, she will be prompt with this message every time which will very quickly become annoying. Could we avoid this? Maybe dropping an empty dotfile to make that file as seen already (.migration.kdbx.keep in my example). Or would this be overkill?

#19 Updated by intrigeri 2017-04-29 09:24:01

• Assignee changed from intrigeri to bertagaz
• % Done changed from 20 to 50
• QA Check changed from Ready for QA to Dev Needed

sajolida’s change breaks l10n by concatenating strings: the filename should be a gettext placeholder, and not a string appended to a translated one. Otherwise, looks good to me!

#20 Updated by bertagaz 2017-05-02 14:42:47

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> sajolida’s change breaks l10n by concatenating strings: the filename should be a gettext placeholder, and not a string appended to a translated one. Otherwise, looks good to me!

Fixed in commit:add95a776a2b35b8339308d4a37d05cacaf2a76a. Please review and merge if happy.

#21 Updated by intrigeri 2017-05-11 08:50:11

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Ready for QA to Dev Needed

Well, no: we’re still concatenating strings, which is never a good idea when l10n matters. So again: the filename should be a gettext placeholder, and not a string appended to a translated one.

#24 Updated by bertagaz 2017-05-15 12:10:04

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> I think that all our software (e.g. Tails Installer and Upgrader and probably a number of our scripts in tails.git) have examples of gettext strings that contain a placeholder.

I already did have a look before, but didn’t find something. I did again after this note of yours, but given ‘gettext placeholder’ is a bit vague, it’s not so easy to find examples.

If your mentionning the ability to use %s, then I’ve spent quite some time trying to use this syntax and failed. Note that I didn’t find such a thing in our code.

I’ve found in the tails-spoof-mac script some kind of variable usage in the gettext string, and I’ve pushed commit:0d165b5f6dd72fca94908db7c40af2edbc290b1e that uses the same kind of things.

Is it what you had in mind?

#25 Updated by intrigeri 2017-05-16 08:56:10

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Ready for QA to Dev Needed

> I already did have a look before, but didn’t find something. I did again after this note of yours, but given ‘gettext placeholder’ is a bit vague, it’s not so easy to find examples.

Sorry about that! I wrongly guessed I could stay in reviewing mode, while clearly I should have switched to “teaching shell script internationalization” mode.

Otherwise I would have pointed you to the gettext doc.

> If your mentionning the ability to use %s, then I’ve spent quite some time trying to use this syntax and failed. Note that I didn’t find such a thing in our code.

Indeed, I mis-remembered: there’s no such thing as %s when doing i18n of shell scripts.

> I’ve found in the tails-spoof-mac script some kind of variable usage in the gettext string, and I’ve pushed commit:0d165b5f6dd72fca94908db7c40af2edbc290b1e that uses the same kind of things.

> Is it what you had in mind?

Yes, that’s it!

But dialog_msg is still built by concatenating localized strings. It should be built from one single localized string.
I didn’t check if other strings in this script are affected by similar problems, please do.

#26 Updated by bertagaz 2017-05-17 11:01:15

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Sorry about that! I wrongly guessed I could stay in reviewing mode, while clearly I should have switched to “teaching shell script internationalization” mode.
>
> Otherwise I would have pointed you to the gettext doc.

I found it by myself, but was a bit confused by the difference between your words (“placeholder”) and the one used in this doc.

> But dialog_msg is still built by concatenating localized strings. It should be built from one single localized string.
> I didn’t check if other strings in this script are affected by similar problems, please do.

Pushed a fix vs string concatenation. There’s no other translated strings in this script, so it should be good now.

#29 Updated by intrigeri 2017-05-17 15:57:38

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Ready for QA to Info Needed

> Pushed a fix vs string concatenation. There’s no other translated strings in this script, so it should be good now.

Thanks! What did you test exactly with the latest code? (Just wanna get some confidence before I merge, and save me some time.)

#30 Updated by bertagaz 2017-05-18 09:13:19

• Assignee changed from bertagaz to intrigeri

intrigeri wrote:
> > Pushed a fix vs string concatenation. There’s no other translated strings in this script, so it should be good now.
>
> Thanks! What did you test exactly with the latest code? (Just wanna get some confidence before I merge, and save me some time.)

I did not rebuild an ISO from the last commit to test it but did adapt a previously build ISO from this branch by modifying the script the exact same way I’ve pushed it. I did not test all the features implemented by this script, as we already did test that extensively and nothing else changed. But the zenity window is working as expected.

#31 Updated by intrigeri 2017-05-18 09:53:19

• QA Check changed from Info Needed to Ready for QA

Thanks!

#32 Updated by intrigeri 2017-05-18 10:03:46

• Assignee changed from intrigeri to bertagaz
• % Done changed from 50 to 80
• QA Check changed from Ready for QA to Dev Needed

Code review passes, except one detail: as most of our wrappers, I think this one should use exec to start the real keepassx binary, so that we don’t keep a useless shell process behind us. (I’m aware that at least another wrapper fails to use exec and this wasn’t noticed when reviewing it, but that’s no reason to introduce the same mistake in another place.)

If you’re bored by this long process and prefer me to implement the change I’m asking for, just let me know.

#33 Updated by bertagaz 2017-05-18 13:58:34

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> Code review passes, except one detail: as most of our wrappers, I think this one should use exec to start the real keepassx binary, so that we don’t keep a useless shell process behind us. (I’m aware that at least another wrapper fails to use exec and this wasn’t noticed when reviewing it, but that’s no reason to introduce the same mistake in another place.)

Good idea!

> If you’re bored by this long process and prefer me to implement the change I’m asking for, just let me know.

That’s fine, I have my good share of responsibility for this long process. I’ve pushed a commit adding exec around calls to keepassx. Hope that’s what you meant.

#37 Updated by intrigeri 2017-05-18 16:24:16

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Ready for QA to Dev Needed

What sajolida proposed should be a trivial one-liner, so IMO it’s worth implementing it.

#38 Updated by intrigeri 2017-05-19 11:32:02

• Target version changed from Tails_3.0~rc1 to Tails_3.0

I’ve merged the current state of the branch into feature/stretch as it improves UX compared to what we had previously. Let’s please improve it a tiny bit more in 3.0 as proposed by sajolida, hence not closing this ticket.

#39 Updated by bertagaz 2017-05-24 12:39:14

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> I’ve probably misunderstood, but I’ve been working for a while under the assumption that this applied to the code as well.
> So: yes, agreed!

Ack, fair enough, didn’t remember that. Pushed two commits on top of the related branch to implement that.

#40 Updated by intrigeri 2017-05-24 16:45:53

• Assignee changed from intrigeri to bertagaz
• QA Check changed from Ready for QA to Dev Needed

> Pushed two commits on top of the related branch to implement that.

This new algorithm seems flawed to me: when the .no_keepassx_db_renaming file exists, it seems to me that we won’t be running keepassx at all, which is a regression compared to what we have in 3.0~rc1. Or did I get it wrong?

I think this (untested) is closer to what you meant to write:

--- a/config/chroot_local-includes/usr/local/bin/keepassx
+++ b/config/chroot_local-includes/usr/local/bin/keepassx
@@ -39,14 +39,13 @@ elif mountpoint -q "$PERSISTENT_DATA_DIR" && \
! [ -e "${NEW_DB}" ] && \
[ "$(find "$PERSISTENT_DATA_DIR" -maxdepth 1 -name '*.kdbx' 2>/dev/null | wc -l)" = "1" ]; then
user_db="$(find "$PERSISTENT_DATA_DIR" -maxdepth 1 -name '*.kdbx' 2>/dev/null)"
-    if ! [ -e "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" ]; then
-        if prompt_for_database_renaming "${user_db}"; then
-            mv "${user_db}" "${NEW_DB}"
-            exec /usr/bin/keepassx "${NEW_DB}"
-        else
-            touch "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming"
-            exec /usr/bin/keepassx "${user_db}"
-        fi
+    if ! [ -e "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" ] \
+        && prompt_for_database_renaming "${user_db}"; then
+        mv "${user_db}" "${NEW_DB}"
+        exec /usr/bin/keepassx "${NEW_DB}"
+    else
+       touch "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming"
+       exec /usr/bin/keepassx "${user_db}"
fi

 # There's an old DB but no new one => import the old DB:

However, if we do that, then the “Renaming it to keepassx.kdbx would allow KeePassX to open it automatically in the future” sentence becomes wrong: we’re opening any .kdbx file we find anyway, so the only advantage of renaming is to be supported by our next migration paths. So I would actually do that instead:

--- a/config/chroot_local-includes/usr/local/bin/keepassx
+++ b/config/chroot_local-includes/usr/local/bin/keepassx
@@ -39,14 +39,13 @@ elif mountpoint -q "$PERSISTENT_DATA_DIR" && \
! [ -e "${NEW_DB}" ] && \
[ "$(find "$PERSISTENT_DATA_DIR" -maxdepth 1 -name '*.kdbx' 2>/dev/null | wc -l)" = "1" ]; then
user_db="$(find "$PERSISTENT_DATA_DIR" -maxdepth 1 -name '*.kdbx' 2>/dev/null)"
-    if ! [ -e "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" ]; then
-        if prompt_for_database_renaming "${user_db}"; then
-            mv "${user_db}" "${NEW_DB}"
-            exec /usr/bin/keepassx "${NEW_DB}"
-        else
-            touch "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming"
-            exec /usr/bin/keepassx "${user_db}"
-        fi
+    if ! [ -e "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming" ] \
+        && prompt_for_database_renaming "${user_db}"; then
+        mv "${user_db}" "${NEW_DB}"
+        exec /usr/bin/keepassx "${NEW_DB}"
+    else
+       touch "${PERSISTENT_DATA_DIR}/.no_keepassx_db_renaming"
+       exec /usr/bin/keepassx
fi

 # There's an old DB but no new one => import the old DB:

This reflects what the dialog prompt says more accurately, avoids running somewhat arbitrary code found with the known-suboptimal find command (due to using shell where it proves not to be a good fit in the end, as usual perhaps), and gives a stronger incentive to rename the DB (and eases future removal of this code).

Please make it clear which ones of the 3 possible cases you’ve tested (or skipped for some reason) next time you’ll reassign to me for QA. Writing down a 2x2 matrix locally may help ensure you’re considering them all.

Thanks!

#42 Updated by bertagaz 2017-06-04 09:31:23

• Assignee changed from bertagaz to intrigeri
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> We’re getting close to the release. I’ll take over on Sunday if there’s no update earlier, or a clear ETA from bertagaz that works for me. Or I can just formally take over now, so you can focus on your high-prio CI stuff, and I can better organize my own time :)

Pushed your commit with your patch which indeed works better in all situations. Bonus point: if the user use a database with a custom name of its own, if she restarts KeepassX in the same session, it will open this custom DB by default, thanks to the KeepassX config file which remind it.

I also pushed a fix removing an exec call that was preventing the folowing shell command to be run.

#43 Updated by intrigeri 2017-06-04 10:17:11

• Status changed from In Progress to Fix committed
• % Done changed from 80 to 100

Applied in changeset commit:3f82316c4d1464019a0bd80b6310c7a03562198b.

#44 Updated by intrigeri 2017-06-04 10:18:27

• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Pass

Woohoo!

#45 Updated by intrigeri 2017-06-12 16:10:08

• Status changed from Fix committed to Resolved