Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-11-15 Thread Martin Kosek 
On 11/14/2013 03:30 PM, Ana Krivokapic wrote:
 On 11/14/2013 03:11 PM, Martin Kosek wrote:
 On 11/13/2013 04:56 PM, Ana Krivokapic wrote:
 On 11/13/2013 03:08 PM, Martin Kosek wrote:
 On 10/29/2013 12:30 PM, Ana Krivokapic wrote:
 On 10/15/2013 06:09 PM, Ana Krivokapic wrote:
 On 09/30/2013 10:02 AM, Petr Viktorin wrote:
 On 09/27/2013 03:12 PM, Martin Kosek wrote:
 On 09/27/2013 03:00 PM, Jan Cholasta wrote:
 On 23.9.2013 19:41, Ana Krivokapic wrote:
 On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
 ...
 Patch 69:

 I think the changes in the update file should be also done in the
 right LDIF
 files in install/share, though I don't know what is the recent
 consensus on this.


 Honza

 Last time I checked, we used to do the change both in LDIF and update
 file. Just to avoid the LDIF become obsolete.

 Martin
 Rob recently said his preference is to move everything from LDIF to 
 updates,
 and out of the the LDIF files:
 http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html

 I would agree, having two places with the same information is redundant 
 and
 error-prone.

 Thanks Honza for the review.

 I incorporated your suggestions in this updated patchset. I attached all 
 the
 patches for more convenient reviewing, but only patches 68 and 70 have 
 changed.

 I haven't done any changes in the LDIF files since the consensus seems 
 to be not
 to do that.
 Patch 70 needed a rebase, attaching the whole patchset again.
 This works pretty fine, I have few comments though:

 1) 0068: the task should be run only for users/hosts base DN - this is 
 where we
 confine our automember and I think admin may be surprised that the rebuild 
 call
 is does not respect it.
 Fixed.

 2) 0068: I am missing some examples for automember-rebuild in the help. At
 least for running rebuild for all users/hosts and for running it for 
 specified
 user/host.
 I added some examples, as well as a general description of the new command.

 3) 0068: I think that the labels/doc for the new command/options should be
 improved. It is not obvious, that automember-rebuild can run for all
 users/hosts, at least from following doc:

 # ipa help automember
 ...
   automember-rebuild   Rebuild auto membership for specified 
 entries.
 ...

 Maybe we should remove the for specified entries part?

 As for the options, we now have this:

 # ipa help automember-rebuild
 Usage: ipa [global-options] automember-rebuild [options]

 Rebuild auto membership for specified entries.
 Options:
   -h, --helpshow this help message and exit
   --type=['group', 'hostgroup']
 Grouping to which the rule applies  --completely 
 stray
   --users=STR   Users for which the rebuild task will be run
   --hosts=STR   Hosts for which the rebuild task will be run


 We should probably also do not mention specified entries here.

 As for option help, maybe the following would better show that it can be 
 run
 for all entries?

   --type=['group', 'hostgroup']
 Rebuild membership for all members of a grouping
   --users=STR   Rebuild membership for specified users
   --hosts=STR   Rebuild membership for specified hosts
 Agreed, labels fixed as per your suggestions.

 This makes me thinking we may want to forbid entering both --type and
 --users/--hosts - i.e. either rebuild all or just selected ones - to make 
 the
 selection even more clear. But I am open to discussion on this one.
 Validation prevents any invalid combination of options (e.g. --type=group 
 and
 --hosts used together, or --type=hostgroup and --users used together). If, 
 for
 example, --users is specified, then --type=group is allowed but not 
 required. I
 think it's clear enough.

 4) 0069: Add Automember Export Updates Task is currently redundant. I 
 think we
 should either have permissions for all 3 possible tasks or for just the 
 one we use.
 I removed the unused permission.

 5) 0069: permissions should be of SYSTEM type as the ACI is out of SUFFIX, 
 so
 that user does not try to modify them (will be able to in future versions).
 Adding Petr3 to CC for heads up on this one.
 Fixed.

 Martin
 Thanks for the review, the updated patchset is attached.
 Looks good. Last thing I would like to get fixed is this part in 0068:

 +types = {
 +'group': ('user', 'users', 'users'),
 +'hostgroup': ('host', 'hosts', 'computers'),
 +}
 +
 +obj_name, opt_name, dn_part = types[gtype]
 +basedn = DN(('cn', dn_part),('cn', 'accounts'), api.env.basedn)
 +obj = self.api.Object[obj_name]

 I know it works now, but if we sometime decide to maybe support different 
 user
 containers and not have it in cn=users,cn=accounts, it will help not user
 hardcoded DNs like that, but rather standard

 DN(api.env.container_users, api.env.basedn)
 or
 DN(api.env.container_hosts, api.env.basedn)

 Martin
 
 Fixed, updated patch attached.
 

