SoupAuthManagerNTLM: fix don't-fallback-to-Basic code

Sessions using NTLM should never fall back to using Basic auth to
access a resource which advertises NTLM auth. But ntlm-test wasn't
actually testing this, and it broke at some point. Fix that, and
add tests to ntlm-test.

4 files changed, 172 insertions, 77 deletions

diff --git a/libsoup/soup-auth-manager-ntlm.c b/libsoup/soup-auth-manager-ntlm.c
index 33043e74..cf5218bd 100644
--- a/libsoup/soup-auth-manager-ntlm.c
+++ b/libsoup/soup-auth-manager-ntlm.c
@@ -388,6 +388,7 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm)
 		SOUP_AUTH_MANAGER_NTLM_GET_PRIVATE (ntlm);
 	SoupNTLMConnection *conn;
 	const char *val;
+	char *challenge = NULL;
 	SoupURI *uri;
 
 	conn = get_connection_for_msg (priv, msg);
@@ -396,10 +397,11 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm)
 
 	val = soup_message_headers_get_list (msg->response_headers,
 					     "WWW-Authenticate");
-	if (val)
-		val = strstr (val, "NTLM ");
 	if (!val)
 		return;
+	challenge = soup_auth_manager_extract_challenge (val, "NTLM");
+	if (!challenge)
+		return;
 
 	if (conn->state > SOUP_NTLM_SENT_REQUEST) {
 		/* We already authenticated, but then got another 401.
@@ -410,7 +412,7 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm)
 		goto done;
 	}
 
-	if (!soup_ntlm_parse_challenge (val, &conn->nonce, &conn->domain)) {
+	if (!soup_ntlm_parse_challenge (challenge, &conn->nonce, &conn->domain)) {
 		conn->state = SOUP_NTLM_FAILED;
 		goto done;
 	}
@@ -418,7 +420,7 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm)
 	conn->auth = soup_auth_ntlm_new (conn->domain,
 					 soup_message_get_uri (msg)->host);
 #ifdef USE_NTLM_AUTH
-	conn->challenge_header = g_strdup (val + 5);
+	conn->challenge_header = g_strdup (challenge + 5);
 	if (conn->state == SOUP_NTLM_SENT_SSO_REQUEST) {
 		conn->state = SOUP_NTLM_RECEIVED_SSO_CHALLENGE;
 		goto done;
@@ -435,6 +437,8 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm)
 	}
 
  done:
+	g_free (challenge);
+
 	/* Remove the WWW-Authenticate headers so the session won't try
 	 * to do Basic auth too.
 	 */
diff --git a/libsoup/soup-auth-manager.c b/libsoup/soup-auth-manager.c
index 12e3f4e3..1aacf488 100644
--- a/libsoup/soup-auth-manager.c
+++ b/libsoup/soup-auth-manager.c
@@ -241,37 +241,84 @@ auth_header_for_message (SoupMessage *msg)
 	}
 }
 
