On 06/17/2016 09:14 AM, Stanislav Laznicka wrote:
On 06/14/2016 04:40 PM, Jan Cholasta wrote:
On 14.6.2016 16:35, Martin Basti wrote:
On 14.06.2016 16:37, Jan Cholasta wrote:
On 14.6.2016 16:29, Martin Basti wrote:
On 08.06.2016 14:17, Stanislav Laznicka wrote:
On 06/07/2016 10:42 AM, Martin Basti wrote:
On 07.06.2016 10:43, Jan Cholasta wrote:
On 7.6.2016 10:22, Martin Basti wrote:
On 07.06.2016 09:07, Jan Cholasta wrote:
On 6.6.2016 18:29, Martin Basti wrote:
On 03.06.2016 14:28, Stanislav Laznicka wrote:
On 06/03/2016 02:19 PM, Martin Basti wrote:
On 03.06.2016 14:13, Stanislav Laznicka wrote:
https://fedorahosted.org/freeipa/ticket/5892

NACK

please remove it from LDAPAddReverseMember too, it contains the
same
code

Martin^2

Please see the modified patch.

Standa

ACK

Pushed to master: c56d65b064e1e0410c03cf1206816cad4d8d86cc

I think the attrs_list was supposed to be passed to the
ldap.get_entry() call rather than removed, which would fix that
every
reverse member command always acts like --all was specified.

I'm really afraid, what can happen if we put attr_list into
get_entry()
instead of '*', because this code were there for 4 years and I don't
feel happy enough to change it now, what we may break.

Should I revert this commit then and postpone the ticket?

It's a bug and should be fixed. The fix is easy so I see no point in postponing it. I see no reason to be really afraid, I'm pretty sure
that removing the objectclass attribute (which is invisible in the
CLI anyway) from the output of all the 4 commands that use this code
won't break anything.


Ok
It seems that tests expect objectClass to be always returned in
derived methods. Is that expected behavior? If so, please see the
attached patch. I wonder if the keys of the passed options should make
it to attrs_list as well (similarly to LDAPUpdate and LDAPCreate)?

I still don't think that we should use that attrs list, because
according git history, attrs_list was really used only with code that
was removed, and as you can see Standa had add extra objectclass
attribute there

So the assumption here is that if it's in git history, it's not a bug?

The extra object class should *not* be included by default.

(Also I don't see any reason to have this split into 2 patches.)

Right. I added the objectclass so that it behaves similar to what the other commands do and so that it does not break tests but it can be removed and tests could be fixed. The question here is - do we want it in 4.4, then? If so can we be sure it won't break anything (although we know that it's broken as it is, and it's been like that for the past 4 years)?

Currently, the LDAP*ReverseMember are used in adding/removing permissions/privileges to privileges/roles so I think we don't want it to go bad - but could it?

After discussion with Honza we agreed that the patch should not break anything while trying to fix the bug. Attached is the new patch with fixed tests.

From 821960d8e84f2f4e075af93321398ea62af2e206 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slazn...@redhat.com>
Date: Tue, 7 Jun 2016 13:51:46 +0200
Subject: [PATCH] The LDAP*ReverseMember shouldn't imply --all is always
 specified

The LDAP*ReverseMember methods would always return the whole LDAP
object even though --all is not specified.
Also had to fix some tests as objectClass will not be returned by
default now.

https://fedorahosted.org/freeipa/ticket/5892
---
 ipaserver/plugins/baseldap.py                  | 8 ++++++--
 ipatests/test_xmlrpc/test_permission_plugin.py | 4 ----
 ipatests/test_xmlrpc/test_role_plugin.py       | 5 -----
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/ipaserver/plugins/baseldap.py b/ipaserver/plugins/baseldap.py
index 62b726da1a0baef9fc9fa4bb386a2101ca1f10b2..6fc56e0f8218a2ace30bb858b91b365490b5d051 100644
--- a/ipaserver/plugins/baseldap.py
+++ b/ipaserver/plugins/baseldap.py
@@ -2126,6 +2126,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
         result = self.api.Command[self.show_command](keys[-1])['result']
         dn = result['dn']
         assert isinstance(dn, DN)
+        entry_attrs = ldap.make_entry(dn, self.args_options_2_entry(**options))
 
         for callback in self.get_callbacks('pre'):
             dn = callback(self, ldap, dn, *keys, **options)
@@ -2135,6 +2136,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
             attrs_list = ['*'] + self.obj.default_attributes
         else:
             attrs_list = set(self.obj.default_attributes)
+            attrs_list.update(entry_attrs.keys())
             if options.get('no_members', False):
                 attrs_list.difference_update(self.obj.attribute_members)
             attrs_list = list(attrs_list)
@@ -2159,7 +2161,7 @@ class LDAPAddReverseMember(LDAPModReverseMember):
                 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
         # Update the member data.
-        entry_attrs = ldap.get_entry(dn, ['*'])
+        entry_attrs = ldap.get_entry(dn, attrs_list)
         self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
         for callback in self.get_callbacks('post'):
@@ -2225,6 +2227,7 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
         result = self.api.Command[self.show_command](keys[-1])['result']
         dn = result['dn']
         assert isinstance(dn, DN)
+        entry_attrs = ldap.make_entry(dn, self.args_options_2_entry(**options))
 
         for callback in self.get_callbacks('pre'):
             dn = callback(self, ldap, dn, *keys, **options)
@@ -2234,6 +2237,7 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
             attrs_list = ['*'] + self.obj.default_attributes
         else:
             attrs_list = set(self.obj.default_attributes)
+            attrs_list.update(entry_attrs.keys())
             if options.get('no_members', False):
                 attrs_list.difference_update(self.obj.attribute_members)
             attrs_list = list(attrs_list)
@@ -2258,7 +2262,7 @@ class LDAPRemoveReverseMember(LDAPModReverseMember):
                 failed['member'][self.reverse_attr].append((attr, unicode(e)))
 
         # Update the member data.
-        entry_attrs = ldap.get_entry(dn, ['*'])
+        entry_attrs = ldap.get_entry(dn, attrs_list)
         self.obj.convert_attribute_members(entry_attrs, *keys, **options)
 
         for callback in self.get_callbacks('post'):
diff --git a/ipatests/test_xmlrpc/test_permission_plugin.py b/ipatests/test_xmlrpc/test_permission_plugin.py
index d7061c62cdd95bf482fa6ff7c77174cd68a7393d..fe4a5b95944463edea379ab9dfac6b9a6679f54d 100644
--- a/ipatests/test_xmlrpc/test_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_permission_plugin.py
@@ -484,7 +484,6 @@ class test_permission(Declarative):
                     'cn': [privilege1],
                     'description': [u'privilege desc. 1'],
                     'memberof_permission': [permission1],
-                    'objectclass': objectclasses.privilege,
                 }
             ),
         ),