Thanks, ACK!

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-11-14 Thread Martin Kosek 
On 11/13/2013 04:56 PM, Ana Krivokapic wrote:
 On 11/13/2013 03:08 PM, Martin Kosek wrote:
 On 10/29/2013 12:30 PM, Ana Krivokapic wrote:
 On 10/15/2013 06:09 PM, Ana Krivokapic wrote:
 On 09/30/2013 10:02 AM, Petr Viktorin wrote:
 On 09/27/2013 03:12 PM, Martin Kosek wrote:
 On 09/27/2013 03:00 PM, Jan Cholasta wrote:
 On 23.9.2013 19:41, Ana Krivokapic wrote:
 On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
 ...
 Patch 69:

 I think the changes in the update file should be also done in the
 right LDIF
 files in install/share, though I don't know what is the recent
 consensus on this.


 Honza

 Last time I checked, we used to do the change both in LDIF and update
 file. Just to avoid the LDIF become obsolete.

 Martin
 Rob recently said his preference is to move everything from LDIF to 
 updates,
 and out of the the LDIF files:
 http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html

 I would agree, having two places with the same information is redundant 
 and
 error-prone.

 Thanks Honza for the review.

 I incorporated your suggestions in this updated patchset. I attached all 
 the
 patches for more convenient reviewing, but only patches 68 and 70 have 
 changed.

 I haven't done any changes in the LDIF files since the consensus seems to 
 be not
 to do that.
 Patch 70 needed a rebase, attaching the whole patchset again.
 This works pretty fine, I have few comments though:

 1) 0068: the task should be run only for users/hosts base DN - this is where 
 we
 confine our automember and I think admin may be surprised that the rebuild 
 call
 is does not respect it.
 
 Fixed.
 

 2) 0068: I am missing some examples for automember-rebuild in the help. At
 least for running rebuild for all users/hosts and for running it for 
 specified
 user/host.
 
 I added some examples, as well as a general description of the new command.
 

 3) 0068: I think that the labels/doc for the new command/options should be
 improved. It is not obvious, that automember-rebuild can run for all
 users/hosts, at least from following doc:

 # ipa help automember
 ...
   automember-rebuild   Rebuild auto membership for specified 
 entries.
 ...

 Maybe we should remove the for specified entries part?

 As for the options, we now have this:

 # ipa help automember-rebuild
 Usage: ipa [global-options] automember-rebuild [options]

 Rebuild auto membership for specified entries.
 Options:
   -h, --helpshow this help message and exit
   --type=['group', 'hostgroup']
 Grouping to which the rule applies  --completely 
 stray
   --users=STR   Users for which the rebuild task will be run
   --hosts=STR   Hosts for which the rebuild task will be run


 We should probably also do not mention specified entries here.

 As for option help, maybe the following would better show that it can be run
 for all entries?

   --type=['group', 'hostgroup']
 Rebuild membership for all members of a grouping
   --users=STR   Rebuild membership for specified users
   --hosts=STR   Rebuild membership for specified hosts
 
 Agreed, labels fixed as per your suggestions.
 

 This makes me thinking we may want to forbid entering both --type and
 --users/--hosts - i.e. either rebuild all or just selected ones - to make the
 selection even more clear. But I am open to discussion on this one.
 
 Validation prevents any invalid combination of options (e.g. --type=group and
 --hosts used together, or --type=hostgroup and --users used together). If, for
 example, --users is specified, then --type=group is allowed but not required. 
 I
 think it's clear enough.
 

 4) 0069: Add Automember Export Updates Task is currently redundant. I think 
 we
 should either have permissions for all 3 possible tasks or for just the one 
 we use.
 
 I removed the unused permission.
 

 5) 0069: permissions should be of SYSTEM type as the ACI is out of SUFFIX, so
 that user does not try to modify them (will be able to in future versions).
 Adding Petr3 to CC for heads up on this one.
 
 Fixed.
 

 Martin
 
 Thanks for the review, the updated patchset is attached.

Looks good. Last thing I would like to get fixed is this part in 0068:

+types = {
+'group': ('user', 'users', 'users'),
+'hostgroup': ('host', 'hosts', 'computers'),
+}
+
+obj_name, opt_name, dn_part = types[gtype]
+basedn = DN(('cn', dn_part),('cn', 'accounts'), api.env.basedn)
+obj = self.api.Object[obj_name]

I know it works now, but if we sometime decide to maybe support different user
containers and not have it in cn=users,cn=accounts, it will help not user
hardcoded DNs like that, but rather standard

