auth: Fix userdb auth cache with username changes

The problem was for example when userdb lookup uses only the username part of the username@domain lookup. Then: * "username" lookup caches the results for "username". Since the username didn't change, it doesn't store in the cache the "user" field. * "username@domain" lookup looks up "username" from cache. Since there is no "user" field, the code didn't think the username had changed. Fix this by saving the "user" field to auth cache, regardless of whether it's the same as the current username.
dovecot · Nov 21, 2024 · 3d19774 · 3d19774
1 parent b3fcfa0
commit 3d19774
Show file tree

Hide file tree

Showing 5 changed files with 17 additions and 17 deletions.
diff --git a/src/auth/auth-request-fields.c b/src/auth/auth-request-fields.c
@@ -395,7 +395,7 @@ bool auth_request_set_username(struct auth_request *request,
 		event_add_str(request->event, "translated_user",
 			      request->fields.translated_username);
 	}
-	request->user_changed_by_lookup = TRUE;
+	request->user_returned_by_lookup = TRUE;
 
 	if (login_username != NULL) {
 		if (!auth_request_set_login_username(request,

diff --git a/src/auth/auth-request.c b/src/auth/auth-request.c
@@ -1128,7 +1128,7 @@ void auth_request_verify_plain(struct auth_request *request,
 		request->mech_password = p_strdup(request->pool, password);
 	else
 		i_assert(request->mech_password == password);
-	request->user_changed_by_lookup = FALSE;
+	request->user_returned_by_lookup = FALSE;
 
 	if (request->policy_processed ||
 	    !request->set->policy_check_before_auth) {
@@ -1319,7 +1319,7 @@ void auth_request_lookup_credentials(struct auth_request *request,
 	if (request->wanted_credentials_scheme == NULL)
 		request->wanted_credentials_scheme =
 			p_strdup(request->pool, scheme);
-	request->user_changed_by_lookup = FALSE;
+	request->user_returned_by_lookup = FALSE;
 
 	if (request->policy_processed ||
 	    !request->set->policy_check_before_auth) {
@@ -1445,7 +1445,7 @@ auth_request_userdb_save_cache(struct auth_request *request,
 		auth_fields_append(request->fields.userdb_reply, str,
 				   AUTH_FIELD_FLAG_CHANGED,
 				   AUTH_FIELD_FLAG_CHANGED, FALSE);
-		if (request->user_changed_by_lookup) {
+		if (request->user_returned_by_lookup) {
 			/* username was changed by passdb or userdb */
 			if (str_len(str) > 0)
 				str_append_c(str, '\t');
@@ -1592,7 +1592,7 @@ void auth_request_userdb_callback(enum userdb_result result,
 		if (result == USERDB_RESULT_INTERNAL_FAILURE)
 			request->userdbs_seen_internal_failure = TRUE;
 
-		request->user_changed_by_lookup = FALSE;
+		request->user_returned_by_lookup = FALSE;
 
 		request->userdb = next_userdb;
 		auth_request_lookup_user(request,
@@ -1666,7 +1666,7 @@ void auth_request_lookup_user(struct auth_request *request,
 	const char *cache_key, *error;
 
 	request->private_callback.userdb = callback;
-	request->user_changed_by_lookup = FALSE;
+	request->user_returned_by_lookup = FALSE;
 	request->userdb_lookup = TRUE;
 	request->userdb_cache_result = AUTH_REQUEST_CACHE_NONE;
 	if (request->fields.userdb_reply == NULL)
@@ -1833,8 +1833,8 @@ auth_request_try_update_username(struct auth_request *request,
 			"username changed %s -> %s",
 			request->fields.user, new_value);
 		auth_request_set_username_forced(request, new_value);
-		request->user_changed_by_lookup = TRUE;
 	}
+	request->user_returned_by_lookup = TRUE;
 	return TRUE;
 }
 

diff --git a/src/auth/auth-request.h b/src/auth/auth-request.h
@@ -207,9 +207,9 @@ struct auth_request {
 	bool in_delayed_failure_queue:1;
 	bool removed_from_handler:1;
 	bool snapshot_have_userdb_prefetch_set:1;
-	/* username was changed by this passdb/userdb lookup. Used by
-	   auth-workers to determine whether to send back a changed username. */
-	bool user_changed_by_lookup:1;
+	/* The last passdb/userdb lookup returned a username field. This may
+	   or may not have changed the current username. */
+	bool user_returned_by_lookup:1;
 	/* each passdb lookup can update the current success-status using the
 	   result_* rules. the authentication succeeds only if this is TRUE
 	   at the end. mechanisms that don't require passdb, but do a passdb

diff --git a/src/auth/auth-worker-server.c b/src/auth/auth-worker-server.c
@@ -231,7 +231,7 @@ static void verify_plain_callback(enum passdb_result result,
 	}
 	if (result != PASSDB_RESULT_INTERNAL_FAILURE) {
 		str_append_c(str, '\t');
-		if (request->user_changed_by_lookup)
+		if (request->user_returned_by_lookup)
 			str_append_tabescaped(str, request->fields.user);
 		str_append_c(str, '\t');
 		if (request->passdb_password != NULL)
@@ -391,7 +391,7 @@ lookup_credentials_callback(enum passdb_result result,
 			str_append(str, "NEXT\t");
 		else
 			str_append(str, "OK\t");
-		if (request->user_changed_by_lookup)
+		if (request->user_returned_by_lookup)
 			str_append_tabescaped(str, request->fields.user);
 		str_append_c(str, '\t');
 		if (request->wanted_credentials_scheme[0] != '\0') {
@@ -528,7 +528,7 @@ lookup_user_callback(enum userdb_result result,
 		break;
 	case USERDB_RESULT_OK:
 		str_append(str, "OK\t");
-		if (auth_request->user_changed_by_lookup)
+		if (auth_request->user_returned_by_lookup)
 			str_append_tabescaped(str, auth_request->fields.user);
 		/* export only the fields changed by this lookup */
 		auth_fields_append(auth_request->fields.userdb_reply, str,

diff --git a/src/auth/userdb-blocking.c b/src/auth/userdb-blocking.c
@@ -37,10 +37,10 @@ static bool user_callback(struct auth_worker_connection *conn ATTR_UNUSED,
 			args += 2;
 		}
 
-		if (username[0] != '\0' &&
-		    strcmp(request->fields.user, username) != 0) {
-			auth_request_set_username_forced(request, username);
-			request->user_changed_by_lookup = TRUE;
+		if (username[0] != '\0') {
+			if (strcmp(request->fields.user, username) != 0)
+				auth_request_set_username_forced(request, username);
+			request->user_returned_by_lookup = TRUE;
 		}
 	} else {
 		result = USERDB_RESULT_INTERNAL_FAILURE;