[Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices
Alexander Bokovoy
abokovoy at redhat.com
Tue Jul 19 09:36:10 UTC 2016
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 at 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": "admin at 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": "admin at 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 at 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": "admin at 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": [
"FALSE"
],
"ipapwdexpadvnotify": [
"4"
],
"ipasearchrecordslimit": [
"100"
],
"ipasearchtimelimit": [
"2"
],
"ipaselinuxusermapdefault": [
"unconfined_u:s0-s0:c0.c1023"
],
"ipaselinuxusermaporder": [
"guest_u:s0$xguest_u:s0$user_u:s0$staff_u:s0-s0:c0.c1023$unconfined_u:s0-s0:c0.c1023"
],
"ipausersearchfields": [
"uid,givenname,sn,telephonenumber,ou,title"
],
"ntp_server_server": [
"f24-master.ipa.ad.test"
]
},
"summary": null,
"value": null
},
"version": "4.4.0.201607151226GIT37bfd1f"
}
Maximum username length: 32
Home directory base: /home
Default shell: /bin/sh
Default users group: ipausers
Default e-mail domain: ipa.ad.test
Search time limit: 2
Search size limit: 100
User search fields: uid,givenname,sn,telephonenumber,ou,title
Group search fields: cn,description
Enable migration mode: FALSE
Certificate Subject base: O=IPA.AD.TEST
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
IPA masters: f24-master.ipa.ad.test
IPA CA servers: f24-master.ipa.ad.test
IPA NTP servers: f24-master.ipa.ad.test
IPA CA renewal master: f24-master.ipa.ad.test
>
>>
>>>>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.
See above. How should I test?
--
/ Alexander Bokovoy
More information about the Freeipa-devel
mailing list