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

[Petr Vobornik]

On 3.9.2014 21:44, Endi Sukma Dewata wrote:
> 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

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

The issue is that it's not an attribute per se.

Nathaniel: do you think we should add a virtual output param(computed 
from object class) for the token type since 'model' could be easily 
overwritten? I would prefer to do it both in Web UI and CLI.

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

Maybe we should first clarify what is a read-only page. All details 
pages are writable if user has the rights or read-only if he doesn't. 
But mostly it depends on individual fields/attributes.


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

My intent was to show everything what user can edit. And hide fields:
1. for which he doesn't have read rights
2. have no (undefined) or empty('') value
3. are explicitly hidden by other logic - not related to this patch

When thinking about #1. Maybe we should base it on a presence or rather 
not presence of a specific objectclass, rather than on a value. That way 
it would be less confusing for newcomers. The logic is: if fields is 
bound to an object class and the class is not present (that also means 
we don't have attribute level rights) -> hide it.

#2 is questionable. IMHO it would require user tests. Also hiding on '' 
value might not be always desirable. Nevertheless I like the behavior 
but it may be caused by the fact that I already know what to expect.

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

I agree - use case #3.

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

btw somewhere is a rebase issue which leads too:

         if (that.readable !== undefined) {
             visible = visible && that.readable;
         }
         if (that.readable !== undefined) {
             visible = visible && that.readable;
         }

I'll fix that when we agree on an approach.


snip

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

Note: all patches except for 727 are ACKed.

-- 
Petr Vobornik