[Freeipa-devel] [PATCH] 0003-3 User life cycle: new stageuser plugin with add verb
thierry bordaz
tbordaz at redhat.com
Wed Mar 25 09:04:51 UTC 2015
On 03/24/2015 01:45 PM, Jan Cholasta wrote:
> Dne 19.3.2015 v 13:07 thierry bordaz napsal(a):
>> 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') ?
>
> Yes, that's correct.
>
>>
>> updated patches (with the appropriate naming and patch versioning).
>>
>> thanks
>> theirry
>>
>
> One more thing:
>
> Instead of:
>
> class stageuser(baseuser):
> ...
> # take_params does not support 'nsaccountlock' option
> stageuser_takes_params_list = []
> for elt in baseuser.takes_params:
> if isinstance(elt, Bool) and elt.param_spec == 'nsaccountlock?':
> pass
> else:
> stageuser_takes_params_list.append(elt)
> takes_params = tuple(stageuser_takes_params_list)
>
> I would remove nsaccountlock from baseuser.takes_params and add it in
> user.takes_params:
>
> class user(baseuser):
> ...
> takes_params = baseuser.takes_params + (
> Bool('nsaccountlock?',
> label=_('Account disabled'),
> flags=['no_option'],
> ),
> )
>
>
Right, making this option specific to active user makes sense.
Thanks
thierry
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150325/815e3d44/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/20150325/815e3d44/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbordaz-0003-7-User-life-cycle-stageuser-add-verb.patch
Type: text/x-patch
Size: 60177 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150325/815e3d44/attachment-0001.bin>
More information about the Freeipa-devel
mailing list