Re: [Freeipa-devel] [PATCH] 0006 add context to exception on LdapEntry decode error

2016-06-09 Thread Martin Basti



On 09.06.2016 14:31, Stanislav Laznicka wrote:

On 06/09/2016 11:58 AM, Florence Blanc-Renaud wrote:

On 06/08/2016 01:14 PM, Stanislav Laznicka wrote:

On 06/08/2016 01:13 PM, Stanislav Laznicka wrote:




On 06/07/2016 05:11 PM, Florence Blanc-Renaud wrote:

On 06/07/2016 04:08 PM, Stanislav Laznicka wrote:

On 06/06/2016 02:47 PM, Florence Blanc-Renaud wrote:


Hi,

please find attached the patch for Ticket 5434 add context to 
exception on LdapEntry decode error


https://fedorahosted.org/freeipa/ticket/5434




Hello,

Similarly to you patch 0003, could you please shorten the commit 
message? I appreciate that you added the way to reproduce the bug 
but I would rather see it in the comments of the ticket.


I haven't tested the patch yet but I think that the try-except 
block should also wrap the self._conn.decode in the raw_dels loop:


 310 for value in raw_dels:
 311 value = self._conn.decode(value, name)
 312 if value in nice_adds:
 313 continue
 314 nice.remove(value)

Standa


Hi Standa,

thanks for your review. I will shorten the commit message as you 
suggested.


Regarding the other decode() called for raw_dels, I was not sure 
in which case this part of the code was called. Do you happen to 
know if it possible for an invalid value to be in the raw_sync 
list but not in the raw list?


Flo.

I'm not sure if there's an easy way to do that. I would just put it 
there should it ever occur to have more information about what 
happened.

Forgot to include devel-list, sorry.


Hi,

updated patch attached.

Flo.


Thank you for the updated patch. It seems to work as expected so ACK.



Pushed to master: 53524fbbff51418a8f1194c8559c9dcfcc5bbb83

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0006 add context to exception on LdapEntry decode error

2016-06-09 Thread Stanislav Laznicka

On 06/09/2016 11:58 AM, Florence Blanc-Renaud wrote:

On 06/08/2016 01:14 PM, Stanislav Laznicka wrote:

On 06/08/2016 01:13 PM, Stanislav Laznicka wrote:




On 06/07/2016 05:11 PM, Florence Blanc-Renaud wrote:

On 06/07/2016 04:08 PM, Stanislav Laznicka wrote:

On 06/06/2016 02:47 PM, Florence Blanc-Renaud wrote:


Hi,

please find attached the patch for Ticket 5434 add context to 
exception on LdapEntry decode error


https://fedorahosted.org/freeipa/ticket/5434




Hello,

Similarly to you patch 0003, could you please shorten the commit 
message? I appreciate that you added the way to reproduce the bug 
but I would rather see it in the comments of the ticket.


I haven't tested the patch yet but I think that the try-except 
block should also wrap the self._conn.decode in the raw_dels loop:


 310 for value in raw_dels:
 311 value = self._conn.decode(value, name)
 312 if value in nice_adds:
 313 continue
 314 nice.remove(value)

Standa


Hi Standa,

thanks for your review. I will shorten the commit message as you 
suggested.


Regarding the other decode() called for raw_dels, I was not sure in 
which case this part of the code was called. Do you happen to know 
if it possible for an invalid value to be in the raw_sync list but 
not in the raw list?


Flo.

I'm not sure if there's an easy way to do that. I would just put it 
there should it ever occur to have more information about what happened.

Forgot to include devel-list, sorry.


Hi,

updated patch attached.

Flo.


Thank you for the updated patch. It seems to work as expected so ACK.
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0006 add context to exception on LdapEntry decode error

2016-06-09 Thread Florence Blanc-Renaud

On 06/08/2016 01:14 PM, Stanislav Laznicka wrote:

On 06/08/2016 01:13 PM, Stanislav Laznicka wrote:




On 06/07/2016 05:11 PM, Florence Blanc-Renaud wrote:

On 06/07/2016 04:08 PM, Stanislav Laznicka wrote:

On 06/06/2016 02:47 PM, Florence Blanc-Renaud wrote:


Hi,

please find attached the patch for Ticket 5434 add context to 
exception on LdapEntry decode error


https://fedorahosted.org/freeipa/ticket/5434




Hello,

Similarly to you patch 0003, could you please shorten the commit 
message? I appreciate that you added the way to reproduce the bug 
but I would rather see it in the comments of the ticket.


I haven't tested the patch yet but I think that the try-except 
block should also wrap the self._conn.decode in the raw_dels loop:


 310 for value in raw_dels:
 311 value = self._conn.decode(value, name)
 312 if value in nice_adds:
 313 continue
 314 nice.remove(value)

Standa


Hi Standa,

thanks for your review. I will shorten the commit message as you 
suggested.


Regarding the other decode() called for raw_dels, I was not sure in 
which case this part of the code was called. Do you happen to know 
if it possible for an invalid value to be in the raw_sync list but 
not in the raw list?


Flo.

I'm not sure if there's an easy way to do that. I would just put it 
there should it ever occur to have more information about what happened.

Forgot to include devel-list, sorry.


Hi,

updated patch attached.

Flo.

From 04d591a2fc0f0e0ae3139aa8f1eccafbe26905aa Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud 
Date: Wed, 8 Jun 2016 18:09:15 +0200
Subject: [PATCH] add context to exception on LdapEntry decode error

