[Freeipa-devel] [PATCH] 0003-3 User life cycle: new stageuser plugin with add verb
Martin Kosek
mkosek at redhat.com
Wed Apr 8 06:20:50 UTC 2015
On 04/08/2015 08:11 AM, Jan Cholasta wrote:
> Dne 25.3.2015 v 10:04 thierry bordaz napsal(a):
>> 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
>
> Thanks, ACK from me.
>
Thanks guys!
Pushed to master: d1691eee88c5462ef1d015617fd5b65eec0319b9
Martin
More information about the Freeipa-devel
mailing list