On 03/11/2014 03:08 PM, Jan Pazdziora wrote:
On Fri, Feb 21, 2014 at 03:30:22PM +0100, Petr Viktorin wrote:
Hello,
A permission object was not removed in permission-add when adding
the ACI failed. Here is a fix.
https://fedorahosted.org/freeipa/ticket/4187
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.
--
Petr³
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
fails
https://fedorahosted.org/freeipa/ticket/4187
---
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
index
64deb99ef98583daf0419a240aa8852b0262874d..cb6f18b478735920bbf6cef4febc91481631c560
100644
--- 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.)
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.
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
