On 05/15/2015 04:44 PM, David Kupka wrote:
Hello Thierry,
thanks for the patch set. Overall functionality of ULC feature looks good to
me and is definitely "alpha ready".

I found following issues but don't insist on fixing it right now:

1) When stageuser-activate fails due to already existent active/deleted user.
DN is show instead of user name that's used in other commands (user-add,
$ ipa user-add tuser --first Test --last User
$ ipa stageuser-add tuser --first Test --last User
$ ipa stageuser-activate tuser
ipa: ERROR: Active user
already exists

Hi David, Jan,

Thanks you so much for all those tests and feedback. I agree, some minor bugs can be fixed separatly from this main patches.

You are right, It should return the user ID not the DN.

2) According to the design there should be '--only-delete' and '--also-delete'
options for user-find command instead there is '--preserved' option.
Honza proposed adding virtual boolean attribute 'deleted' to user entry and
filter on it.
The 'deleted' attribute would be useful also in user-show where is no way to
tell if the displayed user is active or deleted. (Except running with --all
and looking on the dn).

Yes a bit late to resynch the design.
The final option is 'preserved' for user-find and 'preserve' for user-del. '--only-delete' or 'also-delete' are old name that I need to replace in the design.

About the 'deleted' attribute, do you think adding a DS cos virtual attribute ?

3) uidNumber and gidNumber can't be set back to '-1' once set to other value.
This would be useful when admin changes its mind and want IPA to assign them.
IIUC, there should be no validation in cn=staged user container. All
validation should be done during stageuser-activate.

Yes that comes from user plugin that enforce the number to be >0.
That is a good point giving the ability to reset uidNumber/gidNumber.
I will check if it is possible, how (give a value or an option to reset), and also if it would not create other issue.

4) Support for deleted -> stage workflow is still missing. But I'm unsure if we
agreed to finish it now or later.

Yes thanks

5) Twice deleting user with '--preserve' deletes him permanently.
$ ipa user-add tuser --first Test --last User
$ ipa user-del tuser --preserve
$ ipa user-del tuser --preserve
$ ipa user-find --preserved
0 (delete) users matched
Number of entries returned 0

Deleting a deleted (preserved) entry, should permanently remove the entry.
Now if the second time the preserve option is present, it makes sense to not delete it.



----- Original Message -----
From: "thierry bordaz" <tbor...@redhat.com>
To: "Jan Cholasta" <jchol...@redhat.com>, "David Kupka" <dku...@redhat.com>
Cc: "freeipa-devel" <freeipa-devel@redhat.com>
Sent: Tuesday, May 12, 2015 5:05:29 PM
Subject: Re: [Freeipa-devel] [PATCH] 0005 User life cycle: del/mod/find/show 
stageuser commands

On 05/12/2015 02:17 PM, thierry bordaz wrote:
On 05/05/2015 08:57 AM, Jan Cholasta wrote:

