fix(auth): preserve Basic query permissions during browser login

src/handlers/http/oidc.rs

@@ -501,28 +501,32 @@ fn callback_base_url(req: &HttpRequest) -> String {
     format!("{}://{host}/", PARSEABLE.options.get_scheme())
 }
 
-/// Re-verifies the Basic credentials and swaps their session for a cookie one.
+/// Re-verifies the Basic credentials and issues a cookie session for them.
 /// `new_basic_session` re-reads the user under SESSIONS, so an admin change
 /// racing the login cannot leave a cookie session with stale credentials.
+///
+/// The Basic session `/login` authorized through is kept: other requests with
+/// the same credentials may be between their middleware check and a later
+/// `get_permissions`, and would see no permissions if it vanished. Dropping it
+/// revoked nothing, as the next Basic request re-verifies and re-tracks it;
+/// password changes, deletions and role changes still reach it through the
+/// user's tracked sessions.
 pub(crate) fn exchange_basic_for_cookie(
     username: &str,
     password: &str,
     tenant_id: &Option<String>,
     expires_in: TimeDelta,
 ) -> Option<Cookie<'static>> {
     let id = Ulid::new();
-    let mut sessions = Users.new_basic_session(
-        username,
-        password,
-        tenant_id.as_deref().unwrap_or(DEFAULT_TENANT),
-        SessionKey::SessionId(id),
-        Utc::now() + expires_in,
-    )?;
-    sessions.remove_session(&SessionKey::BasicAuth {
-        username: username.to_owned(),
-        password: password.to_owned(),
-    });
-    Some(cookie_session(id))
+    Users
+        .new_basic_session(
+            username,
+            password,
+            tenant_id.as_deref().unwrap_or(DEFAULT_TENANT),
+            SessionKey::SessionId(id),
+            Utc::now() + expires_in,
+        )
+        .map(|_| cookie_session(id))
 }
 
 fn redirect_to_oidc_logout(mut logout_endpoint: Url, redirect: &Url) -> HttpResponse {

src/rbac/mod.rs

@@ -995,7 +995,7 @@ mod lock_order_tests {
     }
 
     /// The session a `/login` cookie names, checked to belong to `login`'s user
-    /// and tenant, with the Basic session `/login` authorized through removed.
+    /// and tenant.
     fn issued_session(
         cookie: Option<Cookie<'static>>,
         login: &SessionKey,
@@ -1007,7 +1007,6 @@ mod lock_order_tests {
             Users.get_userid_from_session(&session),
             Some((credentials(login).0.to_owned(), tenant.to_owned()))
         );
-        assert!(!Users.session_exists(login));
         session
     }
 
@@ -1086,6 +1085,118 @@ mod lock_order_tests {
         assert_eq!(tracked_sessions(&login, &tenant), before);
     }
 
+    /// Runs the query handler's dataset check for `key`.
+    fn query_auth(key: &SessionKey) -> Result<(), actix_web::Error> {
+        futures::executor::block_on(crate::utils::user_auth_for_query(
+            key,
+            "select * from lock_order_stream",
+        ))
+    }
+
+    // Reproduction: `/login` dropped the Basic session it authorized through, so
+    // a query with the same credentials that had passed the middleware then
+    // read no permissions and failed with "does not have access".
+    #[test]
+    fn cookie_login_keeps_the_basic_session_of_an_in_flight_query() {
+        let _serial = auth_state();
+        let tenant = tenant();
+        let (_, login) = native_user(&tenant, &[ROLE]);
+
+        // The query passes the middleware, then `/login` exchanges the same
+        // credentials before the query handler reads permissions.
+        assert_eq!(finish(&first_login(&login), "query"), Response::Authorized);
+        let cookie = finish(&cookie_login(&login, &tenant), "cookie login");
+        let session = issued_session(cookie, &login, &tenant);
+
+        assert!(Users.session_exists(&login));
+        assert!(!Users.get_permissions(&login).is_empty());
+        assert!(query_auth(&login).is_ok());
+        assert!(query_auth(&session).is_ok());
+        // The cookie session expires on its own; the Basic one never does.
+        assert!(!sessions().is_session_expired(&session));
+        assert!(!sessions().is_session_expired(&login));
+        assert_eq!(tracked_sessions(&login, &tenant), 2);
+
+        let cookie = finish(
+            &spawn({
+                let (login, tenant) = (login.clone(), Some(tenant.clone()));
+                move || {
+                    let (username, password) = credentials(&login);
+                    exchange_basic_for_cookie(username, password, &tenant, TimeDelta::seconds(-1))
+                }
+            }),
+            "expired cookie login",
+        );
+        let expired = issued_session(cookie, &login, &tenant);
+        // The middleware rejects expired sessions; the Basic one is unaffected.
+        assert!(sessions().is_session_expired(&expired));
+        assert!(!sessions().is_session_expired(&login));
+        assert!(query_auth(&login).is_ok());
+    }
+
+    // Keeping the Basic session must not let it outlive a logout, password
+    // change, role removal or deletion that also ends the cookie session.
+    #[test]
+    fn revocations_after_a_cookie_login_reach_the_basic_session() {
+        let _serial = auth_state();
+        let tenant = tenant();
+        let roles = HashSet::from([ROLE.to_owned()]);
+        let cookie_and_basic = |login: &SessionKey| {
+            assert_eq!(finish(&first_login(login), "query"), Response::Authorized);
+            let cookie = finish(&cookie_login(login, &tenant), "cookie login");
+            issued_session(cookie, login, &tenant)
+        };
+
+        // Logging out one session leaves the other.
+        let (_, login) = native_user(&tenant, &[ROLE]);
+        let session = cookie_and_basic(&login);
+        Users.remove_session(&session);
+        assert!(!Users.session_exists(&session));
+        assert!(query_auth(&login).is_ok());
+        let session = finish(&cookie_login(&login, &tenant), "cookie login");
+        let session = issued_session(session, &login, &tenant);
+        Users.remove_session(&login);
+        assert!(!Users.session_exists(&login));
+        assert!(query_auth(&session).is_ok());
+
+        let (user, login) = native_user(&tenant, &[ROLE]);
+        let session = cookie_and_basic(&login);
+        Users.change_password_hash(user.userid(), &cheap_hash("changed"), &user.tenant);
+        assert_eq!(tracked_sessions(&login, &tenant), 0);
+        assert!(query_auth(&login).is_err());
+        assert!(query_auth(&session).is_err());
+        assert_eq!(
+            finish(&first_login(&login), "query"),
+            Response::UnAuthorized
+        );
+
+        let (user, login) = native_user(&tenant, &[ROLE]);
+        let session = cookie_and_basic(&login);
+        Users.remove_roles(user.userid(), roles.clone(), &user.tenant);
+        for key in [&login, &session] {
+            assert!(Users.get_permissions(key).is_empty());
+            assert!(query_auth(key).is_err());
+            assert_eq!(
+                Users.authorize(key.clone(), Action::Query, None, None),
+                Response::UnAuthorized
+            );
+        }
+        Users.add_roles(user.userid(), roles, &user.tenant);
+        assert!(query_auth(&login).is_ok());
+        assert!(query_auth(&session).is_ok());
+
+        let (user, login) = native_user(&tenant, &[ROLE]);
+        let session = cookie_and_basic(&login);
+        Users.delete_user(user.userid(), &user.tenant);
+        assert_eq!(tracked_sessions(&login, &tenant), 0);
+        assert!(query_auth(&login).is_err());
+        assert!(query_auth(&session).is_err());
+        assert_eq!(
+            finish(&first_login(&login), "query"),
+            Response::UnAuthorized
+        );
+    }
+
     // The Basic lookups verify passwords after dropping USERS; they must still
     // pick the user's tenant and reject a wrong password or tenant.
     #[test]