DN(api.env.container_users, api.env.basedn)
or
DN(api.env.container_hosts, api.env.basedn)

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-11-14 Thread Ana Krivokapic 
On 11/14/2013 03:11 PM, Martin Kosek wrote:
 On 11/13/2013 04:56 PM, Ana Krivokapic wrote:
 On 11/13/2013 03:08 PM, Martin Kosek wrote:
 On 10/29/2013 12:30 PM, Ana Krivokapic wrote:
 On 10/15/2013 06:09 PM, Ana Krivokapic wrote:
 On 09/30/2013 10:02 AM, Petr Viktorin wrote:
 On 09/27/2013 03:12 PM, Martin Kosek wrote:
 On 09/27/2013 03:00 PM, Jan Cholasta wrote:
 On 23.9.2013 19:41, Ana Krivokapic wrote:
 On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
 ...
 Patch 69:

 I think the changes in the update file should be also done in the
 right LDIF
 files in install/share, though I don't know what is the recent
 consensus on this.


 Honza

 Last time I checked, we used to do the change both in LDIF and update
 file. Just to avoid the LDIF become obsolete.

 Martin
 Rob recently said his preference is to move everything from LDIF to 
 updates,
 and out of the the LDIF files:
 http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html

 I would agree, having two places with the same information is redundant 
 and
 error-prone.

 Thanks Honza for the review.

 I incorporated your suggestions in this updated patchset. I attached all 
 the
 patches for more convenient reviewing, but only patches 68 and 70 have 
 changed.

 I haven't done any changes in the LDIF files since the consensus seems to 
 be not
 to do that.
 Patch 70 needed a rebase, attaching the whole patchset again.
 This works pretty fine, I have few comments though:

 1) 0068: the task should be run only for users/hosts base DN - this is 
 where we
 confine our automember and I think admin may be surprised that the rebuild 
 call
 is does not respect it.
 Fixed.

 2) 0068: I am missing some examples for automember-rebuild in the help. At
 least for running rebuild for all users/hosts and for running it for 
 specified
 user/host.
 I added some examples, as well as a general description of the new command.

 3) 0068: I think that the labels/doc for the new command/options should be
 improved. It is not obvious, that automember-rebuild can run for all
 users/hosts, at least from following doc:

 # ipa help automember
 ...
   automember-rebuild   Rebuild auto membership for specified 
 entries.
 ...

 Maybe we should remove the for specified entries part?

 As for the options, we now have this:

 # ipa help automember-rebuild
 Usage: ipa [global-options] automember-rebuild [options]

 Rebuild auto membership for specified entries.
 Options:
   -h, --helpshow this help message and exit
   --type=['group', 'hostgroup']
 Grouping to which the rule applies  --completely 
 stray
   --users=STR   Users for which the rebuild task will be run
   --hosts=STR   Hosts for which the rebuild task will be run


 We should probably also do not mention specified entries here.

 As for option help, maybe the following would better show that it can be run
 for all entries?

   --type=['group', 'hostgroup']
 Rebuild membership for all members of a grouping
   --users=STR   Rebuild membership for specified users
   --hosts=STR   Rebuild membership for specified hosts
 Agreed, labels fixed as per your suggestions.

 This makes me thinking we may want to forbid entering both --type and
 --users/--hosts - i.e. either rebuild all or just selected ones - to make 
 the
 selection even more clear. But I am open to discussion on this one.
 Validation prevents any invalid combination of options (e.g. --type=group and
 --hosts used together, or --type=hostgroup and --users used together). If, 
 for
 example, --users is specified, then --type=group is allowed but not 
 required. I
 think it's clear enough.

 4) 0069: Add Automember Export Updates Task is currently redundant. I think 
 we
 should either have permissions for all 3 possible tasks or for just the one 
 we use.
 I removed the unused permission.

 5) 0069: permissions should be of SYSTEM type as the ACI is out of SUFFIX, 
 so
 that user does not try to modify them (will be able to in future versions).
 Adding Petr3 to CC for heads up on this one.
 Fixed.

 Martin
 Thanks for the review, the updated patchset is attached.
 Looks good. Last thing I would like to get fixed is this part in 0068:

 +types = {
 +'group': ('user', 'users', 'users'),
 +'hostgroup': ('host', 'hosts', 'computers'),
 +}
 +
 +obj_name, opt_name, dn_part = types[gtype]
 +basedn = DN(('cn', dn_part),('cn', 'accounts'), api.env.basedn)
 +obj = self.api.Object[obj_name]

 I know it works now, but if we sometime decide to maybe support different user
 containers and not have it in cn=users,cn=accounts, it will help not user
 hardcoded DNs like that, but rather standard

 DN(api.env.container_users, api.env.basedn)
 or
 DN(api.env.container_hosts, api.env.basedn)

 Martin

