From c7926bc75cf7da10c92919af148bbbf09980be9f Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 4 Oct 2022 20:12:55 -0700 Subject: [PATCH 01/15] local user CRUD: extract from my initial "passwords" branch --- nexus/src/app/silo.rs | 38 +++++- nexus/src/external_api/http_entrypoints.rs | 39 +++++- nexus/test-utils/src/resource_helpers.rs | 15 +++ nexus/tests/integration_tests/silos.rs | 147 +++++++++++++++++++++ nexus/types/src/external_api/params.rs | 51 ++++++- 5 files changed, 286 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 4d288bacbc..eb62a5bf3d 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -14,13 +14,15 @@ use crate::external_api::params; use crate::external_api::shared; use crate::{authn, authz}; use anyhow::Context; -use omicron_common::api::external::CreateResult; +use nexus_db_model::UserProvisionType; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::UpdateResult; +use omicron_common::api::external::{CreateResult, LookupType}; +use omicron_common::bail_unless; use uuid::Uuid; impl super::Nexus { @@ -140,6 +142,34 @@ impl super::Nexus { // Users + pub async fn silo_fixed_user_create( + &self, + opctx: &OpContext, + new_user_params: params::UserCreate, + ) -> CreateResult { + let datastore = self.datastore(); + let authz_silo = opctx.authn.silo_required()?; + let (.., db_silo) = LookupPath::new(opctx, &datastore) + .silo_id(authz_silo.id()) + .fetch() + .await?; + + if db_silo.user_provision_type != UserProvisionType::Fixed { + return Err(Error::invalid_request(&format!( + "cannot create users in this kind of Silo" + ))); + } + + let silo_user = db::model::SiloUser::new( + authz_silo.id(), + Uuid::new_v4(), + new_user_params.external_id.as_ref().to_owned(), + ); + + let db_silo_user = datastore.silo_user_create(silo_user).await?; + Ok(db_silo_user) + } + pub async fn silo_user_fetch( &self, opctx: &OpContext, @@ -378,6 +408,12 @@ impl super::Nexus { .await?; let authz_idp_list = authz::SiloIdentityProviderList::new(authz_silo); + if db_silo.user_provision_type != UserProvisionType::Jit { + return Err(Error::invalid_request(&format!( + "cannot create identity providers in this kind of Silo" + ))); + } + // This check is not strictly necessary yet. We'll check this // permission in the DataStore when we actually update the list. // But we check now to protect the code that fetches the descriptor from diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index dae2308f9e..f3176a3b0e 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -242,7 +242,9 @@ pub fn external_api() -> NexusApiDescription { api.register(system_image_delete)?; api.register(updates_refresh)?; + api.register(user_list)?; + api.register(user_create)?; // Console API operations api.register(console_api::login_begin)?; @@ -4112,6 +4114,39 @@ async fn user_list( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// Create a user +/// +/// Users can only be created in Silos with `provision_type` == `Fixed`. +/// Otherwise, Silo users are just-in-time (JIT) provisioned when a user first +/// logs in using an external Identity Provider. +#[endpoint { + method = POST, + path = "/users", + tags = ["silos"], +}] +async fn user_create( + rqctx: Arc>>, + new_user_params: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let user = nexus + .silo_fixed_user_create(&opctx, new_user_params.into_inner()) + .await?; + Ok(HttpResponseCreated(user.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Path parameters for Silo User requests +#[derive(Deserialize, JsonSchema)] +struct UserPathParam { + /// The user's internal id + user_id: Uuid, +} + // Built-in (system) users /// List built-in users @@ -4148,7 +4183,7 @@ async fn system_user_list( /// Path parameters for global (system) user requests #[derive(Deserialize, JsonSchema)] -struct UserPathParam { +struct BuiltinUserPathParam { /// The built-in user's unique name. user_name: Name, } @@ -4161,7 +4196,7 @@ struct UserPathParam { }] async fn system_user_view( rqctx: Arc>>, - path_params: Path, + path_params: Path, ) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 3ead59bff4..6b337ff709 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -18,11 +18,13 @@ use omicron_common::api::external::Instance; use omicron_common::api::external::InstanceCpuCount; use omicron_nexus::crucible_agent_client::types::State as RegionState; use omicron_nexus::external_api::params; +use omicron_nexus::external_api::params::UserId; use omicron_nexus::external_api::shared; use omicron_nexus::external_api::shared::IdentityType; use omicron_nexus::external_api::shared::IpRange; use omicron_nexus::external_api::views::IpPool; use omicron_nexus::external_api::views::IpPoolRange; +use omicron_nexus::external_api::views::User; use omicron_nexus::external_api::views::{ Organization, Project, Silo, Vpc, VpcRouter, }; @@ -126,6 +128,19 @@ pub async fn create_silo( .await } +pub async fn create_local_user( + client: &ClientTestContext, + username: &UserId, + password: params::UserPassword, +) -> User { + object_create( + client, + "/users", + ¶ms::UserCreate { external_id: username.to_owned(), password }, + ) + .await +} + pub async fn create_organization( client: &ClientTestContext, organization_name: &str, diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index b264d263a6..f46b69ae47 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -13,6 +13,7 @@ use omicron_nexus::external_api::views::{ use omicron_nexus::external_api::{params, shared}; use omicron_nexus::TestInterfaces as _; use std::collections::HashSet; +use std::str::FromStr; use http::method::Method; use http::StatusCode; @@ -1462,3 +1463,149 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) { // TODO-coverage were we intending to verify something here? } + +#[nexus_test] +async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; + let silo = + create_silo(&client, "jit", true, shared::UserProvisionType::Jit).await; + + // We need one initial user that would in principle have privileges to + // create other users. + let new_silo_user_id = + "6922f0b2-9a92-659b-da6b-93ad4955a3a3".parse().unwrap(); + let admin_user = nexus + .silo_user_create( + silo.identity.id, + new_silo_user_id, + "admin-user".into(), + ) + .await + .unwrap(); + + // Grant this user "admin" privileges on that Silo. + grant_iam( + client, + "/silos/jit", + SiloRole::Admin, + new_silo_user_id, + AuthnMode::PrivilegedUser, + ) + .await; + + // They should not be able to create a local-only user in this JIT Silo. + let password = params::Password::from_str("dummy").unwrap(); + let error: dropshot::HttpErrorResponseBody = + NexusRequest::expect_failure_with_body( + client, + StatusCode::BAD_REQUEST, + Method::POST, + "/users", + ¶ms::UserCreate { + external_id: params::UserId::from_str("dummy").unwrap(), + password: params::UserPassword::Password(password.clone()), + }, + ) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!(error.message, "cannot create users in this kind of Silo"); + +} + +#[nexus_test] +async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; + + // Now, let's try a "fixed" Silo with its own admin user. + let silo = + create_silo(&client, "fixed", true, shared::UserProvisionType::Fixed) + .await; + let new_silo_user_id = + "5b3564b6-8770-4a30-b538-8ef6ae3efa3b".parse().unwrap(); + let _ = nexus + .silo_user_create( + silo.identity.id, + new_silo_user_id, + "admin-user".into(), + ) + .await + .unwrap(); + grant_iam( + client, + "/silos/fixed", + SiloRole::Admin, + new_silo_user_id, + AuthnMode::PrivilegedUser, + ) + .await; + + // It's not allowed to create an identity provider in a fixed Silo. + let error: dropshot::HttpErrorResponseBody = + NexusRequest::expect_failure_with_body( + client, + StatusCode::BAD_REQUEST, + Method::POST, + "/silos/fixed/saml-identity-providers", + ¶ms::SamlIdentityProviderCreate { + identity: IdentityMetadataCreateParams { + name: "some-totally-real-saml-provider" + .to_string() + .parse() + .unwrap(), + description: "a demo provider".to_string(), + }, + + idp_metadata_source: + params::IdpMetadataSource::Base64EncodedXml { + data: base64::encode(SAML_IDP_DESCRIPTOR.to_string()), + }, + + idp_entity_id: "entity_id".to_string(), + sp_client_id: "client_id".to_string(), + acs_url: "http://acs".to_string(), + slo_url: "http://slo".to_string(), + technical_contact_email: "technical@fake".to_string(), + + signing_keypair: None, + + group_attribute_name: None, + }, + ) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + + assert_eq!( + error.message, + "cannot create identity providers in this kind of Silo" + ); + + // The SAML login endpoints should not work, either. + NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + "/login/fixed/foo", + ) + .execute() + .await + .unwrap(); + NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::POST, + "/login/fixed/foo", + ) + .execute() + .await + .unwrap(); +} diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index a1bca2666d..9b87afab0c 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -15,7 +15,7 @@ use serde::{ de::{self, Visitor}, Deserialize, Deserializer, Serialize, Serializer, }; -use std::net::IpAddr; +use std::{net::IpAddr, str::FromStr}; use uuid::Uuid; // Silos @@ -40,6 +40,55 @@ pub struct SiloCreate { pub admin_group_name: Option, } +/// Create-time parameters for a [`User`](crate::external_api::views::User) +#[derive(Clone, Deserialize, Serialize, JsonSchema)] +pub struct UserCreate { + /// username used to log in + pub external_id: UserId, +} + +/// A username for a local-only user +#[derive(Clone, Debug, Deserialize, Serialize)] +#[serde(try_from = "String")] +pub struct UserId(String); + +impl AsRef for UserId { + fn as_ref(&self) -> &str { + self.0.as_ref() + } +} + +impl FromStr for UserId { + type Err = String; + fn from_str(value: &str) -> Result { + UserId::try_from(String::from(value)) + } +} + +/// Used to impl `Deserialize` +impl TryFrom for UserId { + type Error = String; + fn try_from(value: String) -> Result { + // Mostly, this validation exists to cap the input size. The specific + // length is not critical here. For convenience and consistency, we use + // the same rules as `Name`. + let _ = Name::try_from(value.clone())?; + Ok(UserId(value)) + } +} + +impl JsonSchema for UserId { + fn schema_name() -> String { + "UserId".to_string() + } + + fn json_schema( + gen: &mut schemars::gen::SchemaGenerator, + ) -> schemars::schema::Schema { + Name::json_schema(gen) + } +} + // Silo identity providers #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] From f9994944dbb7894aa5cc72dea76b8b634a3e9254 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 4 Oct 2022 20:30:53 -0700 Subject: [PATCH 02/15] local user CRUD: fix up stuff extracted from the initial branch so that it builds (does not yet pass tests) --- nexus/src/app/silo.rs | 21 ++++---- nexus/src/external_api/http_entrypoints.rs | 7 --- nexus/test-utils/src/resource_helpers.rs | 3 +- nexus/tests/integration_tests/endpoints.rs | 14 ++++- nexus/tests/integration_tests/silos.rs | 30 ++++++----- nexus/tests/output/nexus_tags.txt | 1 + openapi/nexus.json | 60 ++++++++++++++++++++++ 7 files changed, 101 insertions(+), 35 deletions(-) diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index eb62a5bf3d..63a7537614 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -15,14 +15,13 @@ use crate::external_api::shared; use crate::{authn, authz}; use anyhow::Context; use nexus_db_model::UserProvisionType; +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::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::UpdateResult; -use omicron_common::api::external::{CreateResult, LookupType}; -use omicron_common::bail_unless; use uuid::Uuid; impl super::Nexus { @@ -154,10 +153,10 @@ impl super::Nexus { .fetch() .await?; - if db_silo.user_provision_type != UserProvisionType::Fixed { - return Err(Error::invalid_request(&format!( - "cannot create users in this kind of Silo" - ))); + if db_silo.user_provision_type != UserProvisionType::ApiOnly { + return Err(Error::invalid_request( + "cannot create users in this kind of Silo", + )); } let silo_user = db::model::SiloUser::new( @@ -165,8 +164,8 @@ impl super::Nexus { Uuid::new_v4(), new_user_params.external_id.as_ref().to_owned(), ); - - let db_silo_user = datastore.silo_user_create(silo_user).await?; + let (_, db_silo_user) = + datastore.silo_user_create(&authz_silo, silo_user).await?; Ok(db_silo_user) } @@ -409,9 +408,9 @@ impl super::Nexus { let authz_idp_list = authz::SiloIdentityProviderList::new(authz_silo); if db_silo.user_provision_type != UserProvisionType::Jit { - return Err(Error::invalid_request(&format!( - "cannot create identity providers in this kind of Silo" - ))); + return Err(Error::invalid_request( + "cannot create identity providers in this kind of Silo", + )); } // This check is not strictly necessary yet. We'll check this diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index f3176a3b0e..aa5754f62a 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -4140,13 +4140,6 @@ async fn user_create( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/// Path parameters for Silo User requests -#[derive(Deserialize, JsonSchema)] -struct UserPathParam { - /// The user's internal id - user_id: Uuid, -} - // Built-in (system) users /// List built-in users diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 6b337ff709..80fc4af0aa 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -131,12 +131,11 @@ pub async fn create_silo( pub async fn create_local_user( client: &ClientTestContext, username: &UserId, - password: params::UserPassword, ) -> User { object_create( client, "/users", - ¶ms::UserCreate { external_id: username.to_owned(), password }, + ¶ms::UserCreate { external_id: username.to_owned() }, ) .await } diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 945d5f8921..e6b55061f8 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -33,6 +33,7 @@ use omicron_nexus::external_api::shared::IpRange; use omicron_nexus::external_api::shared::Ipv4Range; use std::net::IpAddr; use std::net::Ipv4Addr; +use std::str::FromStr; lazy_static! { pub static ref HARDWARE_RACK_URL: String = @@ -262,7 +263,8 @@ lazy_static! { }; } -// Separate lazy_static! blocks to avoid hitting some recursion limit when compiling +// Separate lazy_static! blocks to avoid hitting some recursion limit when +// compiling lazy_static! { // Project Images pub static ref DEMO_IMAGE_NAME: Name = "demo-image".parse().unwrap(); @@ -386,6 +388,11 @@ lazy_static! { group_attribute_name: None, }; + + // Users + pub static ref DEMO_USER_CREATE: params::UserCreate = params::UserCreate { + external_id: params::UserId::from_str("dummy-user").unwrap(), + }; } /// Describes an API endpoint to be verified by the "unauthorized" test @@ -713,6 +720,11 @@ lazy_static! { unprivileged_access: UnprivilegedAccess::ReadOnly, allowed_methods: vec![ AllowedMethod::Get, + AllowedMethod::Post( + serde_json::to_value( + &*DEMO_USER_CREATE + ).unwrap() + ), ], }, diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index f46b69ae47..ef8f4afe82 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -1469,13 +1469,14 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; let silo = - create_silo(&client, "jit", true, shared::UserProvisionType::Jit).await; + create_silo(&client, "jit", true, shared::SiloIdentityMode::SamlJit) + .await; // We need one initial user that would in principle have privileges to // create other users. let new_silo_user_id = "6922f0b2-9a92-659b-da6b-93ad4955a3a3".parse().unwrap(); - let admin_user = nexus + let _ = nexus .silo_user_create( silo.identity.id, new_silo_user_id, @@ -1487,7 +1488,7 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { // Grant this user "admin" privileges on that Silo. grant_iam( client, - "/silos/jit", + "/system/silos/jit", SiloRole::Admin, new_silo_user_id, AuthnMode::PrivilegedUser, @@ -1495,7 +1496,6 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { .await; // They should not be able to create a local-only user in this JIT Silo. - let password = params::Password::from_str("dummy").unwrap(); let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure_with_body( client, @@ -1504,7 +1504,6 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { "/users", ¶ms::UserCreate { external_id: params::UserId::from_str("dummy").unwrap(), - password: params::UserPassword::Password(password.clone()), }, ) .authn_as(AuthnMode::SiloUser(new_silo_user_id)) @@ -1514,7 +1513,6 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { .parsed_body() .unwrap(); assert_eq!(error.message, "cannot create users in this kind of Silo"); - } #[nexus_test] @@ -1522,10 +1520,14 @@ async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; - // Now, let's try a "fixed" Silo with its own admin user. - let silo = - create_silo(&client, "fixed", true, shared::UserProvisionType::Fixed) - .await; + // Now, let's try a "LocalOnly" Silo with its own admin user. + let silo = create_silo( + &client, + "fixed", + true, + shared::SiloIdentityMode::LocalOnly, + ) + .await; let new_silo_user_id = "5b3564b6-8770-4a30-b538-8ef6ae3efa3b".parse().unwrap(); let _ = nexus @@ -1538,7 +1540,7 @@ async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { .unwrap(); grant_iam( client, - "/silos/fixed", + "/system/silos/fixed", SiloRole::Admin, new_silo_user_id, AuthnMode::PrivilegedUser, @@ -1551,7 +1553,7 @@ async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { client, StatusCode::BAD_REQUEST, Method::POST, - "/silos/fixed/saml-identity-providers", + "/system/silos/fixed/identity-providers/saml", ¶ms::SamlIdentityProviderCreate { identity: IdentityMetadataCreateParams { name: "some-totally-real-saml-provider" @@ -1594,7 +1596,7 @@ async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { client, StatusCode::NOT_FOUND, Method::GET, - "/login/fixed/foo", + "/login/fixed/saml/foo", ) .execute() .await @@ -1603,7 +1605,7 @@ async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { client, StatusCode::NOT_FOUND, Method::POST, - "/login/fixed/foo", + "/login/fixed/saml/foo", ) .execute() .await diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index c41df1c661..75c069a1c5 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -99,6 +99,7 @@ API operations found with tag "silos" OPERATION ID URL PATH policy_update /policy policy_view /policy +user_create /users user_list /users API operations found with tag "snapshots" diff --git a/openapi/nexus.json b/openapi/nexus.json index b91b2fd282..d733cdccaf 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -7178,6 +7178,42 @@ } }, "x-dropshot-pagination": true + }, + "post": { + "tags": [ + "silos" + ], + "summary": "Create a user", + "description": "Users can only be created in Silos with `provision_type` == `Fixed`. Otherwise, Silo users are just-in-time (JIT) provisioned when a user first logs in using an external Identity Provider.", + "operationId": "user_create", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/UserCreate" + } + } + }, + "required": true + }, + "responses": { + "201": { + "description": "successful creation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/User" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } } } }, @@ -11168,6 +11204,30 @@ "items" ] }, + "UserCreate": { + "description": "Create-time parameters for a [`User`](crate::external_api::views::User)", + "type": "object", + "properties": { + "external_id": { + "description": "username used to log in", + "allOf": [ + { + "$ref": "#/components/schemas/UserId" + } + ] + } + }, + "required": [ + "external_id" + ] + }, + "UserId": { + "title": "A name unique within the parent collection", + "description": "Names must begin with a lower case ASCII letter, be composed exclusively of lowercase ASCII, uppercase ASCII, numbers, and '-', and may not end with a '-'. Names cannot be a UUID though they may contain a UUID.", + "type": "string", + "pattern": "^(?![0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$)^[a-z][a-z0-9-]*[a-zA-Z0-9]$", + "maxLength": 63 + }, "UserResultsPage": { "description": "A single page of results", "type": "object", From ca321fd1ad9bd74df59d13a849f790750fdce239 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 4 Oct 2022 21:15:01 -0700 Subject: [PATCH 03/15] local user CRUD: tests pass --- nexus/src/app/iam.rs | 3 +- nexus/src/app/silo.rs | 6 ++ nexus/src/authz/api_resources.rs | 57 +++++++++++++++++++ nexus/src/authz/omicron.polar | 37 ++++++++++-- nexus/src/authz/oso_generic.rs | 1 + .../src/authz/policy_test/resource_builder.rs | 17 ++++++ nexus/src/authz/policy_test/resources.rs | 1 + nexus/src/db/datastore/silo_user.rs | 8 ++- nexus/tests/output/authz-roles.out | 34 +++++++++++ 9 files changed, 155 insertions(+), 9 deletions(-) diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index 71cc1692d5..39a1cd5df6 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -67,8 +67,9 @@ impl super::Nexus { .authn .silo_required() .internal_context("listing current silo's users")?; + let authz_silo_user_list = authz::SiloUserList::new(authz_silo.clone()); self.db_datastore - .silo_users_list_by_id(opctx, &authz_silo, pagparams) + .silo_users_list_by_id(opctx, &authz_silo_user_list, pagparams) .await } diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 63a7537614..25f5e3e279 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -164,6 +164,12 @@ impl super::Nexus { Uuid::new_v4(), new_user_params.external_id.as_ref().to_owned(), ); + let authz_silo_user_list = authz::SiloUserList::new(authz_silo.clone()); + // + // TODO-cleanup This authz check belongs in silo_user_create(). + opctx + .authorize(authz::Action::CreateChild, &authz_silo_user_list) + .await?; let (_, db_silo_user) = datastore.silo_user_create(&authz_silo, silo_user).await?; Ok(db_silo_user) diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index ff8f166871..34821072ad 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -559,6 +559,63 @@ impl AuthorizedResource for SiloIdentityProviderList { } } +/// Synthetic resource describing the list of Users in a Silo +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct SiloUserList(Silo); + +impl SiloUserList { + pub fn new(silo: Silo) -> SiloUserList { + SiloUserList(silo) + } + + pub fn silo(&self) -> &Silo { + &self.0 + } +} + +impl oso::PolarClass for SiloUserList { + fn get_polar_class_builder() -> oso::ClassBuilder { + oso::Class::builder() + .with_equality_check() + .add_attribute_getter("silo", |list: &SiloUserList| list.0.clone()) + } +} + +impl AuthorizedResource for SiloUserList { + fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>( + &'a self, + opctx: &'b OpContext, + datastore: &'c DataStore, + authn: &'d authn::Context, + roleset: &'e mut RoleSet, + ) -> futures::future::BoxFuture<'f, Result<(), Error>> + where + 'a: 'f, + 'b: 'f, + 'c: 'f, + 'd: 'f, + 'e: 'f, + { + // There are no roles on this resource, but we still need to load the + // Silo-related roles. + self.silo().load_roles(opctx, datastore, authn, roleset) + } + + fn on_unauthorized( + &self, + _: &Authz, + error: Error, + _: AnyActor, + _: Action, + ) -> Error { + error + } + + fn polar_class(&self) -> oso::Class { + Self::get_polar_class() + } +} + // Main resource hierarchy: Organizations, Projects, and their resources authz_resource! { diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index d01387e9cb..56a1dd76a3 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -413,11 +413,12 @@ resource SiloIdentityProviderList { "list_children" if "read" on "parent_silo"; # Fleet and Silo administrators can manage the Silo's identity provider - # configuration. This is the only area of Silo configuration that Fleet - # Administrators have permissions on. This is also the only case (so - # far) where we need to look two levels up the hierarchy to see if - # somebody has the right permission. For most other things, permissions - # cascade down the hierarchy so we only need to look at the parent. + # configuration. This is one of the only areas of Silo configuration + # that Fleet Administrators have permissions on. This is also one of + # the only cases where we need to look two levels up the hierarchy to + # see if somebody has the right permission. For most other things, + # permissions cascade down the hierarchy so we only need to look at the + # parent. "create_child" if "admin" on "parent_silo"; "create_child" if "admin" on "parent_fleet"; } @@ -426,6 +427,32 @@ has_relation(silo: Silo, "parent_silo", collection: SiloIdentityProviderList) has_relation(fleet: Fleet, "parent_fleet", collection: SiloIdentityProviderList) if collection.silo.fleet = fleet; +# Describes the policy for creating and managing Silo users (mostly intended for +# API-managed users) +resource SiloUserList { + permissions = [ "list_children", "create_child" ]; + + relations = { parent_silo: Silo, parent_fleet: Fleet }; + + # Everyone who can read the Silo (which includes all the users in the + # Silo) can see the users in it. + "list_children" if "read" on "parent_silo"; + + # Fleet and Silo administrators can manage the Silo's users. This is + # one of the only areas of Silo configuration that Fleet Administrators + # have permissions on. This is also one of the few cases (so far) where + # we need to look two levels up the hierarchy to see if somebody has the + # right permission. For most other things, permissions cascade down the + # hierarchy so we only need to look at the parent. + "create_child" if "admin" on "parent_silo"; + "list_children" if "admin" on "parent_fleet"; + "create_child" if "admin" on "parent_fleet"; +} +has_relation(silo: Silo, "parent_silo", collection: SiloUserList) + if collection.silo = silo; +has_relation(fleet: Fleet, "parent_fleet", collection: SiloUserList) + if collection.silo.fleet = fleet; + # These rules grants the external authenticator role the permissions it needs to # read silo users and modify their sessions. This is necessary for login to # work. diff --git a/nexus/src/authz/oso_generic.rs b/nexus/src/authz/oso_generic.rs index 185daa70b0..7cd602546c 100644 --- a/nexus/src/authz/oso_generic.rs +++ b/nexus/src/authz/oso_generic.rs @@ -110,6 +110,7 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result { ConsoleSessionList::get_polar_class(), DeviceAuthRequestList::get_polar_class(), SiloIdentityProviderList::get_polar_class(), + SiloUserList::get_polar_class(), ]; for c in classes { oso_builder = oso_builder.register_class(c)?; diff --git a/nexus/src/authz/policy_test/resource_builder.rs b/nexus/src/authz/policy_test/resource_builder.rs index 815883793e..5607f82ddd 100644 --- a/nexus/src/authz/policy_test/resource_builder.rs +++ b/nexus/src/authz/policy_test/resource_builder.rs @@ -263,3 +263,20 @@ impl DynAuthorizedResource for authz::SiloIdentityProviderList { format!("{}: identity provider list", self.silo().resource_name()) } } + +impl DynAuthorizedResource for authz::SiloUserList { + fn do_authorize<'a, 'b>( + &'a self, + opctx: &'b OpContext, + action: authz::Action, + ) -> BoxFuture<'a, Result<(), Error>> + where + 'b: 'a, + { + opctx.authorize(action, self).boxed() + } + + fn resource_name(&self) -> String { + format!("{}: user list", self.silo().resource_name()) + } +} diff --git a/nexus/src/authz/policy_test/resources.rs b/nexus/src/authz/policy_test/resources.rs index 56471d85ae..103e0bcc52 100644 --- a/nexus/src/authz/policy_test/resources.rs +++ b/nexus/src/authz/policy_test/resources.rs @@ -110,6 +110,7 @@ async fn make_silo( } builder.new_resource(authz::SiloIdentityProviderList::new(silo.clone())); + builder.new_resource(authz::SiloUserList::new(silo.clone())); let norganizations = if first_branch { 2 } else { 1 }; for i in 0..norganizations { diff --git a/nexus/src/db/datastore/silo_user.rs b/nexus/src/db/datastore/silo_user.rs index 393a7087fb..aa2cf50deb 100644 --- a/nexus/src/db/datastore/silo_user.rs +++ b/nexus/src/db/datastore/silo_user.rs @@ -103,14 +103,16 @@ impl DataStore { pub async fn silo_users_list_by_id( &self, opctx: &OpContext, - authz_silo: &authz::Silo, + authz_silo_user_list: &authz::SiloUserList, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { use db::schema::silo_user::dsl; - opctx.authorize(authz::Action::Read, authz_silo).await?; + opctx + .authorize(authz::Action::ListChildren, authz_silo_user_list) + .await?; paginated(dsl::silo_user, dsl::id, pagparams) - .filter(dsl::silo_id.eq(authz_silo.id())) + .filter(dsl::silo_id.eq(authz_silo_user_list.silo().id())) .filter(dsl::time_deleted.is_null()) .select(SiloUser::as_select()) .load_async::(self.pool_authorized(opctx).await?) diff --git a/nexus/tests/output/authz-roles.out b/nexus/tests/output/authz-roles.out index 37f4d4ec26..644271cd0e 100644 --- a/nexus/tests/output/authz-roles.out +++ b/nexus/tests/output/authz-roles.out @@ -134,6 +134,23 @@ resource: Silo "silo1": identity provider list silo1-org1-proj1-viewer ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ unauthenticated ! ! ! ! ! ! ! ! +resource: Silo "silo1": user list + + USER Q R LC RP M MP CC D + fleet-admin ✘ ✘ ✔ ✘ ✘ ✘ ✔ ✘ + fleet-collaborator ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + fleet-viewer ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-admin ✘ ✘ ✔ ✘ ✘ ✘ ✔ ✘ + silo1-collaborator ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-viewer ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-org1-admin ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-org1-collaborator ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-org1-viewer ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-org1-proj1-admin ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-org1-proj1-collaborator ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-org1-proj1-viewer ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + unauthenticated ! ! ! ! ! ! ! ! + resource: Organization "silo1-org1" USER Q R LC RP M MP CC D @@ -559,6 +576,23 @@ resource: Silo "silo2": identity provider list silo1-org1-proj1-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ unauthenticated ! ! ! ! ! ! ! ! +resource: Silo "silo2": user list + + USER Q R LC RP M MP CC D + fleet-admin ✘ ✘ ✔ ✘ ✘ ✘ ✔ ✘ + fleet-collaborator ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + fleet-viewer ✘ ✘ ✔ ✘ ✘ ✘ ✘ ✘ + silo1-admin ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo1-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo1-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo1-org1-admin ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo1-org1-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo1-org1-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo1-org1-proj1-admin ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo1-org1-proj1-collaborator ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + silo1-org1-proj1-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ + unauthenticated ! ! ! ! ! ! ! ! + resource: Organization "silo2-org1" USER Q R LC RP M MP CC D From 377307df465ee0745bddfedad729011a9bd0a637 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 5 Oct 2022 14:45:48 -0700 Subject: [PATCH 04/15] move endpoints around, add "get" and "delete" --- nexus/src/app/iam.rs | 29 +++- nexus/src/app/silo.rs | 52 ++++++- nexus/src/db/datastore/silo_user.rs | 45 ++++++ nexus/src/external_api/console_api.rs | 8 +- nexus/src/external_api/http_entrypoints.rs | 162 +++++++++++++++++---- nexus/tests/integration_tests/silos.rs | 5 +- 6 files changed, 252 insertions(+), 49 deletions(-) diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index 39a1cd5df6..968a0a07f1 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -58,7 +58,8 @@ impl super::Nexus { // Silo users - pub async fn silo_users_list( + /// List users in the current Silo + pub async fn silo_users_list_current( &self, opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, @@ -73,18 +74,38 @@ impl super::Nexus { .await } - pub async fn silo_user_fetch_by_id( + /// Fetch the currently-authenticated Silo user + pub async fn silo_user_fetch_self( &self, opctx: &OpContext, - silo_user_id: &Uuid, ) -> LookupResult { + let &actor = opctx + .authn + .actor_required() + .internal_context("loading current user")?; let (.., db_silo_user) = LookupPath::new(opctx, &self.db_datastore) - .silo_user_id(*silo_user_id) + .silo_user_id(actor.actor_id()) .fetch() .await?; Ok(db_silo_user) } + pub async fn silo_users_list( + &self, + opctx: &OpContext, + silo_name: &Name, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + let (authz_silo, ..) = LookupPath::new(opctx, &self.db_datastore) + .silo_name(silo_name) + .fetch() + .await?; + let authz_silo_user_list = authz::SiloUserList::new(authz_silo); + self.db_datastore + .silo_users_list_by_id(opctx, &authz_silo_user_list, pagparams) + .await + } + // Built-in users pub async fn users_builtin_list( diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 25f5e3e279..0b70a2ca02 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -4,6 +4,7 @@ //! Silos, Users, and SSH Keys. +use crate::authz::ApiResource; use crate::context::OpContext; use crate::db; use crate::db::identity::{Asset, Resource}; @@ -141,15 +142,15 @@ impl super::Nexus { // Users - pub async fn silo_fixed_user_create( + pub async fn silo_api_user_create( &self, opctx: &OpContext, + silo_name: &Name, new_user_params: params::UserCreate, ) -> CreateResult { let datastore = self.datastore(); - let authz_silo = opctx.authn.silo_required()?; - let (.., db_silo) = LookupPath::new(opctx, &datastore) - .silo_id(authz_silo.id()) + let (authz_silo, db_silo) = LookupPath::new(opctx, &datastore) + .silo_name(silo_name) .fetch() .await?; @@ -165,7 +166,7 @@ impl super::Nexus { new_user_params.external_id.as_ref().to_owned(), ); let authz_silo_user_list = authz::SiloUserList::new(authz_silo.clone()); - // + // TODO-cleanup This authz check belongs in silo_user_create(). opctx .authorize(authz::Action::CreateChild, &authz_silo_user_list) @@ -175,16 +176,51 @@ impl super::Nexus { Ok(db_silo_user) } + pub async fn silo_api_user_delete( + &self, + opctx: &OpContext, + silo_name: &Name, + silo_user_id: Uuid, + ) -> DeleteResult { + // Verify that this user is actually in this Silo. + let datastore = self.datastore(); + let (authz_silo, _) = LookupPath::new(opctx, datastore) + .silo_name(silo_name) + .fetch() + .await?; + let (_, authz_silo_user, db_silo_user) = + LookupPath::new(opctx, datastore) + .silo_user_id(silo_user_id) + .fetch_for(authz::Action::Delete) + .await?; + if db_silo_user.silo_id != authz_silo.id() { + return Err(authz_silo_user.not_found()); + } + + self.db_datastore.silo_user_delete(opctx, &authz_silo_user).await + } + pub async fn silo_user_fetch( &self, opctx: &OpContext, + silo_name: &Name, silo_user_id: Uuid, ) -> LookupResult { - let (.., db_silo_user) = LookupPath::new(opctx, &self.datastore()) - .silo_user_id(silo_user_id) + let datastore = self.datastore(); + let (authz_silo_requested, _) = LookupPath::new(opctx, datastore) + .silo_name(silo_name) .fetch() .await?; - Ok(db_silo_user) + let (_, authz_silo_user, db_silo_user) = + LookupPath::new(opctx, datastore) + .silo_user_id(silo_user_id) + .fetch() + .await?; + if db_silo_user.silo_id != authz_silo_requested.id() { + Err(authz_silo_user.not_found()) + } else { + Ok(db_silo_user) + } } /// Based on an authenticated subject, fetch or create a silo user diff --git a/nexus/src/db/datastore/silo_user.rs b/nexus/src/db/datastore/silo_user.rs index aa2cf50deb..8816be54dc 100644 --- a/nexus/src/db/datastore/silo_user.rs +++ b/nexus/src/db/datastore/silo_user.rs @@ -18,11 +18,14 @@ use crate::db::model::SiloUser; use crate::db::model::UserBuiltin; use crate::db::pagination::paginated; use crate::external_api::params; +use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; +use chrono::Utc; use diesel::prelude::*; use nexus_types::identity::Asset; 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::ListResultVec; use omicron_common::api::external::ResourceType; @@ -64,6 +67,48 @@ impl DataStore { }) } + /// Delete a Silo User + pub async fn silo_user_delete( + &self, + opctx: &OpContext, + authz_silo_user: &authz::SiloUser, + ) -> DeleteResult { + opctx.authorize(authz::Action::Delete, authz_silo_user).await?; + + self.pool_authorized(opctx) + .await? + .transaction_async(|mut conn| async move { + // Users in API-managed Silos do not currently support groups. + + // Delete ssh keys. + { + // TODO-scalability This should be done in batches. + use db::schema::ssh_key::dsl; + diesel::update(dsl::ssh_key) + .filter(dsl::silo_user_id.eq(authz_silo_user.id())) + .filter(dsl::time_deleted.is_null()) + .set(dsl::time_deleted.eq(Utc::now())) + .execute_async(&mut conn) + .await?; + } + + // Delete the user record. + // XXX-dap should use check_if_exists/execute_and_check + use db::schema::silo_user::dsl; + diesel::update(dsl::silo_user) + .filter(dsl::id.eq(authz_silo_user.id())) + .filter(dsl::time_deleted.is_null()) + .set(dsl::time_deleted.eq(Utc::now())) + .execute_async(&mut conn) + //.check_if_exists::(authz_silo_user.id()) + //.execute_and_check(&mut conn) + .await?; + Ok(()) + }) + .await + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + } + /// Given an external ID, return /// - Ok(Some((authz::SiloUser, SiloUser))) if that external id refers to an /// existing silo user diff --git a/nexus/src/external_api/console_api.rs b/nexus/src/external_api/console_api.rs index ada514bcb6..7eaa10a49d 100644 --- a/nexus/src/external_api/console_api.rs +++ b/nexus/src/external_api/console_api.rs @@ -35,7 +35,6 @@ use hyper::Body; use lazy_static::lazy_static; use mime_guess; use omicron_common::api::external::Error; -use omicron_common::api::external::InternalContext; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use serde_urlencoded; @@ -600,12 +599,7 @@ pub async fn session_me( // 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 &actor = opctx - .authn - .actor_required() - .internal_context("loading current user")?; - let user = - nexus.silo_user_fetch_by_id(&opctx, &actor.actor_id()).await?; + let user = nexus.silo_user_fetch_self(&opctx).await?; Ok(HttpResponseOk(user.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index aa5754f62a..372eb2a805 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -235,6 +235,11 @@ pub fn external_api() -> NexusApiDescription { api.register(saml_identity_provider_create)?; api.register(saml_identity_provider_view)?; + api.register(local_idp_users_list)?; + api.register(local_idp_user_create)?; + api.register(local_idp_user_view)?; + api.register(local_idp_user_delete)?; + api.register(system_image_list)?; api.register(system_image_create)?; api.register(system_image_view)?; @@ -244,7 +249,6 @@ pub fn external_api() -> NexusApiDescription { api.register(updates_refresh)?; api.register(user_list)?; - api.register(user_create)?; // Console API operations api.register(console_api::login_begin)?; @@ -733,6 +737,134 @@ async fn saml_identity_provider_view( // TODO: no DELETE for identity providers? +// "Local" Identity Provider + +/// List users in the given local identity provider +#[endpoint { + method = GET, + path = "/system/silos/{silo_name}/identity-providers/local/users/all", + tags = ["system"], +}] +async fn local_idp_users_list( + rqctx: Arc>>, + path_params: Path, + query_params: Query, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let silo_name = path_params.into_inner().silo_name; + let query = query_params.into_inner(); + let pagparams = data_page_params_for(&rqctx, &query)?; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let users = nexus + .silo_users_list(&opctx, &silo_name, &pagparams) + .await? + .into_iter() + .map(|i| i.into()) + .collect(); + Ok(HttpResponseOk(ScanById::results_page( + &query, + users, + &|_, user: &User| user.id, + )?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Create a user +/// +/// Users can only be created in Silos with `provision_type` == `Fixed`. +/// Otherwise, Silo users are just-in-time (JIT) provisioned when a user first +/// logs in using an external Identity Provider. +#[endpoint { + method = POST, + path = "/system/silos/{silo_name}/identity-providers/local/users/create", + tags = ["system"], +}] +async fn local_idp_user_create( + rqctx: Arc>>, + path_params: Path, + new_user_params: TypedBody, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let silo_name = path_params.into_inner().silo_name; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let user = nexus + .silo_api_user_create( + &opctx, + &silo_name, + new_user_params.into_inner(), + ) + .await?; + Ok(HttpResponseCreated(user.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Path parameters for Silo User requests +#[derive(Deserialize, JsonSchema)] +struct UserPathParam { + /// The silo's unique name. + silo_name: Name, + /// The user's internal id + user_id: Uuid, +} + +#[endpoint { + method = GET, + path = "/system/silos/{silo_name}/identity-providers/local/users/id/{user_id}", + tags = ["system"], +}] +async fn local_idp_user_view( + rqctx: Arc>>, + path_params: Path, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path_params = path_params.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let user = nexus + .silo_user_fetch( + &opctx, + &path_params.silo_name, + path_params.user_id, + ) + .await?; + Ok(HttpResponseOk(user.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +#[endpoint { + method = DELETE, + path = "/system/silos/{silo_name}/identity-providers/local/users/id/{user_id}", + tags = ["system"], +}] +async fn local_idp_user_delete( + rqctx: Arc>>, + path_params: Path, +) -> Result { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path_params = path_params.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + nexus + .silo_api_user_delete( + &opctx, + &path_params.silo_name, + path_params.user_id, + ) + .await?; + Ok(HttpResponseDeleted()) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + /// List organizations #[endpoint { method = GET, @@ -4100,7 +4232,7 @@ async fn user_list( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let users = nexus - .silo_users_list(&opctx, &pagparams) + .silo_users_list_current(&opctx, &pagparams) .await? .into_iter() .map(|i| i.into()) @@ -4114,32 +4246,6 @@ async fn user_list( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/// Create a user -/// -/// Users can only be created in Silos with `provision_type` == `Fixed`. -/// Otherwise, Silo users are just-in-time (JIT) provisioned when a user first -/// logs in using an external Identity Provider. -#[endpoint { - method = POST, - path = "/users", - tags = ["silos"], -}] -async fn user_create( - rqctx: Arc>>, - new_user_params: TypedBody, -) -> Result, HttpError> { - let apictx = rqctx.context(); - let nexus = &apictx.nexus; - let handler = async { - let opctx = OpContext::for_external_api(&rqctx).await?; - let user = nexus - .silo_fixed_user_create(&opctx, new_user_params.into_inner()) - .await?; - Ok(HttpResponseCreated(user.into())) - }; - apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await -} - // Built-in (system) users /// List built-in users diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index ef8f4afe82..29eb84af23 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -207,8 +207,9 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { .expect("failed to make request"); // Verify silo user was also deleted - nexus - .silo_user_fetch(authn_opctx, new_silo_user_id) + LookupPath::new(&authn_opctx, nexus.datastore()) + .silo_user_id(new_silo_user_id) + .fetch() .await .expect_err("unexpected success"); } From bace52185d66f6f8c1989638358187edface2823 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 5 Oct 2022 16:16:13 -0700 Subject: [PATCH 05/15] fix up tests again --- nexus/src/app/silo.rs | 8 +- nexus/src/authz/omicron.polar | 3 + nexus/src/db/datastore/silo_user.rs | 32 ++- nexus/test-utils/src/resource_helpers.rs | 14 - nexus/tests/integration_tests/endpoints.rs | 43 +++ nexus/tests/integration_tests/silos.rs | 11 +- nexus/tests/integration_tests/unauthorized.rs | 20 +- nexus/tests/output/nexus_tags.txt | 5 +- openapi/nexus.json | 245 +++++++++++++++--- 9 files changed, 322 insertions(+), 59 deletions(-) diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 0b70a2ca02..d803312052 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -184,7 +184,7 @@ impl super::Nexus { ) -> DeleteResult { // Verify that this user is actually in this Silo. let datastore = self.datastore(); - let (authz_silo, _) = LookupPath::new(opctx, datastore) + let (authz_silo, db_silo) = LookupPath::new(opctx, datastore) .silo_name(silo_name) .fetch() .await?; @@ -197,6 +197,12 @@ impl super::Nexus { return Err(authz_silo_user.not_found()); } + if db_silo.user_provision_type != UserProvisionType::ApiOnly { + return Err(Error::invalid_request( + "cannot delete users in this kind of Silo", + )); + } + self.db_datastore.silo_user_delete(opctx, &authz_silo_user).await } diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 56a1dd76a3..65b7df4421 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -252,6 +252,9 @@ has_relation(silo: Silo, "parent_silo", user: SiloUser) has_permission(actor: AuthenticatedActor, _perm: String, silo_user: SiloUser) if actor.equals_silo_user(silo_user); +has_permission(actor: AuthenticatedActor, "read", silo_user: SiloUser) + if silo_user.silo in actor.silo; + resource SiloGroup { permissions = [ "list_children", diff --git a/nexus/src/db/datastore/silo_user.rs b/nexus/src/db/datastore/silo_user.rs index 8816be54dc..ccacece613 100644 --- a/nexus/src/db/datastore/silo_user.rs +++ b/nexus/src/db/datastore/silo_user.rs @@ -75,14 +75,42 @@ impl DataStore { ) -> DeleteResult { opctx.authorize(authz::Action::Delete, authz_silo_user).await?; + // Delete the user and everything associated with them. + // TODO-scalability Many of these should be done in batches. + // TODO-robustness We might consider the RFD 192 "rcgen" pattern as well + // so that people can't, say, login while we do this. self.pool_authorized(opctx) .await? .transaction_async(|mut conn| async move { - // Users in API-managed Silos do not currently support groups. + // Delete console sessions. + { + use db::schema::console_session::dsl; + diesel::delete(dsl::console_session) + .filter(dsl::silo_user_id.eq(authz_silo_user.id())) + .execute_async(&mut conn) + .await?; + } + + // Delete device authentication tokens. + { + use db::schema::device_access_token::dsl; + diesel::delete(dsl::device_access_token) + .filter(dsl::silo_user_id.eq(authz_silo_user.id())) + .execute_async(&mut conn) + .await?; + } + + // Delete group memberships. + { + use db::schema::silo_group_membership::dsl; + diesel::delete(dsl::silo_group_membership) + .filter(dsl::silo_user_id.eq(authz_silo_user.id())) + .execute_async(&mut conn) + .await?; + } // Delete ssh keys. { - // TODO-scalability This should be done in batches. use db::schema::ssh_key::dsl; diesel::update(dsl::ssh_key) .filter(dsl::silo_user_id.eq(authz_silo_user.id())) diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 80fc4af0aa..3ead59bff4 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -18,13 +18,11 @@ use omicron_common::api::external::Instance; use omicron_common::api::external::InstanceCpuCount; use omicron_nexus::crucible_agent_client::types::State as RegionState; use omicron_nexus::external_api::params; -use omicron_nexus::external_api::params::UserId; use omicron_nexus::external_api::shared; use omicron_nexus::external_api::shared::IdentityType; use omicron_nexus::external_api::shared::IpRange; use omicron_nexus::external_api::views::IpPool; use omicron_nexus::external_api::views::IpPoolRange; -use omicron_nexus::external_api::views::User; use omicron_nexus::external_api::views::{ Organization, Project, Silo, Vpc, VpcRouter, }; @@ -128,18 +126,6 @@ pub async fn create_silo( .await } -pub async fn create_local_user( - client: &ClientTestContext, - username: &UserId, -) -> User { - object_create( - client, - "/users", - ¶ms::UserCreate { external_id: username.to_owned() }, - ) - .await -} - pub async fn create_organization( client: &ClientTestContext, organization_name: &str, diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index e6b55061f8..436e5ecd16 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -27,6 +27,8 @@ use omicron_common::api::external::RouterRouteUpdateParams; use omicron_common::api::external::VpcFirewallRuleUpdateParams; use omicron_nexus::authn; use omicron_nexus::authz; +use omicron_nexus::db::fixed_data::silo::DEFAULT_SILO; +use omicron_nexus::db::identity::Resource; use omicron_nexus::external_api::params; use omicron_nexus::external_api::shared; use omicron_nexus::external_api::shared::IpRange; @@ -60,6 +62,22 @@ lazy_static! { identity_mode: shared::SiloIdentityMode::SamlJit, admin_group_name: None, }; + // Use the default Silo for testing the local IdP + pub static ref DEMO_SILO_USERS_CREATE_URL: String = + format!( + "/system/silos/{}/identity-providers/local/users/create", + DEFAULT_SILO.identity().name, + ); + pub static ref DEMO_SILO_USERS_LIST_URL: String = + format!( + "/system/silos/{}/identity-providers/local/users/all", + DEFAULT_SILO.identity().name, + ); + pub static ref DEMO_SILO_USER_ID_URL: String = format!( + "/system/silos/{}/identity-providers/local/users/id/{{id}}", + DEFAULT_SILO.identity().name, + ); + // Organization used for testing pub static ref DEMO_ORG_NAME: Name = "demo-org".parse().unwrap(); @@ -720,6 +738,21 @@ lazy_static! { unprivileged_access: UnprivilegedAccess::ReadOnly, allowed_methods: vec![ AllowedMethod::Get, + ], + }, + + VerifyEndpoint { + url: &*DEMO_SILO_USERS_LIST_URL, + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::ReadOnly, + allowed_methods: vec![ AllowedMethod::Get ], + }, + + VerifyEndpoint { + url: &*DEMO_SILO_USERS_CREATE_URL, + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::ReadOnly, + allowed_methods: vec![ AllowedMethod::Post( serde_json::to_value( &*DEMO_USER_CREATE @@ -728,6 +761,16 @@ lazy_static! { ], }, + VerifyEndpoint { + url: &*DEMO_SILO_USER_ID_URL, + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::ReadOnly, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Delete, + ], + }, + /* Organizations */ VerifyEndpoint { diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index 29eb84af23..cb127cde50 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -1502,7 +1502,7 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { client, StatusCode::BAD_REQUEST, Method::POST, - "/users", + "/system/silos/jit/identity-providers/local/users/create", ¶ms::UserCreate { external_id: params::UserId::from_str("dummy").unwrap(), }, @@ -1517,7 +1517,7 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { } #[nexus_test] -async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { +async fn test_local_silo_constraints(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; @@ -1548,7 +1548,7 @@ async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { ) .await; - // It's not allowed to create an identity provider in a fixed Silo. + // It's not allowed to create an identity provider in an ApiOnly Silo. let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure_with_body( client, @@ -1612,3 +1612,8 @@ async fn test_fixed_silo_constraints(cptestctx: &ControlPlaneTestContext) { .await .unwrap(); } + +// XXX-dap TODO-coverage +// - attempt to create, delete user using API in SamlJit Silo ("create" may be +// covered above) +// - successful user create/fetch/list/delete diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index c807efad2e..68f4995439 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -21,7 +21,6 @@ use nexus_test_utils::http_testing::TestResponse; use nexus_test_utils::resource_helpers::DiskTest; use nexus_test_utils::ControlPlaneTestContext; use nexus_test_utils_macros::nexus_test; -use omicron_common::api::external::IdentityMetadata; use omicron_nexus::authn::external::spoof; // This test hits a list Nexus API endpoints using both unauthenticated and @@ -189,6 +188,14 @@ lazy_static! { body: serde_json::to_value(&*DEMO_SILO_CREATE).unwrap(), id_routes: vec!["/system/by-id/silos/{id}"], }, + // Create a local User + SetupReq::Post { + url: &*DEMO_SILO_USERS_CREATE_URL, + body: serde_json::to_value(&*DEMO_USER_CREATE).unwrap(), + id_routes: vec![ + &*DEMO_SILO_USER_ID_URL, + ], + }, // Create an IP pool SetupReq::Post { url: &*DEMO_IP_POOLS_URL, @@ -281,6 +288,15 @@ lazy_static! { ]; } +/// Contents returned from an endpoint that creates a resource that has an id +/// +/// This is a subset of `IdentityMetadata`. `IdentityMetadata` includes other +/// fields (like "name") that are not present on all objects. +#[derive(serde::Deserialize)] +struct IdMetadata { + id: String, +} + /// Verifies a single API endpoint, described with `endpoint` /// /// (Technically, a single `VerifyEndpoint` struct describes an HTTP resource, @@ -358,7 +374,7 @@ async fn verify_endpoint( Some(response) => endpoint.url.replace( "{id}", response - .parsed_body::() + .parsed_body::() .unwrap() .id .to_string() diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 75c069a1c5..04328099ee 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -99,7 +99,6 @@ API operations found with tag "silos" OPERATION ID URL PATH policy_update /policy policy_view /policy -user_create /users user_list /users API operations found with tag "snapshots" @@ -125,6 +124,10 @@ ip_pool_service_view /system/ip-pools-service/{rack_id} ip_pool_update /system/ip-pools/{pool_name} ip_pool_view /system/ip-pools/{pool_name} ip_pool_view_by_id /system/by-id/ip-pools/{id} +local_idp_user_create /system/silos/{silo_name}/identity-providers/local/users/create +local_idp_user_delete /system/silos/{silo_name}/identity-providers/local/users/id/{user_id} +local_idp_user_view /system/silos/{silo_name}/identity-providers/local/users/id/{user_id} +local_idp_users_list /system/silos/{silo_name}/identity-providers/local/users/all rack_list /system/hardware/racks rack_view /system/hardware/racks/{rack_id} saga_list /system/sagas diff --git a/openapi/nexus.json b/openapi/nexus.json index d733cdccaf..34e0abc95c 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -6765,6 +6765,215 @@ "x-dropshot-pagination": true } }, + "/system/silos/{silo_name}/identity-providers/local/users/all": { + "get": { + "tags": [ + "system" + ], + "summary": "List users in the given local identity provider", + "operationId": "local_idp_users_list", + "parameters": [ + { + "in": "path", + "name": "silo_name", + "description": "The silo's unique name.", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "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/UserResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": true + } + }, + "/system/silos/{silo_name}/identity-providers/local/users/create": { + "post": { + "tags": [ + "system" + ], + "summary": "Create a user", + "description": "Users can only be created in Silos with `provision_type` == `Fixed`. Otherwise, Silo users are just-in-time (JIT) provisioned when a user first logs in using an external Identity Provider.", + "operationId": "local_idp_user_create", + "parameters": [ + { + "in": "path", + "name": "silo_name", + "description": "The silo's unique name.", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/UserCreate" + } + } + }, + "required": true + }, + "responses": { + "201": { + "description": "successful creation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/User" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, + "/system/silos/{silo_name}/identity-providers/local/users/id/{user_id}": { + "get": { + "tags": [ + "system" + ], + "operationId": "local_idp_user_view", + "parameters": [ + { + "in": "path", + "name": "silo_name", + "description": "The silo's unique name.", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "user_id", + "description": "The user's internal id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + }, + "style": "simple" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/User" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + }, + "delete": { + "tags": [ + "system" + ], + "operationId": "local_idp_user_delete", + "parameters": [ + { + "in": "path", + "name": "silo_name", + "description": "The silo's unique name.", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "user_id", + "description": "The user's internal id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + }, + "style": "simple" + } + ], + "responses": { + "204": { + "description": "successful deletion" + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/system/silos/{silo_name}/identity-providers/saml": { "post": { "tags": [ @@ -7178,42 +7387,6 @@ } }, "x-dropshot-pagination": true - }, - "post": { - "tags": [ - "silos" - ], - "summary": "Create a user", - "description": "Users can only be created in Silos with `provision_type` == `Fixed`. Otherwise, Silo users are just-in-time (JIT) provisioned when a user first logs in using an external Identity Provider.", - "operationId": "user_create", - "requestBody": { - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/UserCreate" - } - } - }, - "required": true - }, - "responses": { - "201": { - "description": "successful creation", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/User" - } - } - } - }, - "4XX": { - "$ref": "#/components/responses/Error" - }, - "5XX": { - "$ref": "#/components/responses/Error" - } - } } } }, From f0bd3fb341e6ed16793c3afe98b150a5de021a3e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 5 Oct 2022 16:47:06 -0700 Subject: [PATCH 06/15] remove one XXX --- nexus/src/db/datastore/silo_user.rs | 54 +++++++++++++++++++------- nexus/tests/integration_tests/silos.rs | 2 + 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/nexus/src/db/datastore/silo_user.rs b/nexus/src/db/datastore/silo_user.rs index ccacece613..1f28548a39 100644 --- a/nexus/src/db/datastore/silo_user.rs +++ b/nexus/src/db/datastore/silo_user.rs @@ -79,14 +79,34 @@ impl DataStore { // TODO-scalability Many of these should be done in batches. // TODO-robustness We might consider the RFD 192 "rcgen" pattern as well // so that people can't, say, login while we do this. + let authz_silo_user_id = authz_silo_user.id(); self.pool_authorized(opctx) .await? .transaction_async(|mut conn| async move { + // Check if the user exists first so that we can bail with an + // appropriate error if not. It would be better to use + // `check_if_exists()`/`execute_and_check()` here, but that + // mechanism does not yet support running in a transaction. + // This approach remains correct at least with CockroachDB + // because it guarantees serializability of transactions. With + // other databases, we might need an explicit "SELECT FOR + // UPDATE" here. Either way, we're essentially locking this row + // for the duration of the transaction, which is not great. + { + use db::schema::silo_user::dsl; + let _ = dsl::silo_user + .filter(dsl::id.eq(authz_silo_user_id)) + .filter(dsl::time_deleted.is_null()) + .select(SiloUser::as_select()) + .get_result_async(&mut conn) + .await?; + } + // Delete console sessions. { use db::schema::console_session::dsl; diesel::delete(dsl::console_session) - .filter(dsl::silo_user_id.eq(authz_silo_user.id())) + .filter(dsl::silo_user_id.eq(authz_silo_user_id)) .execute_async(&mut conn) .await?; } @@ -95,7 +115,7 @@ impl DataStore { { use db::schema::device_access_token::dsl; diesel::delete(dsl::device_access_token) - .filter(dsl::silo_user_id.eq(authz_silo_user.id())) + .filter(dsl::silo_user_id.eq(authz_silo_user_id)) .execute_async(&mut conn) .await?; } @@ -104,7 +124,7 @@ impl DataStore { { use db::schema::silo_group_membership::dsl; diesel::delete(dsl::silo_group_membership) - .filter(dsl::silo_user_id.eq(authz_silo_user.id())) + .filter(dsl::silo_user_id.eq(authz_silo_user_id)) .execute_async(&mut conn) .await?; } @@ -113,7 +133,7 @@ impl DataStore { { use db::schema::ssh_key::dsl; diesel::update(dsl::ssh_key) - .filter(dsl::silo_user_id.eq(authz_silo_user.id())) + .filter(dsl::silo_user_id.eq(authz_silo_user_id)) .filter(dsl::time_deleted.is_null()) .set(dsl::time_deleted.eq(Utc::now())) .execute_async(&mut conn) @@ -121,20 +141,24 @@ impl DataStore { } // Delete the user record. - // XXX-dap should use check_if_exists/execute_and_check - use db::schema::silo_user::dsl; - diesel::update(dsl::silo_user) - .filter(dsl::id.eq(authz_silo_user.id())) - .filter(dsl::time_deleted.is_null()) - .set(dsl::time_deleted.eq(Utc::now())) - .execute_async(&mut conn) - //.check_if_exists::(authz_silo_user.id()) - //.execute_and_check(&mut conn) - .await?; + { + use db::schema::silo_user::dsl; + diesel::update(dsl::silo_user) + .filter(dsl::id.eq(authz_silo_user_id)) + .filter(dsl::time_deleted.is_null()) + .set(dsl::time_deleted.eq(Utc::now())) + .execute_async(&mut conn) + .await?; + } Ok(()) }) .await - .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByResource(authz_silo_user), + ) + }) } /// Given an external ID, return diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index cb127cde50..6e4acbbd2e 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -1617,3 +1617,5 @@ async fn test_local_silo_constraints(cptestctx: &ControlPlaneTestContext) { // - attempt to create, delete user using API in SamlJit Silo ("create" may be // covered above) // - successful user create/fetch/list/delete +// - deleting a user that doesn't exist +// - fetch/delete a user that exists in a different Silo From be021c88ce21af7811c3e7f2d8f6a4cfcfbdbd8c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 6 Oct 2022 10:07:13 -0400 Subject: [PATCH 07/15] execute_and_check now generic --- nexus/src/db/datastore/silo_user.rs | 4 +++- nexus/src/db/update_and_check.rs | 11 +++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/nexus/src/db/datastore/silo_user.rs b/nexus/src/db/datastore/silo_user.rs index 1f28548a39..1a3fb48aab 100644 --- a/nexus/src/db/datastore/silo_user.rs +++ b/nexus/src/db/datastore/silo_user.rs @@ -17,6 +17,7 @@ use crate::db::model::Name; use crate::db::model::SiloUser; use crate::db::model::UserBuiltin; use crate::db::pagination::paginated; +use crate::db::update_and_check::UpdateAndCheck; use crate::external_api::params; use async_bb8_diesel::AsyncConnection; use async_bb8_diesel::AsyncRunQueryDsl; @@ -147,7 +148,8 @@ impl DataStore { .filter(dsl::id.eq(authz_silo_user_id)) .filter(dsl::time_deleted.is_null()) .set(dsl::time_deleted.eq(Utc::now())) - .execute_async(&mut conn) + .check_if_exists::(authz_silo_user_id) + .execute_and_check(&mut conn) .await?; } Ok(()) diff --git a/nexus/src/db/update_and_check.rs b/nexus/src/db/update_and_check.rs index ff339f0ace..8c7845b61b 100644 --- a/nexus/src/db/update_and_check.rs +++ b/nexus/src/db/update_and_check.rs @@ -5,7 +5,7 @@ //! CTE implementation for "UPDATE with extended return status". use super::pool::DbConnection; -use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionManager, PoolError}; +use async_bb8_diesel::{AsyncRunQueryDsl, PoolError}; use diesel::associations::HasTable; use diesel::pg::Pg; use diesel::prelude::*; @@ -153,16 +153,19 @@ where /// - Ok(Row exists and was updated) /// - Ok(Row exists, but was not updated) /// - Error (row doesn't exist, or other diesel error) - pub async fn execute_and_check( + pub async fn execute_and_check( self, - pool: &bb8::Pool>, + conn: &(impl async_bb8_diesel::AsyncConnection + + Sync), ) -> Result, PoolError> where // We require this bound to ensure that "Self" is runnable as query. Self: LoadQuery<'static, DbConnection, (Option, Option, Q)>, + ConnErr: From + Send + 'static, + PoolError: From, { let (id0, id1, found) = - self.get_result_async::<(Option, Option, Q)>(pool).await?; + self.get_result_async::<(Option, Option, Q)>(conn).await?; let status = if id0 == id1 { UpdateStatus::Updated } else { From 387a0fc70c0d0f32840f845b4e5cc1631cab148d Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 6 Oct 2022 13:58:01 -0700 Subject: [PATCH 08/15] clean up --- nexus/src/db/datastore/silo_user.rs | 28 +++++----------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/nexus/src/db/datastore/silo_user.rs b/nexus/src/db/datastore/silo_user.rs index 1a3fb48aab..fe14461125 100644 --- a/nexus/src/db/datastore/silo_user.rs +++ b/nexus/src/db/datastore/silo_user.rs @@ -84,22 +84,15 @@ impl DataStore { self.pool_authorized(opctx) .await? .transaction_async(|mut conn| async move { - // Check if the user exists first so that we can bail with an - // appropriate error if not. It would be better to use - // `check_if_exists()`/`execute_and_check()` here, but that - // mechanism does not yet support running in a transaction. - // This approach remains correct at least with CockroachDB - // because it guarantees serializability of transactions. With - // other databases, we might need an explicit "SELECT FOR - // UPDATE" here. Either way, we're essentially locking this row - // for the duration of the transaction, which is not great. + // Delete the user record. { use db::schema::silo_user::dsl; - let _ = dsl::silo_user + diesel::update(dsl::silo_user) .filter(dsl::id.eq(authz_silo_user_id)) .filter(dsl::time_deleted.is_null()) - .select(SiloUser::as_select()) - .get_result_async(&mut conn) + .set(dsl::time_deleted.eq(Utc::now())) + .check_if_exists::(authz_silo_user_id) + .execute_and_check(&mut conn) .await?; } @@ -141,17 +134,6 @@ impl DataStore { .await?; } - // Delete the user record. - { - use db::schema::silo_user::dsl; - diesel::update(dsl::silo_user) - .filter(dsl::id.eq(authz_silo_user_id)) - .filter(dsl::time_deleted.is_null()) - .set(dsl::time_deleted.eq(Utc::now())) - .check_if_exists::(authz_silo_user_id) - .execute_and_check(&mut conn) - .await?; - } Ok(()) }) .await From acb15917d927ce62813eaf033393f17657c9e0c0 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 6 Oct 2022 16:44:15 -0700 Subject: [PATCH 09/15] test (and fix) behavior of using local IdP for SamlJit Silos --- nexus/src/app/iam.rs | 16 --- nexus/src/app/silo.rs | 136 +++++++++++++-------- nexus/src/external_api/http_entrypoints.rs | 4 +- nexus/tests/integration_tests/silos.rs | 61 ++++++++- 4 files changed, 141 insertions(+), 76 deletions(-) diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index 968a0a07f1..74ffb44373 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -90,22 +90,6 @@ impl super::Nexus { Ok(db_silo_user) } - pub async fn silo_users_list( - &self, - opctx: &OpContext, - silo_name: &Name, - pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { - let (authz_silo, ..) = LookupPath::new(opctx, &self.db_datastore) - .silo_name(silo_name) - .fetch() - .await?; - let authz_silo_user_list = authz::SiloUserList::new(authz_silo); - self.db_datastore - .silo_users_list_by_id(opctx, &authz_silo_user_list, pagparams) - .await - } - // Built-in users pub async fn users_builtin_list( diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index d803312052..4425f871c5 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -16,7 +16,7 @@ use crate::external_api::shared; use crate::{authn, authz}; use anyhow::Context; use nexus_db_model::UserProvisionType; -use omicron_common::api::external::CreateResult; +use omicron_common::api::external::{CreateResult, ResourceType}; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; @@ -24,6 +24,7 @@ use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::UpdateResult; use uuid::Uuid; +use std::str::FromStr; impl super::Nexus { // Silos @@ -142,37 +143,85 @@ impl super::Nexus { // Users - pub async fn silo_api_user_create( + /// Helper function for looking up a LocalOnly Silo by name + /// + /// This is called from contexts that are trying to access the "local" + /// identity provider. On failure, it returns a 404 for that identity + /// provider. + async fn silo_api_fetch( &self, opctx: &OpContext, silo_name: &Name, - new_user_params: params::UserCreate, - ) -> CreateResult { - let datastore = self.datastore(); - let (authz_silo, db_silo) = LookupPath::new(opctx, &datastore) + ) -> LookupResult<(authz::Silo, db::model::Silo)> { + let (authz_silo, db_silo) = LookupPath::new(opctx, &self.db_datastore) .silo_name(silo_name) .fetch() .await?; - if db_silo.user_provision_type != UserProvisionType::ApiOnly { - return Err(Error::invalid_request( - "cannot create users in this kind of Silo", + return Err(Error::not_found_by_name( + ResourceType::IdentityProvider, + &omicron_common::api::external::Name::from_str("local").unwrap(), )); } + Ok((authz_silo, db_silo)) + } - let silo_user = db::model::SiloUser::new( - authz_silo.id(), - Uuid::new_v4(), - new_user_params.external_id.as_ref().to_owned(), - ); - let authz_silo_user_list = authz::SiloUserList::new(authz_silo.clone()); + /// Helper function for looking up a user in a Silo + /// + /// The normal Lookup API lets you look up users directly, regardless of + /// what Silo they're in. This helper validates that they're in the + /// expected Silo. + async fn silo_api_user_lookup_by_id( + &self, + opctx: &OpContext, + authz_silo: &authz::Silo, + silo_user_id: Uuid, + action: authz::Action, + ) -> LookupResult<(authz::SiloUser, db::model::SiloUser)> { + let (_, authz_silo_user, db_silo_user) = + LookupPath::new(opctx, self.datastore()) + .silo_user_id(silo_user_id) + .fetch_for(action) + .await?; + if db_silo_user.silo_id != authz_silo.id() { + return Err(authz_silo_user.not_found()); + } + Ok((authz_silo_user, db_silo_user)) + } + + pub async fn silo_api_users_list( + &self, + opctx: &OpContext, + silo_name: &Name, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; + let authz_silo_user_list = authz::SiloUserList::new(authz_silo); + self.db_datastore + .silo_users_list_by_id(opctx, &authz_silo_user_list, pagparams) + .await + } + + pub async fn silo_api_user_create( + &self, + opctx: &OpContext, + silo_name: &Name, + new_user_params: params::UserCreate, + ) -> CreateResult { + let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; + let authz_silo_user_list = authz::SiloUserList::new(authz_silo.clone()); // TODO-cleanup This authz check belongs in silo_user_create(). opctx .authorize(authz::Action::CreateChild, &authz_silo_user_list) .await?; + let silo_user = db::model::SiloUser::new( + authz_silo.id(), + Uuid::new_v4(), + new_user_params.external_id.as_ref().to_owned(), + ); let (_, db_silo_user) = - datastore.silo_user_create(&authz_silo, silo_user).await?; + self.datastore().silo_user_create(&authz_silo, silo_user).await?; Ok(db_silo_user) } @@ -182,51 +231,32 @@ impl super::Nexus { silo_name: &Name, silo_user_id: Uuid, ) -> DeleteResult { - // Verify that this user is actually in this Silo. - let datastore = self.datastore(); - let (authz_silo, db_silo) = LookupPath::new(opctx, datastore) - .silo_name(silo_name) - .fetch() - .await?; - let (_, authz_silo_user, db_silo_user) = - LookupPath::new(opctx, datastore) - .silo_user_id(silo_user_id) - .fetch_for(authz::Action::Delete) - .await?; - if db_silo_user.silo_id != authz_silo.id() { - return Err(authz_silo_user.not_found()); - } - - if db_silo.user_provision_type != UserProvisionType::ApiOnly { - return Err(Error::invalid_request( - "cannot delete users in this kind of Silo", - )); - } - + let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; + let (authz_silo_user, _) = self.silo_api_user_lookup_by_id( + opctx, + &authz_silo, + silo_user_id, + authz::Action::Delete, + ) + .await?; self.db_datastore.silo_user_delete(opctx, &authz_silo_user).await } - pub async fn silo_user_fetch( + pub async fn silo_api_user_fetch( &self, opctx: &OpContext, silo_name: &Name, silo_user_id: Uuid, ) -> LookupResult { - let datastore = self.datastore(); - let (authz_silo_requested, _) = LookupPath::new(opctx, datastore) - .silo_name(silo_name) - .fetch() - .await?; - let (_, authz_silo_user, db_silo_user) = - LookupPath::new(opctx, datastore) - .silo_user_id(silo_user_id) - .fetch() - .await?; - if db_silo_user.silo_id != authz_silo_requested.id() { - Err(authz_silo_user.not_found()) - } else { - Ok(db_silo_user) - } + let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; + let (_, db_silo_user) = self.silo_api_user_lookup_by_id( + opctx, + &authz_silo, + silo_user_id, + authz::Action::Delete, + ) + .await?; + Ok(db_silo_user) } /// Based on an authenticated subject, fetch or create a silo user diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 372eb2a805..52ea0984fb 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -758,7 +758,7 @@ async fn local_idp_users_list( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let users = nexus - .silo_users_list(&opctx, &silo_name, &pagparams) + .silo_api_users_list(&opctx, &silo_name, &pagparams) .await? .into_iter() .map(|i| i.into()) @@ -828,7 +828,7 @@ async fn local_idp_user_view( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let user = nexus - .silo_user_fetch( + .silo_api_user_fetch( &opctx, &path_params.silo_name, path_params.user_id, diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index 6e4acbbd2e..765b66e778 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -1500,7 +1500,7 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure_with_body( client, - StatusCode::BAD_REQUEST, + StatusCode::NOT_FOUND, Method::POST, "/system/silos/jit/identity-providers/local/users/create", ¶ms::UserCreate { @@ -1513,7 +1513,60 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { .unwrap() .parsed_body() .unwrap(); - assert_eq!(error.message, "cannot create users in this kind of Silo"); + assert_eq!( + error.message, + "not found: identity-provider with name \"local\"" + ); + + // Suppose we do create a user (as via JIT). They should not be able to + // view or delete them via the local identity provider. + let other_user_id = "57372ebb-ee76-4a2d-fa3e-e1875a8d11c0".parse().unwrap(); + let _ = nexus + .silo_user_create(silo.identity.id, other_user_id, "other-user".into()) + .await + .unwrap(); + let other_user_url = format!( + "/system/silos/jit/identity-providers/local/users/id/{}", + other_user_id + ); + + for method in [Method::GET, Method::DELETE] { + let error: dropshot::HttpErrorResponseBody = + NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + method, + &other_user_url, + ) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + error.message, + "not found: identity-provider with name \"local\"" + ); + } + + // They should also not be able to list users via the "local" IdP endpoint. + let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + "/system/silos/jit/identity-providers/local/users/all", + ) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + error.message, + "not found: identity-provider with name \"local\"" + ); } #[nexus_test] @@ -1521,7 +1574,7 @@ async fn test_local_silo_constraints(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; - // Now, let's try a "LocalOnly" Silo with its own admin user. + // Create a "LocalOnly" Silo with its own admin user. let silo = create_silo( &client, "fixed", @@ -1614,8 +1667,6 @@ async fn test_local_silo_constraints(cptestctx: &ControlPlaneTestContext) { } // XXX-dap TODO-coverage -// - attempt to create, delete user using API in SamlJit Silo ("create" may be -// covered above) // - successful user create/fetch/list/delete // - deleting a user that doesn't exist // - fetch/delete a user that exists in a different Silo From 8b07c3475d2df869ddd7bfb5d7173909a7bb0ec4 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 6 Oct 2022 17:33:45 -0700 Subject: [PATCH 10/15] more tests --- common/src/sql/dbinit.sql | 7 ++ nexus/db-model/src/silo_user.rs | 8 +- nexus/src/app/silo.rs | 37 +++--- nexus/src/authz/omicron.polar | 11 +- nexus/src/db/datastore/silo_user.rs | 1 + nexus/src/db/lookup.rs | 3 +- nexus/test-utils/src/http_testing.rs | 2 +- nexus/tests/integration_tests/silos.rs | 162 ++++++++++++++++++++++++- 8 files changed, 208 insertions(+), 23 deletions(-) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 313d8e354f..95f6858e82 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -1417,6 +1417,9 @@ CREATE TABLE omicron.public.console_session ( CREATE INDEX ON omicron.public.console_session ( time_created ); +CREATE INDEX ON omicron.public.console_session ( + silo_user_id +); /*******************************************************************/ @@ -1532,6 +1535,10 @@ CREATE UNIQUE INDEX ON omicron.public.device_access_token ( client_id, device_code ); +CREATE INDEX ON omicron.public.device_access_token ( + silo_user_id +); + /* * Roles built into the system * diff --git a/nexus/db-model/src/silo_user.rs b/nexus/db-model/src/silo_user.rs index 16fb6eb32f..3ce09326d8 100644 --- a/nexus/db-model/src/silo_user.rs +++ b/nexus/db-model/src/silo_user.rs @@ -15,6 +15,7 @@ pub struct SiloUser { #[diesel(embed)] identity: SiloUserIdentity, + pub time_deleted: Option>, pub silo_id: Uuid, /// The identity provider's ID for this user. @@ -23,7 +24,12 @@ pub struct SiloUser { impl SiloUser { pub fn new(silo_id: Uuid, user_id: Uuid, external_id: String) -> Self { - Self { identity: SiloUserIdentity::new(user_id), silo_id, external_id } + Self { + identity: SiloUserIdentity::new(user_id), + time_deleted: None, + silo_id, + external_id, + } } } diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 4425f871c5..b445bf19f3 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -16,15 +16,15 @@ use crate::external_api::shared; use crate::{authn, authz}; use anyhow::Context; use nexus_db_model::UserProvisionType; -use omicron_common::api::external::{CreateResult, ResourceType}; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::UpdateResult; -use uuid::Uuid; +use omicron_common::api::external::{CreateResult, ResourceType}; use std::str::FromStr; +use uuid::Uuid; impl super::Nexus { // Silos @@ -160,7 +160,8 @@ impl super::Nexus { if db_silo.user_provision_type != UserProvisionType::ApiOnly { return Err(Error::not_found_by_name( ResourceType::IdentityProvider, - &omicron_common::api::external::Name::from_str("local").unwrap(), + &omicron_common::api::external::Name::from_str("local") + .unwrap(), )); } Ok((authz_silo, db_silo)) @@ -232,13 +233,14 @@ impl super::Nexus { silo_user_id: Uuid, ) -> DeleteResult { let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; - let (authz_silo_user, _) = self.silo_api_user_lookup_by_id( - opctx, - &authz_silo, - silo_user_id, - authz::Action::Delete, - ) - .await?; + let (authz_silo_user, _) = self + .silo_api_user_lookup_by_id( + opctx, + &authz_silo, + silo_user_id, + authz::Action::Delete, + ) + .await?; self.db_datastore.silo_user_delete(opctx, &authz_silo_user).await } @@ -249,13 +251,14 @@ impl super::Nexus { silo_user_id: Uuid, ) -> LookupResult { let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; - let (_, db_silo_user) = self.silo_api_user_lookup_by_id( - opctx, - &authz_silo, - silo_user_id, - authz::Action::Delete, - ) - .await?; + let (_, db_silo_user) = self + .silo_api_user_lookup_by_id( + opctx, + &authz_silo, + silo_user_id, + authz::Action::Read, + ) + .await?; Ok(db_silo_user) } diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 65b7df4421..d5c6fffb43 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -239,14 +239,23 @@ resource SiloUser { "create_child", ]; - relations = { parent_silo: Silo }; + # Fleet and Silo administrators can manage a Silo's users. This is one + # of the only areas of Silo configuration that Fleet Administrators have + # permissions on. + relations = { parent_silo: Silo, parent_fleet: Fleet }; "list_children" if "viewer" on "parent_silo"; "read" if "viewer" on "parent_silo"; "modify" if "admin" on "parent_silo"; "create_child" if "admin" on "parent_silo"; + "list_children" if "admin" on "parent_fleet"; + "modify" if "admin" on "parent_fleet"; + "read" if "admin" on "parent_fleet"; + "create_child" if "admin" on "parent_fleet"; } has_relation(silo: Silo, "parent_silo", user: SiloUser) if user.silo = silo; +has_relation(fleet: Fleet, "parent_fleet", user: SiloUser) + if user.silo.fleet = fleet; # authenticated actors have all permissions on themselves has_permission(actor: AuthenticatedActor, _perm: String, silo_user: SiloUser) diff --git a/nexus/src/db/datastore/silo_user.rs b/nexus/src/db/datastore/silo_user.rs index fe14461125..ff77a33fba 100644 --- a/nexus/src/db/datastore/silo_user.rs +++ b/nexus/src/db/datastore/silo_user.rs @@ -142,6 +142,7 @@ impl DataStore { e, ErrorHandler::NotFoundByResource(authz_silo_user), ) + .internal_context("deleting silo user") }) } diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 957600384d..5307aa8428 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -454,7 +454,8 @@ lookup_resource! { children = [ "SshKey" ], lookup_by_name = false, soft_deletes = true, - primary_key_columns = [ { column_name = "id", rust_type = Uuid } ] + primary_key_columns = [ { column_name = "id", rust_type = Uuid } ], + visible_outside_silo = true } lookup_resource! { diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index cd80ce0e1d..ff1bbad3c0 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -448,7 +448,7 @@ impl TestResponse { /// Specifies what user (if any) the caller wants to use for authenticating to /// the server -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum AuthnMode { UnprivilegedUser, PrivilegedUser, diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index 765b66e778..fd58ce509a 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -1666,7 +1666,165 @@ async fn test_local_silo_constraints(cptestctx: &ControlPlaneTestContext) { .unwrap(); } +#[nexus_test] +async fn test_local_silo_users(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; + + // Create a "LocalOnly" Silo. + let silo = create_silo( + &client, + "local-only", + true, + shared::SiloIdentityMode::LocalOnly, + ) + .await; + + // We'll run through a battery of tests as each of two different users: the + // usual "test-privileged" user (which should have full access because + // they're a Fleet Administrator) as well as a newly-created Silo Admin + // user. + run_user_tests(client, &silo, &AuthnMode::PrivilegedUser, &[]).await; + + let new_silo_user_id = + "5b3564b6-8770-4a30-b538-8ef6ae3efa3b".parse().unwrap(); + let _ = nexus + .silo_user_create( + silo.identity.id, + new_silo_user_id, + "admin-user".into(), + ) + .await + .unwrap(); + grant_iam( + client, + "/system/silos/local-only", + SiloRole::Admin, + new_silo_user_id, + AuthnMode::PrivilegedUser, + ) + .await; + run_user_tests( + client, + &silo, + &AuthnMode::SiloUser(new_silo_user_id), + &[views::User { + id: new_silo_user_id, + display_name: String::from("admin-user"), + }], + ) + .await; +} + +/// Runs a sequence of tests for create, read, and delete of API-managed users +async fn run_user_tests( + client: &dropshot::test_util::ClientTestContext, + silo: &views::Silo, + authn_mode: &AuthnMode, + existing_users: &[views::User], +) { + let url_local_idp_users = format!( + "/system/silos/{}/identity-providers/local/users", + silo.identity.name + ); + let url_all_users = format!("{}/all", url_local_idp_users); + let url_user_create = format!("{}/create", url_local_idp_users); + + // Fetch users and verify it matches what the caller expects. + println!("run_user_tests: as {:?}: fetch all users", authn_mode); + let users = NexusRequest::object_get(client, &url_all_users) + .authn_as(authn_mode.clone()) + .execute() + .await + .expect("failed to list users") + .parsed_body::>() + .unwrap() + .items; + println!("users: {:?}", users); + assert_eq!(users, existing_users); + + // Create a user. + let user_created = NexusRequest::objects_post( + client, + &url_user_create, + ¶ms::UserCreate { + external_id: params::UserId::from_str("a-test-user").unwrap(), + }, + ) + .authn_as(authn_mode.clone()) + .execute() + .await + .expect("failed to create user") + .parsed_body::() + .unwrap(); + assert_eq!(user_created.display_name, "a-test-user"); + println!("created user: {:?}", user_created); + + // Fetch the user we just created. + let user_url = format!("{}/id/{}", url_local_idp_users, user_created.id); + let user_found = NexusRequest::object_get(client, &user_url) + .authn_as(authn_mode.clone()) + .execute() + .await + .expect("failed to fetch user we just created") + .parsed_body::() + .unwrap(); + assert_eq!(user_created, user_found); + + // List users. We should find whatever was there before, plus our new one. + let new_users = NexusRequest::object_get(client, &url_all_users) + .authn_as(authn_mode.clone()) + .execute() + .await + .expect("failed to list users") + .parsed_body::>() + .unwrap() + .items; + println!("new_users: {:?}", new_users); + let new_users = new_users + .iter() + .filter(|new_user| !users.iter().any(|old_user| *new_user == old_user)) + .collect::>(); + assert_eq!(new_users, &[&user_created]); + + // Delete the user that we created. + NexusRequest::object_delete(client, &user_url) + .authn_as(authn_mode.clone()) + .execute() + .await + .expect("failed to delete the user we just created"); + + // We should not be able to fetch or delete the user again. + for method in [Method::GET, Method::DELETE] { + let error = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + method, + &user_url, + ) + .authn_as(authn_mode.clone()) + .execute() + .await + .expect("unexpectedly succeeded in fetching deleted user") + .parsed_body::() + .unwrap(); + let not_found_message = + format!("not found: silo-user with id \"{}\"", user_created.id); + assert_eq!(error.message, not_found_message); + } + + // List users again. We should just find whatever we started with. + let last_users = NexusRequest::object_get(client, &url_all_users) + .authn_as(authn_mode.clone()) + .execute() + .await + .expect("failed to list users") + .parsed_body::>() + .unwrap() + .items; + println!("last_users: {:?}", last_users); + assert_eq!(last_users, existing_users); +} + // XXX-dap TODO-coverage -// - successful user create/fetch/list/delete -// - deleting a user that doesn't exist // - fetch/delete a user that exists in a different Silo From 2e84e10905d9ab8fe8e8975f60354c6f9ce54e3e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 6 Oct 2022 20:16:58 -0700 Subject: [PATCH 11/15] more tests --- nexus/tests/integration_tests/silos.rs | 128 +++++++++++++++++++++---- 1 file changed, 107 insertions(+), 21 deletions(-) diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index fd58ce509a..b97ca64235 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -1671,10 +1671,10 @@ async fn test_local_silo_users(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; - // Create a "LocalOnly" Silo. - let silo = create_silo( + // Create a "LocalOnly" Silo for testing. + let silo1 = create_silo( &client, - "local-only", + "silo1", true, shared::SiloIdentityMode::LocalOnly, ) @@ -1684,21 +1684,24 @@ async fn test_local_silo_users(cptestctx: &ControlPlaneTestContext) { // usual "test-privileged" user (which should have full access because // they're a Fleet Administrator) as well as a newly-created Silo Admin // user. - run_user_tests(client, &silo, &AuthnMode::PrivilegedUser, &[]).await; + run_user_tests(client, &silo1, &AuthnMode::PrivilegedUser, &[]).await; + // Create a Silo Admin in our test Silo and run through the same tests. let new_silo_user_id = "5b3564b6-8770-4a30-b538-8ef6ae3efa3b".parse().unwrap(); - let _ = nexus - .silo_user_create( - silo.identity.id, - new_silo_user_id, - "admin-user".into(), - ) - .await - .unwrap(); + let admin_user = views::User::from( + nexus + .silo_user_create( + silo1.identity.id, + new_silo_user_id, + "admin-user".into(), + ) + .await + .unwrap(), + ); grant_iam( client, - "/system/silos/local-only", + "/system/silos/silo1", SiloRole::Admin, new_silo_user_id, AuthnMode::PrivilegedUser, @@ -1706,14 +1709,100 @@ async fn test_local_silo_users(cptestctx: &ControlPlaneTestContext) { .await; run_user_tests( client, - &silo, + &silo1, &AuthnMode::SiloUser(new_silo_user_id), - &[views::User { - id: new_silo_user_id, - display_name: String::from("admin-user"), - }], + &[admin_user.clone()], + ) + .await; + + // Now create a second Silo and make sure that none of the per-user + // endpoints can be accessed using a user id from the first Silo. + // (Internally, it's possible to look up a user independent of the Silo + // they're in, so this check ensures that we do actually limit results to + // the appropriate Silo.) + let silo2 = create_silo( + &client, + "silo2", + true, + shared::SiloIdentityMode::LocalOnly, ) .await; + let silo1_users_url = format!( + "/system/silos/{}/identity-providers/local/users", + silo1.identity.name, + ); + let silo1_user_url = format!("{}/id/{}", silo1_users_url, new_silo_user_id); + let silo2_users_url = format!( + "/system/silos/{}/identity-providers/local/users", + silo2.identity.name, + ); + let silo2_user_url = format!("{}/id/{}", silo2_users_url, new_silo_user_id); + // Double check that we have the right URL form in the first place. We + // should be able to fetch the user in silo1. + let user = NexusRequest::object_get(client, &silo1_user_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(); + assert_eq!(user.id, new_silo_user_id); + + for method in [Method::GET, Method::DELETE] { + let error = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + method, + &silo2_user_url, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(); + let not_found_message = + format!("not found: silo-user with id \"{}\"", new_silo_user_id); + assert_eq!(error.message, not_found_message); + } + + // Make sure that the list of users in each Silo are different, too. + let silo1_users = + NexusRequest::object_get(client, &format!("{}/all", silo1_users_url)) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::>() + .unwrap() + .items; + assert_eq!(silo1_users, &[admin_user.clone()]); + + let silo2_users_list_url = format!("{}/all", silo2_users_url); + let silo2_users = NexusRequest::object_get(client, &silo2_users_list_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::>() + .unwrap() + .items; + assert!(silo2_users.is_empty()); + + // Make sure that the admin in Silo1 cannot list users in Silo2. + let error = NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + &silo2_users_list_url, + ) + .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap(); + assert_eq!(error.message, "not found: silo with name \"silo2\""); } /// Runs a sequence of tests for create, read, and delete of API-managed users @@ -1825,6 +1914,3 @@ async fn run_user_tests( println!("last_users: {:?}", last_users); assert_eq!(last_users, existing_users); } - -// XXX-dap TODO-coverage -// - fetch/delete a user that exists in a different Silo From 232cf81d285aeafb66678a9f8d9a771cce7c0ae1 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 6 Oct 2022 20:36:04 -0700 Subject: [PATCH 12/15] more comments --- common/src/sql/dbinit.sql | 3 +++ nexus/src/app/silo.rs | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 95f6858e82..b8fc1bcbc2 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -1417,6 +1417,8 @@ CREATE TABLE omicron.public.console_session ( CREATE INDEX ON omicron.public.console_session ( time_created ); + +-- This index is used to remove sessions for a user that's being deleted. CREATE INDEX ON omicron.public.console_session ( silo_user_id ); @@ -1535,6 +1537,7 @@ CREATE UNIQUE INDEX ON omicron.public.device_access_token ( client_id, device_code ); +-- This index is used to remove tokens for a user that's being deleted. CREATE INDEX ON omicron.public.device_access_token ( silo_user_id ); diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index b445bf19f3..947e08aa6d 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -191,6 +191,7 @@ impl super::Nexus { Ok((authz_silo_user, db_silo_user)) } + /// List the users in a specific Silo pub async fn silo_api_users_list( &self, opctx: &OpContext, @@ -204,6 +205,7 @@ impl super::Nexus { .await } + /// Create a user in a specific Silo pub async fn silo_api_user_create( &self, opctx: &OpContext, @@ -226,6 +228,7 @@ impl super::Nexus { Ok(db_silo_user) } + /// Delete a specific user in a specific Silo pub async fn silo_api_user_delete( &self, opctx: &OpContext, @@ -244,6 +247,7 @@ impl super::Nexus { self.db_datastore.silo_user_delete(opctx, &authz_silo_user).await } + /// Fetch a specific user in a specific Silo pub async fn silo_api_user_fetch( &self, opctx: &OpContext, From 92a72baa976e85633a2584528e4c28694342ff74 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 7 Oct 2022 15:15:17 -0700 Subject: [PATCH 13/15] clarify comment and function name --- nexus/src/app/silo.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 947e08aa6d..2f1eee3c95 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -169,10 +169,9 @@ impl super::Nexus { /// Helper function for looking up a user in a Silo /// - /// The normal Lookup API lets you look up users directly, regardless of - /// what Silo they're in. This helper validates that they're in the - /// expected Silo. - async fn silo_api_user_lookup_by_id( + /// `LookupPath` lets you look up users directly, regardless of what Silo + /// they're in. This helper validates that they're in the expected Silo. + async fn silo_user_lookup_by_id( &self, opctx: &OpContext, authz_silo: &authz::Silo, @@ -237,7 +236,7 @@ impl super::Nexus { ) -> DeleteResult { let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; let (authz_silo_user, _) = self - .silo_api_user_lookup_by_id( + .silo_user_lookup_by_id( opctx, &authz_silo, silo_user_id, @@ -256,7 +255,7 @@ impl super::Nexus { ) -> LookupResult { let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; let (_, db_silo_user) = self - .silo_api_user_lookup_by_id( + .silo_user_lookup_by_id( opctx, &authz_silo, silo_user_id, From 0950c221326fb89ec7212f462979577438005a06 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 7 Oct 2022 15:19:44 -0700 Subject: [PATCH 14/15] review feedback: rename local IdP functions --- nexus/src/app/silo.rs | 78 ++++++++++++---------- nexus/src/external_api/http_entrypoints.rs | 8 +-- 2 files changed, 46 insertions(+), 40 deletions(-) diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 2f1eee3c95..b20a794cc3 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -143,30 +143,6 @@ impl super::Nexus { // Users - /// Helper function for looking up a LocalOnly Silo by name - /// - /// This is called from contexts that are trying to access the "local" - /// identity provider. On failure, it returns a 404 for that identity - /// provider. - async fn silo_api_fetch( - &self, - opctx: &OpContext, - silo_name: &Name, - ) -> LookupResult<(authz::Silo, db::model::Silo)> { - let (authz_silo, db_silo) = LookupPath::new(opctx, &self.db_datastore) - .silo_name(silo_name) - .fetch() - .await?; - if db_silo.user_provision_type != UserProvisionType::ApiOnly { - return Err(Error::not_found_by_name( - ResourceType::IdentityProvider, - &omicron_common::api::external::Name::from_str("local") - .unwrap(), - )); - } - Ok((authz_silo, db_silo)) - } - /// Helper function for looking up a user in a Silo /// /// `LookupPath` lets you look up users directly, regardless of what Silo @@ -190,28 +166,56 @@ impl super::Nexus { Ok((authz_silo_user, db_silo_user)) } - /// List the users in a specific Silo - pub async fn silo_api_users_list( + // The "local" identity provider (available only in `LocalOnly` Silos) + + /// Helper function for looking up a LocalOnly Silo by name + /// + /// This is called from contexts that are trying to access the "local" + /// identity provider. On failure, it returns a 404 for that identity + /// provider. + async fn local_idp_fetch_silo( + &self, + opctx: &OpContext, + silo_name: &Name, + ) -> LookupResult<(authz::Silo, db::model::Silo)> { + let (authz_silo, db_silo) = LookupPath::new(opctx, &self.db_datastore) + .silo_name(silo_name) + .fetch() + .await?; + if db_silo.user_provision_type != UserProvisionType::ApiOnly { + return Err(Error::not_found_by_name( + ResourceType::IdentityProvider, + &omicron_common::api::external::Name::from_str("local") + .unwrap(), + )); + } + Ok((authz_silo, db_silo)) + } + + /// List the users in a Silo's local identity provider + pub async fn local_idp_list_users( &self, opctx: &OpContext, silo_name: &Name, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { - let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; + let (authz_silo, _) = + self.local_idp_fetch_silo(opctx, silo_name).await?; let authz_silo_user_list = authz::SiloUserList::new(authz_silo); self.db_datastore .silo_users_list_by_id(opctx, &authz_silo_user_list, pagparams) .await } - /// Create a user in a specific Silo - pub async fn silo_api_user_create( + /// Create a user in a Silo's local identity provider + pub async fn local_idp_create_user( &self, opctx: &OpContext, silo_name: &Name, new_user_params: params::UserCreate, ) -> CreateResult { - let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; + let (authz_silo, _) = + self.local_idp_fetch_silo(opctx, silo_name).await?; let authz_silo_user_list = authz::SiloUserList::new(authz_silo.clone()); // TODO-cleanup This authz check belongs in silo_user_create(). opctx @@ -227,14 +231,15 @@ impl super::Nexus { Ok(db_silo_user) } - /// Delete a specific user in a specific Silo - pub async fn silo_api_user_delete( + /// Delete a user in a Silo's local identity provider + pub async fn local_idp_delete_user( &self, opctx: &OpContext, silo_name: &Name, silo_user_id: Uuid, ) -> DeleteResult { - let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; + let (authz_silo, _) = + self.local_idp_fetch_silo(opctx, silo_name).await?; let (authz_silo_user, _) = self .silo_user_lookup_by_id( opctx, @@ -246,14 +251,15 @@ impl super::Nexus { self.db_datastore.silo_user_delete(opctx, &authz_silo_user).await } - /// Fetch a specific user in a specific Silo - pub async fn silo_api_user_fetch( + /// Fetch a user in a Silo's local identity provider + pub async fn local_idp_fetch_user( &self, opctx: &OpContext, silo_name: &Name, silo_user_id: Uuid, ) -> LookupResult { - let (authz_silo, _) = self.silo_api_fetch(opctx, silo_name).await?; + let (authz_silo, _) = + self.local_idp_fetch_silo(opctx, silo_name).await?; let (_, db_silo_user) = self .silo_user_lookup_by_id( opctx, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 52ea0984fb..3948a74437 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -758,7 +758,7 @@ async fn local_idp_users_list( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let users = nexus - .silo_api_users_list(&opctx, &silo_name, &pagparams) + .local_idp_list_users(&opctx, &silo_name, &pagparams) .await? .into_iter() .map(|i| i.into()) @@ -793,7 +793,7 @@ async fn local_idp_user_create( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let user = nexus - .silo_api_user_create( + .local_idp_create_user( &opctx, &silo_name, new_user_params.into_inner(), @@ -828,7 +828,7 @@ async fn local_idp_user_view( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let user = nexus - .silo_api_user_fetch( + .local_idp_fetch_user( &opctx, &path_params.silo_name, path_params.user_id, @@ -854,7 +854,7 @@ async fn local_idp_user_delete( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; nexus - .silo_api_user_delete( + .local_idp_delete_user( &opctx, &path_params.silo_name, path_params.user_id, From 5bc7654a53bdd9d90d3343eb8cc71b2786324051 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 12 Oct 2022 20:09:07 -0700 Subject: [PATCH 15/15] review feedback: silo user list/view could apply to all Silos --- nexus/src/app/silo.rs | 74 ++-- nexus/src/external_api/http_entrypoints.rs | 146 +++---- nexus/tests/integration_tests/endpoints.rs | 37 +- nexus/tests/integration_tests/silos.rs | 401 +++++++++++------- nexus/tests/integration_tests/unauthorized.rs | 3 +- nexus/tests/output/nexus_tags.txt | 8 +- nexus/tests/output/silo-user-views-output.txt | 72 ++++ openapi/nexus.json | 240 +++++------ 8 files changed, 592 insertions(+), 389 deletions(-) create mode 100644 nexus/tests/output/silo-user-views-output.txt diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index b20a794cc3..155e3ca181 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -166,6 +166,45 @@ impl super::Nexus { Ok((authz_silo_user, db_silo_user)) } + /// List the users in a Silo + pub async fn silo_list_users( + &self, + opctx: &OpContext, + silo_name: &Name, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + let (authz_silo,) = LookupPath::new(opctx, self.datastore()) + .silo_name(silo_name) + .lookup_for(authz::Action::Read) + .await?; + let authz_silo_user_list = authz::SiloUserList::new(authz_silo); + self.db_datastore + .silo_users_list_by_id(opctx, &authz_silo_user_list, pagparams) + .await + } + + /// Fetch a user in a Silo + pub async fn silo_user_fetch( + &self, + opctx: &OpContext, + silo_name: &Name, + silo_user_id: Uuid, + ) -> LookupResult { + let (authz_silo,) = LookupPath::new(opctx, self.datastore()) + .silo_name(silo_name) + .lookup_for(authz::Action::Read) + .await?; + let (_, db_silo_user) = self + .silo_user_lookup_by_id( + opctx, + &authz_silo, + silo_user_id, + authz::Action::Read, + ) + .await?; + Ok(db_silo_user) + } + // The "local" identity provider (available only in `LocalOnly` Silos) /// Helper function for looking up a LocalOnly Silo by name @@ -192,21 +231,6 @@ impl super::Nexus { Ok((authz_silo, db_silo)) } - /// List the users in a Silo's local identity provider - pub async fn local_idp_list_users( - &self, - opctx: &OpContext, - silo_name: &Name, - pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { - let (authz_silo, _) = - self.local_idp_fetch_silo(opctx, silo_name).await?; - let authz_silo_user_list = authz::SiloUserList::new(authz_silo); - self.db_datastore - .silo_users_list_by_id(opctx, &authz_silo_user_list, pagparams) - .await - } - /// Create a user in a Silo's local identity provider pub async fn local_idp_create_user( &self, @@ -251,26 +275,6 @@ impl super::Nexus { self.db_datastore.silo_user_delete(opctx, &authz_silo_user).await } - /// Fetch a user in a Silo's local identity provider - pub async fn local_idp_fetch_user( - &self, - opctx: &OpContext, - silo_name: &Name, - silo_user_id: Uuid, - ) -> LookupResult { - let (authz_silo, _) = - self.local_idp_fetch_silo(opctx, silo_name).await?; - let (_, db_silo_user) = self - .silo_user_lookup_by_id( - opctx, - &authz_silo, - silo_user_id, - authz::Action::Read, - ) - .await?; - Ok(db_silo_user) - } - /// Based on an authenticated subject, fetch or create a silo user pub async fn silo_user_from_authenticated_subject( &self, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 23fb6ef5aa..55a7f34094 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -235,9 +235,7 @@ pub fn external_api() -> NexusApiDescription { api.register(saml_identity_provider_create)?; api.register(saml_identity_provider_view)?; - api.register(local_idp_users_list)?; api.register(local_idp_user_create)?; - api.register(local_idp_user_view)?; api.register(local_idp_user_delete)?; api.register(system_image_list)?; @@ -249,6 +247,8 @@ pub fn external_api() -> NexusApiDescription { api.register(updates_refresh)?; api.register(user_list)?; + api.register(silo_users_list)?; + api.register(silo_user_view)?; api.register(group_list)?; // Console API operations @@ -630,6 +630,76 @@ async fn silo_policy_update( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +// Silo-specific user endpoints + +/// List users in a specific Silo +#[endpoint { + method = GET, + path = "/system/silos/{silo_name}/users/all", + tags = ["system"], +}] +async fn silo_users_list( + rqctx: Arc>>, + path_params: Path, + query_params: Query, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let silo_name = path_params.into_inner().silo_name; + let query = query_params.into_inner(); + let pagparams = data_page_params_for(&rqctx, &query)?; + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let users = nexus + .silo_list_users(&opctx, &silo_name, &pagparams) + .await? + .into_iter() + .map(|i| i.into()) + .collect(); + Ok(HttpResponseOk(ScanById::results_page( + &query, + users, + &|_, user: &User| user.id, + )?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + +/// Path parameters for Silo User requests +#[derive(Deserialize, JsonSchema)] +struct UserPathParam { + /// The silo's unique name. + silo_name: Name, + /// The user's internal id + user_id: Uuid, +} + +#[endpoint { + method = GET, + path = "/system/silos/{silo_name}/users/id/{user_id}", + tags = ["system"], +}] +async fn silo_user_view( + rqctx: Arc>>, + path_params: Path, +) -> Result, HttpError> { + let apictx = rqctx.context(); + let nexus = &apictx.nexus; + let path_params = path_params.into_inner(); + let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; + let user = nexus + .silo_user_fetch( + &opctx, + &path_params.silo_name, + path_params.user_id, + ) + .await?; + Ok(HttpResponseOk(user.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + // Silo identity providers /// List a silo's IDPs @@ -740,39 +810,6 @@ async fn saml_identity_provider_view( // "Local" Identity Provider -/// List users in the given local identity provider -#[endpoint { - method = GET, - path = "/system/silos/{silo_name}/identity-providers/local/users/all", - tags = ["system"], -}] -async fn local_idp_users_list( - rqctx: Arc>>, - path_params: Path, - query_params: Query, -) -> Result>, HttpError> { - let apictx = rqctx.context(); - let nexus = &apictx.nexus; - let silo_name = path_params.into_inner().silo_name; - let query = query_params.into_inner(); - let pagparams = data_page_params_for(&rqctx, &query)?; - let handler = async { - let opctx = OpContext::for_external_api(&rqctx).await?; - let users = nexus - .local_idp_list_users(&opctx, &silo_name, &pagparams) - .await? - .into_iter() - .map(|i| i.into()) - .collect(); - Ok(HttpResponseOk(ScanById::results_page( - &query, - users, - &|_, user: &User| user.id, - )?)) - }; - apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await -} - /// Create a user /// /// Users can only be created in Silos with `provision_type` == `Fixed`. @@ -780,7 +817,7 @@ async fn local_idp_users_list( /// logs in using an external Identity Provider. #[endpoint { method = POST, - path = "/system/silos/{silo_name}/identity-providers/local/users/create", + path = "/system/silos/{silo_name}/identity-providers/local/users", tags = ["system"], }] async fn local_idp_user_create( @@ -805,44 +842,9 @@ async fn local_idp_user_create( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/// Path parameters for Silo User requests -#[derive(Deserialize, JsonSchema)] -struct UserPathParam { - /// The silo's unique name. - silo_name: Name, - /// The user's internal id - user_id: Uuid, -} - -#[endpoint { - method = GET, - path = "/system/silos/{silo_name}/identity-providers/local/users/id/{user_id}", - tags = ["system"], -}] -async fn local_idp_user_view( - rqctx: Arc>>, - path_params: Path, -) -> Result, HttpError> { - let apictx = rqctx.context(); - let nexus = &apictx.nexus; - let path_params = path_params.into_inner(); - let handler = async { - let opctx = OpContext::for_external_api(&rqctx).await?; - let user = nexus - .local_idp_fetch_user( - &opctx, - &path_params.silo_name, - path_params.user_id, - ) - .await?; - Ok(HttpResponseOk(user.into())) - }; - apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await -} - #[endpoint { method = DELETE, - path = "/system/silos/{silo_name}/identity-providers/local/users/id/{user_id}", + path = "/system/silos/{silo_name}/identity-providers/local/users/{user_id}", tags = ["system"], }] async fn local_idp_user_delete( diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 392b767b67..e3377fa4cd 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -63,21 +63,22 @@ lazy_static! { admin_group_name: None, }; // Use the default Silo for testing the local IdP - pub static ref DEMO_SILO_USERS_CREATE_URL: String = - format!( - "/system/silos/{}/identity-providers/local/users/create", - DEFAULT_SILO.identity().name, - ); - pub static ref DEMO_SILO_USERS_LIST_URL: String = - format!( - "/system/silos/{}/identity-providers/local/users/all", - DEFAULT_SILO.identity().name, - ); - pub static ref DEMO_SILO_USER_ID_URL: String = format!( - "/system/silos/{}/identity-providers/local/users/id/{{id}}", + pub static ref DEMO_SILO_USERS_CREATE_URL: String = format!( + "/system/silos/{}/identity-providers/local/users", + DEFAULT_SILO.identity().name, + ); + pub static ref DEMO_SILO_USERS_LIST_URL: String = format!( + "/system/silos/{}/users/all", + DEFAULT_SILO.identity().name, + ); + pub static ref DEMO_SILO_USER_ID_GET_URL: String = format!( + "/system/silos/{}/users/id/{{id}}", + DEFAULT_SILO.identity().name, + ); + pub static ref DEMO_SILO_USER_ID_DELETE_URL: String = format!( + "/system/silos/{}/identity-providers/local/users/{{id}}", DEFAULT_SILO.identity().name, ); - // Organization used for testing pub static ref DEMO_ORG_NAME: Name = "demo-org".parse().unwrap(); @@ -762,11 +763,19 @@ lazy_static! { }, VerifyEndpoint { - url: &*DEMO_SILO_USER_ID_URL, + url: &*DEMO_SILO_USER_ID_GET_URL, visibility: Visibility::Public, unprivileged_access: UnprivilegedAccess::ReadOnly, allowed_methods: vec![ AllowedMethod::Get, + ], + }, + + VerifyEndpoint { + url: &*DEMO_SILO_USER_ID_DELETE_URL, + visibility: Visibility::Public, + unprivileged_access: UnprivilegedAccess::ReadOnly, + allowed_methods: vec![ AllowedMethod::Delete, ], }, diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index d12dba9c21..131fe59f02 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -12,7 +12,8 @@ use omicron_nexus::external_api::views::{ }; use omicron_nexus::external_api::{params, shared}; use omicron_nexus::TestInterfaces as _; -use std::collections::HashSet; +use std::collections::{BTreeMap, HashSet}; +use std::fmt::Write; use std::str::FromStr; use http::method::Method; @@ -29,6 +30,7 @@ use omicron_nexus::authz::{self, SiloRole}; use uuid::Uuid; use httptest::{matchers::*, responders::*, Expectation, Server}; +use omicron_common::api::external::ObjectIdentity; use omicron_nexus::authn::{USER_TEST_PRIVILEGED, USER_TEST_UNPRIVILEGED}; use omicron_nexus::db::fixed_data::silo::SILO_ID; use omicron_nexus::db::identity::Asset; @@ -1471,6 +1473,201 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) { // TODO-coverage were we intending to verify something here? } +/// Tests the behavior of the per-Silo "list users" and "fetch user" endpoints. +/// +/// We'll run the tests separately for both kinds of Silo. The implementation +/// should be the same, but that's why we're verifying it. +#[nexus_test] +async fn test_silo_user_views(cptestctx: &ControlPlaneTestContext) { + let client = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; + + // We use fixed uuids for this test because the sort order is predictable + // and it makes it easier to debug repeated test failures. + let silo1_user1_id = + "1122f0b2-9a92-659b-da6b-93ad4955a3a3".parse().unwrap(); + let silo1_user2_id = + "120600f5-f7f4-e026-e569-ef312c16a7fc".parse().unwrap(); + let silo2_user1_id = + "214b47a9-fe53-41f4-9c08-f89cc9ac5d33".parse().unwrap(); + let silo2_user2_id = + "22d8d84d-8959-cc32-847e-de69fa8ee944".parse().unwrap(); + + // Create the two Silos. + let silo1 = + create_silo(client, "silo1", false, shared::SiloIdentityMode::SamlJit) + .await; + let silo2 = create_silo( + client, + "silo2", + false, + shared::SiloIdentityMode::LocalOnly, + ) + .await; + + // Create two users in each Silo. We need two so that we can verify that an + // ordinary user can see a user other than themselves in each Silo. + let silo1_user1: views::User = nexus + .silo_user_create(silo1.identity.id, silo1_user1_id, "user1".into()) + .await + .unwrap() + .into(); + let silo1_user2: views::User = nexus + .silo_user_create(silo1.identity.id, silo1_user2_id, "user2".into()) + .await + .unwrap() + .into(); + let silo1_expected_users = [silo1_user1.clone(), silo1_user2.clone()]; + let silo2_user1: views::User = nexus + .silo_user_create(silo2.identity.id, silo2_user1_id, "user1".into()) + .await + .unwrap() + .into(); + let silo2_user2: views::User = nexus + .silo_user_create(silo2.identity.id, silo2_user2_id, "user2".into()) + .await + .unwrap() + .into(); + let silo2_expected_users = [silo2_user1.clone(), silo2_user2.clone()]; + + let users_by_id = { + let mut users_by_id: BTreeMap = BTreeMap::new(); + assert_eq!(users_by_id.insert(silo1_user1_id, &silo1_user1), None); + assert_eq!(users_by_id.insert(silo1_user2_id, &silo1_user2), None); + assert_eq!(users_by_id.insert(silo2_user1_id, &silo2_user1), None); + assert_eq!(users_by_id.insert(silo2_user2_id, &silo2_user2), None); + users_by_id + }; + + // We'll run through a battery of tests: + // - for each of our test silos + // - for all *five* users ("test-privileged", plus the two users that we + // created in each Silo) + // - test the "list" endpoint + // - for all five user ids + // - test the "view user" endpoint for that user id + // + // This exercises a lot of different behaviors: + // - on success, the "list" and "view" endpoints always return the right + // contents + // - on failure, the "list" and "view" endpoints always return the right + // status code and message for the failure mode + // - that users can always list and fetch all users in their own Silo via + // /system/silos (/users is tested elsewhere) + // - that users without privileges cannot list or fetch users in other Silos + // - that users with privileges on another Silo can list and fetch users in + // that Silo + // - that a user with id "foo" in Silo1 cannot be accessed by that id in + // Silo 2. This case is easy to miss but would be very bad to get wrong! + let all_callers: Vec = + std::iter::once(AuthnMode::PrivilegedUser) + .chain(users_by_id.keys().map(|k| AuthnMode::SiloUser(*k))) + .collect(); + + struct TestSilo<'a> { + silo: &'a views::Silo, + expected_users: [views::User; 2], + } + + let test_silo1 = + TestSilo { silo: &silo1, expected_users: silo1_expected_users }; + let test_silo2 = + TestSilo { silo: &silo2, expected_users: silo2_expected_users }; + + let mut output = String::new(); + for test_silo in [test_silo1, test_silo2] { + let silo_name = &test_silo.silo.identity().name; + let silo_users_url = + &format!("/system/silos/{}/users", test_silo.silo.identity().name); + + write!(&mut output, "SILO: {}\n", silo_name).unwrap(); + + for calling_user in all_callers.iter() { + write!(&mut output, " test user {:?}:\n", calling_user).unwrap(); + + // Test the "list" endpoint. + write!(&mut output, " list = ").unwrap(); + let test_response = NexusRequest::new(RequestBuilder::new( + client, + Method::GET, + &format!("{}/all", silo_users_url), + )) + .authn_as(calling_user.clone()) + .execute() + .await + .unwrap(); + write!(&mut output, "{}", test_response.status.as_str()).unwrap(); + + // If this succeeded, it must have returned the expected users for + // this Silo. + if test_response.status == http::StatusCode::OK { + let found_users = test_response + .parsed_body::>() + .unwrap() + .items; + assert_eq!(found_users, test_silo.expected_users); + } else { + let error = test_response + .parsed_body::() + .unwrap(); + write!(&mut output, " (message = {:?})", error.message) + .unwrap(); + } + + write!(&mut output, "\n").unwrap(); + + // Test the "view" endpoint for each user in this Silo. + for (user_id, user) in &users_by_id { + let label = if user.silo_id == silo1.identity.id { + format!("silo 1 user {}", user.display_name) + } else { + assert_eq!(user.silo_id, silo2.identity.id); + format!("silo 2 user {}", user.display_name) + }; + write!(&mut output, " view {} ({}) = ", user_id, label,) + .unwrap(); + let test_response = NexusRequest::new(RequestBuilder::new( + client, + Method::GET, + &format!("{}/id/{}", silo_users_url, user_id), + )) + .authn_as(calling_user.clone()) + .execute() + .await + .unwrap(); + write!(&mut output, "{}", test_response.status.as_str()) + .unwrap(); + // If this succeeded, it must have returned the right user back. + if test_response.status == http::StatusCode::OK { + let found_user = + test_response.parsed_body::().unwrap(); + assert_eq!( + found_user.silo_id, + test_silo.silo.identity().id + ); + assert_eq!(found_user, **user); + } else { + let error = test_response + .parsed_body::() + .unwrap(); + write!(&mut output, " (message = {:?})", error.message) + .unwrap(); + } + + write!(&mut output, "\n").unwrap(); + } + + write!(&mut output, "\n").unwrap(); + } + } + + expectorate::assert_contents( + "tests/output/silo-user-views-output.txt", + &output, + ); +} + +/// Tests that LocalOnly-specific endpoints are not available in SamlJit Silos #[nexus_test] async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; @@ -1502,49 +1699,58 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { ) .await; - // They should not be able to create a local-only user in this JIT Silo. - let error: dropshot::HttpErrorResponseBody = - NexusRequest::expect_failure_with_body( - client, - StatusCode::NOT_FOUND, - Method::POST, - "/system/silos/jit/identity-providers/local/users/create", - ¶ms::UserCreate { - external_id: params::UserId::from_str("dummy").unwrap(), - }, - ) - .authn_as(AuthnMode::SiloUser(new_silo_user_id)) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); - assert_eq!( - error.message, - "not found: identity-provider with name \"local\"" - ); + // Neither the "test-privileged" user nor this newly-created admin user + // ought to be able to create a user via the Silo's local identity provider + // (because that provider does not exist). + for caller in + [AuthnMode::PrivilegedUser, AuthnMode::SiloUser(new_silo_user_id)] + { + let error: dropshot::HttpErrorResponseBody = + NexusRequest::expect_failure_with_body( + client, + StatusCode::NOT_FOUND, + Method::POST, + "/system/silos/jit/identity-providers/local/users", + ¶ms::UserCreate { + external_id: params::UserId::from_str("dummy").unwrap(), + }, + ) + .authn_as(caller) + .execute() + .await + .unwrap() + .parsed_body() + .unwrap(); + assert_eq!( + error.message, + "not found: identity-provider with name \"local\"" + ); + } - // Suppose we do create a user (as via JIT). They should not be able to - // view or delete them via the local identity provider. + // Now create another user, as might happen via JIT. let other_user_id = "57372ebb-ee76-4a2d-fa3e-e1875a8d11c0".parse().unwrap(); let _ = nexus .silo_user_create(silo.identity.id, other_user_id, "other-user".into()) .await .unwrap(); - let other_user_url = format!( - "/system/silos/jit/identity-providers/local/users/id/{}", + let user_url_delete = format!( + "/system/silos/jit/identity-providers/local/users/{}", other_user_id ); - for method in [Method::GET, Method::DELETE] { + // Neither the "test-privileged" user nor the Silo Admin ought to be able to + // remove this user via the local identity provider. + for caller in + [AuthnMode::PrivilegedUser, AuthnMode::SiloUser(new_silo_user_id)] + { let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure( client, StatusCode::NOT_FOUND, - method, - &other_user_url, + Method::DELETE, + &user_url_delete, ) - .authn_as(AuthnMode::SiloUser(new_silo_user_id)) + .authn_as(caller) .execute() .await .unwrap() @@ -1555,26 +1761,9 @@ async fn test_jit_silo_constraints(cptestctx: &ControlPlaneTestContext) { "not found: identity-provider with name \"local\"" ); } - - // They should also not be able to list users via the "local" IdP endpoint. - let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure( - client, - StatusCode::NOT_FOUND, - Method::GET, - "/system/silos/jit/identity-providers/local/users/all", - ) - .authn_as(AuthnMode::SiloUser(new_silo_user_id)) - .execute() - .await - .unwrap() - .parsed_body() - .unwrap(); - assert_eq!( - error.message, - "not found: identity-provider with name \"local\"" - ); } +/// Tests that SamlJit-specific endpoints are not available in LocalOnly Silos #[nexus_test] async fn test_local_silo_constraints(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; @@ -1607,7 +1796,7 @@ async fn test_local_silo_constraints(cptestctx: &ControlPlaneTestContext) { ) .await; - // It's not allowed to create an identity provider in an ApiOnly Silo. + // It's not allowed to create an identity provider in a LocalOnly Silo. let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure_with_body( client, @@ -1720,95 +1909,6 @@ async fn test_local_silo_users(cptestctx: &ControlPlaneTestContext) { &[admin_user.clone()], ) .await; - - // Now create a second Silo and make sure that none of the per-user - // endpoints can be accessed using a user id from the first Silo. - // (Internally, it's possible to look up a user independent of the Silo - // they're in, so this check ensures that we do actually limit results to - // the appropriate Silo.) - let silo2 = create_silo( - &client, - "silo2", - true, - shared::SiloIdentityMode::LocalOnly, - ) - .await; - let silo1_users_url = format!( - "/system/silos/{}/identity-providers/local/users", - silo1.identity.name, - ); - let silo1_user_url = format!("{}/id/{}", silo1_users_url, new_silo_user_id); - let silo2_users_url = format!( - "/system/silos/{}/identity-providers/local/users", - silo2.identity.name, - ); - let silo2_user_url = format!("{}/id/{}", silo2_users_url, new_silo_user_id); - // Double check that we have the right URL form in the first place. We - // should be able to fetch the user in silo1. - let user = NexusRequest::object_get(client, &silo1_user_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body::() - .unwrap(); - assert_eq!(user.id, new_silo_user_id); - - for method in [Method::GET, Method::DELETE] { - let error = NexusRequest::expect_failure( - client, - StatusCode::NOT_FOUND, - method, - &silo2_user_url, - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body::() - .unwrap(); - let not_found_message = - format!("not found: silo-user with id \"{}\"", new_silo_user_id); - assert_eq!(error.message, not_found_message); - } - - // Make sure that the list of users in each Silo are different, too. - let silo1_users = - NexusRequest::object_get(client, &format!("{}/all", silo1_users_url)) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body::>() - .unwrap() - .items; - assert_eq!(silo1_users, &[admin_user.clone()]); - - let silo2_users_list_url = format!("{}/all", silo2_users_url); - let silo2_users = NexusRequest::object_get(client, &silo2_users_list_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .unwrap() - .parsed_body::>() - .unwrap() - .items; - assert!(silo2_users.is_empty()); - - // Make sure that the admin in Silo1 cannot list users in Silo2. - let error = NexusRequest::expect_failure( - client, - StatusCode::NOT_FOUND, - Method::GET, - &silo2_users_list_url, - ) - .authn_as(AuthnMode::SiloUser(new_silo_user_id)) - .execute() - .await - .unwrap() - .parsed_body::() - .unwrap(); - assert_eq!(error.message, "not found: silo with name \"silo2\""); } /// Runs a sequence of tests for create, read, and delete of API-managed users @@ -1818,12 +1918,13 @@ async fn run_user_tests( authn_mode: &AuthnMode, existing_users: &[views::User], ) { + let url_all_users = + format!("/system/silos/{}/users/all", silo.identity.name); let url_local_idp_users = format!( "/system/silos/{}/identity-providers/local/users", silo.identity.name ); - let url_all_users = format!("{}/all", url_local_idp_users); - let url_user_create = format!("{}/create", url_local_idp_users); + let url_user_create = format!("{}", url_local_idp_users); // Fetch users and verify it matches what the caller expects. println!("run_user_tests: as {:?}: fetch all users", authn_mode); @@ -1856,8 +1957,11 @@ async fn run_user_tests( println!("created user: {:?}", user_created); // Fetch the user we just created. - let user_url = format!("{}/id/{}", url_local_idp_users, user_created.id); - let user_found = NexusRequest::object_get(client, &user_url) + let user_url_get = format!( + "/system/silos/{}/users/id/{}", + silo.identity.name, user_created.id + ); + let user_found = NexusRequest::object_get(client, &user_url_get) .authn_as(authn_mode.clone()) .execute() .await @@ -1883,7 +1987,11 @@ async fn run_user_tests( assert_eq!(new_users, &[&user_created]); // Delete the user that we created. - NexusRequest::object_delete(client, &user_url) + let user_url_delete = format!( + "/system/silos/{}/identity-providers/local/users/{}", + silo.identity.name, user_created.id + ); + NexusRequest::object_delete(client, &user_url_delete) .authn_as(authn_mode.clone()) .execute() .await @@ -1891,11 +1999,16 @@ async fn run_user_tests( // We should not be able to fetch or delete the user again. for method in [Method::GET, Method::DELETE] { + let url = if method == Method::GET { + &user_url_get + } else { + &user_url_delete + }; let error = NexusRequest::expect_failure( client, StatusCode::NOT_FOUND, method, - &user_url, + url, ) .authn_as(authn_mode.clone()) .execute() diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index 68f4995439..1ff3105a5c 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -193,7 +193,8 @@ lazy_static! { url: &*DEMO_SILO_USERS_CREATE_URL, body: serde_json::to_value(&*DEMO_USER_CREATE).unwrap(), id_routes: vec![ - &*DEMO_SILO_USER_ID_URL, + &*DEMO_SILO_USER_ID_GET_URL, + &*DEMO_SILO_USER_ID_DELETE_URL, ], }, // Create an IP pool diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 5c036ca019..b3ebd936b0 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -125,10 +125,8 @@ ip_pool_service_view /system/ip-pools-service/{rack_id} ip_pool_update /system/ip-pools/{pool_name} ip_pool_view /system/ip-pools/{pool_name} ip_pool_view_by_id /system/by-id/ip-pools/{id} -local_idp_user_create /system/silos/{silo_name}/identity-providers/local/users/create -local_idp_user_delete /system/silos/{silo_name}/identity-providers/local/users/id/{user_id} -local_idp_user_view /system/silos/{silo_name}/identity-providers/local/users/id/{user_id} -local_idp_users_list /system/silos/{silo_name}/identity-providers/local/users/all +local_idp_user_create /system/silos/{silo_name}/identity-providers/local/users +local_idp_user_delete /system/silos/{silo_name}/identity-providers/local/users/{user_id} rack_list /system/hardware/racks rack_view /system/hardware/racks/{rack_id} saga_list /system/sagas @@ -141,6 +139,8 @@ silo_identity_provider_list /system/silos/{silo_name}/identity-prov silo_list /system/silos silo_policy_update /system/silos/{silo_name}/policy silo_policy_view /system/silos/{silo_name}/policy +silo_user_view /system/silos/{silo_name}/users/id/{user_id} +silo_users_list /system/silos/{silo_name}/users/all silo_view /system/silos/{silo_name} silo_view_by_id /system/by-id/silos/{id} sled_list /system/hardware/sleds diff --git a/nexus/tests/output/silo-user-views-output.txt b/nexus/tests/output/silo-user-views-output.txt new file mode 100644 index 0000000000..d484fa85a8 --- /dev/null +++ b/nexus/tests/output/silo-user-views-output.txt @@ -0,0 +1,72 @@ +SILO: silo1 + test user PrivilegedUser: + list = 200 + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 200 + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 200 + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 404 (message = "not found: silo-user with id \"214b47a9-fe53-41f4-9c08-f89cc9ac5d33\"") + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 404 (message = "not found: silo-user with id \"22d8d84d-8959-cc32-847e-de69fa8ee944\"") + + test user SiloUser(1122f0b2-9a92-659b-da6b-93ad4955a3a3): + list = 200 + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 200 + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 200 + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 404 (message = "not found: silo-user with id \"214b47a9-fe53-41f4-9c08-f89cc9ac5d33\"") + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 404 (message = "not found: silo-user with id \"22d8d84d-8959-cc32-847e-de69fa8ee944\"") + + test user SiloUser(120600f5-f7f4-e026-e569-ef312c16a7fc): + list = 200 + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 200 + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 200 + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 404 (message = "not found: silo-user with id \"214b47a9-fe53-41f4-9c08-f89cc9ac5d33\"") + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 404 (message = "not found: silo-user with id \"22d8d84d-8959-cc32-847e-de69fa8ee944\"") + + test user SiloUser(214b47a9-fe53-41f4-9c08-f89cc9ac5d33): + list = 404 (message = "not found: silo with name \"silo1\"") + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 404 (message = "not found: silo with name \"silo1\"") + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 404 (message = "not found: silo with name \"silo1\"") + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 404 (message = "not found: silo with name \"silo1\"") + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 404 (message = "not found: silo with name \"silo1\"") + + test user SiloUser(22d8d84d-8959-cc32-847e-de69fa8ee944): + list = 404 (message = "not found: silo with name \"silo1\"") + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 404 (message = "not found: silo with name \"silo1\"") + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 404 (message = "not found: silo with name \"silo1\"") + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 404 (message = "not found: silo with name \"silo1\"") + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 404 (message = "not found: silo with name \"silo1\"") + +SILO: silo2 + test user PrivilegedUser: + list = 200 + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 404 (message = "not found: silo-user with id \"1122f0b2-9a92-659b-da6b-93ad4955a3a3\"") + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 404 (message = "not found: silo-user with id \"120600f5-f7f4-e026-e569-ef312c16a7fc\"") + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 200 + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 200 + + test user SiloUser(1122f0b2-9a92-659b-da6b-93ad4955a3a3): + list = 404 (message = "not found: silo with name \"silo2\"") + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 404 (message = "not found: silo with name \"silo2\"") + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 404 (message = "not found: silo with name \"silo2\"") + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 404 (message = "not found: silo with name \"silo2\"") + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 404 (message = "not found: silo with name \"silo2\"") + + test user SiloUser(120600f5-f7f4-e026-e569-ef312c16a7fc): + list = 404 (message = "not found: silo with name \"silo2\"") + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 404 (message = "not found: silo with name \"silo2\"") + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 404 (message = "not found: silo with name \"silo2\"") + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 404 (message = "not found: silo with name \"silo2\"") + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 404 (message = "not found: silo with name \"silo2\"") + + test user SiloUser(214b47a9-fe53-41f4-9c08-f89cc9ac5d33): + list = 200 + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 404 (message = "not found: silo-user with id \"1122f0b2-9a92-659b-da6b-93ad4955a3a3\"") + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 404 (message = "not found: silo-user with id \"120600f5-f7f4-e026-e569-ef312c16a7fc\"") + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 200 + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 200 + + test user SiloUser(22d8d84d-8959-cc32-847e-de69fa8ee944): + list = 200 + view 1122f0b2-9a92-659b-da6b-93ad4955a3a3 (silo 1 user user1) = 404 (message = "not found: silo-user with id \"1122f0b2-9a92-659b-da6b-93ad4955a3a3\"") + view 120600f5-f7f4-e026-e569-ef312c16a7fc (silo 1 user user2) = 404 (message = "not found: silo-user with id \"120600f5-f7f4-e026-e569-ef312c16a7fc\"") + view 214b47a9-fe53-41f4-9c08-f89cc9ac5d33 (silo 2 user user1) = 200 + view 22d8d84d-8959-cc32-847e-de69fa8ee944 (silo 2 user user2) = 200 + diff --git a/openapi/nexus.json b/openapi/nexus.json index 4ba86d08bb..da446dbe96 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -6815,77 +6815,7 @@ "x-dropshot-pagination": true } }, - "/system/silos/{silo_name}/identity-providers/local/users/all": { - "get": { - "tags": [ - "system" - ], - "summary": "List users in the given local identity provider", - "operationId": "local_idp_users_list", - "parameters": [ - { - "in": "path", - "name": "silo_name", - "description": "The silo's unique name.", - "required": true, - "schema": { - "$ref": "#/components/schemas/Name" - }, - "style": "simple" - }, - { - "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/UserResultsPage" - } - } - } - }, - "4XX": { - "$ref": "#/components/responses/Error" - }, - "5XX": { - "$ref": "#/components/responses/Error" - } - }, - "x-dropshot-pagination": true - } - }, - "/system/silos/{silo_name}/identity-providers/local/users/create": { + "/system/silos/{silo_name}/identity-providers/local/users": { "post": { "tags": [ "system" @@ -6935,54 +6865,7 @@ } } }, - "/system/silos/{silo_name}/identity-providers/local/users/id/{user_id}": { - "get": { - "tags": [ - "system" - ], - "operationId": "local_idp_user_view", - "parameters": [ - { - "in": "path", - "name": "silo_name", - "description": "The silo's unique name.", - "required": true, - "schema": { - "$ref": "#/components/schemas/Name" - }, - "style": "simple" - }, - { - "in": "path", - "name": "user_id", - "description": "The user's internal id", - "required": true, - "schema": { - "type": "string", - "format": "uuid" - }, - "style": "simple" - } - ], - "responses": { - "200": { - "description": "successful operation", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/User" - } - } - } - }, - "4XX": { - "$ref": "#/components/responses/Error" - }, - "5XX": { - "$ref": "#/components/responses/Error" - } - } - }, + "/system/silos/{silo_name}/identity-providers/local/users/{user_id}": { "delete": { "tags": [ "system" @@ -7208,6 +7091,125 @@ } } }, + "/system/silos/{silo_name}/users/all": { + "get": { + "tags": [ + "system" + ], + "summary": "List users in a specific Silo", + "operationId": "silo_users_list", + "parameters": [ + { + "in": "path", + "name": "silo_name", + "description": "The silo's unique name.", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "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/UserResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": true + } + }, + "/system/silos/{silo_name}/users/id/{user_id}": { + "get": { + "tags": [ + "system" + ], + "operationId": "silo_user_view", + "parameters": [ + { + "in": "path", + "name": "silo_name", + "description": "The silo's unique name.", + "required": true, + "schema": { + "$ref": "#/components/schemas/Name" + }, + "style": "simple" + }, + { + "in": "path", + "name": "user_id", + "description": "The user's internal id", + "required": true, + "schema": { + "type": "string", + "format": "uuid" + }, + "style": "simple" + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/User" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + } + } + }, "/system/updates/refresh": { "post": { "tags": [