After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 658937 - librest: add explict username/password support
librest: add explict username/password support
Status: RESOLVED FIXED
Product: librest
Classification: Platform
Component: other
git master
Other Mac OS
: Normal normal
: ---
Assigned To: librest-maint
Depends on:
Blocks:
 
 
Reported: 2011-09-13 15:14 UTC by Nick Richards
Modified: 2012-06-28 10:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add username/password support (6.51 KB, patch)
2011-12-08 17:49 UTC, Marc-Andre Lureau
committed Details | Review
Avoid infinite loop with wrong HTTP credentials (1.33 KB, patch)
2012-06-06 16:37 UTC, Christophe Fergeau
committed Details | Review
Add RestProxy::authenticate signal (5.01 KB, patch)
2012-06-12 09:22 UTC, Christophe Fergeau
accepted-commit_now Details | Review
Introduce RestProxyAuth type (8.01 KB, patch)
2012-06-13 11:49 UTC, Christophe Fergeau
reviewed Details | Review
Add rest_proxy_auth_[un]pause (3.54 KB, patch)
2012-06-13 11:49 UTC, Christophe Fergeau
reviewed Details | Review
Propagate RestProxyAuth object in ::authenticate signal (3.31 KB, patch)
2012-06-13 11:49 UTC, Christophe Fergeau
reviewed Details | Review
Add RestProxy::authenticate signal (5.02 KB, patch)
2012-06-25 14:53 UTC, Christophe Fergeau
committed Details | Review
Introduce RestProxyAuth type (8.01 KB, patch)
2012-06-25 14:53 UTC, Christophe Fergeau
committed Details | Review
Add rest_proxy_auth_[un]pause (2.73 KB, patch)
2012-06-25 14:53 UTC, Christophe Fergeau
committed Details | Review
Propagate RestProxyAuth object in ::authenticate signal (4.00 KB, patch)
2012-06-25 14:53 UTC, Christophe Fergeau
committed Details | Review

Description Nick Richards 2011-09-13 15:14:34 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.  "
Comment 1 Marc-Andre Lureau 2011-12-08 17:49:28 UTC
Created attachment 203091 [details] [review]
add username/password support
Comment 2 Ross Burton 2011-12-08 18:24:13 UTC
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.
Comment 3 Marc-Andre Lureau 2011-12-09 12:48:42 UTC
Attachment 203091 [details] pushed as 649671b - add username/password support
Comment 4 Christophe Fergeau 2012-06-06 16:37:58 UTC
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.
Comment 5 Marc-Andre Lureau 2012-06-06 17:17:51 UTC
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?
Comment 6 Christophe Fergeau 2012-06-07 08:15:41 UTC
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"
Comment 7 Ross Burton 2012-06-07 08:17:20 UTC
Review of attachment 215759 [details] [review]:

Sounds good then.
Comment 8 Christophe Fergeau 2012-06-07 09:48:07 UTC
Attachment 215759 [details] pushed as d2e81c4 - Avoid infinite loop with wrong HTTP credentials
Comment 9 Christophe Fergeau 2012-06-12 09:22:58 UTC
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).
Comment 10 Ross Burton 2012-06-12 09:24:44 UTC
Review of attachment 216181 [details] [review]:

Good stuff.
Comment 11 Christophe Fergeau 2012-06-12 09:34:42 UTC
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.
Comment 12 Christophe Fergeau 2012-06-13 11:49:22 UTC
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.
Comment 13 Christophe Fergeau 2012-06-13 11:49:25 UTC
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.
Comment 14 Christophe Fergeau 2012-06-13 11:49:28 UTC
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.
Comment 15 Christophe Fergeau 2012-06-13 11:56:05 UTC
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.
Comment 16 Marc-Andre Lureau 2012-06-22 16:10:35 UTC
Review of attachment 216262 [details] [review]:

looks good to me
Comment 17 Marc-Andre Lureau 2012-06-22 16:19:29 UTC
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
Comment 18 Marc-Andre Lureau 2012-06-22 16:20:06 UTC
Review of attachment 216264 [details] [review]:

looks good with documentation from previous patch
Comment 19 Christophe Fergeau 2012-06-25 14:53:09 UTC
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).
Comment 20 Christophe Fergeau 2012-06-25 14:53:14 UTC
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.
Comment 21 Christophe Fergeau 2012-06-25 14:53:16 UTC
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.
Comment 22 Christophe Fergeau 2012-06-25 14:53:20 UTC
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.
Comment 23 Christophe Fergeau 2012-06-25 15:08:22 UTC
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?
Comment 24 Ross Burton 2012-06-25 16:18:19 UTC
Fine by me.
Comment 25 Christophe Fergeau 2012-06-28 10:34:02 UTC
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