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

[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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0189.2-allow-value-output-param-in-commands-without-primary.patch
Type: text/x-patch
Size: 6725 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160720/e9fdd37f/attachment.bin>