[Freeipa-devel] [PATCHES] 0104-0106 Provide means of displaying warning and informational messages on clients
Petr Viktorin
pviktori at redhat.com
Thu Feb 21 14:09:20 UTC 2013
On 02/21/2013 02:06 PM, Martin Kosek wrote:
> On 02/21/2013 12:50 PM, Petr Viktorin wrote:
>> On 02/20/2013 05:17 PM, Martin Kosek wrote:
>>> 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
>>>
>>
>> Here are the rebased patches.
>> Please test from a clean tree, the UI stopped working here when I had some
>> stale files around.
>>
>
> Thanks, everything looks good except our first message (VersionMissing) which
> is sent when I run commands via JSON-RPC which do not have any version
> attached. Now, I get this error:
>
> "messages": [
> {
> "code": 13001,
> "message": "API Version number was not sent. Assuming client
> version 2.47. Current API version is 2.53",
> "name": "VersionMissing",
> "type": "warning"
> }
> ],
>
> There are 2 issues with this message wording:
> 1) 2.47 is deprecated, I assume you wanted VERSION_WITHOUT_CAPABILITIES
> 2) We assume API_VERSION (current API version) and not VERSION_WITHOUT_CAPABILITIES
>
> Martin
>
Fixed, thanks for the catch.
--
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0104-04-Add-the-version-option-to-all-Commands.patch
Type: text/x-patch
Size: 68325 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130221/cc64a78f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0105-04-Add-ipalib.messages.patch
Type: text/x-patch
Size: 17152 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130221/cc64a78f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0106-06-Add-client-capabilities-enable-messages.patch
Type: text/x-patch
Size: 20500 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130221/cc64a78f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0115-05-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/20130221/cc64a78f/attachment-0003.bin>
More information about the Freeipa-devel
mailing list