Fixed, updated patch attached.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-11-13 Thread Martin Kosek 
On 10/29/2013 12:30 PM, Ana Krivokapic wrote:
 On 10/15/2013 06:09 PM, Ana Krivokapic wrote:
 On 09/30/2013 10:02 AM, Petr Viktorin wrote:
 On 09/27/2013 03:12 PM, Martin Kosek wrote:
 On 09/27/2013 03:00 PM, Jan Cholasta wrote:
 On 23.9.2013 19:41, Ana Krivokapic wrote:
 On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
 ...
 Patch 69:

 I think the changes in the update file should be also done in the
 right LDIF
 files in install/share, though I don't know what is the recent
 consensus on this.


 Honza

 Last time I checked, we used to do the change both in LDIF and update
 file. Just to avoid the LDIF become obsolete.

 Martin
 Rob recently said his preference is to move everything from LDIF to updates,
 and out of the the LDIF files:
 http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html

 I would agree, having two places with the same information is redundant and
 error-prone.

 Thanks Honza for the review.

 I incorporated your suggestions in this updated patchset. I attached all the
 patches for more convenient reviewing, but only patches 68 and 70 have 
 changed.

 I haven't done any changes in the LDIF files since the consensus seems to be 
 not
 to do that.
 
 Patch 70 needed a rebase, attaching the whole patchset again.

This works pretty fine, I have few comments though:

1) 0068: the task should be run only for users/hosts base DN - this is where we
confine our automember and I think admin may be surprised that the rebuild call
is does not respect it.

2) 0068: I am missing some examples for automember-rebuild in the help. At
least for running rebuild for all users/hosts and for running it for specified
user/host.

3) 0068: I think that the labels/doc for the new command/options should be
improved. It is not obvious, that automember-rebuild can run for all
users/hosts, at least from following doc:

# ipa help automember
...
  automember-rebuild   Rebuild auto membership for specified 
entries.
...

Maybe we should remove the for specified entries part?

As for the options, we now have this:

# ipa help automember-rebuild
Usage: ipa [global-options] automember-rebuild [options]

Rebuild auto membership for specified entries.
Options:
  -h, --helpshow this help message and exit
  --type=['group', 'hostgroup']
Grouping to which the rule applies  --completely stray
  --users=STR   Users for which the rebuild task will be run
  --hosts=STR   Hosts for which the rebuild task will be run


We should probably also do not mention specified entries here.

As for option help, maybe the following would better show that it can be run
for all entries?

  --type=['group', 'hostgroup']
Rebuild membership for all members of a grouping
  --users=STR   Rebuild membership for specified users
  --hosts=STR   Rebuild membership for specified hosts

This makes me thinking we may want to forbid entering both --type and
--users/--hosts - i.e. either rebuild all or just selected ones - to make the
selection even more clear. But I am open to discussion on this one.

4) 0069: Add Automember Export Updates Task is currently redundant. I think we
should either have permissions for all 3 possible tasks or for just the one we 
use.

5) 0069: permissions should be of SYSTEM type as the ACI is out of SUFFIX, so
that user does not try to modify them (will be able to in future versions).
Adding Petr3 to CC for heads up on this one.

Martin

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

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-11-13 Thread Ana Krivokapic 
On 11/13/2013 03:08 PM, Martin Kosek wrote:
 On 10/29/2013 12:30 PM, Ana Krivokapic wrote:
 On 10/15/2013 06:09 PM, Ana Krivokapic wrote:
 On 09/30/2013 10:02 AM, Petr Viktorin wrote:
 On 09/27/2013 03:12 PM, Martin Kosek wrote:
 On 09/27/2013 03:00 PM, Jan Cholasta wrote:
 On 23.9.2013 19:41, Ana Krivokapic wrote:
 On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
 ...
 Patch 69:

 I think the changes in the update file should be also done in the
 right LDIF
 files in install/share, though I don't know what is the recent
 consensus on this.


 Honza

 Last time I checked, we used to do the change both in LDIF and update
 file. Just to avoid the LDIF become obsolete.

 Martin
 Rob recently said his preference is to move everything from LDIF to 
 updates,
 and out of the the LDIF files:
 http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html

 I would agree, having two places with the same information is redundant and
 error-prone.

 Thanks Honza for the review.

 I incorporated your suggestions in this updated patchset. I attached all the
 patches for more convenient reviewing, but only patches 68 and 70 have 
 changed.

 I haven't done any changes in the LDIF files since the consensus seems to 
 be not
 to do that.
 Patch 70 needed a rebase, attaching the whole patchset again.
 This works pretty fine, I have few comments though:

 1) 0068: the task should be run only for users/hosts base DN - this is where 
 we
 confine our automember and I think admin may be surprised that the rebuild 
 call
 is does not respect it.

