On 21.03.2016 07:48, Jan Cholasta wrote:
On 18.3.2016 13:24, Martin Babinsky wrote:
On 03/15/2016 01:48 PM, Martin Basti wrote:
https://fedorahosted.org/freeipa/ticket/3376

Patch attached.


ACK but fix the 'behavioar' typo in the commit message before pushing.

1) You are breaking the default_attributes contract by declaring ipasshpubkey as default attribute and then removing it from the result. This is a hack, which makes the code less readable ("If ipasshpubkey is in default_attributes, why am I not getting it in the result?") and requires every developer to remember to remove ipasshpubkey themselves if they are using user/host objects in their code and want the same behavior as user/host commands. Please keep the change isolated in the relevant commands.

2) Don't add ipasshpubkey to search_attributes, we don't want user-find and friends to search inside ipasshpubkey.

Updated patch attached.
From 099ed81796047427c5790fff8ef03c963c38759e Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Mon, 14 Mar 2016 17:42:56 +0100
Subject: [PATCH] Do not do extra search for ipasshpubkey to generate
 fingerprints

Host, user and idview commands do unnnecessary extra search for
ipasshpubkey attribute to generate fingerprints.

Note: Host and user plugins shows ipasshpubkey only when the attribute
is changed, idviews show ipasshpubkey always. This behavioar has been
kept by this commit.

common_pre/post_callbacks were fixed in [base|stage]user modules.
common_callbacks requires the same arguments as pre/post_callbacks now
(except baseuser_find.post_common_callback)

Note2: in *-add commands there is no need for managing ipasshpubkey as
this attribute should be shown always there.

https://fedorahosted.org/freeipa/ticket/3376
---
 ipalib/plugins/baseuser.py  | 43 ++++++++++++++++++++++++++++++++-----------
 ipalib/plugins/host.py      | 27 ++++++++++++++++++++++-----
 ipalib/plugins/idviews.py   |  8 ++++----
 ipalib/plugins/stageuser.py | 17 +++++++++++++----
 ipalib/plugins/user.py      | 15 ++++++++++-----
 ipalib/util.py              | 40 ++++++++++++++++++++++++++++++++++------
 6 files changed, 115 insertions(+), 35 deletions(-)

diff --git a/ipalib/plugins/baseuser.py b/ipalib/plugins/baseuser.py
index 9c78a521dcb9a7a7db0be695468c85735d80620c..252d40ae3828417d9692510d5036aaadaeb9edce 100644
--- a/ipalib/plugins/baseuser.py
+++ b/ipalib/plugins/baseuser.py
@@ -32,8 +32,14 @@ from ipalib.request import context
 from ipalib import _
 from ipapython.ipautil import ipa_generate_password
 from ipapython.ipavalidate import Email
-from ipalib.util import (normalize_sshpubkey, validate_sshpubkey,
-    convert_sshpubkey_post)
+from ipalib.util import (
+    normalize_sshpubkey,
+    validate_sshpubkey,
+    convert_sshpubkey_post,
+    remove_sshpubkey_from_output_post,
+    remove_sshpubkey_from_output_list_post,
+    add_sshpubkey_to_attrs_pre,
+)
 
 if six.PY3:
     unicode = str
@@ -490,15 +496,16 @@ class baseuser_add(LDAPCreate):
     """
     Prototype command plugin to be implemented by real plugin
     """
-    def pre_common_callback(self, ldap, dn, entry_attrs, **options):
+    def pre_common_callback(self, ldap, dn, entry_attrs, attrs_list, *keys,
+                            **options):
         assert isinstance(dn, DN)
         self.obj.convert_usercertificate_pre(entry_attrs)
 
-    def post_common_callback(self, ldap, dn, entry_attrs, **options):
+    def post_common_callback(self, ldap, dn, entry_attrs, *keys, **options):
         assert isinstance(dn, DN)
         self.obj.convert_usercertificate_post(entry_attrs, **options)
         self.obj.get_password_attributes(ldap, dn, entry_attrs)
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
         radius_dn2pk(self.api, entry_attrs)
 
 class baseuser_del(LDAPDelete):
