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": "ad...@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": "ad...@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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel