[Freeipa-devel] [PATCH] 0471 permission_add: Remove permission entry if adding the ACI fails

[Petr Viktorin]

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 <pviktori at 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³