Dne 20.5.2015 v 11:26 Jan Cholasta napsal(a):
Dne 18.5.2015 v 10:33 thierry bordaz napsal(a):
On 05/15/2015 04:44 PM, David Kupka wrote:
Hello Thierry,
thanks for the patch set. Overall functionality of ULC feature looks
good to
me and is definitely "alpha ready".

I found following issues but don't insist on fixing it right now:

1) When stageuser-activate fails due to already existent
active/deleted user.
DN is show instead of user name that's used in other commands (user-add,
stageuser-add).
$ ipa user-add tuser --first Test --last User
$ ipa stageuser-add tuser --first Test --last User
$ ipa stageuser-activate tuser
ipa: ERROR: Active user
uid=tuser,cn=users,cn=accounts,dc=abc,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com


already exists

Hi David, Jan,

Thanks you so much for all those tests and feedback. I agree, some minor
bugs can be fixed separatly from this main patches.

You are right, It should return the user ID not the DN.


2) According to the design there should be '--only-delete' and
'--also-delete'
options for user-find command instead there is '--preserved' option.
Honza proposed adding virtual boolean attribute 'deleted' to user
entry and
filter on it.
The 'deleted' attribute would be useful also in user-show where is no
way to
tell if the displayed user is active or deleted. (Except running with
--all
and looking on the dn).

Yes a bit late to resynch the design.
The final option is 'preserved' for user-find and 'preserve' for
user-del. '--only-delete' or 'also-delete' are old name that I need to
replace in the design.

About the 'deleted' attribute, do you think adding a DS cos virtual
attribute ?

See the attached patch.



3) uidNumber and gidNumber can't be set back to '-1' once set to other
value.
This would be useful when admin changes its mind and want IPA to
assign them.
IIUC, there should be no validation in cn=staged user container. All
validation should be done during stageuser-activate.

Yes that comes from user plugin that enforce the number to be >0.
That is a good point giving the ability to reset uidNumber/gidNumber.
I will check if it is possible, how (give a value or an option to
reset), and also if it would not create other issue.

4) Support for deleted -> stage workflow is still missing. But I'm
unsure if we
agreed to finish it now or later.

Yes thanks

5) Twice deleting user with '--preserve' deletes him permanently.
$ ipa user-add tuser --first Test --last User
$ ipa user-del tuser --preserve
$ ipa user-del tuser --preserve
$ ipa user-find --preserved
------------------------
0 (delete) users matched
------------------------
----------------------------
Number of entries returned 0
----------------------------

Deleting a deleted (preserved) entry, should permanently remove the
entry.
Now if the second time the preserve option is present, it makes sense to
not delete it.

BTW: I might be stating the obvious here, but it would be better to use
one boolean parameter rather than two mutually exclusive flags in user-del.



thanks
theirry


Overall, LGTM,

Just 2 nitpicks:
1) preserved attribute label: 'Preserved deleted user' -> 'Preserved user'
2) 'preserved' attribute should be shown in user-{find,show} when '--all' is specified

Updated patch attached.

--
David Kupka
From 7c6e3869ceb64177169b360b21b0af5d73e0405c Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Wed, 20 May 2015 08:12:07 +0000
Subject: [PATCH] User life cycle: provide preserved user virtual attribute

https://fedorahosted.org/freeipa/ticket/3813
---
 API.txt                |  2 +-
 VERSION                |  4 +--
 ipalib/plugins/user.py | 78 +++++++++++++++++++++++++++++++-------------------
 3 files changed, 51 insertions(+), 33 deletions(-)

diff --git a/API.txt b/API.txt
index 9e3f223b7ac338840d7090299f9108e951ea920a..b892eff8bf95c79b7ffdb98738710a7c54000f93 100644
--- a/API.txt
+++ b/API.txt
@@ -5023,7 +5023,7 @@ option: Str('pager', attribute=True, autofill=False, cli_name='pager', multivalu
 option: Flag('pkey_only?', autofill=True, default=False)
 option: Str('postalcode', attribute=True, autofill=False, cli_name='postalcode', multivalue=False, query=True, required=False)
 option: Str('preferredlanguage', attribute=True, autofill=False, cli_name='preferredlanguage', multivalue=False, pattern='^(([a-zA-Z]{1,8}(-[a-zA-Z]{1,8})?(;q\\=((0(\\.[0-9]{0,3})?)|(1(\\.0{0,3})?)))?(\\s*,\\s*[a-zA-Z]{1,8}(-[a-zA-Z]{1,8})?(;q\\=((0(\\.[0-9]{0,3})?)|(1(\\.0{0,3})?)))?)*)|(\\*))$', query=True, required=False)
-option: Flag('preserved?', autofill=True, cli_name='preserved', default=False)
+option: Bool('preserved', attribute=False, autofill=False, cli_name='preserved', default=False, multivalue=False, query=True, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
 option: Int('sizelimit?', autofill=False, minvalue=0)
 option: Str('sn', attribute=True, autofill=False, cli_name='last', multivalue=False, query=True, required=False)
diff --git a/VERSION b/VERSION
index 535b3e228a3500f2013ea793b19a97d9fbd05021..b7321779dcaec4beb5cc172980137ed835d37ace 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
 #                                                      #
 ########################################################
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=126
-# Last change: edewata - added vault-archive and vault-retrieve
+IPA_API_VERSION_MINOR=127
+# Last change: jcholast - User life cycle: provide preserved user virtual attribute
diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index 119294b19f54a395a2df65c6cfd47cd8eb844297..f4a8a2c3c1013d1ecf716c20f4b0a92283567855 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -333,6 +333,11 @@ class user(baseuser):
             label=_('Account disabled'),
             flags=['no_option'],
         ),
+        Bool('preserved?',
+            label=_('Preserved user'),
+            default=False,
+            flags=['virtual_attribute', 'no_create', 'no_update'],
+        ),
     )
 
     def get_dn(self, *keys, **options):
