30/03/14 19:22, Andres Gomez Ramirez wrote:
> Hi,
>
>> Do you think you could rebase your patch on top of the "wheezy" branch
>> of the greeter, and test it in an (experimental) Wheezy-based ISO?
>> You can download such an ISO on
>> http://nightly.tails.boum.org/build_Tails_ISO_feature-wheezy/
>
> yes, done.
I've reviewed and tested the patch, and it implements what we have asked
for in the ticket, but I leaves the following now false statement in the
`password_header` label:
Otherwise it will be disabled for better security.
That part should be removed, since your patch makes it impossible to
leave these fields empty. However, after actually testing that new
behaviour, I'm not so sure what we ask for in the ticket is anything good.
Let me quote the ticket:
> Hook tails-persistence-setup's update_passphrase_ui dynamic warning
> system in.
>
> That is, add to the greeter's admin password and confirmation entry
> process the same kind of UX than the one used in t-p-s to choose a
> passphrase: can't be empty, must enter twice the same passphrase, etc.
Comparing t-p-s' update_passphrase_ui with what we currently have in the
Greeter (before Andres' patch), we see the only missing thing is the
"[password] can't be empty" check. (As for the "etc" part, there's a
TODO about checking the password's strength, nothing more.)
That makes a lot of sense in t-p-s since we *force* the persistent
container to be encrypted with a password. But for the Greeter's
*optional* admin password I'm note sure it makes sense. Previously,
leaving the fields empty meant that the admin password was left as-is,
i.e. disabled. What we (perhaps unknowingly) ask for in the ticket is to
instead *force* the user to set an admin password whenever s/he clicks
"more options", even if s/he is only interested in some of the *other*
options, or just wanted to look at what's available and actually would
like to boot with the defaults.
IMHO this change is a severe loss of functionality, and either:
1. we have to admit that requesting this was a mistake, and keep the
current behaviour.
2. we have to add a checkbox ("Enable an administration password"), and
show the password fields if and only if it's checked. Then the
"[password] can't be empty" check makes sense.
3. I've completely misunderstand what the ticket asks for. I believe it
was you, intrigeri, who filed the ticket (before it was imported to
redmine). Could you please elaborate on the purpose and arguments
for this change, if this is what you actually intended?
As an argument for 1, overloading the fields' semantics with "empty =>
disabled" is a pretty convenient and elegant solution.
As an argument for 2, having the fields hidden unclutters the UI a bit
and perhaps makes it clearer that entering a password is not mandatory
(for users that don't read or understand the text above the fields).
Using a checkbox also makes it more consistent with the other options, FWIW.
I personally think I'm in favour of going with 1. Does any one have any
other good arguments for 2? Does any one with user testing experience
have anything to say about the arguments for 2 I proposed (I've seen no
evidence that there's much confusion with the current UI)? If we end up
going with 2 I'd be happy to implement it on top of Andres' patch.
I for one at least admit that I've made a huge mistake in not realising
this earlier... I guess my brain shut off when it saw "[password] can't
be empty" since that generally is a great idea, and I just accepted it
uncritically. No matter the outcome, I'm really sorry about all this,
Andres!
Cheers!
