[Freeipa-devel] [PATCH] 0005 User life cycle: del/mod/find/show stageuser commands
thierry bordaz
tbordaz at redhat.com
Mon May 18 08:33:33 UTC 2015
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" <tbordaz at redhat.com>
> To: "Jan Cholasta" <jcholast at redhat.com>, "David Kupka" <dkupka at redhat.com>
> Cc: "freeipa-devel" <freeipa-devel at 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 at ... 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
More information about the Freeipa-devel
mailing list