[Freeipa-devel] [PATCH] 0003-3 User life cycle: new stageuser plugin with add verb
Jan Cholasta
jcholast at redhat.com
Thu Mar 19 06:37:11 UTC 2015
Dne 18.3.2015 v 19:39 thierry bordaz napsal(a):
> 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
Please also update the comment below (e.g. "# Last change: tbordaz - Add
stageuser_add command")
> * create the baseuser_* plugins commands and use them in the
> user/stageuser plugin commands
> * Do not redefine the class properties in the subclasses.
There are still some in baseuser command classes:
+class baseuser_add(LDAPCreate):
+ """
+ Prototype command plugin to be implemented by real plugin
+ """
+ active_container_dn = api.env.container_user
+ has_output_params = LDAPCreate.has_output_params
You don't need to set active_container_dn here, you only need to set it
in baseuser. Then in stageuser_add and other subclasses you use
"self.obj.active_container_dn" instead of "self.active_container_dn".
You also don't need to override has_output_params if you are not
changing its value - you are inheriting from LDAPCreate, so
baseuser_add.has_output_params implicitly has the same value as
LDAPCreate.has_output_params.
>
> Thanks
> thierry
>
--
Jan Cholasta
More information about the Freeipa-devel
mailing list