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