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

Wed Feb 20 16:17:33 UTC 2013

On 02/19/2013 12:15 PM, Petr Viktorin wrote:
> 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.
> 

Patch 104 and 106 needs a rebase.

Generally I think this patchset is OK and we will need it as a foundation for
other features.

I may have done my custom rebasing wrong, but my WebUI stopped working with
these patches. I see this in error_log:

[Wed Feb 20 10:38:22.351845 2013] [auth_kerb:error] [pid 22172] [client
10.34.4.72:40777]               gss_display_name() failed: A required input
parameter could not be read: An invalid name was supplied   (, Unknown error),
referer: https://vm-037.idm.lab.bos.redhat.com/ipa/ui/

I also saw one failed test case:
======================================================================
FAIL: Try user_show with no version
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/root/freeipa-master/tests/test_xmlrpc/test_user_plugin.py", line 1760,
in test_user_show_without_version
    assert res['messages'] == (expected_message.to_dict(), )
AssertionError

Martin