[Freeipa-devel] [PATCH 0042] Rework how otptoken defaults are handled

[Jan Cholasta]

On 21.2.2014 17:45, Nathaniel McCallum wrote:
> On Fri, 2014-02-21 at 16:29 +0100, Jan Cholasta wrote:
>> Hi,
>>
>> On 21.2.2014 16:09, Nathaniel McCallum wrote:
>>> On Fri, 2014-02-21 at 09:45 -0500, Nathaniel McCallum wrote:
>>>> We had originally decided to provide defaults on the server side so that
>>>> they could be part of a global config for the admin. However, on further
>>>> reflection, only certain defaults really make sense given the
>>>> limitations of Google Authenticator. Similarly, other defaults may be
>>>> token specific.
>>>>
>>>> Attempting to handle defaults on the server side also makes both the UI
>>>> and the generated documentation unclear.
>>>>
>>>> NOTE: this patch changes an existing API. VERSION says that we should
>>>> bump the major version in this case. But we haven't actually released
>>>> this API yet. Please advise.
>>
>> There were similar changes in the past and bumping minor version was
>> always enough (we never ever bump major version).
>
> Fixed.

Thanks, ACK.

>
>>>
>>> I forgot to mention something else about this patch. The default_from in
>>> the key parameter generates a warning. This is because the default is a
>>> bytes string and internally a check is done against NULLS
>>> (constants.py:36). The u'' in NULLS forces an attempt to convert the
>>> byte string to unicode. When this fails (because ipatokenotpkey is *NOT*
>>> a string but a byte array), a warning is thrown.
>>>
>>> Since '' and u'' evaluate as equal, what is the point of having u'' in
>>> NULLS? I don't see any way to suppress this warning except to remove u''
>>> from NULLS.
>>
>> I think we can get rid of NULLS entirely and do something better instead
>> (like "if not value and value is not False:").
>>
>> Is this worth filing a ticket?
>
> How about a patch?
>
> https://www.redhat.com/archives/freeipa-devel/2014-February/msg00426.html

Even better. But next time please post it in the same thread, so that 
other people don't review it while I'm reviewing it as well.

>
> Nathaniel
>


-- 
Jan Cholasta