[Freeipa-devel] [PATCH] 720-729 OTP usability improvements

Fri Aug 22 08:31:31 UTC 2014

On 12.8.2014 17:59, Endi Sukma Dewata wrote:
> On 8/5/2014 6:31 AM, Petr Vobornik wrote:>>
>> ticket: https://fedorahosted.org/freeipa/ticket/4402

snip (ACK of 720, 721) but patch 720 was replaced by a new version

>
>> [PATCH] 722 webui: add token from user page
>>
>> Add 'Add OTP Token' action to user action menu.
>>
>> This option is disabled in self-service when viewing other users.
>
> ACK, but I'm just wondering if it would be more intuitive to define an
> enabled condition (you're an admin or editing your own user) rather than
> a disabled condition (you're in self-service mode but editing other user).

probably

snip (ACK of 723)

>> [PATCH] 724 webui: display fields based on otp token type
>>
>> - in adder dialog

is it an ACK?

>>

snip (ACK of 725, 726)

>
>> [PATCH] 727 webui: hide empty fields and sections
>>
>> Hide widgets without a value. Must be explicitly turned on. In widget by
>> `hidden_if_empty` flag. Or globally by `hide_empty_widgets` flag. Global
>> hiding can be individually turned off by `ignore_empty_hiding` flag.
>
> In item #2 of ticket #4402 I was originally thinking the widget
> visibility would be determined by the token type.

Originally I wrote it that way but with this patch it became redundant.

> Any plan to add the
> token type field in the future?

maybe, I don't have any strong feelings about it. Will users benefit 
from adding it? If yes, it should be also added to CLI.

Atm. token type is derived from object classes. It exists only in add 
operation as a virtual attribute. We can check a presence of 
ipatokenhotp or ipatokentotp object classes to simulate the field.

> Will the "counter" field strictly have a
> value with HOTP only and "clock offset & interval" fields strictly have
> value with TOTP only? Do these fields contain the configured values or
> the effective values? I was just thinking maybe in the future some of
> these fields might be configured empty and they will use the default
> values instead. If that's not a problem then ACK.

Respective fields are present only in corresponding object classes -> 
there won't be an HTOP token with 'clock offset'. If they are present, 
they have a default value. -> No false hiding -> Shouldn't be a problem.

Btw: Other of my other original intents was to hide it based on ACI 
rights. The issues is that the rights are not present without 
corresponding OC. Hiding in such case is dangerous - explicitly disabled 
in patch 728.

What do you think about setting `hide_empty_widgets` global setting to 
True?

>
>> [PATCH] 728 webui: hide non-readable fields
>>
>> hide widgets if associated field had received attribute level rights
>> without 'r' right.
>>
>> Explicit rights are required to avoid hiding of special widgets which
>> are not associated with any LDAP attribute.
>
> ACK.
>
>> [PATCH] 729 webui: hide otp fields based on token type
>>
>> - uses hide empty feature
>
> Assuming patch #727 is fine then it's ACKed too.
>
> Some other comments/questions:
>
> 1. The "Validity start/end" fields don't show the date/time format. When
> you type the first letter then it will show the validation message with
> the format. It's not a big deal, but it's not very intuitive because
> people might not know what letter to type in the first place. Usually
> the field label should indicate the format/unit and the validation
> message will only appear if the value entered doesn't match the
> format/unit.

+1

We could set one of supported format as a placeholder + later is should 
be transformed into proper datetime select.

Also some of our validators could benefit from more intelligent logic, 
e.g. don't show error while typing if the entered value is a beginning 
of a correct form

>
> 2. The "Digits" field by itself sounds a bit weird. How about "Number of
> digits", or "OTP length" and add the unit in the value (e.g. 6 digits)?

Both UIs should use the same terminology if possible. I like the "OTP 
length" example but it doesn't work for CLI much.

"Number of digits" sounds reasonable. Should be changed in 
ipalib.otptoken though.

>
> 3. The "Clock offset" field doesn't have a unit.

Fixed in patch 720-1, patch 729 was rebased

>
> 4. If no "Owner" is specified when adding a token, it will default to
> admin. Is this the intended behavior?
>

Sadly yes.

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0720-1-webui-add-measurement-unit-to-otp-token-time-fields.patch
Type: text/x-patch
Size: 1693 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140822/e1653a5b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0729-1-webui-hide-otp-fields-based-on-token-type.patch
Type: text/x-patch
Size: 1572 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140822/e1653a5b/attachment-0001.bin>