Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-20 Thread Jan Cholasta

On 20.7.2016 13:15, Martin Babinsky wrote:

On 07/20/2016 12:08 PM, Martin Babinsky wrote:

On 07/19/2016 01:25 PM, Martin Babinsky wrote:

On 07/19/2016 01:13 PM, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Martin Babinsky wrote:

On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > > > On 16.7.2016 12:46, 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. :)
> > > > > > --
> > > > > > Output entry elements may have multiple types allowed. We
need to check
> > > all of them to properly validate the output. Right now, thin
client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),)
tuple
> > > on the thin client side.
> > > > > > Change validation to account this by processing each
separate type
> > > of the element and account for both None and type(None). Raise
type
> > > error only if all of the type checks failed.
> > > > > > https://fedorahosted.org/freeipa/ticket/6061
> > > > NACK, this only hides the real issue, which is that
trustconfig-show
> > (and automember-set-default in #6037) claims to return the primary
key
> > of the object in the 'value' output field, but the object does not
have
> > a primary key, so the client rightfully expects None.
> > > > A proper fix would be to set "has_output =
output.simple_value" for
> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > > > Honza
> > > > The problem is that these commands do not return a simple
boolean in
> 'result' but a full entry dict, so 'simple_value' won't do the
trick in
> this case.
> > But I agree, we should rather fix misbehaving commands rather than
bend
> the framework to accomodate their idiosyncracies, we have enough of
that
> already.
> I am attaching a patch that adds a custom shim for misbehaving
commands so that they work as before. There is also a big fat warning
added to discourage implementation of such commands.

Let's go by this patch. I originally thought changing trust.py to
simply
not supply 'value' field at all but this fix is simpler.


We found out that we still would need to handle
automember-default-group-{set,remove} commands which use the value to
print out summary, so it seems that we cannot win without some more
extensive fixing.



I have botched the wording in the comment, attaching updated patch.




And I have missed adding the new output to `trustconfig-mod` so
attaching another version.


Works for me, ACK.

Pushed to master: f0a61546f552d4df887617167f7dc1378cb95083

--
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] 0210 frontend: fix output validation for multiple type choices

2016-07-20 Thread Martin Babinsky

On 07/20/2016 12:08 PM, Martin Babinsky wrote:

On 07/19/2016 01:25 PM, Martin Babinsky wrote:

On 07/19/2016 01:13 PM, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Martin Babinsky wrote:

