Hi Martin!

On Thu, 12 Apr 2012, Martin Kosek wrote:
Hello Alexander,

I read the patches with focus on Python parts, please check my
comments.

freeipa-abbra-0042-ticket-2192.patch:
1) s4u2proxy records that you add for new replicas should also be
removed
during replica uninstall. Otherwise you will get a warning when the
replica is being re-installed.

You can find the clean up code in replication.py in ipaserver/install in
function replica_cleanup()
thanks.



freeipa-abbra-0044-ticket-1821.patch:
1) Missing i18n:

+trust_output_params = (
+    Str('ipantflatname',
+        label='Domain NetBIOS name'),
+    Str('ipantsecurityidentifier',
+        label='Domain Security Identifier'),
+    Str('trustdirection',
+        label='Trust direction'),
+    Str('trusttype',
+        label='Trust type'),
+)
Ok, will fix.


2) This does not look nice (and returns False (i.e. not str) when level
is out of bounds):
+def trust_type_string(level):
+    return int(level) in (1,2,3) and 
(u'Forest',u'Cross-Forest',u'MIT')[int(level)-1]
+
+def trust_direction_string(level):
+    return int(level) in (1,2,3) and (u'Downlevel',u'Uplevel',u'Both 
directions')[int(level)-1]

Maybe something like this would be better (and i18n-ed):
_trust_type_dict = {1 : _('Forest'),
                   2 : _('Cross-Forest'),
                   3 : _('MIT')}
_trust_type_dict_unknown = _('Unknown')
def trust_type_string(level):
   string = _trust_type_dict.get(int(level), _trust_type_dict_unknown)
   return unicode(string)
ok, makes sense. We'll need to do it to both directions later (not now).

3) I would not try to import ipaserver.dcerpc every time the command is
executed:
+        try:
+            import ipaserver.dcerpc
+        except Exception, e:
+            raise errors.NotFound(name=_('AD Trust setup'),
+                  reason=_('Cannot perform join operation without Samba
4 python bindings installed'))

I would rather do it once in the beginning and set a flag:

try:
   import ipaserver.dcerpc
    _bindings_installed = True
except Exception:
   _bindings_installed = False

...
The idea was that this code is only executed on the server. We need to
differentiate between:
- running on client
- running on server, no samba4 python bindings
- running on server with samba4 python bindings

By making it executed all time you are affecting the client code as
well while with current approach it only affects server side.


+    def execute(self, *keys, **options):
+        # Join domain using full credentials and with random trustdom
+        # secret (will be generated by the join method)
+        trustinstance = None
+        if not _bindings_installed:
+            raise errors.NotFound(name=_('AD Trust setup'),
+                  reason=_('Cannot perform join operation without Samba
4 python bindings installed'))


4) Another import inside a function:
+        def arcfour_encrypt(key, data):
+            from Crypto.Cipher import ARC4
+            c = ARC4.new(key)
+            return c.encrypt(data)
Same here, it is only needed on server side.

Let us get consensus over 3) and 4) and I'll fix patches altogether (and
push).

--
/ Alexander Bokovoy

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to