Fixed.


 2) 0068: I am missing some examples for automember-rebuild in the help. At
 least for running rebuild for all users/hosts and for running it for specified
 user/host.

I added some examples, as well as a general description of the new command.


 3) 0068: I think that the labels/doc for the new command/options should be
 improved. It is not obvious, that automember-rebuild can run for all
 users/hosts, at least from following doc:

 # ipa help automember
 ...
   automember-rebuild   Rebuild auto membership for specified 
 entries.
 ...

 Maybe we should remove the for specified entries part?

 As for the options, we now have this:

 # ipa help automember-rebuild
 Usage: ipa [global-options] automember-rebuild [options]

 Rebuild auto membership for specified entries.
 Options:
   -h, --helpshow this help message and exit
   --type=['group', 'hostgroup']
 Grouping to which the rule applies  --completely 
 stray
   --users=STR   Users for which the rebuild task will be run
   --hosts=STR   Hosts for which the rebuild task will be run


 We should probably also do not mention specified entries here.

 As for option help, maybe the following would better show that it can be run
 for all entries?

   --type=['group', 'hostgroup']
 Rebuild membership for all members of a grouping
   --users=STR   Rebuild membership for specified users
   --hosts=STR   Rebuild membership for specified hosts

Agreed, labels fixed as per your suggestions.


 This makes me thinking we may want to forbid entering both --type and
 --users/--hosts - i.e. either rebuild all or just selected ones - to make the
 selection even more clear. But I am open to discussion on this one.

Validation prevents any invalid combination of options (e.g. --type=group and
--hosts used together, or --type=hostgroup and --users used together). If, for
example, --users is specified, then --type=group is allowed but not required. I
think it's clear enough.


 4) 0069: Add Automember Export Updates Task is currently redundant. I think we
 should either have permissions for all 3 possible tasks or for just the one 
 we use.

I removed the unused permission.


 5) 0069: permissions should be of SYSTEM type as the ACI is out of SUFFIX, so
 that user does not try to modify them (will be able to in future versions).
 Adding Petr3 to CC for heads up on this one.

Fixed.


 Martin

Thanks for the review, the updated patchset is attached.


-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From 977867faec9f9c6702dcf8fde904b462d31d9229 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Thu, 19 Sep 2013 14:10:32 +0200
Subject: [PATCH] Add a privilege and a permission needed for automember
 rebuild command

Design: http://www.freeipa.org/page/V3/Automember_rebuild_membership
https://fedorahosted.org/freeipa/ticket/3752
---
 install/updates/40-delegation.update | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/install/updates/40-delegation.update b/install/updates/40-delegation.update
index 64a6432acc8605f3164d267d16609f51ce02a7ef..3fabdf9c7319b261aa3e0bb20d42a80b807df1ec 100644
--- a/install/updates/40-delegation.update
+++ b/install/updates/40-delegation.update
@@ -373,3 +373,22 @@ dn: cn=Retrieve Certificates from the

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-10-15 Thread Ana Krivokapic 
On 09/30/2013 10:02 AM, Petr Viktorin wrote:
 On 09/27/2013 03:12 PM, Martin Kosek wrote:
 On 09/27/2013 03:00 PM, Jan Cholasta wrote:
 On 23.9.2013 19:41, Ana Krivokapic wrote:
 On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
 ...
 Patch 69:

 I think the changes in the update file should be also done in the
 right LDIF
 files in install/share, though I don't know what is the recent
 consensus on this.


 Honza


 Last time I checked, we used to do the change both in LDIF and update
 file. Just to avoid the LDIF become obsolete.

 Martin

 Rob recently said his preference is to move everything from LDIF to updates,
 and out of the the LDIF files:
 http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html

 I would agree, having two places with the same information is redundant and
 error-prone.


Thanks Honza for the review.

I incorporated your suggestions in this updated patchset. I attached all the
patches for more convenient reviewing, but only patches 68 and 70 have changed.

I haven't done any changes in the LDIF files since the consensus seems to be not
to do that.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From 12426d8945396468bf7ecb85ccd6a6ddab4630d2 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Thu, 19 Sep 2013 14:01:58 +0200
Subject: [PATCH] Add automember rebuild command

Add a new command to IPA CLI: ipa automember-rebuild

The command integrates the automember rebuild membership task functionality
into IPA CLI. It makes it possible to rebuild automember membership for
groups/hostgroups.

