[Freeipa-devel] [PATCH] Group support

[Kevin McCarthy]

Rob Crittenden wrote:
> This quite large patch includes initial support for Groups. It also:
>
> Create separate object for Users and Groups (using same base class)
> Check for uniqueness before adding new users and groups
> Remove user_container from everything but add operations
> Abstract out a number of functions that are common across users and groups
> Make sure all strings passed in to be in a filter are checked
> Add new error message: No modifications specified
>
> It is bound to conflict with other patches floating around. Once those get 
> committed I'll see about merging this in and submitting a new one. Just 
> wanted to get some eyeballs on this first.

Approved.  I have a few comments below we can discuss but shouldn't hold
up the patch committing.

There are going to be a few conflicts with this and previous patches
that are still queued.

Team:  Is there a way we can get these patches committed faster?  Rob
and I are really starting to step on each others toes all the time, and
we are going to spend more time merging conflicts than being productive
if we don't find a solution here.

-Kevin


>
> rob

> # HG changeset patch
> # User rcritten at redhat.com
> # Date 1187984576 14400
> # Node ID 093f73cd3cd2d2cf673f53c0c1da9c137d17b374
> # Parent  91183c36971655d8f9f3b944c4c583d89d7c7768
> Initial support for Groups
> Create separate object for Users and Groups (using same base class)
> Check for uniqueness before adding new users and groups
> Remove user_container from everything but add operations
> Abstract out a number of functions that are common across users and groups
> Make sure all strings passed in to be in a filter are checked
> Add new error message: No modifications specified
> diff -r 91183c369716 -r 093f73cd3cd2 ipa-admintools/ipa-usermod
> --- a/ipa-admintools/ipa-usermod	Thu Aug 23 11:57:25 2007 -0400
> +++ b/ipa-admintools/ipa-usermod	Fri Aug 24 15:42:56 2007 -0400

[snip]
> +
> +class Entity:
> +    """This class represents an IPA user.  An LDAP entry consists of a DN
                                       ^^^^
Comment update here.

> +    and a list of attributes.  Each attribute consists of a name and a list of
> +    values. For the time being I will maintain this.
> +

[snip]

> diff -r 91183c369716 -r 093f73cd3cd2 ipa-server/ipaserver/ipaldap.py
> --- a/ipa-server/ipaserver/ipaldap.py	Thu Aug 23 11:57:25 2007 -0400
> +++ b/ipa-server/ipaserver/ipaldap.py	Fri Aug 24 15:42:56 2007 -0400
> @@ -320,6 +320,9 @@ class IPAdmin(SimpleLDAPObject):
>  
>          modlist = self.generateModList(olduser, newuser)
>  
> +        if len(modlist) == 0:
> +            raise ipaerror.gen_exception(ipaerror.LDAP_EMPTY_MODLIST)

Do you think we should throw an exception in this case?  There's no
wrong or right answer, I think it's a debate for how our UI should
behave.  I can envision a UI that notifies the user no updates were
made (so they can notice that they screwed up), but I can also see some
confusion when (in the web gui) they are sent back to the 'edit' page
with a message saying nothing was changed.



> +        for user in users:
> +            try:
> +                self.remove_user_from_group(user, group, opts)
> +            except ipaerror.exception_for(ipaerror.LDAP_EMPTY_MODLIST):
> +                # User is not in the group
> +                failed.append(user)
> +            except ipaerror.gen_exception(ipaerror.LDAP_NOT_FOUND):
> +                # User or the group does not exist
> +                failed.append(user)

Ahhh.. I see the reason for the exception now.  It looks reasonable, but
again I'm not sure if we want to consider it an "error" if the user was
already in the group.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 2228 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20070827/9295f1f7/attachment.bin>