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

[Jan Cholasta]

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.
>
> 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

-- 
Jan Cholasta