[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 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.



     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.

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

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

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

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


Jan Cholasta

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