[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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.


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.

+        # Get the issuer for the URI
+        issuer = api.env.realm
+        if owner is not None:
+            try:
+                issuer = ldap.get_entry(owner,
+            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:

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

+        # 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.


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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]