@@ -565,8 +572,11 @@ class baseuser_mod(LDAPUpdate):
                     answer = self.api.Object['radiusproxy'].get_dn_if_exists(cl)
                     entry_attrs['ipatokenradiusconfiglink'] = answer
 
-    def pre_common_callback(self, ldap, dn, entry_attrs, **options):
+    def pre_common_callback(self, ldap, dn, entry_attrs, attrs_list, *keys,
+                            **options):
         assert isinstance(dn, DN)
+        add_sshpubkey_to_attrs_pre(self.context, attrs_list)
+
         self.check_namelength(ldap, **options)
 
         self.check_mail(entry_attrs)
@@ -578,7 +588,7 @@ class baseuser_mod(LDAPUpdate):
         self.check_objectclass(ldap, dn, entry_attrs)
         self.obj.convert_usercertificate_pre(entry_attrs)
 
-    def post_common_callback(self, ldap, dn, entry_attrs, **options):
+    def post_common_callback(self, ldap, dn, entry_attrs, *keys, **options):
         assert isinstance(dn, DN)
         if options.get('random', False):
             try:
@@ -589,7 +599,8 @@ class baseuser_mod(LDAPUpdate):
         convert_nsaccountlock(entry_attrs)
         self.obj.get_password_attributes(ldap, dn, entry_attrs)
         self.obj.convert_usercertificate_post(entry_attrs, **options)
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
+        remove_sshpubkey_from_output_post(self.context, entry_attrs)
         radius_dn2pk(self.api, entry_attrs)
 
 class baseuser_find(LDAPSearch):
@@ -615,6 +626,10 @@ class baseuser_find(LDAPSearch):
         if cl in options:
             newoptions[cl] = self.api.Object['radiusproxy'].get_dn(options[cl])
 
+    def pre_common_callback(self, ldap, filters, attrs_list, base_dn, scope,
+                            *args, **options):
+        add_sshpubkey_to_attrs_pre(self.context, attrs_list)
+
     def post_common_callback(self, ldap, entries, lockout=False, **options):
         for attrs in entries:
             self.obj.get_password_attributes(ldap, attrs.dn, attrs)
@@ -623,17 +638,23 @@ class baseuser_find(LDAPSearch):
                 attrs['nsaccountlock'] = True
             else:
                 convert_nsaccountlock(attrs)
-            convert_sshpubkey_post(ldap, attrs.dn, attrs)
+            convert_sshpubkey_post(attrs)
+        remove_sshpubkey_from_output_list_post(self.context, entries)
 
 class baseuser_show(LDAPRetrieve):
     """
     Prototype command plugin to be implemented by real plugin
     """
-    def post_common_callback(self, ldap, dn, entry_attrs, **options):
+    def pre_common_callback(self, ldap, dn, attrs_list, *keys, **options):
+        assert isinstance(dn, DN)
+        add_sshpubkey_to_attrs_pre(self.context, attrs_list)
+
+    def post_common_callback(self, ldap, dn, entry_attrs, *keys, **options):
         assert isinstance(dn, DN)
         self.obj.get_password_attributes(ldap, dn, entry_attrs)
         self.obj.convert_usercertificate_post(entry_attrs, **options)
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
+        remove_sshpubkey_from_output_post(self.context, entry_attrs)
         radius_dn2pk(self.api, entry_attrs)
 
 
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 04bb2991a1d463e17b1ed11e06b35dc3a9829073..19327d8323d945062e06ccdb33ea2106cd1c6a00 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -44,7 +44,10 @@ from ipalib import x509
 from ipalib import output
 from ipalib.request import context
 from ipalib.util import (normalize_sshpubkey, validate_sshpubkey_no_options,
-    convert_sshpubkey_post, validate_hostname)
+    convert_sshpubkey_post, validate_hostname,
+    add_sshpubkey_to_attrs_pre,
+    remove_sshpubkey_from_output_post,
+    remove_sshpubkey_from_output_list_post)
 from ipapython.ipautil import ipa_generate_password, CheckedIPAddress
 from ipapython.dnsutil import DNSName
 from ipapython.ssh import SSHPublicKey
