On 06/07/2016 06:00 PM, Alexander Bokovoy wrote:
On Tue, 07 Jun 2016, Martin Babinsky wrote:
On 06/06/2016 12:33 PM, Alexander Bokovoy wrote:
Hi,

this patch adds support for external trust to Active Directory.

External trust is a trust that can be created between Active Directory
domains that are in different forests or between an Active Directory
domain. Since FreeIPA does not support non-Kerberos means of
communication, external trust to Windows NT 4.0 or earlier domains is
not supported.

The external trust is not transitive and can be established to any
domain in another forest. This means no access beyond the external
domain is possible via the trust link.

Resolves: https://fedorahosted.org/freeipa/ticket/5743



Hi Alexander,

I have a few comments:

1.) there are numerous pep8 errors in the code:

"""
./ipaserver/dcerpc.py:1044:80: E501 line too long (115 > 79 characters)
./ipaserver/dcerpc.py:1044:106: E251 unexpected spaces around keyword
/ parameter equals
./ipaserver/dcerpc.py:1044:108: E251 unexpected spaces around keyword
/ parameter equals
./ipaserver/dcerpc.py:1107:80: E501 line too long (110 > 79 characters)
./ipaserver/dcerpc.py:1108:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1109:80: E501 line too long (114 > 79 characters)
./ipaserver/dcerpc.py:1111:80: E501 line too long (91 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1400:80: E501 line too long (143 > 79 characters)
./ipaserver/dcerpc.py:1401:80: E501 line too long (88 > 79 characters)
./ipaserver/plugins/trust.py:66:80: E501 line too long (91 > 79
characters)
./ipaserver/plugins/trust.py:174:22: E203 whitespace before ':'
./ipaserver/plugins/trust.py:175:80: E501 line too long (106 > 79
characters)
./ipaserver/plugins/trust.py:201:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:209:80: E501 line too long (91 > 79
characters)
./ipaserver/plugins/trust.py:690:9: E124 closing bracket does not
match visual indentation
./ipaserver/plugins/trust.py:694:80: E501 line too long (127 > 79
characters)
"""

Please fix them. You can check whether your commit does not break pep8
by running `git show HEAD -U0 | pep8 --diff`.
Thanks for the review.

The lines will have to be long. Sorry, we are not in the age of
80-column terminals anymore. Try to look at the ipaserver/dcerpc.py from
a bigger perspective -- there are currently 224 errors reported by pep8
in this file. Fixing some of them makes little sense -- for example,
pylint hints have to be where they are and that makes lines long, error
messages have to be what they are and changing them to be smaller is
counterproductive.

I'm updated the patch to have most of the lines within 90 columns.


This is not about 80 column terminals, this is about a) not moving your neck through its full range of motion when reading the code, b) conforming to the rules set up by some project regarding contributions to its codebase [1].

These are all the pep8 violations introduced _only by the code the patch is touching/adding_, not in the whole dcerpc module:


"""
./ipaserver/dcerpc.py:1045:80: E501 line too long (113 > 79 characters)
./ipaserver/dcerpc.py:1109:80: E501 line too long (93 > 79 characters)
./ipaserver/dcerpc.py:1110:80: E501 line too long (82 > 79 characters)
./ipaserver/dcerpc.py:1112:80: E501 line too long (97 > 79 characters)
./ipaserver/dcerpc.py:1114:80: E501 line too long (91 > 79 characters)
./ipaserver/dcerpc.py:1287:12: E111 indentation is not a multiple of four
./ipaserver/dcerpc.py:1379:80: E501 line too long (94 > 79 characters)
./ipaserver/dcerpc.py:1380:80: E501 line too long (94 > 79 characters)
./ipaserver/dcerpc.py:1386:80: E501 line too long (89 > 79 characters)
./ipaserver/dcerpc.py:1388:80: E501 line too long (88 > 79 characters)
./ipaserver/dcerpc.py:1407:80: E501 line too long (94 > 79 characters)
./ipaserver/dcerpc.py:1408:80: E501 line too long (94 > 79 characters)
./ipaserver/dcerpc.py:1411:80: E501 line too long (86 > 79 characters)
./ipaserver/plugins/trust.py:176:22: E203 whitespace before ':'
./ipaserver/plugins/trust.py:177:80: E501 line too long (106 > 79 characters)
./ipaserver/plugins/trust.py:203:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:211:80: E501 line too long (91 > 79 characters)
./ipaserver/plugins/trust.py:695:80: E501 line too long (127 > 79 characters) ./ipaserver/plugins/trust.py:697:9: E124 closing bracket does not match visual indentation ./ipaserver/plugins/trust.py:1062:25: E127 continuation line over-indented for visual indent ./ipaserver/plugins/trust.py:1072:80: E501 line too long (109 > 79 characters) ./ipaserver/plugins/trust.py:1092:80: E501 line too long (80 > 79 characters) ./ipaserver/plugins/trust.py:1103:25: E127 continuation line over-indented for visual indent ./ipaserver/plugins/trust.py:1103:80: E501 line too long (104 > 79 characters) ./ipaserver/plugins/trust.py:1121:80: E501 line too long (80 > 79 characters) ./ipaserver/plugins/trust.py:1123:80: E501 line too long (86 > 79 characters)

"""

