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