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

Fri Mar 6 18:30:27 UTC 2015

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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150306/b5a1c436/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-User-Life-Cycle-Exclude-subtree-for-ipaUniqueID-gene.patch
Type: text/x-patch
Size: 2585 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150306/b5a1c436/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-User-life-cycle-stageuser-add-verb.patch
Type: text/x-patch
Size: 60478 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150306/b5a1c436/attachment-0001.bin>