On 06/06/2016 12:34 PM, Alexander Bokovoy wrote:
Hi,

Add support for additional user name principal suffixes from
trusted Active Directory forests. UPN suffixes are property
of the forest and as such are associated with the forest root
domain.

FreeIPA stores UPN suffixes as ipaNTAdditionalSuffixes multi-valued
attribute of ipaNTTrustedDomain object class.

In order to look up UPN suffixes, netr_DsRGetForestTrustInformation
LSA RPC call is used instead of netr_DsrEnumerateDomainTrusts.

For more details on UPN and naming in Active Directory see
https://technet.microsoft.com/en-us/library/cc739093%28v=ws.10%29.aspx

https://fedorahosted.org/freeipa/ticket/5354




Hi Alexander,

a few comments:

1.)

there are some PEP8 violations in the patch. Not all of them need to be fixed, though (line overhangs < 5 characters are OK by me).

"""
./ipaserver/dcerpc.py:1199:80: E501 line too long (80 > 79 characters)
./ipaserver/dcerpc.py:1200:80: E501 line too long (83 > 79 characters)
./ipaserver/dcerpc.py:1258:40: E203 whitespace before ':'
./ipaserver/dcerpc.py:1263:80: E501 line too long (81 > 79 characters)
./ipaserver/dcerpc.py:1271:80: E501 line too long (90 > 79 characters)
./ipaserver/dcerpc.py:1272:80: E501 line too long (82 > 79 characters)
./ipaserver/plugins/trust.py:490:9: E128 continuation line under-indented for visual indent
./ipaserver/plugins/trust.py:490:80: E501 line too long (93 > 79 characters)
./ipaserver/plugins/trust.py:490:92: E202 whitespace before ']'
./ipaserver/plugins/trust.py:493:80: E501 line too long (83 > 79 characters)
./ipaserver/plugins/trust.py:1461:80: E501 line too long (80 > 79 characters)
./ipaserver/plugins/trust.py:1462:59: E202 whitespace before ']'
./ipaserver/plugins/trust.py:1544:1: E302 expected 2 blank lines, found 1
./ipaserver/plugins/trust.py:1638:1: E302 expected 2 blank lines, found 1
"""

2.)

Should the ipaNTAdditionalSuffixes attribute be case insensitive? It makes sense but I'm just asking so that we don't end changing the schema later.

3.)

"""
     def post_callback(self, ldap, entries, truncated, *args, **options):
         if options.get('pkey_only', False):
             return truncated
         trust_dn = self.obj.get_dn(args[0], trust_type=u'ad')
         trust_entry = ldap.get_entry(trust_dn)
+        blacklist = trust_entry.get('ipantsidblacklistincoming')
         for entry in entries:
-            sid = entry['ipanttrusteddomainsid'][0]
-
-            blacklist = trust_entry.get('ipantsidblacklistincoming')
-            if blacklist is None:
+            sid = entry.get('ipanttrusteddomainsid', [None])[0]
+            if sid is None:
                 continue

             if sid in blacklist:
                 entry['domain_enabled'] = [False]
             else:
                 entry['domain_enabled'] = [True]
         return truncated
"""

Again, you can use `entry.single_value.get('ipanttrusteddomainsid', None)` to test/get the attribute value

4.)


"""
def add_new_domains_from_trust(myapi, trustinstance, trust_entry, domains, **options):
     result = []
     if not domains:
         return result

     trust_name = trust_entry['cn'][0]
# trust range must exist by the time add_new_domains_from_trust is called
     range_name = trust_name.upper() + '_id_range'
     old_range = myapi.Command.idrange_show(range_name, raw=True)['result']
     idrange_type = old_range['iparangetype'][0]

-    for dom in domains:
+    suffixes = list()
+ suffixes.extend(y['cn'] for x,y in domains['suffixes'].iteritems() if x not in domains['domains'])
+
+    for dom in domains['domains'].itervalues():
         dom['trust_type'] = u'ad'

"""

iterkeys/itervalues are not Python 3 compatible, please use `keys()/values()` and do not worry about performance implications in Python 2.

5.)

"""
             result.append(res['result'])

-            if idrange_type != u'ipa-ad-trust-posix':
+ if idrange_type != u'ipa-ad-trust-posix' and dom.get('ipanttrusteddomainsid', False):
                 range_name = name.upper() + '_id_range'

"""

Just a nitpick, I am confused by the 'False' default when getting the value of 'ipanttrusteddomainsid' (which is a string I presume). You could put an empty string/list there since empty sequences evaluate to False.

6.)

"""
             pass
+
+    try:
+        dn = myapi.Object.trust.get_dn(trust_name, trust_type=u'ad')
+        ldap = myapi.Backend.ldap2
+        entry = ldap.get_entry(dn)
+        tlns = list(entry.get('ipantadditionalsuffixes', []))
+        tlns.extend(x for x in suffixes if x not in tlns)
+        entry['ipantadditionalsuffixes'] = tlns
+        ldap.update_entry(entry)
+    except errors.EmptyModlist:
+        pass
+

"""

On this line:

"""
+        tlns = list(entry.get('ipantadditionalsuffixes', []))
"""

is the additional conversion to list really needed? IIRC for multivalued attributes you get a list by default.

7.)

"""
         dn = self.obj.get_dn(keys[0], keys[1], trust_type=u'ad')
         try:
             entry = ldap.get_entry(dn)
-            sid = entry['ipanttrusteddomainsid'][0]
+            sid = entry.get('ipanttrusteddomainsid', [None])[0]

"""

the same as in point 3.)

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