[5/n] Expire sessions by token again

nexus/auth/src/authn/external/session_cookie.rs

@@ -45,7 +45,7 @@ pub trait SessionStore {
     ) -> Option<Self::SessionModel>;
 
     /// Mark session expired
-    async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()>;
+    async fn session_expire(&self, token: String) -> Option<()>;
 
     /// Maximum time session can remain idle before expiring
     fn session_idle_timeout(&self) -> Duration;
@@ -133,7 +133,7 @@ where
         // expired
         let now = Utc::now();
         if session.time_last_used() + ctx.session_idle_timeout() < now {
-            let expired_session = ctx.session_expire(session.id()).await;
+            let expired_session = ctx.session_expire(token).await;
             if expired_session.is_none() {
                 debug!(log, "failed to expire session")
             }
@@ -153,7 +153,7 @@ where
         // existed longer than absolute_timeout, it is expired and we can no
         // longer extend the session
         if session.time_created() + ctx.session_absolute_timeout() < now {
-            let expired_session = ctx.session_expire(session.id()).await;
+            let expired_session = ctx.session_expire(token).await;
             if expired_session.is_none() {
                 debug!(log, "failed to expire session")
             }
@@ -273,9 +273,9 @@ mod test {
             }
         }
 
-        async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> {
+        async fn session_expire(&self, token: String) -> Option<()> {
             let mut sessions = self.sessions.lock().unwrap();
-            sessions.retain(|s| s.id != id);
+            sessions.retain(|s| s.token != token);
             Some(())
         }
 

nexus/db-queries/src/db/datastore/console_session.rs

@@ -17,7 +17,6 @@ use nexus_db_schema::schema::console_session;
 use omicron_common::api::external::CreateResult;
 use omicron_common::api::external::DeleteResult;
 use omicron_common::api::external::Error;
-use omicron_common::api::external::InternalContext;
 use omicron_common::api::external::LookupResult;
 use omicron_common::api::external::LookupType;
 use omicron_common::api::external::ResourceType;
@@ -135,53 +134,6 @@ impl DataStore {
         })
     }
 
