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

[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