[Freeipa-devel] [PATCH] 0005 User life cycle: del/mod/find/show stageuser commands

[Jan Cholasta]

Hi,

Dne 28.4.2015 v 16:40 thierry bordaz napsal(a):
> On 04/28/2015 10:40 AM, David Kupka wrote:
>> On 04/28/2015 10:28 AM, thierry bordaz wrote:
>>> On 04/28/2015 10:23 AM, David Kupka wrote:
>>>> On 04/16/2015 01:00 PM, thierry bordaz wrote:
>>>>> Hello,
>>>>>
>>>>>     Here is the next patch for User life cycle that introduces
>>>>>     del/mod/find and show stageuser plugin commands.
>>>>>
>>>>>   * 0000-User Life Cycle (create containers and scoping  DS plugins):
>>>>>     *pushed*
>>>>>   * 0001-User-Life-Cycle-Exclude-subtree-for-ipaUniqueID-gene.patch:
>>>>>     *pushed*
>>>>>   * 0002-User-life-cycle-stageuser-add-verb.patch: *pushed*
>>>>>   * 0007-User-life-cycle-allows-MODRDN-from-ldap2.patch: *pushed*
>>>>>   * 0003-User-life-cycle-new-stageuser-commands-del-mod-find-*under
>>>>>     review *(this one)**
>>>>>   * 0004-User-life-cycle-new-stageuser-commands-activate.patch
>>>>>   * 0005-User-life-cycle-new-stageuser-commands-activate-prov.patch
>>>>>   * 0006-User-life-cycle-user-del-supports-permanently-preser.patch
>>>>>   * 0008-User-life-cycle-user-find-support-finding-delete-use.patch
>>>>>   * 0009-User-life-cycle-support-of-user-undel.patch
>>>>>   * 0010-User-life-cycle-DNA-DS-plugin-should-exclude-provisi.patch
>>>>>   * 0011-User-life-cycle-lockout-provisioning-stage-and-delet.patch
>>>>>   * 0012-User-life-cycle-Create-stage-Admin-provisioning-acco.patch
>>>>>   * 0013-User-life-cycle-Stage-Admin-permission-priviledge.patch
>>>>>
>>>>> Thanks
>>>>> thierry
>>>>>
>>>>>
>>>>>
>>>>>
>>>> Hi Thierry,
>>>> thanks for the patch, the code looks good to me but there is probably
>>>> a bug in ACIs.
>>>> After creating a stage user and setting password for him I can kinit
>>>> as the stage user. I'm unable to login to the IPA client and id
>>>> command for this stage user responds "no such user" but I can kinit
>>>> and invoke ipa commands.
>>>>
>>>> Steps:
>>>> 0. build freeipa with your patch
>>>> 1. # ipa-server-install
>>>> 2. $ kinit admin
>>>> 3. $ ipa stageuser-add suser0 --first Stage --last User --password
>>>> 4. $ kdestroy
>>>> 5. $ kinit suser0
>>>> 6. $ ipa user-find
>>>>
>>>> Actual:
>>>> Prints out list of ipa users.
>>>>
>>>> Expected:
>>>> kinit fails with "suser0 at ... not found in Kerberos database"
>>>>
>>> Hi David,
>>>
>>> Thank you so much for having looked at this patch :-)
>>> You are right. The Staging users (as well as the Delete users) are not
>>> lockout in that patch.
>>> The patch
>>> 0011-User-life-cycle-lockout-provisioning-stage-and-delet.patch will
>>> take care of this.
>>>
>>> Do you prefer that I merged the two patches right now ?
>>>
>>> thanks
>>> thierry
>>>
>>
>> Hi Thierry,
>> no, it is not necessary to merge the patches it's ok to have it
>> separated. I'm not sure if the patch should be pushed now or rather
>> wait and push it together with the others.
>> I'm looking forward to next ULC patches from you.
>>
>
>
> Hi David,
>
> Here are all the available patches.
> I also attach a test script that is a kind of regression tests that I am
> using.
>
> Thanks again
> thierry
>
>

Some issues I have found during a quick read of the patches:


Patch 0005:

1) This does nothing and can be safely removed:

+    def pre_callback(self, ldap, dn, *keys, **options):
+        assert isinstance(dn, DN)
+        return dn


2) stageuser_{mod,find,show} have a lot of code that seems to be 
copy-pasted from user_{mod,find,show}. I would prefer if the code was 
shared instead of copy-pasted, preferably in baseuser_{mod,find,show}.


Patch 0006:

1) These do nothing and can be safely removed:

+    def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, 
**options):
+        assert isinstance(dn, DN)
+        return dn
+
+    def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
+        assert isinstance(dn, DN)
+        return dn


2) You should use output.standard_entry for has_output, as you are only 
returning one entry. Or you could add support for activating multiple 
users at once.


3) IMO the "time to build the new entry" and "add the active entry" 
parts should be done by calling user-add, so that the (active) user 
creation routine is the same.


4) Please use a single line to separate method definitions in classes.


Patch 008:

1) I guess you forgot to remove these:

+        self.log.error("====> user-add pre_callback 1 %s " % dn)

+        self.log.error("====> user-add pre_callback %s " % dn)


2)

+    takes_options = LDAPDelete.takes_options + (

This should be "baseuser_del.takes_options + ...".


Honza

-- 
Jan Cholasta