diff --git a/nexus/src/authn/mod.rs b/nexus/src/authn/mod.rs index c790e34236..8f1cfbc20a 100644 --- a/nexus/src/authn/mod.rs +++ b/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(), } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index c53b84c7a4..dc24bf022a 100644 --- a/nexus/src/db/datastore.rs +++ b/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() diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index 5c3d8fc5ee..e7eac4440e 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/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 = + 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