Design: http://www.freeipa.org/page/V3/Automember_rebuild_membership
https://fedorahosted.org/freeipa/ticket/3752
---
 API.txt  |   9 
 VERSION  |   2 +-
 ipalib/plugins/automember.py | 118 +++
 3 files changed, 118 insertions(+), 11 deletions(-)

diff --git a/API.txt b/API.txt
index 40871f6a8b105a7b161df34ce4f6feaf785a6107..05c5a988b7738e20e6351f6b3a01026f61cb6063 100644
--- a/API.txt
+++ b/API.txt
@@ -199,6 +199,15 @@ command: automember_mod
 output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (type 'unicode', type 'NoneType'), None)
 output: Output('value', type 'unicode', None)
+command: automember_rebuild
+args: 0,4,3
+option: Str('hosts*')
+option: StrEnum('type', cli_name='type', multivalue=False, required=False, values=(u'group', u'hostgroup'))
+option: Str('users*')
+option: Str('version?', exclude='webui')
+output: Output('result', type 'bool', None)
+output: Output('summary', (type 'unicode', type 'NoneType'), None)
+output: Output('value', type 'unicode', None)
 command: automember_remove_condition
 args: 1,8,5
 arg: Str('cn', cli_name='automember_rule')
diff --git a/VERSION b/VERSION
index c3c6d5a4c28991839a1917f18d2804475a16bcb7..32f6efbc4d4768c77c514a3367cb9feb039205e5 100644
--- a/VERSION
+++ b/VERSION
@@ -89,4 +89,4 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=65
+IPA_API_VERSION_MINOR=66
diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index 4f563f11953dafac0d5bfd00f0309aca2f346f81..b30182d9ee6545d08b0f57fbf7733f11a17dcb6f 100644
--- a/ipalib/plugins/automember.py
+++ b/ipalib/plugins/automember.py
@@ -16,13 +16,11 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see http://www.gnu.org/licenses/.
-
-from ipalib import api, errors
-from ipalib import Str, StrEnum
+import uuid
+import ldap as _ldap
+from ipalib import api, errors, Str, StrEnum, _, ngettext
 from ipalib.plugins.baseldap import *
-from ipalib import _, ngettext
 from ipalib.request import context
-import ldap as _ldap
 from ipapython.dn import DN
 
 __doc__ = _(
@@ -184,14 +182,17 @@ class automember(LDAPObject):
 ),
 )
 
-def dn_exists(self, grouptype, groupname, *keys):
+def dn_exists(self, otype, oname):
 ldap = self.api.Backend.ldap2
-dn = self.api.Object[grouptype].get_dn(groupname)
+dn = self.api.Object[otype].get_dn(oname)
 try:
-(gdn, entry_attrs) = ldap.get_entry(dn, [])
+entry = ldap.get_entry(dn, [])
 except errors.NotFound:
-raise errors.NotFound(reason=_(u'Group: %s not found!') % groupname)
-return gdn
+raise errors.NotFound(
+reason=_(u'%(otype)s %(oname)s not found') %
+dict(otype=otype, oname=oname)
+)
+return entry.dn
 
 def get_dn(self, *keys, **options):
 if self.parent_object:
@@ -587,3 +588,100 @@ def execute(self, *keys, **options):
 return result
 
 api.register(automember_default_group_show)
+
+
+class

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-09-30 Thread Petr Viktorin 

On 09/27/2013 03:12 PM, Martin Kosek wrote:

On 09/27/2013 03:00 PM, Jan Cholasta wrote:

On 23.9.2013 19:41, Ana Krivokapic wrote:

On 09/19/2013 03:29 PM, Ana Krivokapic wrote:

...

Patch 69:

I think the changes in the update file should be also done in the
right LDIF
files in install/share, though I don't know what is the recent
consensus on this.


Honza



Last time I checked, we used to do the change both in LDIF and update
file. Just to avoid the LDIF become obsolete.

Martin


Rob recently said his preference is to move everything from LDIF to 
updates, and out of the the LDIF files:

http://www.redhat.com/archives/freeipa-devel/2013-September/msg00106.html

I would agree, having two places with the same information is redundant 
and error-prone.


--
Petr³

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

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-09-27 Thread Jan Cholasta 

On 23.9.2013 19:41, Ana Krivokapic wrote:

On 09/19/2013 03:29 PM, Ana Krivokapic wrote:

Hello,

This patch set adds automember rebuild membership functionality to IPA CLI.

Design:http://www.freeipa.org/page/V3/Automember_rebuild_membership
Ticket:https://fedorahosted.org/freeipa/ticket/3752



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


This updated patch set introduces only one automember-rebuild command,
instead of three. The changes are covered in the design document:
http://www.freeipa.org/page/V3/Automember_rebuild_membership#CLI.

