On 03/11/2014 03:08 PM, Jan Pazdziora wrote:
On Fri, Feb 21, 2014 at 03:30:22PM +0100, Petr Viktorin wrote:
A permission object was not removed in permission-add when adding
the ACI failed. Here is a fix.


Earlier we agreed that patch authors should bug the reviewer. I
guess now this means I should set Patch-review-by in the ticket,
right? So:
Martin, you reviewed the other ACI patches so I think you should
continue. If you don't agree, unset the field in the ticket.


 From 5ad2066b71b09248d348a5c4c85ef2ace0c553a4 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

  ipalib/plugins/permission.py                   | 15 ++++++++++++++-
  ipatests/test_xmlrpc/test_permission_plugin.py | 25 +++++++++++++++++++++++++
  2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -812,7 +812,20 @@ def pre_callback(self, ldap, dn, entry, attrs_list, *keys, 
          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
+            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

I'm not totally happy about this patch.

What happens when the ACI is already in LDAP and some part of that
self.obj.add_aci(entry) operation fails? Won't you go and instead of
doing noop, remove the ACI instead?

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.


Freeipa-devel mailing list

Reply via email to