@@ -712,7 +715,7 @@ class host_add(LDAPCreate):
             # fetched anywhere.
             entry_attrs['has_keytab'] = False
 
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
 
         return dn
 
@@ -927,6 +930,8 @@ class host_mod(LDAPUpdate):
             if 'krbticketpolicyaux' not in entry_attrs['objectclass']:
                 entry_attrs['objectclass'].append('krbticketpolicyaux')
 
+        add_sshpubkey_to_attrs_pre(self.context, attrs_list)
+
         return dn
 
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
@@ -947,7 +952,8 @@ class host_mod(LDAPUpdate):
 
         self.obj.suppress_netgroup_memberof(ldap, entry_attrs)
 
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
+        remove_sshpubkey_from_output_post(self.context, entry_attrs)
         convert_ipaassignedidview_post(entry_attrs, options)
 
         return dn
@@ -1015,6 +1021,8 @@ class host_find(LDAPSearch):
                         (filter, hosts_filter), ldap.MATCH_ALL
                     )
 
+        add_sshpubkey_to_attrs_pre(self.context, attrs_list)
+
         return (filter.replace('locality', 'l'), base_dn, scope)
 
     def post_callback(self, ldap, entries, truncated, *args, **options):
@@ -1034,9 +1042,12 @@ class host_find(LDAPSearch):
             if options.get('all', False):
                 entry_attrs['managing'] = self.obj.get_managed_hosts(entry_attrs.dn)
 
-            convert_sshpubkey_post(ldap, entry_attrs.dn, entry_attrs)
+            convert_sshpubkey_post(entry_attrs)
+            remove_sshpubkey_from_output_post(self.context, entry_attrs)
             convert_ipaassignedidview_post(entry_attrs, options)
 
+        remove_sshpubkey_from_output_list_post(self.context, entries)
+
         return truncated
 
 
@@ -1053,6 +1064,11 @@ class host_show(LDAPRetrieve):
 
     member_attributes = ['managedby']
 
+    def pre_callback(self, ldap, dn, attrs_list, *keys, **options):
+        assert isinstance(dn, DN)
+        add_sshpubkey_to_attrs_pre(self.context, attrs_list)
+        return dn
+
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         assert isinstance(dn, DN)
         self.obj.get_password_attributes(ldap, dn, entry_attrs)
@@ -1070,7 +1086,8 @@ class host_show(LDAPRetrieve):
 
         self.obj.suppress_netgroup_memberof(ldap, entry_attrs)
 
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
+        remove_sshpubkey_from_output_post(self.context, entry_attrs)
         convert_ipaassignedidview_post(entry_attrs, options)
 
         return dn
diff --git a/ipalib/plugins/idviews.py b/ipalib/plugins/idviews.py
index 6f8bdc7a8f5f50e82d77aa6696092ce6b43aeb9d..bfbec56457bc0122fbb223fe26f5cf09708bdd3e 100644
--- a/ipalib/plugins/idviews.py
+++ b/ipalib/plugins/idviews.py
@@ -954,7 +954,7 @@ class idoverrideuser_add(baseidoverride_add):
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         dn = super(idoverrideuser_add, self).post_callback(ldap, dn,
                  entry_attrs, *keys, **options)
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
         return dn
 
 
@@ -990,7 +990,7 @@ class idoverrideuser_mod(baseidoverride_mod):
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         dn = super(idoverrideuser_mod, self).post_callback(ldap, dn,
                  entry_attrs, *keys, **options)
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
         return dn
 
 
@@ -1004,7 +1004,7 @@ class idoverrideuser_find(baseidoverride_find):
         truncated = super(idoverrideuser_find, self).post_callback(
             ldap, entries, truncated, *args, **options)
         for entry in entries:
-            convert_sshpubkey_post(ldap, entry.dn, entry)
+            convert_sshpubkey_post(entry)
         return truncated
 
 
@@ -1015,7 +1015,7 @@ class idoverrideuser_show(baseidoverride_show):
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         dn = super(idoverrideuser_show, self).post_callback(ldap, dn,
                  entry_attrs, *keys, **options)
