[Freeipa-devel] [PATCH] 0003 User life cycle: new stageuser plugin with add verb

[Petr Viktorin]

On 09/02/2014 11:40 AM, thierry bordaz wrote:
> On 09/01/2014 01:08 PM, Petr Viktorin wrote:
>> On 08/08/2014 03:54 PM, thierry bordaz wrote:
>>> Hi,
>>>
>>> The attached patch is related to 'User Life Cycle'
>>> (https://fedorahosted.org/freeipa/ticket/3813)
>>>
>>> It creates a stageuser plugin with a first function stageuser-add. Stage
>>> user entries are provisioned under 'cn=staged
>>> users,cn=accounts,cn=provisioning,SUFFIX'.
>>>
>>> Thanks
>>> thierry
>>
>> Avoid `from ipalib.plugins.baseldap import *` in new code; instead
>> import the module itself and use e.g. `baseldap.LDAPObject`.
>>
>> The stageuser help (docstring) is copied from the user plugin, and
>> discusses things like account lockout and disabling users. It should
>> rather explain what stageuser itself does. (And I don't very much like
>> the Note about the interface being badly designed...)
>> Also decide if the docs should call it "staged user" or "stage user"
>> or "stageuser".
> Hello Petr,
>
>     Thanks for your review.
>
>     I will rewrite the docstring to be only staged user related and I
>     will adopt 'stage user' and 'stageuser'.
>     About the interface, that is correct that I was not able to find a
>     good solution.
>     I need to add a 'stageuser' I use 'stageuser-add' and  '--first' and
>     '--last' are required.
>     Now when a user got deleted ('user-del') and is later move to 'stage
>     user' it also use the command line 'stageuser-add'.
>     At this time the delete user is know/found by it 'user login'/uid.
>     So '--first' and '--last' are not required to find it (can be used
>     to check givenname/sn).
>     Now I do not expect that 'stageuser-add' will be frequently used to
>     move a Delete user to Stage user, so it is not a so painful constraint.

Right, it's probably not worth worrying about. But you could make them 
optional formally, and validate them in the pre_callback.

>> A lot of the code is copied and pasted over from the users plugin.
>> Don't do that. Either import things (e.g. validate_nsaccountlock) from
>> the users plugin, or move the reused code into a shared module.
>>
>> For the `user` object, since so much is the same, it might be best to
>> create a common base class for user and stageuser; and similarly for
>> the Command plugins.
>
>     I agree that user and stageuser have a lot of code in common. So it
>     would be beneficial to have a common base class. Is it ok to put
>     this in a file under freeipa/ipalib ? About the name of this class,
>     what about 'accounts' or 'user_accounts' ?

Either ipalib/accounts.py or ipalib/plugins/accounts.py is fine. It 
would inherit from code in baseldap, which is in plugins, so putting it 
in plugins would be somewhat more consistent.

>     What do you mean by 'similarly for Command plugins' ?. If I create,

'user' and 'stageuser' are Object plugins, 'user_add' and 
'stageuser_add' are Command plugins.

>     for example, a freeipa/ipalib/accounts.py containing all the common
>     code for 'user' and 'stageuser'. Then import it into
>     freeipa/ipalib/plugins/user and freeipa/ipalib/plugins/stageuser I
>     believe it will refactore the 'Command plugins'.

Yes, that's the idea.

>> The default permissions need different names, and you don't need
>> another copy of the 'non_object' ones. Also, run the makeaci script.
>
> Ok. I will update the names. 'non_object' is not clear to me, what does
> this exactly mean when a managed_permission has 'non_object' True.
> In update_managed_permissions.py it is said that if True, it target a
> defaults value. But on the other side, the managed permissions also
> define 'ipapermtarget' (like ipauser or UPG)

'non_object' means that it doesn't take attributes, such as 
ipapermlocation, from the Object it's defined in. If 'non_object' is 
false (or missing), the defaults are taken from the object but can be 
overridden.


-- 
Petr³