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

[Simo Sorce]

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

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York