[Freeipa-devel] [PATCH] 0003-3 User life cycle: new stageuser plugin with add verb

[David Kupka]

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!

-- 
David Kupka