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

Tue Feb 19 11:15:32 UTC 2013

On 02/13/2013 11:18 AM, Petr Viktorin wrote:
> 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.
>
>

Rebasing patch 104 to current master.

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0104-02-Add-the-version-option-to-all-Commands.patch
Type: text/x-patch
Size: 68082 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130219/da2367be/attachment.bin>