GNOME Bugzilla – Bug 701657
Add rest_proxy_call_set_content
Last modified: 2021-05-25 12:45:59 UTC
Created attachment 246094 [details] [review] Patch to add rest_proxy_call_set_content I really don't know how common this case is, but I've been experimenting with the TweetMarker API[1] and it requires me to make an OAuth Echo proxy and set the prox call's content(although the linked documentation is kinda unclear about this imo, but tests have shown that it's the only way). Also, Twitter's OAuth Echo documentation[2] say that "The POST should contain[…] two additional items as HTTP headers: X-Auth-Service-Provider, X-Verify-Credentials-Authorization", so the following change made my code really work: diff --git a/rest/oauth-proxy-call.c b/rest/oauth-proxy-call.c index 63c859f..cf48fde 100644 --- a/rest/oauth-proxy-call.c +++ b/rest/oauth-proxy-call.c @@ -298,7 +298,7 @@ _prepare (RestProxyCall *call, GError **error) s = make_authorized_header (oauth_params); if (priv->oauth_echo) { rest_proxy_call_add_header (call, "X-Verify-Credentials-Authorization", s); - rest_proxy_call_add_param (call, "X-Auth-Service-Provider", priv->service_url); + rest_proxy_call_add_header (call, "X-Auth-Service-Provider", priv->service_url); } else { rest_proxy_call_add_header (call, "Authorization", s); } -- 1.8.2.2 I don't know if that is a bug or not, I also don't know of any application using librest *and* doing any OAuth Echo call and there's no example/test for it as far as I can see, so testing if that breaks anything is kinda hard. [1] https://github.com/manton/tweetmarker/blob/master/documentation/v2.md [2] https://dev.twitter.com/docs/auth/oauth/oauth-echo
Any news/opinions about this?
Your "also..." comment is correct, please can you submit this as another patch. I think that _set_content() shouldn't assume text/plain but take a byte array/size/mime-type, so that you can use it for generic posting in situations where the service wants data but doesn't want the full MIME thing.
Created attachment 250487 [details] [review] OAuthProxyCall: Set the X-Auth-Service-Provider as header and not as parameter
Created attachment 250496 [details] [review] RestProxyCall:Add rest_proxy_call_set_content This patch is similar to the previous one but _set_content now also takes a mimetype and size argument. Testing oauth proxies is horrible(due to dependencies 2 external services) but I quickly performed a tweetmarker test request and it worked.
Any objections on this?
ping
Created attachment 290628 [details] [review] RestProxyCall: Add rest_proxy_call_set_content
Any news on this? There is a working patch attached to this report.
I'm wondering if it would be possible/better to add a way to achieve that through RestParam, similar to what you can do with RestParam *param = rest_param_new_full("foo", REST_MEMORY_COPY, "barbaz", 7, "x-foo/x-bar", NULL); rest_proxy_call_add_param_full (call, param); How is this supposed to interact with serialize_params and multipart messages (generated using the API mentioned above)? At the moment, I think it will overwrite what is returned by serialize_params, and behaviour is weird with multipart message (this changes Content-Type, but not the body, could be a libsoup bug). Imo the behaviour should be consistent. Ideally we would not overwrite any of these, but we should at least have a warning when this happens (?) Should set_content have a RestMemoryUse parameter ?
(In reply to Christophe Fergeau from comment #9) > I'm wondering if it would be possible/better to add a way to achieve that > through RestParam, similar to what you can do with > > RestParam *param = rest_param_new_full("foo", REST_MEMORY_COPY, "barbaz", 7, > "x-foo/x-bar", NULL); > rest_proxy_call_add_param_full (call, param); Not sure how the API would really look. Would you look at the content_type and just use the data of a x-proxy-call-contents (or similar) param as the call's contents? What happens when adding multiple such RestParams? > How is this supposed to interact with serialize_params and multipart > messages (generated using the API mentioned above)? At the moment, I think > it will overwrite what is returned by serialize_params, and behaviour is > weird with multipart message (this changes Content-Type, but not the body, > could be a libsoup bug). Yes, it seems to just override anything serialize_params does (haven't looked at the patch myself in a while...). I mean, *set*_content is pretty clear about the behavior (i.e. it would set/overwrite all contents) and I don't know if an API using both normal params *and* a custom content can ever work or exists. > Imo the behaviour should be consistent. Ideally we would not overwrite any > of these, but we should at least have a warning when this happens (?) Not overwrite what? I mean, we have to overwrite the message's contents of course, I guess you mean the other parameters? > Should set_content have a RestMemoryUse parameter ? You tell me? :) I'm not overly familiar with the libsoup API, I guess that's just an optimization to not copy the data if that's not necessary?
I discussed this on IRC with Timm to get more details about the use case of this new method. In particular I asked if subclassing RestProxyCall and overriding RestProxyCall::serialize would be enough for what this TweetMarker interaction. This is what https://git.gnome.org/browse/libgovirt/tree/govirt/ovirt-action-rest-call.c is doing to serialize RestParams to XML and set the request body to contain that XML data.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/librest/-/issues/ Thank you for your understanding and your help.