[Freeipa-devel] [PATCH] 0003-3 User life cycle: new stageuser plugin with add verb
Jan Cholasta
jcholast at redhat.com
Tue Mar 24 12:45:50 UTC 2015
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'],
),
)
--
Jan Cholasta
More information about the Freeipa-devel
mailing list