On 10/16/2014 12:45 PM, Martin Kosek wrote:
On 10/16/2014 12:08 PM, thierry bordaz wrote:
On 10/15/2014 04:33 PM, Martin Kosek wrote:
On 10/15/2014 01:57 PM, thierry bordaz wrote:
On 10/15/2014 01:26 PM, Martin Kosek wrote:
On 10/15/2014 01:08 PM, thierry bordaz wrote:
https://fedorahosted.org/freeipa/ticket/4523
I see 2 issues with the patch:

1) Patch description should not contain "
Reviewed by:", this gets added later by a script (or human)
ok
2) The exception handling clause should be as focused as possible, i.e. not
including whole command, but rather just the failing call, i.e.:

       def post_callback(self, ldap, dn, entry, *keys, **options):
           try:
               self.obj.add_aci(entry)
           except Exception:

You can use

       try:
           ...
       except errors.NotFound:
           self.obj.handle_not_found(*keys)

to raise the right error.

Martin
Currently the exception is handled on the failure of
baseldap.LDAPCreate.execute(). Do you recommend to make the fix inside
baseldap.LDAPCreate.execute rather than at the 'permission_add.execute' level ?
No, not there. I thought that the exception happens in

      def post_callback(self, ldap, dn, entry, *keys, **options):
          try:
              self.obj.add_aci(entry)
          except Exception:
             ...

Also using handle_not_found looks good, but it reports something like:

     ipa permission-add user1 --right read --attrs cn --subtree
     'cn=compat,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com'
     ipa: ERROR: user1: permission not found


If the entry 'user1' exists, it is not clear what was not found.
Displaying the dn of the entry would help to know that we are updating an entry
into the 'compat' tree.
Ah, sorry, I think I mislead you with this advise. You probably could use the
same except clause as already used:

              except errors.NotFound:
                  raise errors.ValidationError(
                      name='ipapermlocation',
                      error=_('Entry %s does not exist') % location)

Martin
Thanks Martin for your review. Here is a second fix.
I am sorry, this is still not what I meant. You cannot wrap whole
permission-add command, wait for very general error and then report very
specific error. Try-except clauses need to be as close to the guarded problem
as possible.

See PEP8 for example (http://legacy.python.org/dev/peps/pep-0008/):

..
Additionally, for all try/except clauses, limit the try clause to the absolute
minimum amount of code necessary. Again, this avoids masking bugs
...

To speed up the resolution of this patch, please see a proposed patch, I hope
it works for you.

Martin
Thanks Martin for the patch and the explanations. I missed that the LDAPCreate triggered the exception when calling permission_add.post_callback. Now I understand what you meant by catching the exception as close as possible.

Thierry
From 5e653ce403415dfc69fd7bbc724877d9a46f0b2c Mon Sep 17 00:00:00 2001
From: "Thierry bordaz (tbordaz)" <tbor...@redhat.com>
Date: Tue, 7 Oct 2014 18:41:44 +0200
Subject: [PATCH] permission-add gives confusing error when adding ACI to
 generated tree

Raise that an ACI can not be store in 'ipapermlocation' entry

https://fedorahosted.org/freeipa/ticket/4523
---
 ipalib/plugins/permission.py | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 9028f02483bc113c19c75b94d70dd1b133272524..ad74d976e78f942456d21ba5ef330ce1a899c27c 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -972,7 +972,7 @@ class permission_add(baseldap.LDAPCreate):
     def post_callback(self, ldap, dn, entry, *keys, **options):
         try:
             self.obj.add_aci(entry)
-        except Exception:
+        except Exception, e:
             # 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.)
@@ -988,6 +988,13 @@ class permission_add(baseldap.LDAPCreate):
                 self.api.Backend['ldap2'].delete_entry(entry)
             except errors.NotFound:
                 pass
+            if isinstance(e, errors.NotFound):
+                # add_aci may raise NotFound if the subtree is only virtual
+                # like cn=compat,SUFFIX and thus passes the LDAP get entry test
+                location = DN(entry.single_value['ipapermlocation'])
+                raise errors.ValidationError(
+                                             name='ipapermlocation',
+                                             error=_('Cannot store permission ACI to %s') % location)
             # Re-raise original exception
             raise
         self.obj.postprocess_result(entry, options)
-- 
1.7.11.7

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to