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

[Martin Kosek]

On 06/27/2012 01:34 PM, Petr Viktorin wrote:
> 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.
> 

I also miss test cases for the new functionality. New exceptions were added,
behavior was changed - we need to cover this in our unit tests.

Martin