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

Thu Jun 18 11:22:45 UTC 2015

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.

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.
-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0880-webui-adjust-user-deleter-dialog-to-new-api.patch
Type: text/x-patch
Size: 5403 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150618/d935e323/attachment.bin>