On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > > > On 16.7.2016 12:46, 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. :)
> > > > > > --
> > > > > > Output entry elements may have multiple types allowed. We
need to check
> > > all of them to properly validate the output. Right now, thin
client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),)
tuple
> > > on the thin client side.
> > > > > > Change validation to account this by processing each
separate type
> > > of the element and account for both None and type(None). Raise
type
> > > error only if all of the type checks failed.
> > > > > > https://fedorahosted.org/freeipa/ticket/6061
> > > > NACK, this only hides the real issue, which is that
trustconfig-show
> > (and automember-set-default in #6037) claims to return the primary
key
> > of the object in the 'value' output field, but the object does not
have
> > a primary key, so the client rightfully expects None.
> > > > A proper fix would be to set "has_output =
output.simple_value" for
> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > > > Honza
> > > > The problem is that these commands do not return a simple
boolean in
> 'result' but a full entry dict, so 'simple_value' won't do the
trick in
> this case.
> > But I agree, we should rather fix misbehaving commands rather than
bend
> the framework to accomodate their idiosyncracies, we have enough of
that
> already.
> I am attaching a patch that adds a custom shim for misbehaving
commands so that they work as before. There is also a big fat warning
added to discourage implementation of such commands.

Let's go by this patch. I originally thought changing trust.py to simply
not supply 'value' field at all but this fix is simpler.


We found out that we still would need to handle
automember-default-group-{set,remove} commands which use the value to
print out summary, so it seems that we cannot win without some more
extensive fixing.



I have botched the wording in the comment, attaching updated patch.



And I have missed adding the new output to `trustconfig-mod` so 
attaching another version.


--
Martin^3 Babinsky
From d65b24d92dadb31a3499a654096397151a2fa53b Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 18 Jul 2016 13:18:44 +0200
Subject: [PATCH] allow 'value' output param in commands without primary key

`PrimaryKey` output param works only for API objects that have primary keys,
otherwise it expects None (nothing is associated with this param). Since the
validation of command output was tightened durng thin client effort, some
commands not honoring this contract began to fail output validation.

A custom output was implemented for them to restore their functionality. It
should however be considered as a fix for broken commands and not used
further.

https://fedorahosted.org/freeipa/ticket/6037
https://fedorahosted.org/freeipa/ticket/6061
---
 API.txt | 10 +-
 VERSION |  4 ++--
 ipalib/output.py| 10 ++
 ipaserver/plugins/automember.py |  3 +++
 ipaserver/plugins/trust.py  |  2 ++
 5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/API.txt b/API.txt
index eb33c1fb7f94f5af45ec0b38fc7e45e484a1044e..535d8ec9a4990395207e2455a09a8c1bdef5529a 100644
--- a/API.txt
+++ b/API.txt
@@ -144,7 +144,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_set/1
 args: 0,6,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -155,7 +155,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_show/1
 args: 0,4,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -164,7 +164,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_del/1
 args: 1,2,3
 arg: Str('cn+', cli_name='automember_rule')
@@ -5574,7 +5574,7 @@ option: StrEnum('trust_type', autofill=True, 

Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-20 Thread Martin Babinsky

On 07/19/2016 01:25 PM, Martin Babinsky wrote:

On 07/19/2016 01:13 PM, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Martin Babinsky wrote:

On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > > > On 16.7.2016 12:46, 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. :)
> > > > > > --
> > > > > > Output entry elements may have multiple types allowed. We
need to check
> > > all of them to properly validate the output. Right now, thin
client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),)
tuple
> > > on the thin client side.
> > > > > > Change validation to account this by processing each
separate type
> > > of the element and account for both None and type(None). Raise
type
> > > error only if all of the type checks failed.
> > > > > > https://fedorahosted.org/freeipa/ticket/6061
> > > > NACK, this only hides the real issue, which is that
trustconfig-show
> > (and automember-set-default in #6037) claims to return the primary
key
> > of the object in the 'value' output field, but the object does not
have
> > a primary key, so the client rightfully expects None.
> > > > A proper fix would be to set "has_output =
output.simple_value" for
> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > > > Honza
> > > > The problem is that these commands do not return a simple
boolean in
> 'result' but a full entry dict, so 'simple_value' won't do the
trick in
> this case.
> > But I agree, we should rather fix misbehaving commands rather than
bend
> the framework to accomodate their idiosyncracies, we have enough of
that
> already.
> I am attaching a patch that adds a custom shim for misbehaving
commands so that they work as before. There is also a big fat warning
added to discourage implementation of such commands.

Let's go by this patch. I originally thought changing trust.py to simply
not supply 'value' field at all but this fix is simpler.


We found out that we still would need to handle
automember-default-group-{set,remove} commands which use the value to
print out summary, so it seems that we cannot win without some more
extensive fixing.



I have botched the wording in the comment, attaching updated patch.

--
Martin^3 Babinsky
From 5b45004ffac840aedec989728321afeeeff15ff6 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 18 Jul 2016 13:18:44 +0200
Subject: [PATCH] allow 'value' output param in commands without primary key

`PrimaryKey` output param works only for API objects that have primary keys,
otherwise it expects None (nothing is associated with this param). Since the
validation of command output was tightened durng thin client effort, some
commands not honoring this contract began to fail output validation.

A custom output was implemented for them to restore their functionality. It
should however be considered as a fix for broken commands and not used
further.

https://fedorahosted.org/freeipa/ticket/6037
https://fedorahosted.org/freeipa/ticket/6061
---
 API.txt |  8 
 VERSION |  4 ++--
 ipalib/output.py| 10 ++
 ipaserver/plugins/automember.py |  3 +++
 ipaserver/plugins/trust.py  |  1 +
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/API.txt b/API.txt
index eb33c1fb7f94f5af45ec0b38fc7e45e484a1044e..cbe23f4bde3a29cf3f28a9e361f83e176ede08e0 100644
--- a/API.txt
+++ b/API.txt
@@ -144,7 +144,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_set/1
 args: 0,6,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -155,7 +155,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_show/1
 args: 0,4,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -164,7 +164,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_del/1
 args: 1,2,3
 arg: Str('cn+', cli_name='automember_rule')
@@ -5584,7 +5584,7 @@ option: StrEnum('trust_type', autofill=True, cli_name='type', default=u'ad', val
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: 

Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Martin Babinsky

On 07/19/2016 01:13 PM, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Martin Babinsky wrote:

On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > > > On 16.7.2016 12:46, 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. :)
> > > > > > --
> > > > > > Output entry elements may have multiple types allowed. We
need to check
> > > all of them to properly validate the output. Right now, thin client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),)
tuple
> > > on the thin client side.
> > > > > > Change validation to account this by processing each
separate type
> > > of the element and account for both None and type(None). Raise type
> > > error only if all of the type checks failed.
> > > > > > https://fedorahosted.org/freeipa/ticket/6061
> > > > NACK, this only hides the real issue, which is that
trustconfig-show
> > (and automember-set-default in #6037) claims to return the primary
key
> > of the object in the 'value' output field, but the object does not
have
> > a primary key, so the client rightfully expects None.
> > > > A proper fix would be to set "has_output =
output.simple_value" for
> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > > > Honza
> > > > The problem is that these commands do not return a simple
boolean in
> 'result' but a full entry dict, so 'simple_value' won't do the trick in
> this case.
> > But I agree, we should rather fix misbehaving commands rather than
bend
> the framework to accomodate their idiosyncracies, we have enough of
that
> already.
> I am attaching a patch that adds a custom shim for misbehaving
commands so that they work as before. There is also a big fat warning
added to discourage implementation of such commands.

Let's go by this patch. I originally thought changing trust.py to simply
not supply 'value' field at all but this fix is simpler.

We found out that we still would need to handle 
automember-default-group-{set,remove} commands which use the value to 
print out summary, so it seems that we cannot win without some more 
extensive fixing.


--
Martin^3 Babinsky

--
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] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Alexander Bokovoy

On Mon, 18 Jul 2016, Martin Babinsky wrote:

On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > 
> > On 16.7.2016 12:46, 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. :)
> > > 
> > > --
> > > 
> > > Output entry elements may have multiple types allowed. We need to check

> > > all of them to properly validate the output. Right now, thin client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),) tuple
> > > on the thin client side.
> > > 
> > > Change validation to account this by processing each separate type

> > > of the element and account for both None and type(None). Raise type
> > > error only if all of the type checks failed.
> > > 
> > > https://fedorahosted.org/freeipa/ticket/6061
> > 
> > NACK, this only hides the real issue, which is that trustconfig-show

> > (and automember-set-default in #6037) claims to return the primary key
> > of the object in the 'value' output field, but the object does not have
> > a primary key, so the client rightfully expects None.
> > 
> > A proper fix would be to set "has_output = output.simple_value" for

> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > 
> > Honza
> > 
> 
> The problem is that these commands do not return a simple boolean in

> 'result' but a full entry dict, so 'simple_value' won't do the trick in
> this case.
> 
> But I agree, we should rather fix misbehaving commands rather than bend

> the framework to accomodate their idiosyncracies, we have enough of that
> already.
> 
I am attaching a patch that adds a custom shim for misbehaving commands so 
that they work as before. There is also a big fat warning added to 
discourage implementation of such commands.

Let's go by this patch. I originally thought changing trust.py to simply
not supply 'value' field at all but this fix is simpler.

--
/ 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] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Jan Cholasta

On 19.7.2016 12:31, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 19.7.2016 11:36, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 19.7.2016 10:40, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 10:12, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to
check
all of them to properly validate the output. Right now, thin
client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),)
tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise
type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that
trustconfig-show
(and automember-set-default in #6037) claims to return the primary
key
of the object in the 'value' output field, but the object does not
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?


Because I took the liberty of not putting any extra effort just to
keep old hacks working on thin client. In this case,
output.PrimaryKey
was used for the 'value' output field before, which always allowed
unicode in addition to the primary key type. On thin client,
output.Output with the primary key type is used instead, which is
less
forgiving and uncovers bogus command definitions. (There aren't any
besides the ones already mentioned, I remember fixing them but the
commit got lost somehow. Oh well.)

This means thin client would not work against old server which would
return a non-primary key value. I think it is unacceptable.


Not true, as this only applies to servers with API schema (4.4+).

Only because thin client does not work against servers without API
schema at all:

[root@f24-master ~]# ipa -vv -e xmlrpc_uri=https://id.vda.li/ipa/xml
config-show
ipa: INFO: trying https://id.vda.li/ipa/json
ipa: INFO: Request: {
  "id": 0,"method": "ping","params": [
  [],{}
  ]
}
ipa: INFO: Response: {
  "error": null,"id": 0,"principal": "ad...@vda.li",
"result": {
  "messages": [
  {
  "code": 13001,"message": "API Version
number was not sent, forward compatibility not guaranteed. Assuming
server's API version, 2.156","name": "VersionMissing",
  "type": "warning"
  }
  ],"summary": "IPA server version 4.2.3. API version 2.156"
  },"version": "4.2.3"
}
ipa: INFO: Forwarding 'config_show/1' to json server
'https://id.vda.li/ipa/json'
ipa: INFO: Request: {
  "id": 0,"method": "config_show/1","params": [
  [],{
  "version": "2.210"
  }
  ]
}
ipa: INFO: Response: {
  "error": {
  "code": 905,"message": "unknown command 'config_show/1'",
  "name": "CommandError"
  },"id": 0,"principal": "ad...@vda.li","result": null,
"version": "4.2.3"
}
ipa: ERROR: unknown command 'config_show/1'


Works fine for me:

$ rpm -q freeipa-client
freeipa-client-4.4.0.201607180602GITa2891af3-0.fc24.x86_64

$ ipa ping
---
IPA server version 4.2.3. API version 2.156
---

$ ipa config-show
 Maximum username length: 32
 Home directory base: /home
 Default shell: /bin/sh
 Default users group: ipausers
 Default e-mail domain: abc.idm.lab.eng.brq.redhat.com
 Search time limit: -1
 Search size limit: -1
 User search fields: uid,givenname,sn,telephonenumber,ou,title
 Group search fields: cn,description
 Enable migration mode: FALSE
 Certificate Subject base: O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM
 Password Expiration Notification (days): 4
 Password plugin features: AllowNThash
 SELinux user map order:
guest_u:s0$xguest_u:s0$user_u:s0$staff_u:s0-s0:c0.c1023$unconfined_u:s0-s0:c0.c1023

 Default SELinux user: unconfined_u:s0-s0:c0.c1023
 Default PAC types: nfs:NONE, MS-PAC


Try removing ~/.cache/ipa/servers/ - if you
reinstalled the server in the last hour (the default cache TTL), it
should help.

Yes, this did help, thanks. I didn't reinstall the server at all but I
ran ipa command before adding CA cert of that install to my client's NSS
database. I guess it was the cache from that attempt that broke it.


Hmm, that does not sound right. If there was no CA cert for the server, 
then it wasn't possible to talk to it, so there should not have been any 
information cached for it. I will investigate further, but it might be a 
bug in caching.




Coming back to 

Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Alexander Bokovoy

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 19.7.2016 11:36, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 19.7.2016 10:40, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 10:12, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to
check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),)
tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary
key
of the object in the 'value' output field, but the object does not
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?


