[Freeipa-devel] [PATCH] 0005 User life cycle: del/mod/find/show stageuser commands

[Jan Cholasta]

Dne 20.5.2015 v 11:26 Jan Cholasta napsal(a):
> Dne 18.5.2015 v 10:33 thierry bordaz napsal(a):
>> 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 ?
>
> See the attached patch.

Can someone please review the patch?

>
>>
>>>
>>> 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.
>
> BTW: I might be stating the obvious here, but it would be better to use
> one boolean parameter rather than two mutually exclusive flags in user-del.

I would like an opinion on this as well.

-- 
Jan Cholasta