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 631679 - Credentials passed within URL's should always be taken into account for authentication
Credentials passed within URL's should always be taken into account for authe...
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: HTTP Transport
2.31.x
Other Linux
: Normal normal
: ---
Assigned To: Sergio Villar
libsoup-maint@gnome.bugs
: 625854 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-10-08 13:52 UTC by Sergio Villar
Modified: 2010-11-14 16:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow relogin with URL credentials (11.68 KB, patch)
2010-10-08 13:56 UTC, Sergio Villar
reviewed Details | Review
Allow relogin with URL credentials (13.42 KB, patch)
2010-11-12 18:16 UTC, Sergio Villar
none Details | Review

Description Sergio Villar 2010-10-08 13:52:21 UTC
Currently libsoup does not check the presence of credentials passed in the URLs if the session is currently authenticated. This way trying to login with different credentials when the session is already authenticated is not possible as libsoup will always use the currently authenticated SoupAuth.

It'd be nice to allow relogin without having to deauthenticate.
Comment 1 Sergio Villar 2010-10-08 13:56:43 UTC
Created attachment 171950 [details] [review]
Allow relogin with URL credentials

With this patch libsoup checks the presence of credentials in SoupMessage's SoupURI in authenticate_auth. If present it will try to authenticate using those credentials.

This patch also updates the auth-test.c file. Some changes:
   * Added some tests for the re-login feature
   * Refactored some code. Now there is a do_batch_test method that runs both all the old auth tests plus the new tests for the re-login feature.
Comment 2 Dan Winship 2010-10-13 21:32:36 UTC
Comment on attachment 171950 [details] [review]
Allow relogin with URL credentials

>+	SoupURI *uri = soup_message_get_uri (msg);
> 
>-	if (soup_auth_is_authenticated (auth))
>+	if (soup_auth_is_authenticated (auth) && !uri->user && !uri->password)
> 		return TRUE;
> 
> 	if (proxy) {

this is wrong, you're checking the message URI regardless of whether this is regular or proxy auth

>-static int ntests = sizeof (tests) / sizeof (tests[0]);

I'd say keep this, just have num_main_tests and num_relogin_tests. (Oh, and use G_N_ELEMENTS rather than sizeof/sizeof!)

>+do_batch_tests (const gchar *base_uri, gint ntests)

No gchar and gint please. Use char and int.

>+		soup_uri = soup_message_get_uri (msg);
>+		if (tests[i].url_auth) {
>+			gchar *username = g_strdup_printf ("user%c", tests[i].provided[0]);
>+			gchar *password = g_strdup_printf ("realm%c", tests[i].provided[0]);
>+			soup_uri_set_user (soup_uri, username);
>+			soup_uri_set_password (soup_uri, password);

Um... no, don't change the SoupURI itself after creating the message. Create the URI the way you want it first, and then call soup_message_new/soup_message_new_with_uri using that.

>+	tests = main_tests;
>+	ntests = sizeof (main_tests) / sizeof (main_tests[0]);
>+	do_batch_tests (base_uri, ntests);

the gratuitous extra global variable seems unnecessary. Can't you just have do_batch_tests() take a SoupAuthTest* in addition to the base_uri and length?

>+
>+    AuthType Basic
>+    AuthName realm12
>+    AuthUserFile ./htpasswd
>+    Require valid-user
>   </Proxy>

This part is for proxy auth... I don't think your test requires allowing realm12 there.
Comment 3 Sergio Villar 2010-11-12 18:16:39 UTC
Created attachment 174343 [details] [review]
Allow relogin with URL credentials
Comment 4 Dan Winship 2010-11-14 16:57:02 UTC
ok, I changed the soup-auth-manager part a little bit more, verified that
it still passed your tests, and then committed the tests as is. Thanks for
the patch (and especially the tests)
Comment 5 Dan Winship 2010-11-14 16:57:42 UTC
*** Bug 625854 has been marked as a duplicate of this bug. ***