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

[Jan Cholasta]

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.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-446-User-life-cycle-change-user-del-flags-to-be-CLI-spec.patch
Type: text/x-patch
Size: 4544 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150618/4be86ac6/attachment.bin>