Re: [Freeipa-devel] [PATCH] 0211-0212 Make sure --raw option works for trust-add

2016-07-21 Thread Jan Cholasta 

On 19.7.2016 11:32, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 12:06, Martin Babinsky wrote:

On 07/16/2016 12:50 PM, Alexander Bokovoy wrote:

Hi,


I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

Note that this fix (patch 0211) has potential for a break but also
introduces a correct behavior in my view as we should not really have
non-lower cased keys in LDAP dictionaries in entry_to_dict() for both
normal and --raw modes.

- 0211

Since commit a8dd7aa337f25abd938a582d0fcba51d3b356410 if IPA command
is called with --raw option, a retrieved LDAP entry's attribute
names aren't normalized to lower case when converting the entry
to a dictionary. This breaks overall assumption that dictionary
keys are in lower case.

Partially fixes 'ipa trust-add --raw' issues.

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

- 0212

Make sure we display raw values for 'trust-add --raw' case.

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






Hi Alexander,

I am CC'ing Jan since I hope he knows why
a8dd7aa337f25abd938a582d0fcba51d3b356410 was implemented in this way. I
think there was a reason behind this decision and we should not revert
it without further discussion.


I think it was just because with --raw, the raw LDAP entry should be
returned, letter case and all. I don't really care if the names are
lowercased or not, but you should never assume anything about raw
results in your code anyway, so I think the revert would be pointless.
There were a few bugs with --raw unrelated to letter case in the past
and most of the offending code was fixed, the same should be done here
IMO.

This is not about LDAP entry, this is about Python dictionary from
entry_to_dict(). LDAP attribute name is case-insensitive.

We call entry_to_dict() in multiple places and we expect that
dictionary keys are lowcased in Python code. I don't think it is fair to
have this assumption broken when --raw is used -- after all, we are
dealing with Python dictionary and LDAP attributes are case insensitive.


We don't expect anything about raw results anywhere in our code base, 
except for buggy code, which needs be fixed anyway.


Martin's patch fixes the issue without any intrusive changes, so ACK. We 
can talk bigger changes later for 4.5.


Pushed to master: 2234a774416309a44aecb84f27e6cf4c6a1a990f

--
Jan Cholasta

--
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] 0211-0212 Make sure --raw option works for trust-add

2016-07-19 Thread Alexander Bokovoy 

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 12:06, Martin Babinsky wrote:

On 07/16/2016 12:50 PM, Alexander Bokovoy wrote:

Hi,


I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

Note that this fix (patch 0211) has potential for a break but also
introduces a correct behavior in my view as we should not really have
non-lower cased keys in LDAP dictionaries in entry_to_dict() for both
normal and --raw modes.

- 0211

Since commit a8dd7aa337f25abd938a582d0fcba51d3b356410 if IPA command
is called with --raw option, a retrieved LDAP entry's attribute
names aren't normalized to lower case when converting the entry
to a dictionary. This breaks overall assumption that dictionary
keys are in lower case.

Partially fixes 'ipa trust-add --raw' issues.

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

- 0212

Make sure we display raw values for 'trust-add --raw' case.

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






Hi Alexander,

I am CC'ing Jan since I hope he knows why
a8dd7aa337f25abd938a582d0fcba51d3b356410 was implemented in this way. I
think there was a reason behind this decision and we should not revert
it without further discussion.


I think it was just because with --raw, the raw LDAP entry should be 
returned, letter case and all. I don't really care if the names are 
lowercased or not, but you should never assume anything about raw 
results in your code anyway, so I think the revert would be pointless. 
There were a few bugs with --raw unrelated to letter case in the past 
and most of the offending code was fixed, the same should be done here 
IMO.

This is not about LDAP entry, this is about Python dictionary from
entry_to_dict(). LDAP attribute name is case-insensitive.

We call entry_to_dict() in multiple places and we expect that
dictionary keys are lowcased in Python code. I don't think it is fair to
have this assumption broken when --raw is used -- after all, we are
dealing with Python dictionary and LDAP attributes are case insensitive.

--
/ Alexander Bokovoy

--
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] 0211-0212 Make sure --raw option works for trust-add

2016-07-19 Thread Jan Cholasta 

On 18.7.2016 12:06, Martin Babinsky wrote:

On 07/16/2016 12:50 PM, Alexander Bokovoy wrote:

Hi,


I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

Note that this fix (patch 0211) has potential for a break but also
introduces a correct behavior in my view as we should not really have
non-lower cased keys in LDAP dictionaries in entry_to_dict() for both
normal and --raw modes.

- 0211

Since commit a8dd7aa337f25abd938a582d0fcba51d3b356410 if IPA command
is called with --raw option, a retrieved LDAP entry's attribute
names aren't normalized to lower case when converting the entry
to a dictionary. This breaks overall assumption that dictionary
keys are in lower case.

Partially fixes 'ipa trust-add --raw' issues.

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

- 0212

Make sure we display raw values for 'trust-add --raw' case.

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






Hi Alexander,

I am CC'ing Jan since I hope he knows why
a8dd7aa337f25abd938a582d0fcba51d3b356410 was implemented in this way. I
think there was a reason behind this decision and we should not revert
it without further discussion.


