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

[thierry bordaz]

On 06/15/2015 05:00 PM, Simo Sorce wrote:
> On Mon, 2015-06-15 at 16:48 +0200, Petr Vobornik wrote:
>> On 06/09/2015 02:02 PM, Jan Cholasta wrote:
>>> 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.
>> +1, but no-op if default behavior is "preserve"
>>
>>>>> Now if the second time the preserve option is present, it makes sense to
>>>>> not delete it.
>> +1, should be no-op
>>
>>>> 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.
>>>
>> So the proposal is, e.g.,:
>>
>> Replace:
>>     ipa user del fbar --preserve
>>     ipa user del fbar --permanently
>> with:
>>     ipa user del fbar --permanently=False
>>     ipa user del fbar --permanently=True
>> and
>>     ipa user del fbar
>> uses the default behavior(permanently atm.)
>>
>> I don't think there is a big difference. A boolean is easier for
>> scripting. 2 options are more descriptive for humans. With a single
>> boolean, I would be afraid that omitting it would imply False to some
>> users which is not always the same as "the default behavior" [1].
>>
>> With Web UI developer hat I would vote for single boolean but as a CLI
>> user I would like the current options.
>>
>> Given that Web UI or any other API client should not define CLI, I would
>> keep the current options.
>>
>> my 2c
>>
>> [1] http://www.freeipa.org/page/V4/User_Life-Cycle_Management#Delete_User
>> -- 
>> Petr Vobornik
>>
> +1 --preserve is 100x better for a human than --permanently=False

I also prefere --preserve for usability of  'user del'.

In addition we have 'user find|show --preserved' to retrieve users that 
have been preserved. So it seems to me better that the action that 
preserved the user uses the option '--preserve' rather 
'--permanently=False'.

>
> Simo.
>