[Freeipa-devel] [PATCH] External group membership for trusted domains

[Petr Viktorin]

On 06/27/2012 12:36 PM, Sumit Bose wrote:
> On Wed, Jun 27, 2012 at 12:56:56PM +0300, Alexander Bokovoy wrote:
>> On Mon, 25 Jun 2012, Alexander Bokovoy wrote:
>>> On Mon, 25 Jun 2012, Sumit Bose wrote:
>>>> Hi Alexander,
>>>>
>>>> On Thu, Jun 21, 2012 at 06:26:02PM +0300, Alexander Bokovoy wrote:
>>>>> Hi!
>>>>>
>>>>> Attached is the patch to support external group membership for trusted
>>>>> domains. This is needed to get proper group membership with the work
>>>>> Sumit and Jan are doing on both IPA and SSSD sides.
>>>>>
>>>>> We already have ipaExternalGroup class that includes ipaExternalMember
>>>>> attribute (multivalued case-insensitive string). The group that has
>>>>> ipaExternalGroup object class will have to be non-POSIX and
>>>>> ipaExternalMember
>>>>> attribute will contain security identifiers (SIDs) of members from
>>>>> trusted domains.
>>>>>
>>>>> The patch takes care of three things:
>>>>> 1. Extends 'ipa group-add' with --external option to add
>>>>>    ipaExternalGroup object class to a new group
>>>>> 2. Modifies 'ipa group-add-member' to accept --external CSV argument
>>>>>    to specify SIDs
>>>>> 3. Modifies 'ipa group-del-member' to allow removing external members.
>>>>
>>>> thank you for the patch, it works as expected, but I have a few
>>>> comments:
>>>>
>>>> - there is a trailing whitespace at the end of the "This means we can't
>>>> check the correctness of a trusted domain SIDs" line
>>>> - when using ipa group-add-member with --external there are still prompt
>>>> for [member user] and [member group], can those be suppressed?
>>>> - with ipa group-mod --posix it is possible to add the posxiGroup
>>>> objectclass together with a GID to the extern group object. This
>>>> should result in an error and also the other way round, adding
>>>> --external to Posix groups.
>>> Updated patch is attached. It fixes whitespace and group-mod.
>> New revision.
>
> Thank you. This version works well in my tests, so ACK.
>
> It would be nice if someone can have a short look at the changes to
> baseldap.py to see if there are any unexpected side effects.
>
> bye,
> Sumit
>


I'm concerned about this:

      membername = entry[0].lower()
      member_dn = api.Object[membertype].get_dn(membername)
      if membername not in external_entries and \
+      entry[0] not in external_entries and \
        member_dn not in members:

Do you want to do a case-insensitive compare here? In that case it would 
be better to do:

    lowercase_external_entries = set(e.lower() for e in external_entries)
    if membername not in lowercase_external_entries ...

instead of comparing the lowercased entry and the entry itself to the 
original list.
The `in` operator is also faster on a set.
You should also update the `elif membername in external_entries` block 
below this one.
There's a similar situation in remove_external_post_callback.

Anyway, if you ran into a situation where the `entry[0] not in 
external_entries` check is needed, there should be a test for it.


I think something is rotten with add_external_post_callback: it's 
defined as add_external_post_callback(... *keys, **options), but 
invariably called as add_external_post_callback(... keys, options). That 
existed before the patch, though, so I guess it warrants a separate ticket.


I also have a few obligatory style nitpicks.

For line continuation, instead of backslashes:

     if membername not in external_entries and \
       entry[0] not in external_entries and \
       member_dn not in members:

we prefer parentheses:

     if (membername not in external_entries and
         entry[0] not in external_entries and
         member_dn not in members):

Instead of:

     normalize = True
     if 'external_callback_normalize' in options:
         normalize = options['external_callback_normalize']

you can use:

     options.get('external_callback_normalize', True)

And in group.py:

- 'memberofindirect': ['group', 'netgroup', 'role', 'hbacrule',
- 'sudorule'],
+ 'memberofindirect': ['group', 'netgroup', 'role', 'hbacrule', 'sudorule'],

Our style guide limits lines to 80 characters. Not much of IPA follows 
that rule currently, but I don't see a reason for a change that *only* 
breaks the rule.

-- 
Petr³