Also, while working on these patches, I hit this bug again:
https://fedorahosted.org/freeipa/ticket/2708. Since the fix is pretty
straightforward, and the bug is related to automember feature, I
included a fix for it in this patch set (patch 0071).



Patch 68:

-(gdn, entry_attrs) = ldap.get_entry(dn, [])
+(odn, entry_attrs) = ldap.get_entry(dn, [])

Please use the new LDAP API in new code.


+reason=_(u'%(otype)s: %(oname)s not found!') %

The colon looks weird here and looked weird in the original code too. 
Can you please remove it? And while you are at it, you could also add 
quotes around %(oname)s.



+class automember_rebuild(LDAPQuery):

You don't use any LDAPQuery, PKQuery, BaseLDAPCommand or Method 
specifics here, so you can inherit straight from Command. Just make sure 
to use self.api.Backend.ldap2 instead of self.obj.backend in the code.



The takes_options attribute should look like this:

takes_options = (
group_type[0].clone(required=False),
Str(
'users*',
label=_('Users'),
doc=_('Users for which the rebuild task will be run'),
),
Str(
'hosts*',
label=_('Hosts'),
doc=_('Hosts for which the rebuild task will be run'),
),
)

Or you can keep 'type' required and use default_from to create a default 
value for it based on --users and --hosts if it's not specified:


def _rebuild_type_default(users, hosts):
if users and not hosts:
return 'group'
elif hosts and not users:
return 'hostgroup'

...
takes_options = (
group_type[0].clone(default_from=_rebuild_type_default),
...

This should also simplify the rest of the code, as you will always have 
a value for 'type' if there were no errors.



In the validate method, you raise ValidationError with name='options'. 
This is wrong, as name should refer to a parameter name. You can use 
MutuallyExclusiveError for the errors:


def validate(self, **kw):
if kw.get('users') and kw.get('hosts'):
raise errors.MutuallyExclusiveError(reason=_(users and 
hosts cannot both be set))

if kw.get('type') == 'group' and kw.get('hosts'):
raise errors.MutuallyExclusiveError(reason=_(hosts cannot 
be set when type is 'group'))

if kw.get('type') == 'hostgroup' and kw.get('users'):
raise errors.MutuallyExclusiveError(reason=_(users cannot 
be set when type is 'hostgroup'))

super(automember_rebuild, self).validate(**kw)

Note that if you use default_from for the 'type' parameter, the validate 
method above will fail with RequirementError if 'type' is not specified 
(both explicitly and implicitly).



+basedn = realm_to_suffix(api.env.realm)

This should be api.env.basedn.


+if users or hosts:
+filters = []
+for u in users:
+self.obj.dn_exists('user', u)
+filters.append(ldap.make_filter_from_attr('uid', u))
+for h in hosts:
+self.obj.dn_exists('host', h)
+filters.append(ldap.make_filter_from_attr('fqdn', h))
+search_filter = ldap.combine_filters(filters, ldap.MATCH_ANY)
+else:
+search_filter = '(uid=*)' if group_type == 'group' else 
'(fqdn=*)'


You can look all the attribute names in the right object plugin, no need 
to specify them explicitly. I think you can use something like this instead:


types = {
'group': ('user', 'users'),
'hostgroup': ('host', 'hosts'),
}

obj_name, opt_name = types[kw['type']]
obj = self.api.Object[obj_name]

names = kw.get(opt_name)
if names:
for name in names:
obj.get_dn_if_exists(name)
search_filter = 
ldap.make_filter_from_attr(obj.primary_key.name, names, 
rules=ldap.MATCH_ANY)

else:
search_filter = '(%s=*)' % obj.primary_key.name


Patch 69:

I think the changes in the update file should be also done in the right 
LDIF files in install/share, though I don't know what is the recent 
consensus on this.



Honza

--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-09-27 Thread Martin Kosek 

On 09/27/2013 03:00 PM, Jan Cholasta wrote:

On 23.9.2013 19:41, Ana Krivokapic wrote:

On 09/19/2013 03:29 PM, Ana Krivokapic wrote:

...

Patch 69:

I think the changes in the update file should be also done in the right LDIF
files in install/share, though I don't know what is the recent consensus on 
this.


Honza



Last time I checked, we used to do the change both in LDIF and update file. 
Just to avoid the LDIF become obsolete.


Martin

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

Re: [Freeipa-devel] [PATCHES] 0068-0070 Automember rebuild membership

2013-09-23 Thread Ana Krivokapic 
On 09/19/2013 03:29 PM, Ana Krivokapic wrote:
 Hello,

 This patch set adds automember rebuild membership functionality to IPA CLI.

 Design: http://www.freeipa.org/page/V3/Automember_rebuild_membership
 Ticket: https://fedorahosted.org/freeipa/ticket/3752



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

This updated patch set introduces only one automember-rebuild command, instead
of three. The changes are covered in the design document:
http://www.freeipa.org/page/V3/Automember_rebuild_membership#CLI.

Also, while working on these patches, I hit this bug again:
https://fedorahosted.org/freeipa/ticket/2708. Since the fix is pretty
straightforward, and the bug is related to automember feature, I included a fix
for it in this patch set (patch 0071).

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From 9ceeec11494ddb11dc95d15e0a89597e3423ff38 Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Thu, 19 Sep 2013 14:01:58 +0200
Subject: [PATCH] Add automember rebuild command

Add a new command to IPA CLI: ipa automember-rebuild

The command integrates the automember rebuild membership task functionality
into IPA CLI. It makes it possible to rebuild automember membership for
groups/hostgroups.

Design: http://www.freeipa.org/page/V3/Automember_rebuild_membership
https://fedorahosted.org/freeipa/ticket/3752
---
 API.txt  |   9 +++
 VERSION  |   2 +-
 ipalib/plugins/automember.py | 129 +++
 3 files changed, 129 insertions(+), 11 deletions(-)

diff --git a/API.txt b/API.txt
index 761d1d175b5ce48bb6e27ce60e404f89790bfe6b..bd51634c4f564d7043fb86398b7cabbe1af745bf 100644
--- a/API.txt
+++ b/API.txt
@@ -199,6 +199,15 @@ command: automember_mod
 output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (type 'unicode', type 'NoneType'), None)
 output: Output('value', type 'unicode', None)
