GNOME Bugzilla – Bug 658937
librest: add explict username/password support
Last modified: 2012-06-28 10:34:15 UTC
Importing from meego bugzilla: https://bugs.meego.com/show_bug.cgi?id=9061 "There should be a way of setting the HTTP Auth username/password easily on a RestProxy. "
Created attachment 203091 [details] [review] add username/password support
Review of attachment 203091 [details] [review]: Looks good, please commit. Long-term I'd like to also see something that exposes the authenticate signal, but this will do for now.
Attachment 203091 [details] pushed as 649671b - add username/password support
Created attachment 215759 [details] [review] Avoid infinite loop with wrong HTTP credentials When provided with wrong credentials, libsoup will try to connect and emit its 'authenticate' signal as long as its callback calls soup_auth_authenticate. It will fail the request and report to the caller if this function is not called. Since the 'retrying' parameter to the 'authenticate' callback lets us know when the credentials we provided are the wrong ones, this commit makes sure we stop calling soup_auth_authenticate after trying the credentials once. Without this, libsoup will try the same request again and again without ever returning when provided with wrong credentials.
Review of attachment 215759 [details] [review]: shouldn't it signal to the client that the credentials are incorrect instead of silently failing? Is it already doing that somehow?
It's already doing that, this is what I meant with "It will fail the request and report to the caller if this function is not called", I can try to make this more obvious in the log. http://developer.gnome.org/libsoup/stable/libsoup-client-howto.html#id603393 "If the server doesn't accept the username and password provided, the session will emit authenticate again, with the retrying parameter set to TRUE. This lets the application know that the information it provided earlier was incorrect, and gives it a chance to try again. If this username/password pair also doesn't work, the session will contine to emit authenticate again and again until the provided username/password successfully authenticates, or until the signal handler fails to call soup_auth_authenticate, at which point libsoup will allow the message to fail (with status 401 or 407). " I've also checked that with this patch applied, calling rest_proxy_call_sync with wrong credentials returns an error, and the GError message says "Unauthorized"
Review of attachment 215759 [details] [review]: Sounds good then.
Attachment 215759 [details] pushed as d2e81c4 - Avoid infinite loop with wrong HTTP credentials
Created attachment 216181 [details] [review] Add RestProxy::authenticate signal If caught by application, this signal can be used to set the credentials to use when authentication is needed. If not caught, librest behaviour will be unchanged (try to use what the username/password properties were set to first, and don't try to reuse them if this fails).
Review of attachment 216181 [details] [review]: Good stuff.
One nice thing that is possible with SoupSession::authenticate which is not made possible by this patch is this: « If you need to handle authentication asynchronously (eg, to pop up a password dialog without recursively entering the main loop), you can do that as well. Just call soup_session_pause_message on the message before returning from the signal handler, and g_object_ref the SoupAuth. Then, later on, after calling soup_auth_authenticate (or deciding not to), call soup_session_unpause_message to resume the paused message. » If want to add support for this, we probably need at least 1 additional RestAuth arg to the signal, doing this would break the ABI introduced by this patch. With the change which make the "username" and "password" properties non-CONSTRUCT_ONLY, the git-only rest_proxy_new_with_auth method is a bit redundant, but it's a bit more obvious how to do auth if the default behaviour is good enough.
Created attachment 216262 [details] [review] Introduce RestProxyAuth type This will be used by the RestProxy authentication code to be able to "pause" the sending of the current message. This will give applications the opportunity to get back to the main loop to do the authentication work before resuming the current request.
Created attachment 216263 [details] [review] Add rest_proxy_auth_[un]pause They can be used in RestProxy::authenticate signals to suspend the current authentication attempt. This allows to get back to the mainloop to get the credentials, and to then rerun the call with the correct credentials.
Created attachment 216264 [details] [review] Propagate RestProxyAuth object in ::authenticate signal This will make it possible to pause/resume the current call during authentication callbacks to be able to get back to the mainloop to get authentication credentials.
These apply on top of the previous patch and make it possible to do rest_proxy_auth_pause(auth); in the ::authenticate callback, and then to rest_proxy_auth_unpause(auth) once username/password have been set on the RestProxy object. I've lightly tested this with my virt-viewer changes by moving the authentication dialog to run in an idle instead of directly in ::authenticate. Arguably the API is a bit weird, the libsoup one is a bit more natural in that messages are paused/unpaused, and credentials are set on the SoupAuth object.
Review of attachment 216262 [details] [review]: looks good to me
Review of attachment 216263 [details] [review]: since librest only supports user/pass authentication now, that looks good to me ::: rest/rest-proxy-auth.c @@ +93,3 @@ +{ + g_return_if_fail (REST_IS_PROXY_AUTH (auth)); + I would add: if (auth->priv->paused) return; @@ +105,3 @@ + gchar *password; + + g_return_if_fail (REST_IS_PROXY_AUTH (auth)); might be worth adding g_return_if_fail (auth->priv->paused) ::: rest/rest-proxy.c @@ +361,3 @@ + * that @auth does not get destroyed with g_object_ref(). + * You can then unpause the authentication with + * rest_proxy_auth_unpause() when everything is ready for it this should go in the following patch
Review of attachment 216264 [details] [review]: looks good with documentation from previous patch
Created attachment 217210 [details] [review] Add RestProxy::authenticate signal If caught by application, this signal can be used to set the credentials to use when authentication is needed. If not caught, librest behaviour will be unchanged (try to use what the username/password properties were set to first, and don't try to reuse them if this fails).
Created attachment 217211 [details] [review] Introduce RestProxyAuth type This will be used by the RestProxy authentication code to be able to "pause" the sending of the current message. This will give applications the opportunity to get back to the main loop to do the authentication work before resuming the current request.
Created attachment 217212 [details] [review] Add rest_proxy_auth_[un]pause They can be used in RestProxy::authenticate signals to suspend the current authentication attempt. This allows to get back to the mainloop to get the credentials, and to then rerun the call with the correct credentials.
Created attachment 217213 [details] [review] Propagate RestProxyAuth object in ::authenticate signal This will make it possible to pause/resume the current call during authentication callbacks to be able to get back to the mainloop to get authentication credentials.
This fixes the various comments from Marc-André. Ross, I'd like to push this week unless you have reservations about the API or implementation, does that work with you?
Fine by me.
Attachment 217210 [details] pushed as a61a286 - Add RestProxy::authenticate signal Attachment 217211 [details] pushed as 948e5d9 - Introduce RestProxyAuth type Attachment 217212 [details] pushed as 5e22174 - Add rest_proxy_auth_[un]pause Attachment 217213 [details] pushed as df70faa - Propagate RestProxyAuth object in ::authenticate signal