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

[Endi Sukma Dewata]

On 8/22/2014 3:31 AM, Petr Vobornik wrote:
> 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

ACK.

>>> [PATCH] 724 webui: display fields based on otp token type
>>>
>>> - in adder dialog
>
> is it an ACK?

Sorry, missed this one. ACK.

>>> [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.

Can the users tell which type of token they have based on the existing 
fields? There is a model field which is populated with the type (e.g. 
totp or hotp). But since the value can be changed to anything it's not a 
reliable way to determine the type. I don't think it's very 
user-friendly to ask the user to see whether the type-specific fields 
are shown in order to determine the type.

I can't say there's a big benefit by adding the field, but maybe some 
admins might find it useful, and it can be used to sort/filter out 
search results.

> 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.

Yes, if people can add an attribute usually they will expect to be able 
to see what they added.

>> 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?

On a read-only page I think it's OK to hide empty fields. But on edit 
page I don't think we should do that by default.

Does "empty" mean "unset", or "blank" like empty string/array? Does 
"unset" always mean "non-editable and invisible" and "blank" always mean 
"editable and visible but it's just empty"? If this definition isn't 
strictly followed there could be an editable field that accidentally 
gets hidden because it's empty.

Generally if a field is supposed to be hidden in an edit page because of 
other condition (e.g. token type), not because of the value, the code 
should also use that condition instead of relying on the value being 
unset. In this particular case, instead of:

   if (field !== undefined) {
       // display field
   }

we probably should do:

   var type = ...

   if (type == 'hotp') {
       // display hotp fields

   } else { // totp
       // display totp fields
   }

For now the type can be determined by the availability of certain 
type-specific fields, but in the future we may add the type itself.

>> 3. The "Clock offset" field doesn't have a unit.
>
> Fixed in patch 720-1, patch 729 was rebased

ACK.

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

Maybe the adder dialog should show admin (or the current user?) as the 
default owner instead of empty?

-- 
Endi S. Dewata