[Freeipa-devel] [PATCH] 0202 support UPNs for trusted domain users

[Martin Babinsky]

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