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

[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