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

Wed Mar 18 18:39:48 UTC 2015

On 03/17/2015 08:01 AM, Jan Cholasta wrote:
> Dne 16.3.2015 v 12:06 David Kupka napsal(a):
>> On 03/06/2015 07:30 PM, thierry bordaz wrote:
>>> On 02/19/2015 04:19 PM, Martin Basti wrote:
>>>> On 19/02/15 13:01, thierry bordaz wrote:
>>>>> On 02/04/2015 05:14 PM, Jan Cholasta wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Dne 4.2.2015 v 15:25 David Kupka napsal(a):
>>>>>>> On 02/03/2015 11:50 AM, thierry bordaz wrote:
>>>>>>>> On 09/17/2014 12:32 PM, 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".
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>>> The default permissions need different names, and you don't need
>>>>>>>>>> another copy of the 'non_object' ones. Also, run the makeaci
>>>>>>>>>> script.
>>>>>>>>>>
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>>     This modified patch is mainly moving common base class into a
>>>>>>>>> new
>>>>>>>>>     plugin: accounts.py. user/stageuser plugin inherits from
>>>>>>>>> accounts.
>>>>>>>>>     It also creates a better description of what are stage user,
>>>>>>>>> how
>>>>>>>>>     to add a new stage user, updates ACI.txt and separate
>>>>>>>>> active/stage
>>>>>>>>>     user managed permissions.
>>>>>>>>>
>>>>>>>>> thanks
>>>>>>>>> thierry
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> Freeipa-devel mailing list
>>>>>>>>> Freeipa-devel at redhat.com
>>>>>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks David for the reviews. Here the last patches
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Freeipa-devel mailing list
>>>>>>>> Freeipa-devel at redhat.com
>>>>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>>>>>>
>>>>>>>
>>>>>>> The freeipa-tbordaz-0002 patch had trailing whitespaces on few
>>>>>>> lines so
>>>>>>> I'm attaching fixed version (and unchanged patch
>>>>>>> freeipa-tbordaz-0003-3
>>>>>>> to keep them together).
>>>>>>>
>>>>>>> The ULC feature is still WIP but these patches look good to me and
>>>>>>> don't
>>>>>>> break anything as far as I tested.
>>>>>>> We should push them now to avoid further rebases. Thierry can then
>>>>>>> prepare other patches delivering the rest of ULC functionality.
>>>>>>
>>>>>> Few comments from just reading the patches:
>>>>>>
>>>>>> 1) I would name the base class "baseuser", "account" does not
>>>>>> necessarily mean user account.
>>>>>>
>>>>>> 2) This is very wrong:
>>>>>>
>>>>>> -class user_add(LDAPCreate):
>>>>>> +class user_add(user, LDAPCreate):
>>>>>>
>>>>>> You are creating a plugin which is both an object and an command.
>>>>>>
>>>>>> 3) This is purely subjective, but I don't like the name
>>>>>> "deleteuser", as it has a verb in it. We usually don't do that and
>>>>>> IMHO we shouldn't do that.
>>>>>>
>>>>>> Honza
>>>>>>
>>>>>
>>>>> Thank you for the review. I am attaching the updates patches
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Freeipa-devel mailing list
>>>>> Freeipa-devel at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>> Hello,
>>>> I'm getting errors during make rpms:
>>>>
>>>> if [ "" != "yes" ]; then \
>>>>     ./makeapi --validate; \
>>>>     ./makeaci --validate; \
>>>> fi
>>>>
>>>> /root/freeipa/ipalib/plugins/baseuser.py:641 command "baseuser_add"
>>>> doc is not internationalized
>>>> /root/freeipa/ipalib/plugins/baseuser.py:653 command "baseuser_find"
>>>> doc is not internationalized
>>>> /root/freeipa/ipalib/plugins/baseuser.py:647 command "baseuser_mod"
>>>> doc is not internationalized
>>>> 0 commands without doc, 3 commands whose doc is not i18n
>>>> Command baseuser_add in ipalib, not in API
>>>> Command baseuser_find in ipalib, not in API
>>>> Command baseuser_mod in ipalib, not in API
>>>>
>>>> There are one or more new commands defined.
>>>> Update API.txt and increment the minor version in VERSION.
>>>>
>>>> There are one or more documentation problems.
>>>> You must fix these before preceeding
>>>>
>>>> Issues probably caused by this:
>>>> 1)
>>>> You should not use the register decorator, if this class is just for
>>>> inheritance
>>>> @register()
>>>> class baseuser_add(LDAPCreate):
>>>>
>>>> @register()
>>>> class baseuser_mod(LDAPUpdate):
>>>>
>>>> @register()
>>>> class baseuser_find(LDAPSearch):
>>>>
>>>> see dns.py plugin and "DNSZoneBase" and "dnszone" classes
>>>>
>>>> 2)
>>>> there might be an issue with
>>>> @register()
>>>> class baseuser(LDAPObject):
>>>>
>>>> the register decorator should not be there, I was warned by Petr^3 to
>>>> not use permission in parent class. The same permission should be
>>>> specified only in one place (for example user class), (otherwise they
>>>> will be generated twice??) I don't know more details about it.
>>>>
>>>> -- 
>>>> Martin Basti
>>>
>>> Hello Martin, Jan,
>>>
>>> Thanks for your review.
>>> I changed the patch so that it does not register baseuser_*. Also
>>> increase the minor version because of new command.
>>> Finally I moved the managed_permission definition out of the parent
>>> baseuser class.
>>>
>>>
>>>
>>>
>>>
>>
>> Martin, could you please verify that the issues you encountered are 
>> fixed?
>>
>> Thanks!
>>
>
> You bumped wrong version variable:
>
> -IPA_VERSION_MINOR=1
> +IPA_VERSION_MINOR=2
>
> It should have been IPA_API_VERSION_MINOR (at the bottom of the file), 
> including the last change comment below it.
>
>
> IMO baseuser should include superclasses for all the usual commands 
> (add, mod, del, show, find) and stageuser/deleteuser commands should 
> inherit from them.
>
>
> You don't need to override class properties like active_container_dn 
> and takes_params on baseuser subclasses when they have the same value 
> as in baseuser.
>
>
> Honza
>
Hello Honza,

    Thanks for the review. I did the modifications you recommended
    within that attached patches

      * Change version
      * create the baseuser_* plugins commands and use them in the
        user/stageuser plugin commands
      * Do not redefine the class properties in the subclasses.

    Thanks
    thierry

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150318/851f8243/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-User-life-cycle-stageuser-add-verb.patch
Type: text/x-patch
Size: 60937 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150318/851f8243/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-User-Life-Cycle-Exclude-subtree-for-ipaUniqueID-gene.patch
Type: text/x-patch
Size: 2588 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150318/851f8243/attachment-0001.bin>