Because I took the liberty of not putting any extra effort just to
keep old hacks working on thin client. In this case, output.PrimaryKey
was used for the 'value' output field before, which always allowed
unicode in addition to the primary key type. On thin client,
output.Output with the primary key type is used instead, which is less
forgiving and uncovers bogus command definitions. (There aren't any
besides the ones already mentioned, I remember fixing them but the
commit got lost somehow. Oh well.)

This means thin client would not work against old server which would
return a non-primary key value. I think it is unacceptable.


Not true, as this only applies to servers with API schema (4.4+).

Only because thin client does not work against servers without API
schema at all:

[root@f24-master ~]# ipa -vv -e xmlrpc_uri=https://id.vda.li/ipa/xml
config-show
ipa: INFO: trying https://id.vda.li/ipa/json
ipa: INFO: Request: {
  "id": 0,"method": "ping","params": [
  [],{}
  ]
}
ipa: INFO: Response: {
  "error": null,"id": 0,"principal": "ad...@vda.li",
"result": {
  "messages": [
  {
  "code": 13001,"message": "API Version
number was not sent, forward compatibility not guaranteed. Assuming
server's API version, 2.156","name": "VersionMissing",
  "type": "warning"
  }
  ],"summary": "IPA server version 4.2.3. API version 2.156"
  },"version": "4.2.3"
}
ipa: INFO: Forwarding 'config_show/1' to json server
'https://id.vda.li/ipa/json'
ipa: INFO: Request: {
  "id": 0,"method": "config_show/1","params": [
  [],{
  "version": "2.210"
  }
  ]
}
ipa: INFO: Response: {
  "error": {
  "code": 905,"message": "unknown command 'config_show/1'",
  "name": "CommandError"
  },"id": 0,"principal": "ad...@vda.li","result": null,
"version": "4.2.3"
}
ipa: ERROR: unknown command 'config_show/1'


Works fine for me:

$ rpm -q freeipa-client
freeipa-client-4.4.0.201607180602GITa2891af3-0.fc24.x86_64

$ ipa ping
---
IPA server version 4.2.3. API version 2.156
---

$ ipa config-show
 Maximum username length: 32
 Home directory base: /home
 Default shell: /bin/sh
 Default users group: ipausers
 Default e-mail domain: abc.idm.lab.eng.brq.redhat.com
 Search time limit: -1
 Search size limit: -1
 User search fields: uid,givenname,sn,telephonenumber,ou,title
 Group search fields: cn,description
 Enable migration mode: FALSE
 Certificate Subject base: O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM
 Password Expiration Notification (days): 4
 Password plugin features: AllowNThash
 SELinux user map order: 
guest_u:s0$xguest_u:s0$user_u:s0$staff_u:s0-s0:c0.c1023$unconfined_u:s0-s0:c0.c1023
 Default SELinux user: unconfined_u:s0-s0:c0.c1023
 Default PAC types: nfs:NONE, MS-PAC


Try removing ~/.cache/ipa/servers/ - if you 
reinstalled the server in the last hour (the default cache TTL), it 
should help.

