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@... 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

--
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

Reply via email to