[Freeipa-devel] [PATCHES] 0104-0106 Provide means of displaying warning and informational messages on clients

Fri Jan 4 18:20:15 UTC 2013

On 12/14/2012 09:04 AM, Jan Cholasta wrote:
> On 13.12.2012 18:09, Petr Viktorin wrote:
>> On 12/13/2012 04:43 PM, Martin Kosek wrote:
>>> On 12/13/2012 10:59 AM, Petr Viktorin wrote:
>>>> It's time to give this to another set of eyes :)
>>>>
>>>> Design document: http://freeipa.org/page/V3/Messages
>>>> Ticket: https://fedorahosted.org/freeipa/ticket/2732
>>>>
>>>> More info is in commit messages.
>>>>
>>>>
>>>> Because of https://fedorahosted.org/freeipa/ticket/3294, I needed to
>>>> change the
>>>> design document: when the client doesn't send the API version, it is
>>>> assumed
>>>> it's at a version before capabilities were introduced (i.e. 2.47).
>>>> The client still gets a warning if the version is missing. Except for
>>>> those
>>>> commands where IPA didn't send a version -- ping, cert-show, etc. --
>>>> the
>>>> warning wouldn't pass validation on old clients. (I'm assuming that
>>>> our client
>>>> is so far the only one that validates so strictly.)
>>>
>>> I did a basic test of this patch and also quickly read through the
>>> patches and
>>> besides nitpicks (like unused inspect module in
>>> tests/test_ipalib/test_messages.py in patch 0105) I did not find any
>>> obvious
>>> errors in the Python code.
>>
>> Noted, will fix in future versions of the patch.
>>
>>>
>>> However, this patch breaks WebUI badly, I did not even get to a log in
>>> screen.
>>> Cooperation with Petr Vobornik will be needed. In my case, I got blank
>>> screen
>>> and Javascript error:
>>>
>>> TypeError: IPA.messages.dialogs is undefined
>>> https://vm-037.idm.lab.bos.redhat.com/ipa/ui/ipa.js
>>> Line 1460
>>>
>>> I assume this is related to the Internal Error that was returned in
>>> the JSON call
>>>
>>> {
>>>      "error": null,
>>>      "id": null,
>>>      "principal": "admin at IDM.LAB.BOS.REDHAT.COM",
>>>      "result": {
>>>          "count": 5,
>>>          "results": [
>>>              {
>>>                  "error": "an internal error has occurred",
>>>                  "error_code": 903,
>>>                  "error_name": "InternalError"
>>>              },
>>>              {
>>> ...
>>>
>>> This can be reproduced with:
>>>
>>> # curl -v -H "Content-Type:application/json" -H
>>> "referer:https://`hostname`/ipa" -H "Accept:applicaton/json"
>>> --negotiate -u :
>>> --cacert /etc/ipa/ca.crt -d
>>> '{"method":"i18n_messages","params":[[],{}],"id":0}' -X POST
>>> https://`hostname`/ipa/json
>>
>> Good catch! The i18n_messages plugin already defines a "messages"
>> output. When I renamed this from "warnings" to "messages" I forgot to
>> check for clashes.
>> Since i18n_messages is an internal command only used by the Web UI, we
>> can rename its output to "texts" without breaking compatibility.
>>
>> I'm attaching a preliminary fix (for both backend and UI), but hopefully
>> it won't be necessary, see below.
>>
>>> I am also not sure I like the requirement of a specific version option
>>> to be
>>> always passed. I would prefer that missing version option would mean
>>> "I use the
>>> most recent version of API" instead - it would make the custom
>>> JSONRPC/XMLRPC
>>> calls easier to use.
>>>
>>> But since the version option was not being sent for some commands, we
>>> may not
>>> have a choice anyway if we do not want to break old clients in case we
>>> add some
>>> capabilities to these commands.
>>>
>>
>> I see three other options, all worse:
>> - Do not use capabilities for the affected commands, meaning no new
>> functionality can be added to them (and by extension, no new
>> functionality common to all commands can be added).
>> - Treat a missing version as the current version
>> - Break backwards compatibility
>>
>> And one possibly better (thanks to Petr¹ and Martin for opening my eyes
>> off-list!):
>> - Deprecate XML-RPC. All XML-RPC requests would be pinned to current
>> version (2.47). Capabilities/messages would only apply to JSON-RPC.
>>
>> This would also allow us to solve the above name-clashing problem
>> elegantly. Here is a reminder of what a JSON response looks like:
>>
>> {
>>      "error": null,
>>      "id": 0,
>>      "principal": "admin at IDM.LAB.BOS.REDHAT.COM",
>>      "result": {
>>          "summary": "IPA server version 3.1.0GIT2e4bd02. API version
>> 2.47"
>>      },
>>      "version": "3.1.0GIT2e4bd02"
>> }
>>
>> A XML-RPC response only contains the "result" part of that.
>> So with JSON, we can put the messages in the top level, which is much
>> better design.

Custom info in the "top level" seems to be a violation of the JSON-RPC 
spec. I'd rather not do more of those, so I'm withdrawing this idea.

>>
>> XML-RPC sucks in other ways too. We already have a workaround for its
>> inability to attach extra info to errors (commit
>> 88262a75ffe7a25640333dcc4da2100830cae821, Add instructions support to
>> PublicError).
>>
>> I've opened a RFC here: https://fedorahosted.org/freeipa/ticket/3299.
>>
>
> +1, XML-RPC sucks. This should have been done a long time ago.
>
> Honza
>

Here are new patches.

XML-RPC requests with missing version are assumed to be old (the version 
before capabilities are introduced, 2.47). This takes care of backcompat 
with clients with bug 3294.
JSON-RPC requests with missing version are assumed to be testing calls 
(e.g. curl), they get behavior of the latest version but also a warning.
I've also added this info to the design doc.

It turns out that these patches don't depend on whether our client uses 
XML-RPC or JSON-RPC. If/when it supports both, I'll be able to add some 
extra unit tests.

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0105-02-Add-ipalib.messages.patch
Type: text/x-patch
Size: 17068 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130104/baa47d7b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0106-02-Add-client-capabilities-enable-messages.patch
Type: text/x-patch
Size: 18949 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130104/baa47d7b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0115-02-Rename-the-messages-Output-of-the-i18n_messages-comm.patch
Type: text/x-patch
Size: 3691 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130104/baa47d7b/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0104-Add-the-version-option-to-all-Commands.patch
Type: text/x-patch
Size: 68089 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130104/baa47d7b/attachment-0003.bin>