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

Tue Jan 29 16:06:15 UTC 2013

On 01/04/2013 07:20 PM, Petr Viktorin wrote:
> 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.
>

Patch 106 had a minor conflict with master, attaching fixed version.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0106-03-Add-client-capabilities-enable-messages.patch
Type: text/x-patch
Size: 18925 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130129/cd040950/attachment.bin>