Yes, this did help, thanks. I didn't reinstall the server at all but I
ran ipa command before adding CA cert of that install to my client's NSS
database. I guess it was the cache from that attempt that broke it.

Coming back to trustconfig-show, older server response's 'value' field
is ignored. I guess we can actually remove the value completely because
it is of no use anywhere.

As to other commands without primary key and returning value, may be you
can revive your patch?
--
/ Alexander Bokovoy

--
Manage your 

Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Jan Cholasta

On 19.7.2016 11:36, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 19.7.2016 10:40, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 10:12, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to
check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),)
tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary
key
of the object in the 'value' output field, but the object does not
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?


Because I took the liberty of not putting any extra effort just to
keep old hacks working on thin client. In this case, output.PrimaryKey
was used for the 'value' output field before, which always allowed
unicode in addition to the primary key type. On thin client,
output.Output with the primary key type is used instead, which is less
forgiving and uncovers bogus command definitions. (There aren't any
besides the ones already mentioned, I remember fixing them but the
commit got lost somehow. Oh well.)

This means thin client would not work against old server which would
return a non-primary key value. I think it is unacceptable.


Not true, as this only applies to servers with API schema (4.4+).

Only because thin client does not work against servers without API
schema at all:

[root@f24-master ~]# ipa -vv -e xmlrpc_uri=https://id.vda.li/ipa/xml
config-show
ipa: INFO: trying https://id.vda.li/ipa/json
ipa: INFO: Request: {
   "id": 0,"method": "ping","params": [
   [],{}
   ]
}
ipa: INFO: Response: {
   "error": null,"id": 0,"principal": "ad...@vda.li",
"result": {
   "messages": [
   {
   "code": 13001,"message": "API Version
number was not sent, forward compatibility not guaranteed. Assuming
server's API version, 2.156","name": "VersionMissing",
   "type": "warning"
   }
   ],"summary": "IPA server version 4.2.3. API version 2.156"
   },"version": "4.2.3"
}
ipa: INFO: Forwarding 'config_show/1' to json server
'https://id.vda.li/ipa/json'
ipa: INFO: Request: {
   "id": 0,"method": "config_show/1","params": [
   [],{
   "version": "2.210"
   }
   ]
}
ipa: INFO: Response: {
   "error": {
   "code": 905,"message": "unknown command 'config_show/1'",
   "name": "CommandError"
   },"id": 0,"principal": "ad...@vda.li","result": null,
"version": "4.2.3"
}
ipa: ERROR: unknown command 'config_show/1'


Works fine for me:

$ rpm -q freeipa-client
freeipa-client-4.4.0.201607180602GITa2891af3-0.fc24.x86_64

$ ipa ping
---
IPA server version 4.2.3. API version 2.156
---

$ ipa config-show
  Maximum username length: 32
  Home directory base: /home
  Default shell: /bin/sh
  Default users group: ipausers
  Default e-mail domain: abc.idm.lab.eng.brq.redhat.com
  Search time limit: -1
  Search size limit: -1
  User search fields: uid,givenname,sn,telephonenumber,ou,title
  Group search fields: cn,description
  Enable migration mode: FALSE
  Certificate Subject base: O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM
  Password Expiration Notification (days): 4
  Password plugin features: AllowNThash
  SELinux user map order: 
guest_u:s0$xguest_u:s0$user_u:s0$staff_u:s0-s0:c0.c1023$unconfined_u:s0-s0:c0.c1023

  Default SELinux user: unconfined_u:s0-s0:c0.c1023
  Default PAC types: nfs:NONE, MS-PAC


Try removing ~/.cache/ipa/servers/ - if you reinstalled 
the server in the last hour (the default cache TTL), it should help.




Same happens for every other command so I cannot even test the behavior.
It works against the same server as the thin client is.

