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

[Nathaniel McCallum]

On Wed, 2013-12-11 at 13:31 +0100, Martin Kosek wrote:
> On 12/11/2013 01:24 PM, 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.
> ...
> > +        # 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.
> 
> Right. I think what Nathaniel wanted to do is:
> 
> except errors.NotFound:
>      pass

Exactly.

> Bare except-pass is wrong on several levels anyway. For example it also catches 
> syntax or interrupt exceptions.

I left this blank to figure out what the exception was and forgot about
it. Thanks! :)

> > +        # 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.
> >
> > Honza
> >
> 
> I think referint plugin would just remove the attribute containing DN to user, 
> not the whole entry. I.e. I think Nathaniel need to do it this way anyway.
> 
> Though I think it would be more efficient if we do just one 'otptoken_del' call 
> with the list of tokens as primary key to it. All standard LDAPDelete based 
> plugins can do that. You may also want to pass continue=True in the list of 
> it's options in this case.

This logic needs to be here and will get more complicated in the future.
The big future patch I see here is distinguishing between different
types of tokens. For instance, we should probably delete soft-tokens but
orphan hard-tokens for reassignment. Future admins may also want this to
be configurable on a per-token basis.

Nathaniel