From e248f8617ef7d6ee51b9f922b15bb3d1b0837900 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Oct 2022 16:18:31 -0500 Subject: [PATCH 1/6] paginated /session/me/groups instead of returning groups on /session/me --- nexus/db-model/src/silo_group.rs | 6 + nexus/src/app/iam.rs | 3 +- nexus/src/db/datastore/silo_group.rs | 3 +- nexus/src/external_api/console_api.rs | 48 ++++++- nexus/src/external_api/http_entrypoints.rs | 1 + nexus/tests/integration_tests/console_api.rs | 46 ++++++- nexus/tests/integration_tests/saml.rs | 36 +----- nexus/tests/output/nexus_tags.txt | 1 + nexus/types/src/external_api/views.rs | 19 +-- openapi/nexus.json | 128 ++++++++++++++----- 10 files changed, 199 insertions(+), 92 deletions(-) diff --git a/nexus/db-model/src/silo_group.rs b/nexus/db-model/src/silo_group.rs index 49e594c7527..c266ea44354 100644 --- a/nexus/db-model/src/silo_group.rs +++ b/nexus/db-model/src/silo_group.rs @@ -41,6 +41,12 @@ impl SiloGroupMembership { } } +impl From for views::SiloGroupMembership { + fn from(gm: SiloGroupMembership) -> Self { + Self { silo_group_id: gm.silo_group_id } + } +} + impl From for views::Group { fn from(group: SiloGroup) -> Self { Self { diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index 2e28c9ac03b..5ad93c171b6 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -93,8 +93,9 @@ impl super::Nexus { pub async fn silo_user_fetch_groups_for_self( &self, opctx: &OpContext, + pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { - self.db_datastore.silo_group_membership_for_self(opctx).await + self.db_datastore.silo_group_membership_for_self(opctx, pagparams).await } // Silo groups diff --git a/nexus/src/db/datastore/silo_group.rs b/nexus/src/db/datastore/silo_group.rs index 5d83d0af99e..459227773ca 100644 --- a/nexus/src/db/datastore/silo_group.rs +++ b/nexus/src/db/datastore/silo_group.rs @@ -109,6 +109,7 @@ impl DataStore { pub async fn silo_group_membership_for_self( &self, opctx: &OpContext, + pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { // Similar to session_hard_delete (see comment there), we do not do a // typical authz check, instead effectively encoding the policy here @@ -119,7 +120,7 @@ impl DataStore { .internal_context("fetching current user's group memberships")?; use db::schema::silo_group_membership::dsl; - dsl::silo_group_membership + paginated(dsl::silo_group_membership, dsl::silo_group_id, pagparams) .filter(dsl::silo_user_id.eq(actor.actor_id())) .select(SiloGroupMembership::as_returning()) .get_results_async(self.pool_authorized(opctx).await?) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index 1886626bce6..ea008e220de 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -28,12 +28,15 @@ use dropshot::{ endpoint, http_response_found, http_response_see_other, HttpError, HttpResponseFound, HttpResponseHeaders, HttpResponseOk, HttpResponseSeeOther, HttpResponseUpdatedNoContent, Path, Query, - RequestContext, TypedBody, + RequestContext, ResultsPage, TypedBody, }; use http::{header, Response, StatusCode}; use hyper::Body; use lazy_static::lazy_static; use mime_guess; +use omicron_common::api::external::http_pagination::{ + data_page_params_for, PaginatedById, ScanById, ScanParams, +}; use omicron_common::api::external::Error; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -592,7 +595,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,17 +604,52 @@ 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_for_self(&opctx).await?; - Ok(HttpResponseOk(views::SessionMe { + Ok(HttpResponseOk(views::User { 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 } +/// Fetch the silo groups of which the current user is a member +#[endpoint { + method = GET, + path = "/session/me/groups", + tags = ["hidden"], + }] +pub async fn session_me_groups( + rqctx: Arc>>, + query_params: Query, +) -> Result>, HttpError> +{ + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let query = query_params.into_inner(); + let handler = async { + // We don't care about authentication method, as long as they are authed + // as _somebody_. We could restrict this to session auth only, but it's + // not clear what the advantage would be. + let opctx = OpContext::for_external_api(&rqctx).await?; + let group_memberships = nexus + .silo_user_fetch_groups_for_self( + &opctx, + &data_page_params_for(&rqctx, &query)?, + ) + .await? + .into_iter() + .map(|d| d.into()) + .collect(); + Ok(HttpResponseOk(ScanById::results_page( + &query, + group_memberships, + &|_, group: &views::SiloGroupMembership| group.silo_group_id, + )?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + pub async fn console_index_or_login_redirect( rqctx: Arc>>, ) -> Result, HttpError> { diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 55a7f340944..653b6eb9fff 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -260,6 +260,7 @@ pub fn external_api() -> NexusApiDescription { api.register(console_api::logout)?; api.register(console_api::session_me)?; + api.register(console_api::session_me_groups)?; api.register(console_api::console_page)?; api.register(console_api::console_root)?; api.register(console_api::console_settings_page)?; diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index 42c93e7dafb..b2bf1b93a32 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -3,6 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use dropshot::test_util::ClientTestContext; +use dropshot::ResultsPage; use http::header::HeaderName; use http::{header, method::Method, StatusCode}; use std::env::current_dir; @@ -335,16 +336,15 @@ 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::SessionMe { + views::User { id: USER_TEST_PRIVILEGED.id(), display_name: USER_TEST_PRIVILEGED.external_id.clone(), silo_id: DEFAULT_SILO.id(), - group_ids: vec![], } ); @@ -353,20 +353,54 @@ async fn test_session_me(cptestctx: &ControlPlaneTestContext) { .execute() .await .expect("failed to get current user") - .parsed_body::() + .parsed_body::() .unwrap(); assert_eq!( unpriv_user, - views::SessionMe { + views::User { id: USER_TEST_UNPRIVILEGED.id(), display_name: USER_TEST_UNPRIVILEGED.external_id.clone(), silo_id: DEFAULT_SILO.id(), - group_ids: vec![], } ); } +#[nexus_test] +async fn test_session_me_groups(cptestctx: &ControlPlaneTestContext) { + let testctx = &cptestctx.external_client; + + // hitting /session/me without being logged in is a 401 + RequestBuilder::new(&testctx, Method::GET, "/session/me/groups") + .expect_status(Some(StatusCode::UNAUTHORIZED)) + .execute() + .await + .expect("failed to 401 on unauthed request"); + + // now make same request with auth + let priv_user_groups = + NexusRequest::object_get(testctx, "/session/me/groups") + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to get current user") + .parsed_body::>() + .unwrap(); + + assert_eq!(priv_user_groups.items, vec![]); + + let unpriv_user_groups = + NexusRequest::object_get(testctx, "/session/me/groups") + .authn_as(AuthnMode::UnprivilegedUser) + .execute() + .await + .expect("failed to get current user") + .parsed_body::>() + .unwrap(); + + assert_eq!(unpriv_user_groups.items, vec![]); +} + #[nexus_test] async fn test_login_redirect(cptestctx: &ControlPlaneTestContext) { let testctx = &cptestctx.external_client; diff --git a/nexus/tests/integration_tests/saml.rs b/nexus/tests/integration_tests/saml.rs index b3542e88767..3d2eee9e0bc 100644 --- a/nexus/tests/integration_tests/saml.rs +++ b/nexus/tests/integration_tests/saml.rs @@ -2,8 +2,6 @@ // 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::{ @@ -21,9 +19,7 @@ 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 @@ -963,7 +959,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { signing_keypair: None, - group_attribute_name: Some("groups".into()), + group_attribute_name: None, }, ) .await; @@ -988,7 +984,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { ) .raw_body(Some( serde_urlencoded::to_string(SamlLoginPost { - saml_response: base64::encode(SAML_RESPONSE_WITH_GROUPS), + saml_response: base64::encode(SAML_RESPONSE), relay_state: None, }) .unwrap(), @@ -1013,24 +1009,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { 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(); - - assert_same_items(silo_group_names, vec!["SRE", "Admins"]); - - let session_me: views::SessionMe = NexusRequest::new( + let session_me: views::User = NexusRequest::new( RequestBuilder::new(client, Method::GET, "/session/me") .header(http::header::COOKIE, session_cookie_value) .expect_status(Some(StatusCode::OK)), @@ -1042,15 +1021,6 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { .unwrap(); assert_eq!(session_me.display_name, "some@customer.com"); - 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 diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index b3ebd936b0a..eb929544f19 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -15,6 +15,7 @@ device_auth_request /device/auth login_spoof /login logout /logout session_me /session/me +session_me_groups /session/me/groups API operations found with tag "images" OPERATION ID URL PATH diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index e5e472d696c..45c98e0ad13 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -322,19 +322,6 @@ pub struct User { pub silo_id: Uuid, } -/// Client view of a [`User`] and their groups -#[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`] @@ -349,6 +336,12 @@ pub struct Group { pub silo_id: Uuid, } +/// Client view of a [`Group`] +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)] +pub struct SiloGroupMembership { + pub silo_group_id: Uuid, +} + // BUILT-IN USERS /// Client view of a [`UserBuiltin`] diff --git a/openapi/nexus.json b/openapi/nexus.json index 9363e9ad8e9..3fe29c694da 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5202,7 +5202,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/SessionMe" + "$ref": "#/components/schemas/User" } } } @@ -5216,6 +5216,66 @@ } } }, + "/session/me/groups": { + "get": { + "tags": [ + "hidden" + ], + "summary": "Fetch the silo groups of which the current user is a member", + "operationId": "session_me_groups", + "parameters": [ + { + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + }, + "style": "form" + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retrieve the subsequent page", + "schema": { + "nullable": true, + "type": "string" + }, + "style": "form" + }, + { + "in": "query", + "name": "sort_by", + "schema": { + "$ref": "#/components/schemas/IdSortMode" + }, + "style": "form" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SiloGroupMembershipResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": true + } + }, "/session/me/sshkeys": { "get": { "tags": [ @@ -11002,38 +11062,6 @@ "technical_contact_email" ] }, - "SessionMe": { - "description": "Client view of a [`User`] and their groups", - "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", @@ -11117,6 +11145,40 @@ "name" ] }, + "SiloGroupMembership": { + "description": "Client view of a [`Group`]", + "type": "object", + "properties": { + "silo_group_id": { + "type": "string", + "format": "uuid" + } + }, + "required": [ + "silo_group_id" + ] + }, + "SiloGroupMembershipResultsPage": { + "description": "A single page of results", + "type": "object", + "properties": { + "items": { + "description": "list of items on this page of results", + "type": "array", + "items": { + "$ref": "#/components/schemas/SiloGroupMembership" + } + }, + "next_page": { + "nullable": true, + "description": "token used to fetch the next page of results (if any)", + "type": "string" + } + }, + "required": [ + "items" + ] + }, "SiloIdentityMode": { "description": "Describes how identities are managed and users are authenticated in this Silo", "oneOf": [ From c8a38bdb0866970ab4470656cdc2e2be07c08226 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Oct 2022 16:25:18 -0500 Subject: [PATCH 2/6] put back saml integration test, use groups endpoint instead --- nexus/tests/integration_tests/saml.rs | 52 +++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/nexus/tests/integration_tests/saml.rs b/nexus/tests/integration_tests/saml.rs index 3d2eee9e0bc..cc99bd8b5c1 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::{ @@ -19,7 +21,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 +963,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 +988,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(), @@ -1009,9 +1013,26 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { 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(); + + assert_same_items(silo_group_names, vec!["SRE", "Admins"]); + let session_me: views::User = NexusRequest::new( RequestBuilder::new(client, Method::GET, "/session/me") - .header(http::header::COOKIE, session_cookie_value) + .header(http::header::COOKIE, session_cookie_value.clone()) .expect_status(Some(StatusCode::OK)), ) .execute() @@ -1021,6 +1042,31 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { .unwrap(); assert_eq!(session_me.display_name, "some@customer.com"); + + let session_me: ResultsPage = + NexusRequest::new( + RequestBuilder::new(client, Method::GET, "/session/me/groups") + .header(http::header::COOKIE, session_cookie_value) + .expect_status(Some(StatusCode::OK)), + ) + .execute() + .await + .expect("expected success") + .parsed_body() + .unwrap(); + + let session_me_group_ids = + session_me.items.iter().map(|g| g.silo_group_id).collect::>(); + + 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 From 380f7ed16ea2170377f5e1b2283e3d8627904a23 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Oct 2022 16:59:22 -0500 Subject: [PATCH 3/6] change groups endpoint to return actual groups --- nexus/db-model/src/silo_group.rs | 6 ---- nexus/src/app/iam.rs | 4 +-- nexus/src/db/datastore/silo_group.rs | 14 ++++---- nexus/src/external_api/console_api.rs | 9 +++-- nexus/tests/integration_tests/console_api.rs | 4 +-- nexus/tests/integration_tests/saml.rs | 23 ++++++------- nexus/types/src/external_api/views.rs | 6 ---- openapi/nexus.json | 36 +------------------- 8 files changed, 28 insertions(+), 74 deletions(-) diff --git a/nexus/db-model/src/silo_group.rs b/nexus/db-model/src/silo_group.rs index c266ea44354..49e594c7527 100644 --- a/nexus/db-model/src/silo_group.rs +++ b/nexus/db-model/src/silo_group.rs @@ -41,12 +41,6 @@ impl SiloGroupMembership { } } -impl From for views::SiloGroupMembership { - fn from(gm: SiloGroupMembership) -> Self { - Self { silo_group_id: gm.silo_group_id } - } -} - impl From for views::Group { fn from(group: SiloGroup) -> Self { Self { diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index 5ad93c171b6..5963dcd5129 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -94,8 +94,8 @@ impl super::Nexus { &self, opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { - self.db_datastore.silo_group_membership_for_self(opctx, pagparams).await + ) -> ListResultVec { + self.db_datastore.silo_groups_for_self(opctx, pagparams).await } // Silo groups diff --git a/nexus/src/db/datastore/silo_group.rs b/nexus/src/db/datastore/silo_group.rs index 459227773ca..0261dc55428 100644 --- a/nexus/src/db/datastore/silo_group.rs +++ b/nexus/src/db/datastore/silo_group.rs @@ -106,11 +106,11 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } - pub async fn silo_group_membership_for_self( + pub async fn silo_groups_for_self( &self, opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { + ) -> ListResultVec { // 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 @@ -119,10 +119,12 @@ impl DataStore { .actor_required() .internal_context("fetching current user's group memberships")?; - use db::schema::silo_group_membership::dsl; - paginated(dsl::silo_group_membership, dsl::silo_group_id, pagparams) - .filter(dsl::silo_user_id.eq(actor.actor_id())) - .select(SiloGroupMembership::as_returning()) + use db::schema::{silo_group as sg, silo_group_membership as sgm}; + paginated(sg::dsl::silo_group, sg::id, pagparams) + .inner_join(sgm::table.on(sgm::silo_group_id.eq(sg::id))) + .filter(sgm::silo_user_id.eq(actor.actor_id())) + .filter(sg::time_deleted.is_null()) + .select(SiloGroup::as_returning()) .get_results_async(self.pool_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index ea008e220de..b9e059fbe15 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -622,8 +622,7 @@ pub async fn session_me( pub async fn session_me_groups( rqctx: Arc>>, query_params: Query, -) -> Result>, HttpError> -{ +) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); @@ -632,7 +631,7 @@ pub async fn session_me_groups( // as _somebody_. We could restrict this to session auth only, but it's // not clear what the advantage would be. let opctx = OpContext::for_external_api(&rqctx).await?; - let group_memberships = nexus + let groups = nexus .silo_user_fetch_groups_for_self( &opctx, &data_page_params_for(&rqctx, &query)?, @@ -643,8 +642,8 @@ pub async fn session_me_groups( .collect(); Ok(HttpResponseOk(ScanById::results_page( &query, - group_memberships, - &|_, group: &views::SiloGroupMembership| group.silo_group_id, + groups, + &|_, group: &views::Group| group.id, )?)) }; 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 b2bf1b93a32..e220d8551ed 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -384,7 +384,7 @@ async fn test_session_me_groups(cptestctx: &ControlPlaneTestContext) { .execute() .await .expect("failed to get current user") - .parsed_body::>() + .parsed_body::>() .unwrap(); assert_eq!(priv_user_groups.items, vec![]); @@ -395,7 +395,7 @@ async fn test_session_me_groups(cptestctx: &ControlPlaneTestContext) { .execute() .await .expect("failed to get current user") - .parsed_body::>() + .parsed_body::>() .unwrap(); assert_eq!(unpriv_user_groups.items, vec![]); diff --git a/nexus/tests/integration_tests/saml.rs b/nexus/tests/integration_tests/saml.rs index cc99bd8b5c1..eb2827e9070 100644 --- a/nexus/tests/integration_tests/saml.rs +++ b/nexus/tests/integration_tests/saml.rs @@ -1043,20 +1043,19 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) { assert_eq!(session_me.display_name, "some@customer.com"); - let session_me: ResultsPage = - NexusRequest::new( - RequestBuilder::new(client, Method::GET, "/session/me/groups") - .header(http::header::COOKIE, session_cookie_value) - .expect_status(Some(StatusCode::OK)), - ) - .execute() - .await - .expect("expected success") - .parsed_body() - .unwrap(); + let session_me: ResultsPage = NexusRequest::new( + RequestBuilder::new(client, Method::GET, "/session/me/groups") + .header(http::header::COOKIE, session_cookie_value) + .expect_status(Some(StatusCode::OK)), + ) + .execute() + .await + .expect("expected success") + .parsed_body() + .unwrap(); let session_me_group_ids = - session_me.items.iter().map(|g| g.silo_group_id).collect::>(); + session_me.items.iter().map(|g| g.id).collect::>(); assert_same_items(session_me_group_ids, silo_group_ids); } diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index 45c98e0ad13..c160a12f3db 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -336,12 +336,6 @@ pub struct Group { pub silo_id: Uuid, } -/// Client view of a [`Group`] -#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)] -pub struct SiloGroupMembership { - pub silo_group_id: Uuid, -} - // BUILT-IN USERS /// Client view of a [`UserBuiltin`] diff --git a/openapi/nexus.json b/openapi/nexus.json index 3fe29c694da..6e3c23eb526 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5261,7 +5261,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/SiloGroupMembershipResultsPage" + "$ref": "#/components/schemas/GroupResultsPage" } } } @@ -11145,40 +11145,6 @@ "name" ] }, - "SiloGroupMembership": { - "description": "Client view of a [`Group`]", - "type": "object", - "properties": { - "silo_group_id": { - "type": "string", - "format": "uuid" - } - }, - "required": [ - "silo_group_id" - ] - }, - "SiloGroupMembershipResultsPage": { - "description": "A single page of results", - "type": "object", - "properties": { - "items": { - "description": "list of items on this page of results", - "type": "array", - "items": { - "$ref": "#/components/schemas/SiloGroupMembership" - } - }, - "next_page": { - "nullable": true, - "description": "token used to fetch the next page of results (if any)", - "type": "string" - } - }, - "required": [ - "items" - ] - }, "SiloIdentityMode": { "description": "Describes how identities are managed and users are authenticated in this Silo", "oneOf": [ From 952a41c5e21dbdad3a948a16117a38aad616cdc1 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Oct 2022 17:05:34 -0500 Subject: [PATCH 4/6] less embarrassing docstring --- nexus/src/external_api/console_api.rs | 2 +- openapi/nexus.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index b9e059fbe15..ffae01632c5 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -613,7 +613,7 @@ pub async fn session_me( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/// Fetch the silo groups of which the current user is a member +/// Fetch the silo groups the current user belongs to #[endpoint { method = GET, path = "/session/me/groups", diff --git a/openapi/nexus.json b/openapi/nexus.json index 6e3c23eb526..741894b5e70 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5221,7 +5221,7 @@ "tags": [ "hidden" ], - "summary": "Fetch the silo groups of which the current user is a member", + "summary": "Fetch the silo groups the current user belongs to", "operationId": "session_me_groups", "parameters": [ { From a1695babc61c79c26b2ab6cf4ba963e82fbbcafd Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Oct 2022 17:15:19 -0500 Subject: [PATCH 5/6] dummy commit because I confused buildomat with a stray branch --- nexus/src/external_api/console_api.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index ffae01632c5..1f4aead4322 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -604,11 +604,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?; - Ok(HttpResponseOk(views::User { - id: user.id(), - display_name: user.external_id, - silo_id: user.silo_id, - })) + Ok(HttpResponseOk(user.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } From 1f0da11a71d316880414e9932c9f5f5207d76931 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 19 Oct 2022 20:44:12 -0500 Subject: [PATCH 6/6] fix unauthorized coverage --- nexus/tests/integration_tests/endpoints.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index e3377fa4cd4..ac4834e8afc 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -1542,6 +1542,14 @@ lazy_static! { AllowedMethod::Get, ], }, + VerifyEndpoint { + url: "/session/me/groups", + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::ReadOnly, + allowed_methods: vec![ + AllowedMethod::Get, + ], + }, /* SSH keys */