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

Thu Jun 18 13:54:50 UTC 2015

On 06/18/2015 03:42 PM, David Kupka wrote:
> Dne 18.6.2015 v 13:22 Petr Vobornik napsal(a):
>> 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.
>>
>>
>
> pvoborni's patch 0880 works for me, ACK. I also applied jcholast's patch
> 446.1 and did not encounter any issue.
>

pushed to master:
* 1d608251383e4842b89c941a76dbd13529558f42 User life cycle: change 
user-del flags to be CLI-specific
* baca55c665b2bdfa5cb9a6ad88daeccef0500999 webui: adjust user deleter 
dialog to new api

-- 
Petr Vobornik