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
From 1bf907be3f78c9da4ba39a24a85e0a7915967c66 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Thu, 16 Oct 2014 12:40:24 +0200 Subject: [PATCH] Raise better error message for permission added to generated tree 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..43481b9ebf6a2b35eae301cc2f99315539a2ab1e 100644 --- a/ipalib/plugins/permission.py +++ b/ipalib/plugins/permission.py @@ -972,7 +972,7 @@ def pre_callback(self, ldap, dn, entry, attrs_list, *keys, **options): 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 @@ def post_callback(self, ldap, dn, entry, *keys, **options): 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.9.3
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
