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

Petr Viktorin pviktori at redhat.com
Wed Feb 13 10:18:28 UTC 2013


On 01/29/2013 05:06 PM, Petr Viktorin wrote:
> 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.
>

Patches 106 & 115 need an API version bump.
I also noticed that `makeapi --validate` wasn't checking capabilities 
properly. Fixed.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0106-04-Add-client-capabilities-enable-messages.patch
Type: text/x-patch
Size: 20543 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130213/919f2613/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0115-03-Rename-the-messages-Output-of-the-i18n_messages-comm.patch
Type: text/x-patch
Size: 3739 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130213/919f2613/attachment-0001.bin>


More information about the Freeipa-devel mailing list