I think it was just because with --raw, the raw LDAP entry should be 
returned, letter case and all. I don't really care if the names are 
lowercased or not, but you should never assume anything about raw 
results in your code anyway, so I think the revert would be pointless. 
There were a few bugs with --raw unrelated to letter case in the past 
and most of the offending code was fixed, the same should be done here IMO.




I had the fix for trust-add ready on Friday but was unable to test it
properly because of some issues with my VMs. I am attaching it for
reference, since it is similar to your patch 212 but relies on
attrs_list passed in to LDAP search in order to fetch lowercased
attributes.




--
Jan Cholasta

--
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] 0211-0212 Make sure --raw option works for trust-add

2016-07-18 Thread Martin Babinsky 

On 07/16/2016 12:50 PM, Alexander Bokovoy wrote:

Hi,


I had some time and was blocked by these bugs to do my tickets so I
actually fixed these three problems that are assigned to Martin
Babinsky. Hopefully, Martin wouldn't be offended by that. :)

Note that this fix (patch 0211) has potential for a break but also
introduces a correct behavior in my view as we should not really have
non-lower cased keys in LDAP dictionaries in entry_to_dict() for both
normal and --raw modes.

- 0211

Since commit a8dd7aa337f25abd938a582d0fcba51d3b356410 if IPA command
is called with --raw option, a retrieved LDAP entry's attribute
names aren't normalized to lower case when converting the entry
to a dictionary. This breaks overall assumption that dictionary
keys are in lower case.

Partially fixes 'ipa trust-add --raw' issues.

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

- 0212

Make sure we display raw values for 'trust-add --raw' case.

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






Hi Alexander,

I am CC'ing Jan since I hope he knows why 
a8dd7aa337f25abd938a582d0fcba51d3b356410 was implemented in this way. I 
think there was a reason behind this decision and we should not revert 
it without further discussion.


I had the fix for trust-add ready on Friday but was unable to test it 
properly because of some issues with my VMs. I am attaching it for 
reference, since it is similar to your patch 212 but relies on 
attrs_list passed in to LDAP search in order to fetch lowercased attributes.


--
Martin^3 Babinsky
From c3c197822364c01d62626acdc3d69274a14ca291 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 15 Jul 2016 12:38:00 +0200
Subject: [PATCH] trust-add: handle `--all/--raw` options properly

`trust-add` command did not handle these options correctly often resulting in
internal errors or mangled output. This patch implements a behavior which is
more in-line with the rest of the API commands.

https://fedorahosted.org/freeipa/ticket/6059
---
 ipaserver/plugins/trust.py | 41 +++--
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/ipaserver/plugins/trust.py b/ipaserver/plugins/trust.py
index 8536202b9b785507bd27b3c7b1896b721f8c5927..18a7cfd3447143393fb087f840edb406e98285a4 100644
--- a/ipaserver/plugins/trust.py
+++ b/ipaserver/plugins/trust.py
@@ -710,6 +710,25 @@ sides.
 msg_summary = _('Added Active Directory trust for realm "%(value)s"')
 msg_summary_existing = _('Re-established trust to domain "%(value)s"')
 
+def _format_trust_attrs(self, result, **options):
+
+# Format the output into human-readable values
+attributes = int(result['result'].get('ipanttrustattributes', [0])[0])
+
+if not options.get('raw', False):
+result['result']['trusttype'] = [trust_type_string(
+result['result']['ipanttrusttype'][0], attributes)]
+result['result']['trustdirection'] = [trust_direction_string(
+result['result']['ipanttrustdirection'][0])]
+result['result']['truststatus'] = [trust_status_string(
+result['verified'])]
+
+if attributes:
+result['result'].pop('ipanttrustattributes', None)
+
+result['result'].pop('ipanttrustauthoutgoing', None)
+result['result'].pop('ipanttrustauthincoming', None)
+
 def execute(self, *keys, **options):
 ldap = self.obj.backend
 
@@ -729,10 +748,15 @@ sides.
 else:
 created_range_type = old_range['result']['iparangetype'][0]
 
+attrs_list = self.obj.default_attributes
+if options.get('all', False):
+attrs_list.append('*')
+
 trust_filter = "cn=%s" % result['value']
 (trusts, truncated) = ldap.find_entries(
  base_dn=DN(self.api.env.container_trusts, self.api.env.basedn),
- filter=trust_filter)
+ filter=trust_filter,
+ attrs_list=attrs_list)
 
 result['result'] = entry_to_dict(trusts[0], **options)
 
@@ -761,20 +785,9 @@ sides.
 # add_new_domains_from_trust() on its own.
 fetch_trusted_domains_over_dbus(self.api, self.log, result['value'])
 
-# Format the output into human-readable values
-attributes = int(result['result'].get('ipanttrustattributes', [0])[0])
-result['result']['trusttype'] = [trust_type_string(
-result['result']['ipanttrusttype'][0], attributes)]
-result['result']['trustdirection'] = [trust_direction_string(
-result['result']['ipanttrustdirection'][0])]
-result['result']['truststatus'] = [trust_status_string(
-result['verified'])]
-if attributes:
-result['result'].pop('ipanttrustattributes', None)
-
+# Format the output into human-readable values unless `--raw` is given
+self._format_trust_attrs(result, **options)