delete session by token instead of by ID

Author: david-crespo

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(())
     }