@@ -3067,7 +3066,6 @@ def _make_permission_flag_tests(flags, expected_message):
                     'cn': [privilege1],
                     'description': [u'privilege desc. 1'],
                     'memberof_permission': [permission1],
-                    'objectclass': objectclasses.privilege,
                 }
             ),
         ),
@@ -3310,7 +3308,6 @@ class test_permission_bindtype(Declarative):
                     dn=privilege1_dn,
                     cn=[privilege1],
                     description=[u'privilege desc. 1'],
-                    objectclass=objectclasses.privilege,
                 ),
             ),
         ),
@@ -3419,7 +3416,6 @@ class test_permission_bindtype(Declarative):
                     cn=[privilege1],
                     description=[u'privilege desc. 1'],
                     memberof_permission=[permission1],
-                    objectclass=objectclasses.privilege,
                 )
             ),
         ),
diff --git a/ipatests/test_xmlrpc/test_role_plugin.py b/ipatests/test_xmlrpc/test_role_plugin.py
index 74ff505cc166f348526f6b44866c9765cb73e987..6771cfad503e998a2c1433cc4c012210e815eab9 100644
--- a/ipatests/test_xmlrpc/test_role_plugin.py
+++ b/ipatests/test_xmlrpc/test_role_plugin.py
@@ -199,7 +199,6 @@ class test_role(Declarative):
                     'cn': [role1],
                     'description': [u'role desc 1'],
                     'memberof_privilege': [privilege1],
-                    'objectclass': objectclasses.role,
                 }
             ),
         ),
@@ -221,7 +220,6 @@ class test_role(Declarative):
                     'cn': [role1],
                     'description': [u'role desc 1'],
                     'memberof_privilege': [privilege1],
-                    'objectclass': objectclasses.role,
                 }
             ),
         ),
@@ -243,7 +241,6 @@ class test_role(Declarative):
                     'cn': [role1],
                     'description': [u'role desc 1'],
                     'memberof_privilege': [privilege1],
-                    'objectclass': objectclasses.role,
                 }
             ),
         ),
@@ -603,7 +600,6 @@ class test_role(Declarative):
                     'dn': role1_dn,
                     'cn': [role1],
                     'description': [u'New desc 1'],
-                    'objectclass': objectclasses.role,
                 }
             ),
         ),
@@ -625,7 +621,6 @@ class test_role(Declarative):
                     'dn': role1_dn,
                     'cn': [role1],
                     'description': [u'New desc 1'],
-                    'objectclass': objectclasses.role,
                 }
             ),
         ),
-- 
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