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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