-        convert_sshpubkey_post(ldap, dn, entry_attrs)
+        convert_sshpubkey_post(entry_attrs)
         return dn
 
 
diff --git a/ipalib/plugins/stageuser.py b/ipalib/plugins/stageuser.py
index d34d6c0218e3b49614a67e3bfab72c77e962552b..c147f63c467c7ceeec171e9931cc3a271bcc87fb 100644
--- a/ipalib/plugins/stageuser.py
+++ b/ipalib/plugins/stageuser.py
@@ -374,7 +374,8 @@ class stageuser_add(baseuser_add):
                 answer = self.api.Object['radiusproxy'].get_dn_if_exists(cl)
                 entry_attrs['ipatokenradiusconfiglink'] = answer
 
-        self.pre_common_callback(ldap, dn, entry_attrs, **options)
+        self.pre_common_callback(ldap, dn, entry_attrs, attrs_list, *keys,
+                                 **options)
 
         return dn
 
@@ -394,7 +395,7 @@ class stageuser_add(baseuser_add):
                 # if both randompassword and userpassword options were used
                 pass
 
-        self.post_common_callback(ldap, dn, entry_attrs, **options)
+        self.post_common_callback(ldap, dn, entry_attrs, *keys, **options)
         return dn
 
 @register()
@@ -412,7 +413,8 @@ class stageuser_mod(baseuser_mod):
     has_output_params = baseuser_mod.has_output_params + stageuser_output_params
 
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
-        self.pre_common_callback(ldap, dn, entry_attrs, **options)
+        self.pre_common_callback(ldap, dn, entry_attrs, attrs_list, *keys,
+                                 **options)
         # Make sure it is not possible to authenticate with a Stage user account
         if 'nsaccountlock' in entry_attrs:
             del entry_attrs['nsaccountlock']
@@ -433,6 +435,8 @@ class stageuser_find(baseuser_find):
 
     def pre_callback(self, ldap, filter, attrs_list, base_dn, scope, *keys, **options):
         assert isinstance(base_dn, DN)
+        self.pre_common_callback(ldap, filter, attrs_list, base_dn, scope,
+                                 *keys, **options)
 
         container_filter = "(objectclass=posixaccount)"
         # provisioning system can create non posixaccount stage user
@@ -458,9 +462,14 @@ class stageuser_show(baseuser_show):
 
     has_output_params = baseuser_show.has_output_params + stageuser_output_params
 
+    def pre_callback(self, ldap, dn, attrs_list, *keys, **options):
+        assert isinstance(dn, DN)
+        self.pre_common_callback(ldap, dn, attrs_list, *keys, **options)
+        return dn
+
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         entry_attrs['nsaccountlock'] = True
-        self.post_common_callback(ldap, dn, entry_attrs, **options)
+        self.post_common_callback(ldap, dn, entry_attrs, *keys, **options)
         return dn
 
 
diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index 102f821ceefac854c896d2a28d1cb7a4246660d2..adcee2c5ba683a0dc98e1e5df98ed510c1d1b685 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -542,7 +542,8 @@ class user_add(baseuser_add):
             answer = self.api.Object['radiusproxy'].get_dn_if_exists(rcl)
             entry_attrs['ipatokenradiusconfiglink'] = answer
 
-        self.pre_common_callback(ldap, dn, entry_attrs, **options)
+        self.pre_common_callback(ldap, dn, entry_attrs, attrs_list, *keys,
+                                 **options)
 
         return dn
 
@@ -588,7 +589,7 @@ class user_add(baseuser_add):
 
         self.obj.get_preserved_attribute(entry_attrs, options)
 
-        self.post_common_callback(ldap, dn, entry_attrs, **options)
+        self.post_common_callback(ldap, dn, entry_attrs, *keys, **options)
 
         return dn
 
@@ -752,12 +753,13 @@ class user_mod(baseuser_mod):
 
     def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
         dn = self.obj.get_either_dn(*keys, **options)
