On 03/12/2014 10:20 AM, Jan Pazdziora wrote:
On Tue, Mar 11, 2014 at 04:09:37PM +0100, Petr Viktorin wrote:


Unfortunately, yes, these operations are racy. When something fails,
or when doing two operations simultaneously, it is possible that the
objects are not both added.
If that happens, it is the ACI that should be missing. The
permission is added first, and the ACI is deleted first. This means
that when things fail, access is denied, which is both more secure
and easier to spot than having a stray ACI floating around.

(In the long term, I'd really like to see a DS plugin for
permission/ACI sync, so we can leverage transactions -- IPA is
really the wrong layer to re-implement transactions in.)

To answer your question, if the permission+ACI is already in LDAP,
the call will fail with a DuplicateEntry error and post_callback
won't get called.

For the case that another permission_add command is called to add a
permission of the same name, the existence of the permission entry
acts as a "lock": while it's there, the other permission_add will
fail, and removing it ("releasing the lock") is the last thing done
in the error handler.

I guess it would be good to add a comment saying this.

Thank you for the explanation.

In that case, ack on the patch, provided you add a nice comment. ;-)



Thanks, added comment and pushed to master: d3a34591a807f1420042ddbb53b3d5ac846927aa

--
PetrĀ³
From 453d79c6bc7496e75611d00c6386be34bbf3ff1d Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Fri, 21 Feb 2014 13:58:15 +0100
Subject: [PATCH] permission_add: Remove permission entry if adding the ACI
 fails

https://fedorahosted.org/freeipa/ticket/4187
---
 ipalib/plugins/permission.py                   | 21 ++++++++++++++++++++-
 ipatests/test_xmlrpc/test_permission_plugin.py | 25 +++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index d4181a6b44fb1d1b308fc9eebdbfb776c0e18212..bd7f5da6adf7fd7f576d69f4a9c6c4051ec1ac94 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -830,7 +830,26 @@ def pre_callback(self, ldap, dn, entry, attrs_list, *keys, **options):
         return dn
 
     def post_callback(self, ldap, dn, entry, *keys, **options):
-        self.obj.add_aci(entry)
+        try:
+            self.obj.add_aci(entry)
+        except Exception:
+            # Adding the ACI failed.
+            # We want to be 100% sure the ACI is not there, so try to
+            # remove it. (This is a no-op if the ACI was not added.)
+            self.obj.remove_aci(entry)
+            # Remove the entry.
+            # The permission entry serves as a "lock" tho prevent
+            # permission-add commands started at the same time from
+            # interfering. As long as the entry is there, the other
+            # permission-add will fail with DuplicateEntry.
+            # So deleting entry ("releasing the lock") must be the last
+            # thing we do here.
+            try:
+                self.api.Backend['ldap2'].delete_entry(entry)
+            except errors.NotFound:
+                pass
+            # Re-raise original exception
+            raise
         self.obj.postprocess_result(entry, options)
         return dn
 
diff --git a/ipatests/test_xmlrpc/test_permission_plugin.py b/ipatests/test_xmlrpc/test_permission_plugin.py
index 725fe0ab48a597168a3a08d3c49d3ef6d9db0f7a..62ff20e563c4653a41223959d02170c7a6e519f8 100644
--- a/ipatests/test_xmlrpc/test_permission_plugin.py
+++ b/ipatests/test_xmlrpc/test_permission_plugin.py
@@ -220,6 +220,31 @@ class test_permission_negative(Declarative):
         verify_permission_aci_missing(permission1, users_dn),
 
         dict(
+            desc='Try creating %r with bad attribute name' % permission1,
+            command=(
+                'permission_add', [permission1], dict(
+                    type=u'user',
+                    ipapermright=u'write',
+                    attrs=u'bogusattr',
+                )
+            ),
+            expected=errors.InvalidSyntax(
+                attr=r'targetattr "bogusattr" does not exist in schema. '
+                     r'Please add attributeTypes "bogusattr" to '
+                     r'schema if necessary. '
+                     r'ACL Syntax Error(-5):'
+                     r'(targetattr = \22bogusattr\22)'
+                     r'(targetfilter = \22(objectclass=posixaccount)\22)'
+                     r'(version 3.0;acl \22permission:%(name)s\22;'
+                     r'allow (write) groupdn = \22ldap:///%(dn)s\22;)' % dict(
+                        name=permission1,
+                        dn=permission1_dn),
+            ),
+        ),
+
+        verify_permission_aci_missing(permission1, users_dn),
+
+        dict(
             desc='Create %r so we can try breaking it' % permission1,
             command=(
                 'permission_add', [permission1], dict(
-- 
1.8.5.3

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to