+command: automember_rebuild
+args: 0,4,3
+option: Str('hosts', multivalue=True, required=False)
+option: StrEnum('type', required=False, values=(u'group', u'hostgroup'))
+option: Str('users', multivalue=True, required=False)
+option: Str('version?', exclude='webui')
+output: Output('result', type 'bool', None)
+output: Output('summary', (type 'unicode', type 'NoneType'), None)
+output: Output('value', type 'unicode', None)
 command: automember_remove_condition
 args: 1,8,5
 arg: Str('cn', cli_name='automember_rule')
diff --git a/VERSION b/VERSION
index c3c6d5a4c28991839a1917f18d2804475a16bcb7..32f6efbc4d4768c77c514a3367cb9feb039205e5 100644
--- a/VERSION
+++ b/VERSION
@@ -89,4 +89,4 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=65
+IPA_API_VERSION_MINOR=66
diff --git a/ipalib/plugins/automember.py b/ipalib/plugins/automember.py
index 4f563f11953dafac0d5bfd00f0309aca2f346f81..308f3cdcba5aae8af018aea2f77e09da7541e3e6 100644
--- a/ipalib/plugins/automember.py
+++ b/ipalib/plugins/automember.py
@@ -16,14 +16,13 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see http://www.gnu.org/licenses/.
-
-from ipalib import api, errors
-from ipalib import Str, StrEnum
+import uuid
+import ldap as _ldap
+from ipalib import api, errors, Str, StrEnum, _, ngettext
 from ipalib.plugins.baseldap import *
-from ipalib import _, ngettext
 from ipalib.request import context
-import ldap as _ldap
 from ipapython.dn import DN
+from ipapython.ipautil import realm_to_suffix
 
 __doc__ = _(
 Auto Membership Rule.
@@ -184,14 +183,17 @@ class automember(LDAPObject):
 ),
 )
 
-def dn_exists(self, grouptype, groupname, *keys):
+def dn_exists(self, otype, oname):
 ldap = self.api.Backend.ldap2
-dn = self.api.Object[grouptype].get_dn(groupname)
+dn = self.api.Object[otype].get_dn(oname)
 try:
-(gdn, entry_attrs) = ldap.get_entry(dn, [])
+(odn, entry_attrs) = ldap.get_entry(dn, [])
 except errors.NotFound:
-raise errors.NotFound(reason=_(u'Group: %s not found!') % groupname)
-return gdn
+raise errors.NotFound(
+reason=_(u'%(otype)s: %(oname)s not found!') %
+dict(otype=otype, oname=oname)
+)
+return odn
 
 def get_dn(self, *keys, **options):
 if self.parent_object:
@@ -587,3 +589,110 @@ def execute(self, *keys, **options):
 return result
 
 api.register(automember_default_group_show)
+
+
+class automember_rebuild(LDAPQuery):
+__doc__ = _('Rebuild auto membership for specified entries.')
+# TODO: Add a --dry-run option:
+#