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

[Nathaniel McCallum]

On Fri, 2013-12-13 at 14:50 -0500, Nathaniel McCallum wrote:
> On Wed, 2013-12-11 at 13:24 +0100, Jan Cholasta wrote:
> > On 14.11.2013 20:23, Nathaniel McCallum wrote:
> > > On Wed, 2013-10-30 at 08:57 +0100, Jan Cholasta wrote:
> > >> On 8.10.2013 16:35, Nathaniel McCallum wrote:
> > >>> On Tue, 2013-10-08 at 09:19 +0200, Jan Cholasta wrote:
> > >>>>
> > >>>> +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
> > >>>
> > >>
> > >> I think you can use ConversionError instead. I don't see any reason why
> > >> base32/64 decoding errors should be special cased like this and would
> > >> like to see Base64DecodeError gone.
> > >
> > > Fixed.
> > >
> > >
> > 
> > Thanks.
> > 
> >       format = _("invalid '%(name)s': %(error)s")
> > 
> > -
> >   class ValidationError(InvocationError):
> >       """
> >       **3009** Raised when a parameter value fails a validation rule.
> > @@ -1306,6 +1305,7 @@ class PosixGroupViolation(ExecutionError):
> >       errno = 4030
> >       format = _('This is already a posix group and cannot be converted 
> > to external one')
> > 
> > +
> >   class BuiltinError(ExecutionError):
> > 
> > This white space shuffling is not necessary.
> 
> Fixed. I missed this, thanks!
> 
> > +def _normalize_owner(userobj, entry_attrs):
> > +    if 'ipatokenowner' in entry_attrs:
> > +        entry_attrs['ipatokenowner'] = map(userobj.get_primary_key_from_dn,
> > +                                           entry_attrs['ipatokenowner'])
> > 
> > If the --raw option is specified, ipatokenowner value should be full DN.
> 
> Fixed.
> 
> > +        # Resolve the user's dn
> > +        owner = entry_attrs.get('ipatokenowner', None)
> > +        if owner is not None:
> > +            owner = self.api.Object.user.get_dn(owner)
> > +            entry_attrs['ipatokenowner'] = owner
> > 
> > You have a _normalize_owner function, I think the code above should go 
> > into a _convert_owner function (use the function in 
> > otptoken_{mod,show,find} as well).
> 
> Fixed for mod and find. Show doesn't make sense.
> 
> > +        # Get the issuer for the URI
> > +        issuer = api.env.realm
> > +        if owner is not None:
> > +            try:
> > +                issuer = ldap.get_entry(owner, 
> > ['krbprincipalname'])['krbprincipalname'][0]
> > +            except:
> > +                pass
> > 
> > Please use "except PublicError" here, we don't want internal errors to 
> > be ignored.
> 
> Fixed: (NotFound, IndexError)
> 
> > +        # Delete all tokens owned by this user
> > +        owner = self.api.Object.user.get_primary_key_from_dn(dn)
> > +        results = 
> > self.api.Command.otptoken_find(ipatokenowner=owner)['result']
> > +        for token in results:
> > +            token = 
> > self.api.Object.otptoken.get_primary_key_from_dn(token['dn'])
> > +            self.api.Command.otptoken_del(token)
> > 
> > This should probably be handled by the referint plugin.
> 
> See my reply to Martin.

ARGH! I should try not to break stuff. New patch attached...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-npmccallum-0024-5-Add-OTP-support-to-ipalib-CLI.patch
Type: text/x-patch
Size: 32654 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131213/956ad08a/attachment.bin>