On 11.3.2014 16:09, Petr Viktorin wrote:
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, **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
+            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.)

This calls for
[RFE] Support LDAP transactions

Maybe we should always add a comment about each particular use case to give it the right priority ...

Petr^2 Spacek

Freeipa-devel mailing list

Reply via email to