From a0b8735991914904b006997f18f67b15a2bba5ff Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 17 Oct 2022 11:34:52 -0500 Subject: [PATCH 1/7] add group IDs to /session/me response --- nexus/src/app/iam.rs | 7 ++++ nexus/src/db/datastore/silo_group.rs | 22 +++++++++++++ nexus/src/external_api/console_api.rs | 10 ++++-- nexus/tests/integration_tests/console_api.rs | 11 ++++--- nexus/types/src/external_api/views.rs | 13 ++++++++ openapi/nexus.json | 34 +++++++++++++++++++- 6 files changed, 90 insertions(+), 7 deletions(-) diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index a5ae495bacd..c1698789637 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -90,6 +90,13 @@ impl super::Nexus { Ok(db_silo_user) } + pub async fn silo_user_fetch_groups( + &self, + opctx: &OpContext, + ) -> ListResultVec { + Ok(self.db_datastore.silo_group_membership_for_self(opctx).await?) + } + // Silo groups pub async fn silo_groups_list( diff --git a/nexus/src/db/datastore/silo_group.rs b/nexus/src/db/datastore/silo_group.rs index 014a672a1a0..4afbef6fa7b 100644 --- a/nexus/src/db/datastore/silo_group.rs +++ b/nexus/src/db/datastore/silo_group.rs @@ -23,6 +23,7 @@ use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; +use omicron_common::api::external::InternalContext; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::UpdateResult; @@ -105,6 +106,27 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } + pub async fn silo_group_membership_for_self( + &self, + opctx: &OpContext, + ) -> ListResultVec { + // no authz check beyond pulling the current actor because current user + // can always list their own memberships + + let &actor = opctx + .authn + .actor_required() + .internal_context("fetching current user's group memberships")?; + + use db::schema::silo_group_membership::dsl; + dsl::silo_group_membership + .filter(dsl::silo_user_id.eq(actor.actor_id())) + .select(SiloGroupMembership::as_returning()) + .get_results_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + } + /// Update a silo user's group membership: /// /// - add the user to groups they are supposed to be a member of, and diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index ff35d570c28..e9c5d8eae15 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -592,7 +592,7 @@ pub async fn login_begin( }] pub async fn session_me( rqctx: Arc>>, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let handler = async { @@ -601,7 +601,13 @@ pub async fn session_me( // not clear what the advantage would be. let opctx = OpContext::for_external_api(&rqctx).await?; let user = nexus.silo_user_fetch_self(&opctx).await?; - Ok(HttpResponseOk(user.into())) + let groups = nexus.silo_user_fetch_groups(&opctx).await?; + Ok(HttpResponseOk(views::SessionMe { + id: user.id(), + display_name: user.external_id, + silo_id: user.silo_id, + group_ids: groups.iter().map(|g| g.silo_group_id).collect(), + })) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index ad4908dcca0..ffde96b80d7 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -335,33 +335,36 @@ async fn test_session_me(cptestctx: &ControlPlaneTestContext) { .execute() .await .expect("failed to get current user") - .parsed_body::() + .parsed_body::() .unwrap(); assert_eq!( priv_user, - views::User { + views::SessionMe { id: USER_TEST_PRIVILEGED.id(), display_name: USER_TEST_PRIVILEGED.external_id.clone(), silo_id: DEFAULT_SILO.id(), + group_ids: vec![], } ); + // TODO: fails bc user can't pull their own group memberships? // make sure it returns different things for different users let unpriv_user = NexusRequest::object_get(testctx, "/session/me") .authn_as(AuthnMode::UnprivilegedUser) .execute() .await .expect("failed to get current user") - .parsed_body::() + .parsed_body::() .unwrap(); assert_eq!( unpriv_user, - views::User { + views::SessionMe { id: USER_TEST_UNPRIVILEGED.id(), display_name: USER_TEST_UNPRIVILEGED.external_id.clone(), silo_id: DEFAULT_SILO.id(), + group_ids: vec![], } ); } diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index c160a12f3db..24c2d2fdd99 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -322,6 +322,19 @@ pub struct User { pub silo_id: Uuid, } +/// Client view of a [`User`] with some extra stuff that's useful to the console +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)] +pub struct SessionMe { + pub id: Uuid, + /** Human-readable name that can identify the user */ + pub display_name: String, + + /** Uuid of the silo to which this user belongs */ + pub silo_id: Uuid, + + pub group_ids: Vec, +} + // SILO GROUPS /// Client view of a [`Group`] diff --git a/openapi/nexus.json b/openapi/nexus.json index 9babf1f1f30..cb25287ee41 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5202,7 +5202,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/User" + "$ref": "#/components/schemas/SessionMe" } } } @@ -11002,6 +11002,38 @@ "technical_contact_email" ] }, + "SessionMe": { + "description": "Client view of a [`User`] with some extra stuff that's useful to the console", + "type": "object", + "properties": { + "display_name": { + "description": "Human-readable name that can identify the user", + "type": "string" + }, + "group_ids": { + "type": "array", + "items": { + "type": "string", + "format": "uuid" + } + }, + "id": { + "type": "string", + "format": "uuid" + }, + "silo_id": { + "description": "Uuid of the silo to which this user belongs", + "type": "string", + "format": "uuid" + } + }, + "required": [ + "display_name", + "group_ids", + "id", + "silo_id" + ] + }, "Silo": { "description": "Client view of a ['Silo']", "type": "object", From 7479c5dd127e7f695b12e4a20a6feeebb0841953 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 17 Oct 2022 14:48:51 -0500 Subject: [PATCH 2/7] you win this round, clippy --- nexus/src/app/iam.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index c1698789637..37a58abed24 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -94,7 +94,7 @@ impl super::Nexus { &self, opctx: &OpContext, ) -> ListResultVec { - Ok(self.db_datastore.silo_group_membership_for_self(opctx).await?) + self.db_datastore.silo_group_membership_for_self(opctx).await } // Silo groups From 8acee427a6978886f9b2536f116205e6cf36e816 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 17 Oct 2022 14:54:32 -0500 Subject: [PATCH 3/7] more smarter comment --- nexus/src/db/datastore/silo_group.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nexus/src/db/datastore/silo_group.rs b/nexus/src/db/datastore/silo_group.rs index 4afbef6fa7b..5d83d0af99e 100644 --- a/nexus/src/db/datastore/silo_group.rs +++ b/nexus/src/db/datastore/silo_group.rs @@ -110,9 +110,9 @@ impl DataStore { &self, opctx: &OpContext, ) -> ListResultVec { - // no authz check beyond pulling the current actor because current user - // can always list their own memberships - + // Similar to session_hard_delete (see comment there), we do not do a + // typical authz check, instead effectively encoding the policy here + // that any user is allowed to fetch their own group memberships let &actor = opctx .authn .actor_required() From 5f0ae14b551e15840329ee337daee2aaa966ad9b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 18 Oct 2022 14:23:42 -0500 Subject: [PATCH 4/7] address PR comments --- nexus/src/app/iam.rs | 2 +- nexus/src/external_api/console_api.rs | 2 +- nexus/tests/integration_tests/console_api.rs | 2 -- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index 37a58abed24..2e28c9ac03b 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -90,7 +90,7 @@ impl super::Nexus { Ok(db_silo_user) } - pub async fn silo_user_fetch_groups( + pub async fn silo_user_fetch_groups_for_self( &self, opctx: &OpContext, ) -> ListResultVec { diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index e9c5d8eae15..1886626bce6 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -601,7 +601,7 @@ pub async fn session_me( // not clear what the advantage would be. let opctx = OpContext::for_external_api(&rqctx).await?; let user = nexus.silo_user_fetch_self(&opctx).await?; - let groups = nexus.silo_user_fetch_groups(&opctx).await?; + let groups = nexus.silo_user_fetch_groups_for_self(&opctx).await?; Ok(HttpResponseOk(views::SessionMe { id: user.id(), display_name: user.external_id, diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index ffde96b80d7..42c93e7dafb 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -348,8 +348,6 @@ async fn test_session_me(cptestctx: &ControlPlaneTestContext) { } ); - // TODO: fails bc user can't pull their own group memberships? - // make sure it returns different things for different users let unpriv_user = NexusRequest::object_get(testctx, "/session/me") .authn_as(AuthnMode::UnprivilegedUser) .execute() From 29501624384998a3d3dcef2c030a71fd153df786 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 18 Oct 2022 15:26:22 -0500 Subject: [PATCH 5/7] let's not be so casual in the comments --- nexus/types/src/external_api/views.rs | 2 +- openapi/nexus.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index 24c2d2fdd99..e5e472d696c 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -322,7 +322,7 @@ pub struct User { pub silo_id: Uuid, } -/// Client view of a [`User`] with some extra stuff that's useful to the console +/// Client view of a [`User`] and their groups #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)] pub struct SessionMe { pub id: Uuid, diff --git a/openapi/nexus.json b/openapi/nexus.json index cb25287ee41..9363e9ad8e9 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -11003,7 +11003,7 @@ ] }, "SessionMe": { - "description": "Client view of a [`User`] with some extra stuff that's useful to the console", + "description": "Client view of a [`User`] and their groups", "type": "object", "properties": { "display_name": { From 0e61d09a3fd528892fb9ee60021098a47c2acdbb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 18 Oct 2022 16:26:17 -0500 Subject: [PATCH 6/7] test that session me returns group IDs when it's supposed to --- nexus/tests/integration_tests/saml.rs | 39 ++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/nexus/tests/integration_tests/saml.rs b/nexus/tests/integration_tests/saml.rs index 55746338120..e5383192a77 100644 --- a/nexus/tests/integration_tests/saml.rs +++ b/nexus/tests/integration_tests/saml.rs @@ -19,7 +19,9 @@ use nexus_test_utils::resource_helpers::{create_silo, object_create}; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; +use dropshot::ResultsPage; use httptest::{matchers::*, responders::*, Expectation, Server}; +use uuid::Uuid; // Valid SAML IdP entity descriptor from https://en.wikipedia.org/wiki/SAML_metadata#Identity_provider_metadata // note: no signing keys @@ -959,7 +961,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { signing_keypair: None, - group_attribute_name: None, + group_attribute_name: Some("groups".into()), }, ) .await; @@ -984,7 +986,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { ) .raw_body(Some( serde_urlencoded::to_string(SamlLoginPost { - saml_response: base64::encode(SAML_RESPONSE), + saml_response: base64::encode(SAML_RESPONSE_WITH_GROUPS), relay_state: None, }) .unwrap(), @@ -1006,12 +1008,32 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { .await .expect("expected success"); - let _session_user: views::User = NexusRequest::new( + let session_cookie_value = + result.headers["Set-Cookie"].to_str().unwrap().to_string(); + + let groups: ResultsPage = NexusRequest::new( + RequestBuilder::new(client, Method::GET, "/groups") + .header(http::header::COOKIE, session_cookie_value.clone()) + .expect_status(Some(StatusCode::OK)), + ) + .execute() + .await + .expect("expected success") + .parsed_body() + .unwrap(); + + let silo_group_names: Vec<&str> = + groups.items.iter().map(|g| g.display_name.as_str()).collect(); + let silo_group_ids: Vec = groups.items.iter().map(|g| g.id).collect(); + + // use contains because order is not consistent + assert_eq!(silo_group_names.len(), 2); + assert!(silo_group_names.contains(&"SRE")); + assert!(silo_group_names.contains(&"Admins")); + + let session_me: views::SessionMe = NexusRequest::new( RequestBuilder::new(client, Method::GET, "/session/me") - .header( - http::header::COOKIE, - result.headers["Set-Cookie"].to_str().unwrap().to_string(), - ) + .header(http::header::COOKIE, session_cookie_value) .expect_status(Some(StatusCode::OK)), ) .execute() @@ -1019,6 +1041,9 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { .expect("expected success") .parsed_body() .unwrap(); + + assert_eq!(session_me.display_name, "some@customer.com"); + assert_eq!(session_me.group_ids, silo_group_ids); // user has all the groups } // Test correct SAML response with relay state From 52b9f46e6f851d554c30bc47439d790d7ee13588 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Oct 2022 11:44:50 -0500 Subject: [PATCH 7/7] order agnostic vec equality helper --- nexus/tests/integration_tests/saml.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/nexus/tests/integration_tests/saml.rs b/nexus/tests/integration_tests/saml.rs index e5383192a77..b3542e88767 100644 --- a/nexus/tests/integration_tests/saml.rs +++ b/nexus/tests/integration_tests/saml.rs @@ -2,6 +2,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use std::fmt::Debug; + use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder}; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_nexus::authn::silos::{ @@ -1026,10 +1028,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { groups.items.iter().map(|g| g.display_name.as_str()).collect(); let silo_group_ids: Vec = groups.items.iter().map(|g| g.id).collect(); - // use contains because order is not consistent - assert_eq!(silo_group_names.len(), 2); - assert!(silo_group_names.contains(&"SRE")); - assert!(silo_group_names.contains(&"Admins")); + assert_same_items(silo_group_names, vec!["SRE", "Admins"]); let session_me: views::SessionMe = NexusRequest::new( RequestBuilder::new(client, Method::GET, "/session/me") @@ -1043,7 +1042,15 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { .unwrap(); assert_eq!(session_me.display_name, "some@customer.com"); - assert_eq!(session_me.group_ids, silo_group_ids); // user has all the groups + assert_same_items(session_me.group_ids, silo_group_ids); +} + +/// Order-agnostic vec equality +fn assert_same_items(v1: Vec, v2: Vec) { + assert_eq!(v1.len(), v2.len(), "{:?} and {:?} don't match", v1, v2); + for item in v1.iter() { + assert!(v2.contains(item), "{:?} and {:?} don't match", v1, v2); + } } // Test correct SAML response with relay state