[Freeipa-devel] [PATCH] 0003-3 User life cycle: new stageuser plugin with add verb
thierry bordaz
tbordaz at redhat.com
Thu Mar 19 12:07:52 UTC 2015
On 03/19/2015 07:37 AM, Jan Cholasta wrote:
> 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
>>
>
Hello Honza,
Thanks for your patience .. :-)
I understand my mistake. Just a question, in a plugin command
(user_add), is 'self.obj' referring to the plugin object (like 'user') ?
updated patches (with the appropriate naming and patch versioning).
thanks
theirry
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150319/9b392fda/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbordaz-0002-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/20150319/9b392fda/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbordaz-0003-6-User-life-cycle-stageuser-add-verb.patch
Type: text/x-patch
Size: 60620 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150319/9b392fda/attachment-0001.bin>
More information about the Freeipa-devel
mailing list