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

[Jan Pazdziora]

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?

-- 
Jan Pazdziora
Principal Software Engineer, Identity Management Engineering, Red Hat