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@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@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@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 -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code