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,
stageuser-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
uid=tuser,cn=users,cn=accounts,dc=abc,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com
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.
thanks
theirry
David
----- 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:
Hi,
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:
Hello,
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
plugins):
*pushed*
*
0001-User-Life-Cycle-Exclude-subtree-for-ipaUniqueID-gene.patch:
*pushed*
* 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
Thanks
thierry
Hi Thierry,
thanks for the patch, the code looks good to me but there is
probably
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.
Steps:
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
Actual:
Prints out list of ipa users.
Expected:
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
not
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 ?
thanks
thierry
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
using.
Thanks again
thierry
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,
**options):
+ 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)
2)
+ takes_options = LDAPDelete.takes_options + (
This should be "baseuser_del.takes_options + ...".
Honza
Hello,
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.
Thanks
thierry
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
thanks
thierry
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