-        self.pre_common_callback(ldap, dn, entry_attrs, **options)
+        self.pre_common_callback(ldap, dn, entry_attrs, attrs_list, *keys,
+                                 **options)
         validate_nsaccountlock(entry_attrs)
         return dn
 
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
-        self.post_common_callback(ldap, dn, entry_attrs, **options)
+        self.post_common_callback(ldap, dn, entry_attrs, *keys, **options)
         self.obj.get_preserved_attribute(entry_attrs, options)
         return dn
 
@@ -782,6 +784,8 @@ class user_find(baseuser_find):
 
     def pre_callback(self, ldap, filter, attrs_list, base_dn, scope, *keys, **options):
         assert isinstance(base_dn, DN)
+        self.pre_common_callback(ldap, filter, attrs_list, base_dn, scope,
+                                 *keys, **options)
 
         if options.get('whoami'):
             return ("(&(objectclass=posixaccount)(krbprincipalname=%s))"%\
@@ -830,11 +834,12 @@ class user_show(baseuser_show):
 
     def pre_callback(self, ldap, dn, attrs_list, *keys, **options):
         dn = self.obj.get_either_dn(*keys, **options)
+        self.pre_common_callback(ldap, dn, attrs_list, *keys, **options)
         return dn
 
     def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
         convert_nsaccountlock(entry_attrs)
-        self.post_common_callback(ldap, dn, entry_attrs, **options)
+        self.post_common_callback(ldap, dn, entry_attrs, *keys, **options)
         self.obj.get_preserved_attribute(entry_attrs, options)
         return dn
 
diff --git a/ipalib/util.py b/ipalib/util.py
index 262acf926e73ba1521faa151154e2149875be4b7..709bfbb8ef90924768823ed4316aacb9a0b9d79b 100644
--- a/ipalib/util.py
+++ b/ipalib/util.py
@@ -290,12 +290,9 @@ def validate_sshpubkey_no_options(ugettext, value):
     if pubkey.has_options():
         return _('options are not allowed')
 
-def convert_sshpubkey_post(ldap, dn, entry_attrs):
-    if 'ipasshpubkey' in entry_attrs:
-        pubkeys = entry_attrs['ipasshpubkey']
-    else:
-        old_entry_attrs = ldap.get_entry(dn, ['ipasshpubkey'])
-        pubkeys = old_entry_attrs.get('ipasshpubkey')
+
+def convert_sshpubkey_post(entry_attrs):
+    pubkeys = entry_attrs.get('ipasshpubkey')
     if not pubkeys:
         return
 
@@ -321,6 +318,37 @@ def convert_sshpubkey_post(ldap, dn, entry_attrs):
     if fingerprints:
         entry_attrs['sshpubkeyfp'] = fingerprints
 
+
+def add_sshpubkey_to_attrs_pre(context, attrs_list):
+    """
+    Attribute ipasshpubkey should be added to attrs_list to be able compute
+    ssh fingerprint. This attribute must be removed later if was added here
+    (see remove_sshpubkey_from_output_post).
+    """
+    if not ('ipasshpubkey' in attrs_list or '*' in attrs_list):
+        setattr(context, 'ipasshpubkey_added', True)
+        attrs_list.append('ipasshpubkey')
+
+
+def remove_sshpubkey_from_output_post(context, entry_attrs):
+    """
+    Remove ipasshpubkey from output if it was added in pre_callbacks
+    """
+    if getattr(context, 'ipasshpubkey_added', False):
+        entry_attrs.pop('ipasshpubkey', None)
+        delattr(context, 'ipasshpubkey_added')
+
+
+def remove_sshpubkey_from_output_list_post(context, entries):
+    """
+    Remove ipasshpubkey from output if it was added in pre_callbacks
+    """
+    if getattr(context, 'ipasshpubkey_added', False):
+        for entry_attrs in entries:
+            entry_attrs.pop('ipasshpubkey', None)
+        delattr(context, 'ipasshpubkey_added')
+
+
 class cachedproperty(object):
     """
     A property-like attribute that caches the return value of a method call.
-- 
2.5.5

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