logout doesn't work

nexus/src/authn/mod.rs

@@ -204,12 +204,18 @@ impl Context {
     /// (for testing only)
     #[cfg(test)]
     pub fn unprivileged_test_user() -> Context {
+        Self::test_silo_user(
+            USER_TEST_UNPRIVILEGED.silo_id,
+            USER_TEST_UNPRIVILEGED.id(),
+        )
+    }
+
+    /// Returns an authenticated context for a given silo user
+    #[cfg(test)]
+    pub fn test_silo_user(silo_id: Uuid, silo_user_id: Uuid) -> Context {
         Context {
             kind: Kind::Authenticated(Details {
-                actor: Actor::SiloUser {
-                    silo_user_id: USER_TEST_UNPRIVILEGED.id(),
-                    silo_id: USER_TEST_UNPRIVILEGED.silo_id,
-                },
+                actor: Actor::SiloUser { silo_user_id, silo_id },
             }),
             schemes_tried: Vec::new(),
         }

nexus/src/db/datastore.rs

@@ -3369,10 +3369,35 @@ impl DataStore {
         opctx: &OpContext,
         authz_session: &authz::ConsoleSession,
     ) -> DeleteResult {
-        opctx.authorize(authz::Action::Delete, authz_session).await?;
+        // 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()?;
+
+        // 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 = match actor.actor_type() {
+            IdentityType::SiloUser => actor.actor_id(),
+            IdentityType::UserBuiltin => {
+                return Err(Error::invalid_request("not a Silo user"))
+            }
+        };
 
         use db::schema::console_session::dsl;
         diesel::delete(dsl::console_session)
+            .filter(dsl::silo_user_id.eq(silo_user_id))
             .filter(dsl::token.eq(authz_session.id()))
             .execute_async(self.pool_authorized(opctx).await?)
             .await
@@ -4591,12 +4616,29 @@ mod test {
             .unwrap();
         assert!(fetched.time_last_used > session.time_last_used);
 
-        // delete it and fetch should come back with nothing
+        // 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 = LookupPath::new(&opctx, &datastore)
+            .console_session_token(&token)
+            .fetch()
+            .await;
+        assert!(fetched.is_ok());
 
-        // this will be a not found after #347
+        // delete it and fetch should come back with nothing
+        let silo_user_opctx = OpContext::for_background(
+            logctx.log.new(o!()),
+            Arc::new(authz::Authz::new(&logctx.log)),
+            authn::Context::test_silo_user(*SILO_ID, silo_user_id),
+            Arc::clone(&datastore),
+        );
+        let delete = datastore
+            .session_hard_delete(&silo_user_opctx, &authz_session)
+            .await;
+        assert_eq!(delete, Ok(()));
         let fetched = LookupPath::new(&opctx, &datastore)
             .console_session_token(&token)
             .fetch()

nexus/tests/integration_tests/console_api.rs

@@ -10,16 +10,19 @@ use std::env::current_dir;
 use nexus_test_utils::http_testing::{
     AuthnMode, NexusRequest, RequestBuilder, TestResponse,
 };
+use nexus_test_utils::resource_helpers::grant_iam;
 use nexus_test_utils::{
     load_test_config, test_setup_with_config, ControlPlaneTestContext,
 };
 use nexus_test_utils_macros::nexus_test;
 use omicron_common::api::external::IdentityMetadataCreateParams;
 use omicron_nexus::authn::{USER_TEST_PRIVILEGED, USER_TEST_UNPRIVILEGED};
-use omicron_nexus::db::identity::Asset;
+use omicron_nexus::authz::SiloRole;
+use omicron_nexus::db::fixed_data::silo::DEFAULT_SILO;
+use omicron_nexus::db::identity::{Asset, Resource};
 use omicron_nexus::external_api::console_api::SpoofLoginBody;
 use omicron_nexus::external_api::params::OrganizationCreate;
-use omicron_nexus::external_api::views;
+use omicron_nexus::external_api::{shared, views};
 
 #[nexus_test]
 async fn test_sessions(cptestctx: &ControlPlaneTestContext) {
@@ -61,6 +64,29 @@ async fn test_sessions(cptestctx: &ControlPlaneTestContext) {
         .await
         .expect("failed to 302 on unauthed console page request");
 
+    // Our test uses the "unprivileged" user to make sure login/logout works
+    // without other privileges.  However, they _do_ need the privilege to
+    // create Organizations because we'll be testing that as a smoke test.
+    // We'll remove that privilege afterwards.
+    let silo_url = format!("/silos/{}", DEFAULT_SILO.identity().name);
+    let policy_url = format!("{}/policy", silo_url);
+    let initial_policy: shared::Policy<SiloRole> =
+        NexusRequest::object_get(testctx, &policy_url)
+            .authn_as(AuthnMode::PrivilegedUser)
+            .execute()
+            .await
+            .expect("failed to fetch Silo policy")
+            .parsed_body()
+            .expect("failed to parse Silo policy");
+    grant_iam(
+        testctx,
+        &silo_url,
+        SiloRole::Collaborator,
+        USER_TEST_UNPRIVILEGED.id(),
+        AuthnMode::PrivilegedUser,
+    )
+    .await;
+
     // now make same requests with cookie
     RequestBuilder::new(&testctx, Method::POST, "/organizations")
         .header(header::COOKIE, &session_token)
@@ -78,6 +104,12 @@ async fn test_sessions(cptestctx: &ControlPlaneTestContext) {
         .await
         .expect("failed to get console page with session cookie");
 
+    NexusRequest::object_put(testctx, &policy_url, Some(&initial_policy))
+        .authn_as(AuthnMode::PrivilegedUser)
+        .execute()
+        .await
+        .expect("failed to restore Silo policy");
+
     // logout with an actual session should delete the session in the db
     RequestBuilder::new(&testctx, Method::POST, "/logout")
         .header(header::COOKIE, &session_token)
@@ -282,7 +314,7 @@ fn get_header_value(resp: TestResponse, header_name: HeaderName) -> String {
 
 async fn log_in_and_extract_token(testctx: &ClientTestContext) -> String {
     let login = RequestBuilder::new(&testctx, Method::POST, "/login")
-        .body(Some(&SpoofLoginBody { username: "privileged".to_string() }))
+        .body(Some(&SpoofLoginBody { username: "unprivileged".to_string() }))
         .expect_status(Some(StatusCode::OK))
         .execute()
         .await