[Freeipa-devel] [PATCH] 0003-3 User life cycle: new stageuser plugin with add verb
Jan Cholasta
jcholast at redhat.com
Wed Apr 8 06:11:46 UTC 2015
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.
--
Jan Cholasta
More information about the Freeipa-devel
mailing list