@@ -369,6 +374,15 @@ class user(baseuser):
         """
         return super(user, self).normalize_manager(manager, self.active_container_dn)
 
+    def get_preserved_attribute(self, entry, options):
+        if options.get('raw', False):
+            return
+        delete_container_dn = DN(self.delete_container_dn, api.env.basedn)
+        if entry.dn.endswith(delete_container_dn):
+            entry['preserved'] = True
+        elif options.get('all', False):
+            entry['preserved'] = False
+
 
 @register()
 class user_add(baseuser_add):
@@ -540,6 +554,7 @@ class user_add(baseuser_add):
         self.obj.get_password_attributes(ldap, dn, entry_attrs)
         convert_sshpubkey_post(ldap, dn, entry_attrs)
         radius_dn2pk(self.api, entry_attrs)
+        self.obj.get_preserved_attribute(entry_attrs, options)
         return dn
 
 
@@ -665,6 +680,7 @@ class user_mod(baseuser_mod):
 
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         self.post_common_callback(ldap, dn, entry_attrs, **options)
+        self.obj.get_preserved_attribute(entry_attrs, options)
         return dn
 
 
@@ -675,57 +691,57 @@ class user_find(baseuser_find):
     member_attributes = ['memberof']
     has_output_params = baseuser_find.has_output_params + user_output_params
 
+    msg_summary = ngettext(
+        '%(count)d user matched', '%(count)d users matched', 0
+    )
+
     takes_options = LDAPSearch.takes_options + (
         Flag('whoami',
             label=_('Self'),
             doc=_('Display user record for current Kerberos principal'),
         ),
-        Flag('preserved?',
-            doc=_('Display preserved deleted user'),
-            cli_name='preserved',
-            default=False,
-        ),
     )
 
-    def execute(self, *args, **options):
-        if self.original_msg_summary:
-            object.__setattr__(self, 'msg_summary', self.original_msg_summary)
-        newoptions = {}
-        self.common_enhance_options(newoptions, **options)
-        options.update(newoptions)
-
-        for arg in args:
-            self.log.debug("user-find- exec arg %r" % (arg))
-        if options['preserved']:
-            self.obj.container_dn = baseuser.delete_container_dn
-            self.msg_summary = ngettext('%(count)d (delete) user matched', '%(count)d (delete) users matched', 0)
-
-            ret = super(user_find, self).execute(self, *args, **options)
-
-            self.obj.container_dn = baseuser.active_container_dn
-            return ret
-        else:
-            return super(user_find, self).execute(self, *args, **options)
-
     def pre_callback(self, ldap, filter, attrs_list, base_dn, scope, *keys, **options):
         assert isinstance(base_dn, DN)
+
         if options.get('whoami'):
             return ("(&(objectclass=posixaccount)(krbprincipalname=%s))"%\
                         getattr(context, 'principal'), base_dn, scope)
 
+        newoptions = {}
+        self.common_enhance_options(newoptions, **options)
+        options.update(newoptions)
+
+        preserved = options.get('preserved', False)
+        if preserved is None:
+            base_dn = self.api.env.basedn
+            scope = ldap.SCOPE_SUBTREE
+        elif preserved:
+            base_dn = DN(self.obj.delete_container_dn, self.api.env.basedn)
+        else:
+            base_dn = DN(self.obj.active_container_dn, self.api.env.basedn)
+
         return (filter, base_dn, scope)
 
     def post_callback(self, ldap, entries, truncated, *args, **options):
         if options.get('pkey_only', False):
             return truncated
+
+        if options.get('preserved', False) is None:
+            base_dns = (
+                DN(self.obj.active_container_dn, self.api.env.basedn),
+                DN(self.obj.delete_container_dn, self.api.env.basedn),
+            )
+            entries[:] = [e for e in entries
+                          if any(e.dn.endswith(bd) for bd in base_dns)]
+
         self.post_common_callback(ldap, entries, lockout=False, **options)
+        for entry in entries:
+            self.obj.get_preserved_attribute(entry, options)
+
         return truncated
 
-    msg_summary = ngettext(
-        '%(count)d user matched', '%(count)d users matched', 0
-    )
-    original_msg_summary = msg_summary
-
 
 @register()
 class user_show(baseuser_show):
@@ -736,6 +752,7 @@ class user_show(baseuser_show):
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         convert_nsaccountlock(entry_attrs)
         self.post_common_callback(ldap, dn, entry_attrs, **options)
+        self.obj.get_preserved_attribute(entry_attrs, options)
         return dn
 
 @register()
@@ -944,6 +961,7 @@ class user_status(LDAPQuery):
                 convert_nsaccountlock(entry)
                 if 'nsaccountlock' in entry:
                     disabled = entry['nsaccountlock']
+                self.obj.get_preserved_attribute(entry, options)
                 entries.append(newresult)
                 count += 1
             except errors.NotFound:
-- 
2.4.2

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