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

[Martin Kosek]

On 06/18/2015 01:22 PM, Petr Vobornik wrote:
> On 06/18/2015 12:52 PM, Jan Cholasta wrote:
>> Dne 18.6.2015 v 09:30 Jan Cholasta napsal(a):
>>> Dne 15.6.2015 v 17:29 thierry bordaz napsal(a):
>>>> 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'.
>>>
>>> It's ridiculous that the CLI taints the RPC API and it should be fixed.
>>>
>>> Also on a more nitpicky side, I think the flag should be called
>>> --no-preserve rather than --permanently. There is plenty of commands
>>> (rm, cp, ...) which have --no-preserve as opposite of --preserve.
>>>
>>> The attached patch fixes both.
>>
>> ... and it also accidentaly changes the default behavior.
>>
>> Updated patch attached.
>>
> 
> ACK if others are ok with changing --permanently to --no-preserve.

I am. This looks as a change that should make it to Alpha :-)

> 
> Patch 446 fixed also issue #5, patch 446.1 doesn't fix it. Could be fixed
> separately.
> 
> Attaching patch which addresses this API change in Web UI.
> 
>