[root@f24-master ~]# ipa -vv config-show
ipa: INFO: trying https://f24-master.ipa.ad.test/ipa/json
ipa: INFO: Forwarding 'config_show/1' to json server
'https://f24-master.ipa.ad.test/ipa/json'
ipa: INFO: Request: {
   "id": 0,"method": "config_show/1","params": [
   [],{
   "version": "2.210"
   }
   ]
}
ipa: INFO: Response: {
   "error": null,"id": 0,

Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Alexander Bokovoy

On Tue, 19 Jul 2016, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 19.7.2016 10:40, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 10:12, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to
check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary key
of the object in the 'value' output field, but the object does not
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?


Because I took the liberty of not putting any extra effort just to
keep old hacks working on thin client. In this case, output.PrimaryKey
was used for the 'value' output field before, which always allowed
unicode in addition to the primary key type. On thin client,
output.Output with the primary key type is used instead, which is less
forgiving and uncovers bogus command definitions. (There aren't any
besides the ones already mentioned, I remember fixing them but the
commit got lost somehow. Oh well.)

This means thin client would not work against old server which would
return a non-primary key value. I think it is unacceptable.


Not true, as this only applies to servers with API schema (4.4+).

Only because thin client does not work against servers without API
schema at all:

[root@f24-master ~]# ipa -vv -e xmlrpc_uri=https://id.vda.li/ipa/xml config-show
ipa: INFO: trying https://id.vda.li/ipa/json
ipa: INFO: Request: {
  "id": 0,"method": "ping","params": [
  [],{}
  ]
}
ipa: INFO: Response: {
  "error": null,"id": 0,"principal": "ad...@vda.li",
"result": {

  "messages": [
  {
  "code": 13001,"message": "API Version 
number was not sent, forward compatibility not guaranteed. Assuming 
server's API version, 2.156","name": "VersionMissing",
"type": "warning"

  }
  ],"summary": "IPA server version 4.2.3. API version 
2.156"

  },"version": "4.2.3"
}
ipa: INFO: Forwarding 'config_show/1' to json server 
'https://id.vda.li/ipa/json'
ipa: INFO: Request: {
  "id": 0,"method": "config_show/1","params": [
  [],{
  "version": "2.210"
  }
  ]
}
ipa: INFO: Response: {
  "error": {
  "code": 905,"message": "unknown command 
'config_show/1'","name": "CommandError"
  },"id": 0,"principal": "ad...@vda.li","result": null,
"version": "4.2.3"

}
ipa: ERROR: unknown command 'config_show/1'

Same happens for every other command so I cannot even test the behavior.
It works against the same server as the thin client is.

[root@f24-master ~]# ipa -vv config-show
ipa: INFO: trying https://f24-master.ipa.ad.test/ipa/json
ipa: INFO: Forwarding 'config_show/1' to json server 
'https://f24-master.ipa.ad.test/ipa/json'
ipa: INFO: Request: {
  "id": 0,"method": "config_show/1","params": [
  [],{
  "version": "2.210"
  }
  ]
}
ipa: INFO: Response: {
  "error": null,"id": 0,"principal": "ad...@ipa.ad.test",
"result": {

  "result": {
  "ca_renewal_master_server": "f24-master.ipa.ad.test",
"ca_server_server": [

  "f24-master.ipa.ad.test"
  ],"dn": 
"cn=ipaConfig,cn=etc,dc=ipa,dc=ad,dc=test",
"ipa_master_server": [

  "f24-master.ipa.ad.test"
  ],"ipacertificatesubjectbase": [
  "O=IPA.AD.TEST"
  ],"ipaconfigstring": [
  "AllowNThash"
  ],"ipadefaultemaildomain": [
  "ipa.ad.test"
  ],"ipadefaultloginshell": [
  "/bin/sh"
  ],"ipadefaultprimarygroup": [
  "ipausers"
  ],"ipagroupsearchfields": [
  "cn,description"
  ],"ipahomesrootdir": [
  "/home"
  ],"ipakrbauthzdata": [
  "nfs:NONE","MS-PAC"
  ],"ipamaxusernamelength": [
  "32"
  ],"ipamigrationenabled": 

Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Alexander Bokovoy

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 19.7.2016 10:40, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 10:12, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to
check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary key
of the object in the 'value' output field, but the object does not
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?


Because I took the liberty of not putting any extra effort just to
keep old hacks working on thin client. In this case, output.PrimaryKey
was used for the 'value' output field before, which always allowed
unicode in addition to the primary key type. On thin client,
output.Output with the primary key type is used instead, which is less
forgiving and uncovers bogus command definitions. (There aren't any
besides the ones already mentioned, I remember fixing them but the
commit got lost somehow. Oh well.)

This means thin client would not work against old server which would
return a non-primary key value. I think it is unacceptable.


Not true, as this only applies to servers with API schema (4.4+).

Only because thin client does not work against servers without API
schema at all:

[root@f24-master ~]# ipa -vv -e xmlrpc_uri=https://id.vda.li/ipa/xml config-show
ipa: INFO: trying https://id.vda.li/ipa/json
ipa: INFO: Request: {
   "id": 0, 
   "method": "ping", 
   "params": [
   [], 
   {}

   ]
}
ipa: INFO: Response: {
   "error": null, 
   "id": 0, 
   "principal": "ad...@vda.li", 
   "result": {

   "messages": [
   {
   "code": 13001, 
   "message": "API Version number was not sent, forward compatibility not guaranteed. Assuming server's API version, 2.156", 
   "name": "VersionMissing", 
   "type": "warning"

   }
   ], 
   "summary": "IPA server version 4.2.3. API version 2.156"
   }, 
   "version": "4.2.3"

}
ipa: INFO: Forwarding 'config_show/1' to json server 
'https://id.vda.li/ipa/json'
ipa: INFO: Request: {
   "id": 0, 
   "method": "config_show/1", 
   "params": [
   [], 
   {

   "version": "2.210"
   }
   ]
}
ipa: INFO: Response: {
   "error": {
   "code": 905, 
   "message": "unknown command 'config_show/1'", 
   "name": "CommandError"
   }, 
   "id": 0, 
   "principal": "ad...@vda.li", 
   "result": null, 
   "version": "4.2.3"

}
ipa: ERROR: unknown command 'config_show/1'

Same happens for every other command so I cannot even test the behavior.
It works against the same server as the thin client is.

[root@f24-master ~]# ipa -vv config-show
ipa: INFO: trying https://f24-master.ipa.ad.test/ipa/json
ipa: INFO: Forwarding 'config_show/1' to json server 
'https://f24-master.ipa.ad.test/ipa/json'
ipa: INFO: Request: {
   "id": 0, 
   "method": "config_show/1", 
   "params": [
   [], 
   {

   "version": "2.210"
   }
   ]
}
ipa: INFO: Response: {
   "error": null, 
   "id": 0, 
   "principal": "ad...@ipa.ad.test", 
   "result": {

   "result": {
   "ca_renewal_master_server": "f24-master.ipa.ad.test", 
   "ca_server_server": [

   "f24-master.ipa.ad.test"
   ], 
   "dn": "cn=ipaConfig,cn=etc,dc=ipa,dc=ad,dc=test", 
   "ipa_master_server": [

   "f24-master.ipa.ad.test"
   ], 
   "ipacertificatesubjectbase": [

   "O=IPA.AD.TEST"
   ], 
   "ipaconfigstring": [

   "AllowNThash"
   ], 
   "ipadefaultemaildomain": [

   "ipa.ad.test"
   ], 
   "ipadefaultloginshell": [

   "/bin/sh"
   ], 
   "ipadefaultprimarygroup": [

   "ipausers"
   ], 
   "ipagroupsearchfields": [

   "cn,description"
   ], 
   "ipahomesrootdir": [

   "/home"
   ], 
   "ipakrbauthzdata": [
   "nfs:NONE", 
   "MS-PAC"
   ], 
   "ipamaxusernamelength": [

   "32"
 

Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Jan Cholasta

On 19.7.2016 10:40, Alexander Bokovoy wrote:

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 10:12, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to
check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary key
of the object in the 'value' output field, but the object does not
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?


Because I took the liberty of not putting any extra effort just to
keep old hacks working on thin client. In this case, output.PrimaryKey
was used for the 'value' output field before, which always allowed
unicode in addition to the primary key type. On thin client,
output.Output with the primary key type is used instead, which is less
forgiving and uncovers bogus command definitions. (There aren't any
besides the ones already mentioned, I remember fixing them but the
commit got lost somehow. Oh well.)

This means thin client would not work against old server which would
return a non-primary key value. I think it is unacceptable.


Not true, as this only applies to servers with API schema (4.4+).




Aside from that, comparing "(type(None),) is None" will never give you
True on the thin client side. At the server side we have "None is None"
and that works. So the question is also why there is a change like that
between client and server sides?


I don't see how this has anything to do with anything. In output
validation, "type = None" means that value of any type is allowed,
"type = (type(None),)" means that only None is allowed. Nothing
changed with regard to that in thin client.

Really? Previous code worked against corresponding server, new code
doesn't work against the very same server. I consider it a breakage.


Really, since commit b6e4972e. If something is broken, please file a ticket.

--
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] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Alexander Bokovoy

On Tue, 19 Jul 2016, Jan Cholasta wrote:

On 18.7.2016 10:12, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary key
of the object in the 'value' output field, but the object does not
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?


Because I took the liberty of not putting any extra effort just to 
keep old hacks working on thin client. In this case, output.PrimaryKey 
was used for the 'value' output field before, which always allowed 
unicode in addition to the primary key type. On thin client, 
output.Output with the primary key type is used instead, which is less 
forgiving and uncovers bogus command definitions. (There aren't any 
besides the ones already mentioned, I remember fixing them but the 
commit got lost somehow. Oh well.)

This means thin client would not work against old server which would
return a non-primary key value. I think it is unacceptable.


Aside from that, comparing "(type(None),) is None" will never give you
True on the thin client side. At the server side we have "None is None"
and that works. So the question is also why there is a change like that
between client and server sides?


I don't see how this has anything to do with anything. In output 
validation, "type = None" means that value of any type is allowed, 
"type = (type(None),)" means that only None is allowed. Nothing 
changed with regard to that in thin client.

Really? Previous code worked against corresponding server, new code
doesn't work against the very same server. I consider it a breakage.

--
/ 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] 0210 frontend: fix output validation for multiple type choices

2016-07-19 Thread Jan Cholasta

On 18.7.2016 10:12, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary key
of the object in the 'value' output field, but the object does not
have a primary key, so the client rightfully expects None.

Why did it work before introducing thin client?


Because I took the liberty of not putting any extra effort just to keep 
old hacks working on thin client. In this case, output.PrimaryKey was 
used for the 'value' output field before, which always allowed unicode 
in addition to the primary key type. On thin client, output.Output with 
the primary key type is used instead, which is less forgiving and 
uncovers bogus command definitions. (There aren't any besides the ones 
already mentioned, I remember fixing them but the commit got lost 
somehow. Oh well.)




Aside from that, comparing "(type(None),) is None" will never give you
True on the thin client side. At the server side we have "None is None"
and that works. So the question is also why there is a change like that
between client and server sides?


I don't see how this has anything to do with anything. In output 
validation, "type = None" means that value of any type is allowed, "type 
= (type(None),)" means that only None is allowed. Nothing changed with 
regard to that in thin client.





A proper fix would be to set "has_output = output.simple_value" for
these commands (all of automember_default_group_{set,remove,show},
trustconfig_{mod,show}).

Honza

--
Jan Cholasta





--
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] 0210 frontend: fix output validation for multiple type choices

2016-07-18 Thread Martin Babinsky

On 07/18/2016 12:29 PM, Martin Babinsky wrote:

On 07/18/2016 10:01 AM, Jan Cholasta wrote:

Hi,

On 16.7.2016 12:46, 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. :)

--

Output entry elements may have multiple types allowed. We need to check
all of them to properly validate the output. Right now, thin client
receives type specifications for elements as tuples of types, so
what is seen as 'None' on the server side becomes (type(None),) tuple
on the thin client side.

Change validation to account this by processing each separate type
of the element and account for both None and type(None). Raise type
error only if all of the type checks failed.

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


NACK, this only hides the real issue, which is that trustconfig-show
(and automember-set-default in #6037) claims to return the primary key
of the object in the 'value' output field, but the object does not have
a primary key, so the client rightfully expects None.

A proper fix would be to set "has_output = output.simple_value" for
these commands (all of automember_default_group_{set,remove,show},
trustconfig_{mod,show}).

Honza



The problem is that these commands do not return a simple boolean in
'result' but a full entry dict, so 'simple_value' won't do the trick in
this case.

But I agree, we should rather fix misbehaving commands rather than bend
the framework to accomodate their idiosyncracies, we have enough of that
already.

I am attaching a patch that adds a custom shim for misbehaving commands 
so that they work as before. There is also a big fat warning added to 
discourage implementation of such commands.


--
Martin^3 Babinsky
From 1f3dd199bb1d0c02c70d099884d13dde6277253c Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 18 Jul 2016 13:18:44 +0200
Subject: [PATCH] allow 'value' output param in commands without primary key

`PrimaryKey` output param works only for API objects that have primary keys,
otherwise it expects None (nothing is associated with this param). Since the
validation of command output was tightened durng thin client effort, some
commands not honoring this contract began to fail output validation.

A custom output was implemented for them to restore their functionality. It
should however be considered as a fix for broken commands and not used
further.

https://fedorahosted.org/freeipa/ticket/6037
https://fedorahosted.org/freeipa/ticket/6061
---
 API.txt |  8 
 VERSION |  4 ++--
 ipalib/output.py| 10 ++
 ipaserver/plugins/automember.py |  3 +++
 ipaserver/plugins/trust.py  |  1 +
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/API.txt b/API.txt
index eb33c1fb7f94f5af45ec0b38fc7e45e484a1044e..cbe23f4bde3a29cf3f28a9e361f83e176ede08e0 100644
--- a/API.txt
+++ b/API.txt
@@ -144,7 +144,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_set/1
 args: 0,6,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -155,7 +155,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_show/1
 args: 0,4,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -164,7 +164,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_del/1
 args: 1,2,3
 arg: Str('cn+', cli_name='automember_rule')
@@ -5584,7 +5584,7 @@ option: StrEnum('trust_type', autofill=True, cli_name='type', default=u'ad', val
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: trustdomain_add/1
 args: 2,8,3
 arg: Str('trustcn', cli_name='trust')
diff --git a/VERSION b/VERSION
index 0559741451a858dd0adfa99a8bf653261d771601..ca489965050f32d2d8987dfd251ec2b2a0ba1768 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=210
-# Last change: Add --ca option to cert-status
+IPA_API_VERSION_MINOR=211
+# Last change: mbabinsk: allow 'value' output param in commands without primary key
diff --git a/ipalib/output.py b/ipalib/output.py
index