On 06/07/2016 06:38 PM, Alexander Bokovoy wrote:
On Tue, 07 Jun 2016, Martin Babinsky wrote:
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
"""
I've fixed trust.py part, the dcerpc.py fixes in 0201 should be enough
now -- breaking following line is not giving any reasonable benefit:

           res['ipantflatname'] =
unicode(t.forest_trust_data.netbios_domain_name.string)


Looking at the code, it would be IMHO more readable to directly append dict literals to the result like so:

"""
--- a/ipaserver/dcerpc.py
+++ b/ipaserver/dcerpc.py
@@ -1269,18 +1269,20 @@ def fetch_domains(api, mydomain, trustdomain, creds=None, server=None):
             tname = unicode(t.forest_trust_data.dns_domain_name.string)
             if tname == trustdomain:
                 continue
-            res = dict()
-            res['cn'] = tname
- res['ipantflatname'] = unicode(t.forest_trust_data.netbios_domain_name.string) - res['ipanttrusteddomainsid'] = unicode(t.forest_trust_data.domain_sid)
-            result['domains'][tname] = res
+
+            result['domains'][tname] = {
+                'cn': tname,
+                'ipantflatname': unicode(
+                    t.forest_trust_data.netbios_domain_name.string),
+                'ipanttrusteddomainsid': unicode(
+                    t.forest_trust_data.domain_sid)
+            }
         elif t.type == lsa.LSA_FOREST_TRUST_TOP_LEVEL_NAME:
             tname = unicode(t.forest_trust_data.string)
             if tname == trustdomain:
                 continue
-            res = dict()
-            res['cn'] = tname
-            result['suffixes'][tname] = res
+
+            result['suffixes'][tname] = {'cn': tname}
     return result
"""

Also there is a whitespace before colon here:
"""
+    result = {'domains': {}, 'suffixes' : {}}
                                        ^
"""
Please fix that and I will be a happy engineer.

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.
ipaNTAdditionalSuffixes is defined as

attributeTypes: ( 2.16.840.1.113730.3.8.11.75 NAME
'ipaNTAdditionalSuffixes' DESC 'Suffix for the user principal name
associated with the domain' EQUALITY caseIgnoreMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.15)

There should be no problem with case sensitivity already.

OK I presume that the UPN suffixes on AD are also case-insensitive so everything should be alright.

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


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.
I switched to six.iteritems().


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.
the attribute may not be in the object so I need to have something to
evaluate there. I don't want to have empty string there as a poor man
indicator of a missing attribute, though. False here represents better
what is expected for an error case.


I usually use 'None' as default to `get()` in these cases.


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


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.)
Fixed.



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