-    // putting "hard" in the name because we don't do this with any other model
-    pub async fn session_hard_delete(
-        &self,
-        opctx: &OpContext,
-        authz_session: &authz::ConsoleSession,
-    ) -> DeleteResult {
-        // We don't do a typical authz check here.  Instead, knowing that every
-        // user is allowed to delete their own session, the query below filters
-        // on the session's silo_user_id matching the current actor's id.
-        //
-        // We could instead model this more like other authz checks.  That would
-        // involve fetching the session record from the database, storing the
-        // associated silo_user_id into the `authz::ConsoleSession`, and having
-        // an Oso rule saying you can delete a session whose associated silo
-        // user matches the authenticated actor.  This would be a fair bit more
-        // complicated and more work at runtime work than what we're doing here.
-        // The tradeoff is that we're effectively encoding policy here, but it
-        // seems worth it in this case.
-        let actor = opctx
-            .authn
-            .actor_required()
-            .internal_context("deleting current user's session")?;
-
-        // This check shouldn't be required in that there should be no overlap
-        // between silo user ids and other types of identity ids.  But it's easy
-        // to check, and if we add another type of Actor, we'll be forced here
-        // to consider if they should be able to have console sessions and log
-        // out of them.
-        let silo_user_id = actor
-            .silo_user_id()
-            .ok_or_else(|| Error::invalid_request("not a Silo user"))?;
-
-        use nexus_db_schema::schema::console_session::dsl;
-        diesel::delete(dsl::console_session)
-            .filter(dsl::silo_user_id.eq(silo_user_id))
-            .filter(dsl::id.eq(authz_session.id().into_untyped_uuid()))
-            .execute_async(&*self.pool_connection_authorized(opctx).await?)
-            .await
-            .map(|_rows_deleted| ())
-            .map_err(|e| {
-                Error::internal_error(&format!(
-                    "error deleting session: {:?}",
-                    e
-                ))
-            })
-    }
-
     pub async fn session_hard_delete_by_token(
         &self,
         opctx: &OpContext,

nexus/db-queries/src/db/datastore/mod.rs

@@ -659,17 +659,6 @@ mod test {
             .unwrap();
         assert!(fetched.time_last_used > session.time_last_used);
 
-        // deleting it using `opctx` (which represents the test-privileged user)
-        // should succeed but not do anything -- you can't delete someone else's
-        // session
-        let delete =
-            datastore.session_hard_delete(&opctx, &authz_session).await;
-        assert_eq!(delete, Ok(()));
-        let fetched = datastore
-            .session_lookup_by_token(&authn_opctx, token.clone())
-            .await;
-        assert!(fetched.is_ok());
-
         // delete it and fetch should come back with nothing
         let silo_user_opctx = OpContext::for_background(
             logctx.log.new(o!()),
@@ -682,7 +671,7 @@ mod test {
             Arc::clone(&datastore) as Arc<dyn nexus_auth::storage::Storage>,
         );
         let delete = datastore
-            .session_hard_delete(&silo_user_opctx, &authz_session)
+            .session_hard_delete_by_token(&silo_user_opctx, token.clone())
             .await;
         assert_eq!(delete, Ok(()));
         let fetched = datastore
@@ -695,7 +684,7 @@ mod test {
 
         // deleting an already nonexistent is considered a success
         let delete_again =
-            datastore.session_hard_delete(&opctx, &authz_session).await;
+            datastore.session_hard_delete_by_token(&opctx, token.clone()).await;
         assert_eq!(delete_again, Ok(()));
 
         let delete_again =

nexus/src/app/session.rs

@@ -80,19 +80,6 @@ impl super::Nexus {
         self.db_datastore.session_update_last_used(opctx, &authz_session).await
     }
 
-    pub(crate) async fn session_hard_delete(
-        &self,
-        opctx: &OpContext,
-        id: ConsoleSessionUuid,
-    ) -> DeleteResult {
-        let authz_session = authz::ConsoleSession::new(
-            authz::FLEET,
-            id,
-            LookupType::ById(id.into_untyped_uuid()),
-        );
-        self.db_datastore.session_hard_delete(opctx, &authz_session).await
-    }
-
     pub(crate) async fn session_hard_delete_by_token(
         &self,
         opctx: &OpContext,

nexus/src/context.rs

@@ -477,9 +477,9 @@ impl SessionStore for ServerContext {
         self.nexus.session_update_last_used(&opctx, id).await.ok()
     }
 
-    async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> {
+    async fn session_expire(&self, token: String) -> Option<()> {
         let opctx = self.nexus.opctx_external_authn();
-        self.nexus.session_hard_delete(opctx, id).await.ok()
+        self.nexus.session_hard_delete_by_token(opctx, token).await.ok()
     }
 
     fn session_idle_timeout(&self) -> Duration {

nexus/tests/integration_tests/authn_http.rs

@@ -397,9 +397,9 @@ impl session_cookie::SessionStore for WhoamiServerState {
         }
     }
 
-    async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> {
+    async fn session_expire(&self, token: String) -> Option<()> {
         let mut sessions = self.sessions.lock().unwrap();
-        sessions.retain(|s| s.id != id);
+        sessions.retain(|s| s.token != token);
         Some(())
     }
 

nexus/tests/integration_tests/console_api.rs

@@ -8,6 +8,7 @@ use dropshot::ResultsPage;
 use dropshot::test_util::ClientTestContext;
 use http::header::HeaderName;
 use http::{StatusCode, header, method::Method};
+use nexus_auth::context::OpContext;
 use std::env::current_dir;
 
 use crate::integration_tests::saml::SAML_RESPONSE_IDP_DESCRIPTOR;
@@ -30,7 +31,7 @@ use nexus_test_utils_macros::nexus_test;
 use nexus_types::external_api::params::{self, ProjectCreate};
 use nexus_types::external_api::shared::{SiloIdentityMode, SiloRole};
 use nexus_types::external_api::{shared, views};
-use omicron_common::api::external::IdentityMetadataCreateParams;
+use omicron_common::api::external::{Error, IdentityMetadataCreateParams};
 use omicron_sled_agent::sim;
 use omicron_test_utils::dev::poll::{CondCheckError, wait_for_condition};
 
@@ -918,3 +919,62 @@ async fn expect_redirect(testctx: &ClientTestContext, from: &str, to: &str) {
         .await
         .expect("did not find expected redirect");
 }
+
+/// Make sure an expired session gets deleted when you try to use it
+///
+/// This is not the best kind of test because it breaks the API abstraction
+/// boundary, but in this case it's necessary because by design we do not
+/// expose through the API why authn failed, i.e., whether it's because
+/// the session was found but is expired. vs not found at all
+#[tokio::test]
+async fn test_session_idle_timeout_deletes_session() {
+    // set idle timeout to 1 second so we can test expiration
+    let mut config = load_test_config();
+    config.pkg.console.session_idle_timeout_minutes = 0;
+    let cptestctx = test_setup_with_config::<omicron_nexus::Server>(
+        "test_session_idle_timeout_deletes_session",
+        &mut config,
+        sim::SimMode::Explicit,
+        None,
+        0,
+    )
+    .await;
+    let testctx = &cptestctx.external_client;
+
+    // Start session
+    let session_cookie = log_in_and_extract_token(&cptestctx).await;
+
+    // sleep here not necessary given TTL of 0
+
+    // Make a request with the expired session cookie
+    let me_response = RequestBuilder::new(testctx, Method::GET, "/v1/me")
+        .header(header::COOKIE, &session_cookie)
+        .expect_status(Some(StatusCode::UNAUTHORIZED))
+        .execute()
+        .await
+        .expect("first request with expired cookie did not 401");
+
+    let error_body: dropshot::HttpErrorResponseBody =
+        me_response.parsed_body().unwrap();
+    assert_eq!(error_body.message, "credentials missing or invalid");
+
+    // check that the session actually got deleted
+
+    let nexus = &cptestctx.server.server_context().nexus;
+    let datastore = nexus.datastore();
+    let opctx =
+        OpContext::for_tests(cptestctx.logctx.log.new(o!()), datastore.clone());
+
+    let token = session_cookie.strip_prefix("session=").unwrap();
+    let db_token_error = nexus
+        .datastore()
+        .session_lookup_by_token(&opctx, token.to_string())
+        .await
+        .expect_err("session should be deleted");
+    assert_matches::assert_matches!(
+        db_token_error,
+        Error::ObjectNotFound { .. }
+    );
+
+    cptestctx.teardown().await;
+}