Re: [Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree

2014-10-16 Thread thierry bordaz

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.

From 5e0fff734c830878b4b0a3869931ee70629aff84 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 'ipapermlocation' is invalid when it applies on a non existing entry

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

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 9028f02483bc113c19c75b94d70dd1b133272524..35ed2e483a65fb266bb2dfe78b020d0f5b549759 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -943,7 +943,13 @@ class permission_add(baseldap.LDAPCreate):
 # the whole command, not just the callbacks
 def execute(self, *keys, **options):
 self.obj.preprocess_options(options, merge_targetfilter=True)
-return super(permission_add, self).execute(*keys, **options)
+try:
+res = super(permission_add, self).execute(*keys, **options)
+except errors.NotFound:
+raise errors.ValidationError(
+ name='ipapermlocation',
+ error=_('Entry %s does not exist') % self.obj.get_dn(*keys, **options))
+return res
 
 def get_args(self):
 for arg in super(permission_add, self).get_args():
-- 
1.7.11.7

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

Re: [Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree

2014-10-16 Thread Martin Kosek
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

Re: [Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree

2014-10-16 Thread thierry bordaz

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

Re: [Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree

2014-10-16 Thread Martin Kosek
On 10/16/2014 03:32 PM, thierry bordaz wrote:
 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
 

I understand that as an ACK from you then :-)

Given the effort I had to develop the patch, I will push that original version
and add you as a reviewer.

Pushed to:
master: 061f7ff331531fa01801fb597feed924de6a2fd7
ipa-4-1: 0a54b1c948e5c25d971f5b0ef4bc079cbd605069

Thanks!
Martin

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


Re: [Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree

2014-10-15 Thread Martin Kosek
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)

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

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


Re: [Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree

2014-10-15 Thread thierry bordaz

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 ?


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.


thanks
thierry


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

Re: [Freeipa-devel] [PATCH] 0004 permission-add gives confusing error when adding ACI to generated tree

2014-10-15 Thread Martin Kosek
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

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