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

Tue Mar 17 07:01:18 UTC 2015

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

-- 
Jan Cholasta