When reading the content of an invalid LDAP entry, the exception
only displays the attribute name and value, but not the DN of the entry.
Because of this, it is difficult to identify the root cause of the
problem.

The fix raises a ValueError exception which also contains the entry DN.

https://fedorahosted.org/freeipa/ticket/5434
---
 ipapython/ipaldap.py | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 9fb7fd3f5a49dec0fd855c05f0e64004593e1306..410ddae2c484f060abf2bbf3e897549a26b0ebc9 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -308,7 +308,11 @@ class LDAPEntry(collections.MutableMapping):
 raw.remove(value)
 
 for value in raw_dels:
-value = self._conn.decode(value, name)
+try:
+value = self._conn.decode(value, name)
+except ValueError as e:
+raise ValueError("{error} in LDAP entry '{dn}'".format(
+error=e, dn=self._dn))
 if value in nice_adds:
 continue
 nice.remove(value)
@@ -320,7 +324,11 @@ class LDAPEntry(collections.MutableMapping):
 raw.append(value)
 
 for value in raw_adds:
-value = self._conn.decode(value, name)
+try:
+value = self._conn.decode(value, name)
+except ValueError as e:
+raise ValueError("{error} in LDAP entry '{dn}'".format(
+error=e, dn=self._dn))
 if value in nice_dels:
 continue
 nice.append(value)
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0006 add context to exception on LdapEntry decode error

2016-06-08 Thread Stanislav Laznicka

On 06/08/2016 01:13 PM, Stanislav Laznicka wrote:




On 06/07/2016 05:11 PM, Florence Blanc-Renaud wrote:

On 06/07/2016 04:08 PM, Stanislav Laznicka wrote:

On 06/06/2016 02:47 PM, Florence Blanc-Renaud wrote:


Hi,

please find attached the patch for Ticket 5434 add context to 
exception on LdapEntry decode error


https://fedorahosted.org/freeipa/ticket/5434




Hello,

Similarly to you patch 0003, could you please shorten the commit 
message? I appreciate that you added the way to reproduce the bug 
but I would rather see it in the comments of the ticket.


I haven't tested the patch yet but I think that the try-except block 
should also wrap the self._conn.decode in the raw_dels loop:


 310 for value in raw_dels:
 311 value = self._conn.decode(value, name)
 312 if value in nice_adds:
 313 continue
 314 nice.remove(value)

Standa


Hi Standa,

thanks for your review. I will shorten the commit message as you 
suggested.


Regarding the other decode() called for raw_dels, I was not sure in 
which case this part of the code was called. Do you happen to know if 
it possible for an invalid value to be in the raw_sync list but not 
in the raw list?


Flo.

I'm not sure if there's an easy way to do that. I would just put it 
there should it ever occur to have more information about what happened.

Forgot to include devel-list, sorry.
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH] 0006 add context to exception on LdapEntry decode error

2016-06-06 Thread Florence Blanc-Renaud

Hi,

please find attached the patch for Ticket 5434 add context to exception 
on LdapEntry decode error


https://fedorahosted.org/freeipa/ticket/5434

From 8094fca2e0a11c1c108959da3a8f05c3d9c62bb7 Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud 
Date: Fri, 3 Jun 2016 14:56:35 +0200
Subject: [PATCH] add context to exception on LdapEntry decode error

When reading the content of an invalid LDAP entry, the exception
only displays the attribute name and value, but not the DN of the entry.
Because of this, it is difficult to identify the root cause of the
problem.

The fix raises a ValueError exception which also contains the entry DN.
Note that this does not change the overall behavior: the web browser
will still display

An error has occurred (IPA Error 903: InternalError)
Please try the following options:

Refresh the page.
Return to the main page and retry the operation
Reload the browser.

If the problem persists please contact the system administrator.


The exception is printed in httpd/error_log as
ValueError: unable to convert the attribute u'memberofindirect' value 'Y249ZWV5ZV9ob3N0LGNuPWhvc3Rncm91cHMsY249YWNjb3VudHMsZGM9cHJvZCxkYLg' to type  in LDAP entry 'fqdn=vm-058-124.abc.idm.lab.eng.brq.redhat.com,cn=computers,cn=accounts,dc=dom-058-232,dc=abc,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com'

The issue can be reproduced by disabling the schema checking and
manually modifying a host entry with a value that is not base64-decodable.
Example ldif:

dn: cn=config
changetype: modify
replace: nsslapd-syntaxcheck
nsslapd-syntaxcheck: off

dn: fqdn=,cn=computers,cn=accounts,
changetype: modify
add: memberof
memberof: Y249ZWV5ZV9ob3N0LGNuPWhvc3Rncm91cHMsY249YWNjb3VudHMsZGM9cHJvZCxkYLg

https://fedorahosted.org/freeipa/ticket/5434
---
 ipapython/ipaldap.py | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py
index 9fb7fd3f5a49dec0fd855c05f0e64004593e1306..6bade5bbc6f87ba9dd3d99b6f6befbfa2ee086c5 100644
--- a/ipapython/ipaldap.py
+++ b/ipapython/ipaldap.py
@@ -320,7 +320,11 @@ class LDAPEntry(collections.MutableMapping):
 raw.append(value)
 
 for value in raw_adds:
-value = self._conn.decode(value, name)
+try:
+value = self._conn.decode(value, name)
+except ValueError as e:
+raise ValueError("{error} in LDAP entry '{dn}'".format(
+error=e, dn=self._dn))
 if value in nice_dels:
 continue
 nice.append(value)
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code