Again, we only require contributors to follow PEP8 when adding new code/directly touching old one. Please note that there are more serious transgressions than a couple of long lines that should _definitely_ be fixed (indentation errors, whitespace around operators etc.).

We as a contributors to the project _all_ have to conform to the style guide[1] to at least keep some uniform style in the codebase we touch. I fail to see why your contributions should be exempt from the rules we all strive to adhere to.

(also you can sandwich # pylint: disable/ # pylint: enable directives around the offending long live to keep pep8 satisfied, just saying)

[1] http://www.freeipa.org/page/Python_Coding_Style


2.)

I have difficulty understanding the following code:

"""
+        self.__allow_behavior = 0

        domain_validator = DomainValidator(api)
        self.configured = domain_validator.is_configured()

        if self.configured:
            self.local_flatname = domain_validator.flatname
            self.local_dn = domain_validator.dn
            self.__populate_local_domain()

+    def allow_behavior(self, behavior_set):
+        if type(behavior_set) is int:
+           behavior_set = [behavior_set]
+        if TRUST_JOIN_EXTERNAL in behavior_set:
+           self.__allow_behavior |= TRUST_JOIN_EXTERNAL
+
"""
I assume this is made like this to accommodate setting some other
behavior flags which can pop up in the future (otherwise a simple
Boolean to indicate external trust should be enough), int hat case I
would propose to rewrite it in this form and document it:


"""
def allow_behavior(self, *flags):
   """
   Set the expected trust join behavior
   :param flags: trust flags to set
   """
   for flag in flags:
       self.__allow_behavior |= flag
"""

Also, I am not a big fan of doubly underscored methods/variables since
they do not 'hide' anything (Python's name mangling can be bypassed
with ease anyway). If you want to indicate that the 'allow_behavior'
attribute belongs to the internal implementation of the class prefix
it with single underscore.
You cannot have a property and a method named the same in the same
class. I changed the code to follow your other suggestion. I kept the
__allow_behavior as it is, though, because there is no difference
between __ and _ semantically to a person who would be reading the code
in question but I keep __ memorized. ;)


Fair enough, although you could have more meaningful method and attribute names, like `self.__behavior_flags` and `def set_behavior_flags()`.


3.)

"""
        if self.remote_domain.info['dns_domain'] !=
self.remote_domain.info['dns_forest']:
-            raise
errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'],
domain=self.remote_domain.info['dns_domain'])
+            if not (self.__allow_behavior & TRUST_JOIN_EXTERNAL):
+                raise
errors.NotAForestRootError(forest=self.remote_domain.info['dns_forest'],
domain=self.remote_domain.info['dns_domain'])

        if not self.remote_domain.read_only:
            trustdom_pass = samba.generate_random_password(128, 128)
            self.get_realmdomains()
-            self.remote_domain.establish_trust(self.local_domain,
trustdom_pass, trust_type)
-            self.local_domain.establish_trust(self.remote_domain,
trustdom_pass, trust_type)
+            self.remote_domain.establish_trust(self.local_domain,
trustdom_pass, trust_type, bool(self.__allow_behavior &
TRUST_JOIN_EXTERNAL))
+            self.local_domain.establish_trust(self.remote_domain,
trustdom_pass, trust_type, bool(self.__allow_behavior &
TRUST_JOIN_EXTERNAL))
            # if trust is inbound, we don't need to verify it because
AD DC will respond
            # with WERR_NO_SUCH_DOMAIN -- in only does verification
for outbound trusts.
            ...

"""

Please use a separate variable to store the result of bitwise AND you
pass to the methods, e.g.:

"""
join_as_external = bool(self.__allow_behavior & TRUST_JOIN_EXTERNAL)
if self.remote_domain.info['dns_domain'] !=
self.remote_domain.info['dns_forest']:
   ...

"""

There is enough copy-pasta we have to deal with when fixing bugs,
better not add any more of that.
Thanks, changed this part.

4.)

In trust-find post-callback you can use
`attrs.single_value.get('ipanttrustattributes', 0)` to get trust
attributes from the LDAP entry (they haven't been converted to dicts
yet). It makes code bit more readable. The same can be said for
trust-show post-callback.

"""
  def post_callback(self, ldap, entries, truncated, *args, **options):
       if options.get('pkey_only', False):
            return truncated

       for attrs in entries:
            # Translate ipanttrusttype to trusttype if --raw not used
            trust_type = attrs.get('ipanttrusttype', [None])[0]
+            attributes = attrs.get('ipanttrustattributes', [0])[0]
            if not options.get('raw', False) and trust_type is not None:
-                attrs['trusttype'] =
trust_type_string(attrs['ipanttrusttype'][0])
+                attrs['trusttype'] = [trust_type_string(trust_type,
attributes)]
                del attrs['ipanttrusttype']
+                if attributes:
+                    del attrs['ipanttrustattributes']
"""
Updated patch is attached.


Also when sending revised patches please try to conform to the common patch naming convention: http://www.freeipa.org/page/Contribute/Patch_Format#Naming

Surprisingly I have found out that my own patches actually do not follow it. Shame on me!

--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to