-static char *
-extract_challenge (const char *challenges, const char *scheme)
+static GSList *
+next_challenge_start (GSList *items)
 {
-	GSList *items, *i;
-	int schemelen = strlen (scheme);
-	char *item, *space, *equals;
-	GString *challenge;
-
-	/* The relevant grammar:
+	/* The relevant grammar (from httpbis):
 	 *
 	 * WWW-Authenticate   = 1#challenge
 	 * Proxy-Authenticate = 1#challenge
-	 * challenge          = auth-scheme 1#auth-param
+	 * challenge          = auth-scheme [ 1*SP ( b64token / #auth-param ) ]
 	 * auth-scheme        = token
-	 * auth-param         = token "=" ( token | quoted-string )
+	 * auth-param         = token BWS "=" BWS ( token / quoted-string )
+	 * b64token           = 1*( ALPHA / DIGIT /
+	 *                          "-" / "." / "_" / "~" / "+" / "/" ) *"="
 	 *
 	 * The fact that quoted-strings can contain commas, equals
 	 * signs, and auth scheme names makes it tricky to "cheat" on
-	 * the parsing. We just use soup_header_parse_list(), and then
-	 * reassemble the pieces after we find the one we want.
+	 * the parsing. So soup_auth_manager_extract_challenge() will
+	 * have used soup_header_parse_list() to split the header into
+	 * items. Given the grammar above, the possible items are:
+	 *
+	 *   auth-scheme
+	 *   auth-scheme 1*SP b64token
+	 *   auth-scheme 1*SP auth-param
+	 *   auth-param
+	 *
+	 * where the first three represent the start of a new challenge and
+	 * the last one does not.
 	 */
 
+	for (; items; items = items->next) {
+		const char *item = items->data;
+		const char *sp = strpbrk (item, "\t\r\n ");
+		const char *eq = strchr (item, '=');
+
+		if (!eq) {
+			/* No "=", so it can't be an auth-param */
+			return items;
+		}
+		if (!sp || sp > eq) {
+			/* No space, or first space appears after the "=",
+			 * so it must be an auth-param.
+			 */
+			continue;
+		}
+		while (g_ascii_isspace (*++sp))
+			;
+		if (sp == eq) {
+			/* First "=" appears immediately after the first
+			 * space, so this must be an auth-param with
+			 * space around the "=".
+			 */
+			continue;
+		}
+
+		/* "auth-scheme auth-param" or "auth-scheme b64token" */
+		return items;
+	}
+
+	return NULL;
+}
+
+char *
+soup_auth_manager_extract_challenge (const char *challenges, const char *scheme)
+{
+	GSList *items, *i, *next;
+	int schemelen = strlen (scheme);
+	char *item;
+	GString *challenge;
+
 	items = soup_header_parse_list (challenges);
 
-	/* First item will start with the scheme name, followed by a
-	 * space and then the first auth-param.
+	/* First item will start with the scheme name, followed by
+	 * either nothing, or else a space and then the first
+	 * auth-param.
 	 */
-	for (i = items; i; i = i->next) {
+	for (i = items; i; i = next_challenge_start (i->next)) {
 		item = i->data;
 		if (!g_ascii_strncasecmp (item, scheme, schemelen) &&
-		    g_ascii_isspace (item[schemelen]))
+		    (!item[schemelen] || g_ascii_isspace (item[schemelen])))
 			break;
 	}
 	if (!i) {
@@ -279,17 +326,10 @@ extract_challenge (const char *challenges, const char *scheme)
 		return NULL;
 	}
 
-	/* The challenge extends from this item until the end, or until
-	 * the next item that has a space before an equals sign.
-	 */
+	next = next_challenge_start (i->next);
 	challenge = g_string_new (item);
-	for (i = i->next; i; i = i->next) {
+	for (i = i->next; i != next; i = i->next) {
 		item = i->data;
-		space = strpbrk (item, " \t");
-		equals = strchr (item, '=');
-		if (!equals || (space && equals > space))
-			break;
-
 		g_string_append (challenge, ", ");
 		g_string_append (challenge, item);
 	}
@@ -313,7 +353,7 @@ create_auth (SoupAuthManagerPrivate *priv, SoupMessage *msg)
 
 	for (i = priv->auth_types->len - 1; i >= 0; i--) {
 		auth_class = priv->auth_types->pdata[i];
-		challenge = extract_challenge (header, auth_class->scheme_name);
+		challenge = soup_auth_manager_extract_challenge (header, auth_class->scheme_name);
 		if (challenge)
 			break;
 	}
@@ -336,7 +376,7 @@ check_auth (SoupMessage *msg, SoupAuth *auth)
 	if (!header)
 		return FALSE;
 
-	challenge = extract_challenge (header, soup_auth_get_scheme_name (auth));
+	challenge = soup_auth_manager_extract_challenge (header, soup_auth_get_scheme_name (auth));
 	if (!challenge)
 		return FALSE;
 
diff --git a/libsoup/soup-auth-manager.h b/libsoup/soup-auth-manager.h
index 493960ae..d82fbb1d 100644
--- a/libsoup/soup-auth-manager.h
+++ b/libsoup/soup-auth-manager.h
@@ -32,10 +32,13 @@ typedef struct {
 
 GType soup_auth_manager_get_type (void);
 
-void soup_auth_manager_emit_authenticate (SoupAuthManager *manager,
-					  SoupMessage     *msg,
-					  SoupAuth        *auth,
-					  gboolean         retrying);
+void  soup_auth_manager_emit_authenticate (SoupAuthManager *manager,
+					   SoupMessage     *msg,
+					   SoupAuth        *auth,
+					   gboolean         retrying);
+
+char *soup_auth_manager_extract_challenge (const char      *challenges,
+					   const char      *scheme);
 
 G_END_DECLS
 
diff --git a/tests/ntlm-test.c b/tests/ntlm-test.c
index 46ac46e6..f5462c69 100644
--- a/tests/ntlm-test.c
+++ b/tests/ntlm-test.c
@@ -58,29 +58,25 @@ server_callback (SoupServer *server, SoupMessage *msg,
 	SoupSocket *socket;
 	const char *auth;
 	NTLMServerState state, required_user = 0;
-	gboolean auth_required = FALSE, not_found = FALSE;
-	gboolean basic_allowed = FALSE, ntlm_allowed = FALSE;
+	gboolean auth_required, not_found = FALSE;
+	gboolean basic_allowed = TRUE, ntlm_allowed = TRUE;
 
 	if (msg->method != SOUP_METHOD_GET) {
 		soup_message_set_status (msg, SOUP_STATUS_NOT_IMPLEMENTED);
 		return;
 	}
 
-	if (!strncmp (path, "/alice", 6)) {
-		auth_required = TRUE;
-		ntlm_allowed = TRUE;
+	if (!strncmp (path, "/alice", 6))
 		required_user = NTLM_AUTHENTICATED_ALICE;
-	} else if (!strncmp (path, "/bob", 4)) {
-		auth_required = TRUE;
-		ntlm_allowed = TRUE;
+	else if (!strncmp (path, "/bob", 4))
 		required_user = NTLM_AUTHENTICATED_BOB;
-	} else if (!strncmp (path, "/either", 7)) {
-		auth_required = TRUE;
-		ntlm_allowed = basic_allowed = TRUE;
-	} else if (!strncmp (path, "/basic", 6)) {
-		auth_required = TRUE;
-		basic_allowed = TRUE;
-	}
+	else if (!strncmp (path, "/either", 7))
+		;
+	else if (!strncmp (path, "/basic", 6))
+		ntlm_allowed = FALSE;
+	else if (!strncmp (path, "/noauth", 7))
+		basic_allowed = ntlm_allowed = FALSE;
+	auth_required = ntlm_allowed || basic_allowed;
 
 	if (strstr (path, "/404"))
 		not_found = TRUE;
@@ -95,7 +91,9 @@ server_callback (SoupServer *server, SoupMessage *msg,
 			if (!strncmp (auth + 5, NTLM_REQUEST_START,
 				      strlen (NTLM_REQUEST_START))) {
 				state = NTLM_RECEIVED_REQUEST;
-				/* If they start, they must finish */
+				/* If they start, they must finish, even if
+				 * it was unnecessary.
+				 */
 				auth_required = ntlm_allowed = TRUE;
 				basic_allowed = FALSE;
 			} else if (state == NTLM_SENT_CHALLENGE &&
@@ -104,12 +102,15 @@ server_callback (SoupServer *server, SoupMessage *msg,
 				state = NTLM_RESPONSE_USER (auth + 5);
 			} else
 				state = NTLM_UNAUTHENTICATED;
-		} else if (!strncmp (auth, "Basic ", 6) && basic_allowed) {
+		} else if (basic_allowed && !strncmp (auth, "Basic ", 6)) {
 			gsize len;
 			char *decoded = (char *)g_base64_decode (auth + 6, &len);
 
-			if (!strncmp (decoded, "alice:password", len) ||
-			    !strncmp (decoded, "bob:password", len))
+			if (!strncmp (decoded, "alice:password", len) &&
+			    required_user != NTLM_AUTHENTICATED_BOB)
+				auth_required = FALSE;
+			else if (!strncmp (decoded, "bob:password", len) &&
+				 required_user != NTLM_AUTHENTICATED_ALICE)
 				auth_required = FALSE;
 			g_free (decoded);
 		}
@@ -122,13 +123,13 @@ server_callback (SoupServer *server, SoupMessage *msg,
 	if (auth_required) {
 		soup_message_set_status (msg, SOUP_STATUS_UNAUTHORIZED);
 
-		if (basic_allowed) {
+		if (basic_allowed && state != NTLM_RECEIVED_REQUEST) {
 			soup_message_headers_append (msg->response_headers,
 						     "WWW-Authenticate",
 						     "Basic realm=\"ntlm-test\"");
 		}
 
-		if (state == NTLM_RECEIVED_REQUEST) {
+		if (ntlm_allowed && state == NTLM_RECEIVED_REQUEST) {
 			soup_message_headers_append (msg->response_headers,
 						     "WWW-Authenticate",
 						     "NTLM " NTLM_CHALLENGE);
@@ -158,7 +159,8 @@ static void
 authenticate (SoupSession *session, SoupMessage *msg,
 	      SoupAuth *auth, gboolean retrying, gpointer user)
 {
-	soup_auth_authenticate (auth, user, "password");
+	if (!retrying)
+		soup_auth_authenticate (auth, user, "password");
 }
 
 typedef struct {
@@ -180,11 +182,9 @@ prompt_check (SoupMessage *msg, gpointer user_data)
 						"WWW-Authenticate");
 	if (header && strstr (header, "Basic "))
 		state->got_basic_prompt = TRUE;
-	if (!state->sent_ntlm_request) {
-		if (header && strstr (header, "NTLM") &&
-		    !strstr (header, NTLM_CHALLENGE))
-			state->got_ntlm_prompt = TRUE;
-	}
+	if (header && strstr (header, "NTLM") &&
+	    !strstr (header, NTLM_CHALLENGE))
+		state->got_ntlm_prompt = TRUE;
 }
 
 static void
@@ -333,10 +333,11 @@ static void
 do_ntlm_round (SoupURI *base_uri, gboolean use_ntlm, const char *user)
 {
 	SoupSession *session;
-	gboolean alice = use_ntlm && !strcmp (user, "alice");
-	gboolean bob = use_ntlm && !strcmp (user, "bob");
-
-	g_return_if_fail (use_ntlm || !alice);
+	gboolean alice = !g_strcmp0 (user, "alice");
+	gboolean bob = !g_strcmp0 (user, "bob");
+	gboolean alice_via_ntlm = use_ntlm && alice;
+	gboolean bob_via_ntlm = use_ntlm && bob;
+	gboolean alice_via_basic = !use_ntlm && alice;
 
 	session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL);
 	if (use_ntlm)
@@ -347,40 +348,87 @@ do_ntlm_round (SoupURI *base_uri, gboolean use_ntlm, const char *user)
 				  G_CALLBACK (authenticate), (char *)user);
 	}
 
+	/* 1. Server doesn't request auth, so both get_ntlm_prompt and
+	 * get_basic_prompt are both FALSE, and likewise do_basic. But
+	 * if we're using NTLM we'll try that even without the server
+	 * asking.
+	 */
 	do_message (session, base_uri, "/noauth",
 		    FALSE, use_ntlm,
 		    FALSE, FALSE,
 		    SOUP_STATUS_OK);
+
+	/* 2. Server requires auth as Alice, so it will request that
+	 * if we didn't already authenticate the connection to her in
+	 * the previous step. If we authenticated as Bob in the
+	 * previous step, then we'll just immediately get a 401 here.
+	 * So in no case will we see the client try to do_ntlm.
+	 */
 	do_message (session, base_uri, "/alice",
-		    !use_ntlm || bob, FALSE,
-		    FALSE, FALSE,
+		    !alice_via_ntlm, FALSE,
+		    !alice_via_ntlm, alice_via_basic,
 		    alice ? SOUP_STATUS_OK :
 		    SOUP_STATUS_UNAUTHORIZED);
+
+	/* 3. Server still requires auth as Alice, but this URI
+	 * doesn't exist, so Alice should get a 404, but others still
+	 * get 401. Alice-via-NTLM is still authenticated, and so
+	 * won't get prompts, and Alice-via-Basic knows at this point
+	 * to send auth without it being requested, so also won't get
+	 * prompts. But Bob/nobody will.
+	 */
 	do_message (session, base_uri, "/alice/404",
-		    !use_ntlm, bob,
-		    FALSE, FALSE,
+		    !alice, bob_via_ntlm,
+		    !alice, alice_via_basic,
 		    alice ? SOUP_STATUS_NOT_FOUND :
 		    SOUP_STATUS_UNAUTHORIZED);
+
+	/* 4. Should be exactly the same as #3, except the status code */
 	do_message (session, base_uri, "/alice",
-		    !use_ntlm, bob,
-		    FALSE, FALSE,
+		    !alice, bob_via_ntlm,
+		    !alice, alice_via_basic,
 		    alice ? SOUP_STATUS_OK :
 		    SOUP_STATUS_UNAUTHORIZED);
+
+	/* 5. This path requires auth as Bob; Alice-via-NTLM will get
+	 * an immediate 401 and not try to reauthenticate.
+	 * Alice-via-Basic will get a 401 and then try to do Basic
+	 * (and fail). Bob-via-NTLM will try to do NTLM right away and
+	 * succeed.
+	 */
 	do_message (session, base_uri, "/bob",
-		    !use_ntlm || alice, bob,
-		    FALSE, FALSE,
+		    !bob_via_ntlm, bob_via_ntlm,
+		    !bob_via_ntlm, alice_via_basic,
 		    bob ? SOUP_STATUS_OK :
 		    SOUP_STATUS_UNAUTHORIZED);
+
+	/* 6. Back to /alice. Somewhat the inverse of #5; Bob-via-NTLM
+	 * will get an immediate 401 and not try again, Alice-via-NTLM
+	 * will try to do NTLM right away and succeed. Alice-via-Basic
+	 * still knows about this path, so will try Basic right away
+	 * and succeed.
+	 */
 	do_message (session, base_uri, "/alice",
-		    !use_ntlm || bob, alice,
-		    FALSE, FALSE,
+		    !alice_via_ntlm, alice_via_ntlm,
+		    !alice_via_ntlm, alice_via_basic,
 		    alice ? SOUP_STATUS_OK :
 		    SOUP_STATUS_UNAUTHORIZED);
+
+	/* 7. Server accepts Basic auth from either user, but not NTLM.
+	 * Since Bob-via-NTLM is unauthenticated at this point, he'll try
+	 * NTLM before realizing that the server doesn't support it.
+	 */
 	do_message (session, base_uri, "/basic",
-		    FALSE, bob,
+		    FALSE, bob_via_ntlm,
 		    TRUE, user != NULL,
 		    user != NULL ? SOUP_STATUS_OK :
 		    SOUP_STATUS_UNAUTHORIZED);
+
+	/* 8. Server accepts Basic or NTLM from either user.
+	 * Alice-via-NTLM is still authenticated at this point from #6,
+	 * and Bob-via-NTLM is authenticated from #7, so neither
+	 * of them will do anything.
+	 */
 	do_message (session, base_uri, "/either",
 		    !use_ntlm, FALSE,
 		    !use_ntlm, !use_ntlm && user != NULL,