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

[Nathaniel McCallum]

On Tue, 2013-10-08 at 09:19 +0200, Jan Cholasta wrote:
> 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.)

I added the new error due to the existence of a Base64DecodeError. I
figured changing the existing error to be more generic would break api.

Nathaniel