[Freeipa-devel] [PATCH 0024] Add OTP support to ipalib CLI

[Jan Cholasta]

On 7.10.2013 23:34, Nathaniel McCallum wrote:
> On Fri, 2013-10-04 at 16:16 -0400, Nathaniel McCallum wrote:
>> This patch supersedes my patch 0017 and requires patches 0020-0023. I
>> believe I have solved all of the outstanding issues from the review of
>> patch 0017, unless otherwise noted:
>>
>> 1. I'm not actually sure what the format of the date parameters is.
>> Could someone clarify this for me? Should I do something differently
>> here?
>>
>> 2. In this new version of the patch, we are writing default values for
>> many of the token attributes. It would be nice to have some global
>> defaults for these default values, but this is not currently
>> implemented. I think this would make a clean secondary patch on top of
>> this current patch.
>>
>> 3. Dmitri brought up the idea of having tokens automatically expire by
>> default. Is this a good idea? I think this dovetails nicely with #2
>> above.
>>
>> 4. This patch does not currently protect the deletion of the last token
>> as previously discussed. Here is why I think this is still needed, but
>> in the form of a DS plugin:
>>
>> We need to account for a state when the user is enabled for OTP but has
>> not yet configured any tokens. I believe this state should be when the
>> "otp" user auth type is set, but the user has no assigned tokens. In
>> this state, the user should be able to log in with single factor
>> authentication.
>>
>> Once the user has added tokens, however, should we allow the user to
>> remove all his own tokens and return to single factor authentication? If
>> yes, nothing further is needed. If no, then protection in the FreeIPA
>> framework is not sufficient and this needs to be checked at the DS
>> plugin level. I suspect Dmitri might answer that this needs to be a
>> matter of policy.
>>
>> 5. There appears to be some sort of permissions issue with users and
>> adding their own tokens. I have not looked into this yet, but I will
>> review this early next week. Since this is a small bug fix to an
>> existing feature, I figured it was out of scope for this patch.
>>
>> 6. When a user is deleted, all his tokens are deleted as well. This is
>> sensible default behavior. However, in the case of hardware tokens, it
>> may be more desirable to orphan these objects for future assignment to
>> new users. Does anyone have any opinions on this topic?
>>
>> Nathaniel
>
> v2. Fixes API.txt and OTPTokenKey issues caused by bugs in previous
> patches. Whitespace cleanup.
>

+class Base32DecodeError(ExecutionError):

Is this really necessary? Are we going to add <encoding>DecodeError for 
every kind of new encoding in IPA? Can't we just have generic 
DecodeError? (This is not an issue in your patch per se, I'm just 
wondering if we can do it better in the framework.)

Honza

-- 
Jan Cholasta