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

[Petr Spacek]

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:
>>> 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.)

This calls for
https://fedorahosted.org/389/ticket/581
[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