On 06/28/2012 02:58 PM, Alexander Bokovoy wrote:
On Thu, 28 Jun 2012, Petr Viktorin wrote:
On 06/28/2012 02:16 PM, Alexander Bokovoy wrote:
On Wed, 27 Jun 2012, Alexander Bokovoy wrote:
On Wed, 27 Jun 2012, 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.
Given that this list going to be short (~dozen members in most
cases) it
is affordable to produce new set. I'll change it.

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.
Originally this callback was forcing all references to lower case
before
comparing. This was applied both to existing and truly external
references. However, for external references we cannot guarantee that
lower case is the right one -- and, indeed, with SIDs one has to follow
SID format which is S-1-* so lowcasing the value is not possible as
that
value will be used by SSSD and other sides (DCERPC requests) which
don't
expect it to break the format.

Thus I tried to keep the format.

I've added several tests:
1. Create group with external membership
2. Attempt to convert posix group to external one
3. Attempt to convert external group to posix
4. Attempt to add external member to it.
5. Delete external membership group to avoid disturbing other tests
  (group-find, etc) that depend on number of groups.

In the #4 I'm only checking that we are getting exceptions --
either ValidationError or NotFound. You can't do more without
setting up
the full trust.

Even to do that I had to introduce new type of checkers -- checkers
that
can be activated with a 'expected' attribute being a callable in a
declarative test definition in xmlrpc tests. This is an easiest way
to deal with multiple exceptions -- just define a lambda that tests
various conditions and let it be executed by the checker.

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):
Don't shoot the follower, it is what was there before me. :)

Fixed.

Instead of:

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

you can use:

  options.get('external_callback_normalize', True)
Fixed.


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 find it unreadable when a single element of a list is on the separate
line and also doesn't follow logical identation for its level.

New patch is attached.
And revised patch after review on IRC with Petr.


I'm definitely not a fan of adding new magic to the test suite, but we
couldn't find a better way to check for one of two errors that didn't
involve rewriting the Declarative tests.

So, with this patch, the 'expected' value of a test can be a callable,
in which case it's called with two arguments (exception, output) and
must return true for the test to pass.


There are still some failures in test_cmdline/test_cli.py, caused by
the "external" flag added to group-add. Otherwise the patch works fine.
Fixed these too. New patch attached.

Thanks for the thorough review!


ACK for the Python part, please push.
Thank you for the patch!

--
PetrĀ³


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to