Dne 28.4.2015 v 16:40 thierry bordaz napsal(a):
On 04/28/2015 10:40 AM, David Kupka wrote:
On 04/28/2015 10:28 AM, thierry bordaz wrote:
On 04/28/2015 10:23 AM, David Kupka wrote:
On 04/16/2015 01:00 PM, thierry bordaz wrote:

     Here is the next patch for User life cycle that introduces
     del/mod/find and show stageuser plugin commands.

   * 0000-User Life Cycle (create containers and scoping  DS
   * 0002-User-life-cycle-stageuser-add-verb.patch: *pushed*
   * 0007-User-life-cycle-allows-MODRDN-from-ldap2.patch: *pushed*
   * 0003-User-life-cycle-new-stageuser-commands-del-mod-find-*under
     review *(this one)**
   * 0004-User-life-cycle-new-stageuser-commands-activate.patch
   * 0005-User-life-cycle-new-stageuser-commands-activate-prov.patch
   * 0006-User-life-cycle-user-del-supports-permanently-preser.patch
   * 0008-User-life-cycle-user-find-support-finding-delete-use.patch
   * 0009-User-life-cycle-support-of-user-undel.patch
   * 0010-User-life-cycle-DNA-DS-plugin-should-exclude-provisi.patch
   * 0011-User-life-cycle-lockout-provisioning-stage-and-delet.patch
   * 0012-User-life-cycle-Create-stage-Admin-provisioning-acco.patch
   * 0013-User-life-cycle-Stage-Admin-permission-priviledge.patch


Hi Thierry,
thanks for the patch, the code looks good to me but there is
a bug in ACIs.
After creating a stage user and setting password for him I can kinit
as the stage user. I'm unable to login to the IPA client and id
command for this stage user responds "no such user" but I can kinit
and invoke ipa commands.

0. build freeipa with your patch
1. # ipa-server-install
2. $ kinit admin
3. $ ipa stageuser-add suser0 --first Stage --last User --password
4. $ kdestroy
5. $ kinit suser0
6. $ ipa user-find

Prints out list of ipa users.

kinit fails with "suser0@... not found in Kerberos database"

Hi David,

Thank you so much for having looked at this patch :-)
You are right. The Staging users (as well as the Delete users) are
lockout in that patch.
The patch
0011-User-life-cycle-lockout-provisioning-stage-and-delet.patch will
take care of this.

Do you prefer that I merged the two patches right now ?


Hi Thierry,
no, it is not necessary to merge the patches it's ok to have it
separated. I'm not sure if the patch should be pushed now or rather
wait and push it together with the others.
I'm looking forward to next ULC patches from you.

Hi David,

Here are all the available patches.
I also attach a test script that is a kind of regression tests that
I am

Thanks again

Some issues I have found during a quick read of the patches:

Patch 0005:

1) This does nothing and can be safely removed:

+    def pre_callback(self, ldap, dn, *keys, **options):
+        assert isinstance(dn, DN)
+        return dn

2) stageuser_{mod,find,show} have a lot of code that seems to be
copy-pasted from user_{mod,find,show}. I would prefer if the code was
shared instead of copy-pasted, preferably in baseuser_{mod,find,show}.

Patch 0006:

1) These do nothing and can be safely removed:

+    def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys,
+        assert isinstance(dn, DN)
+        return dn
+    def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
+        assert isinstance(dn, DN)
+        return dn

2) You should use output.standard_entry for has_output, as you are
only returning one entry. Or you could add support for activating
multiple users at once.

3) IMO the "time to build the new entry" and "add the active entry"
parts should be done by calling user-add, so that the (active) user
creation routine is the same.

4) Please use a single line to separate method definitions in classes.

Patch 008:

1) I guess you forgot to remove these:

+        self.log.error("====> user-add pre_callback 1 %s " % dn)

+        self.log.error("====> user-add pre_callback %s " % dn)


+    takes_options = LDAPDelete.takes_options + (

This should be "baseuser_del.takes_options + ...".



     This is the set of revisited patches for ULC. Here are the changes
     done versus the previous patches

         Patch 0005:

             Refactoring stageuser+user => baseuser

         Patch 0006:

             Lock stage/delete users, add activated user into ipausers,
             stageuser-activate process a single entry

         Patch 0007

             Refactoring stageuser+user => baseuser, GID number lost
             during activation

         Patch 0008

             user take_options from baseuser_del rather than LDAPDelete

         Patch 0009

             Refactoring stageuser+user => baseuser, remove debug traces

         Patch 0010

             Refactoring stageuser+user => baseuser,, add undelete user
             into ipausers

         Patch 0011 (previous patch 0011 was merged in Patch 0006:
         lockout stage/delete users)

             Change due to CSV

         Patch 0012

             Change due to CSV, permission tests

     It does not take into account your request Honza to use 'user-add'
     command when activating a user.
     I will work on that now and submit an other patch later for that.


IPA_API_VERSION_MINOR was invalid in the first patch 0005. I am sorry
for that.

Here is the next set of patches. Only patch 0005 differs from my
previous email


Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to