diff --git a/nexus/db-fixed-data/src/silo_user.rs b/nexus/db-fixed-data/src/silo_user.rs index 38252826f37..b46fca7ca99 100644 --- a/nexus/db-fixed-data/src/silo_user.rs +++ b/nexus/db-fixed-data/src/silo_user.rs @@ -4,7 +4,8 @@ //! Built-in Silo Users use nexus_db_model as model; -use nexus_types::{identity::Asset, silo::DEFAULT_SILO_ID}; +use nexus_types::identity::Asset; +use nexus_types::silo::DEFAULT_SILO_ID; use omicron_common::api::external::ResourceType; use std::sync::LazyLock; @@ -15,7 +16,7 @@ use std::sync::LazyLock; // not automatically at Nexus startup. See omicron#2305. pub static USER_TEST_PRIVILEGED: LazyLock = LazyLock::new(|| { - model::SiloUser::new( + model::SiloUser::new_api_only_user( DEFAULT_SILO_ID, // "4007" looks a bit like "root". "001de000-05e4-4000-8000-000000004007".parse().unwrap(), @@ -50,7 +51,7 @@ pub static ROLE_ASSIGNMENTS_PRIVILEGED: LazyLock> = // not automatically at Nexus startup. See omicron#2305. pub static USER_TEST_UNPRIVILEGED: LazyLock = LazyLock::new(|| { - model::SiloUser::new( + model::SiloUser::new_api_only_user( DEFAULT_SILO_ID, // 60001 is the decimal uid for "nobody" on Helios. "001de000-05e4-4000-8000-000000060001".parse().unwrap(), diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 36cee81bea7..49e0f9b84d3 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(195, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(196, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(196, "user-provision-type-for-silo-user-and-group"), KnownVersion::new(195, "tuf-pruned-index"), KnownVersion::new(194, "tuf-pruned"), KnownVersion::new(193, "nexus-lockstep-port"), diff --git a/nexus/db-model/src/silo_group.rs b/nexus/db-model/src/silo_group.rs index 384b74edf53..793eb3ef921 100644 --- a/nexus/db-model/src/silo_group.rs +++ b/nexus/db-model/src/silo_group.rs @@ -2,12 +2,11 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use super::UserProvisionType; use crate::DbTypedUuid; use crate::to_db_typed_uuid; use db_macros::Asset; use nexus_db_schema::schema::{silo_group, silo_group_membership}; -use nexus_types::external_api::views; -use nexus_types::identity::Asset; use omicron_uuid_kinds::SiloGroupKind; use omicron_uuid_kinds::SiloGroupUuid; use omicron_uuid_kinds::SiloUserKind; @@ -20,17 +19,50 @@ use uuid::Uuid; #[asset(uuid_kind = SiloGroupKind)] pub struct SiloGroup { #[diesel(embed)] - identity: SiloGroupIdentity, + pub identity: SiloGroupIdentity, + pub time_deleted: Option>, pub silo_id: Uuid, - /// The identity provider's name for this group. - pub external_id: String, + /// If the user provision type is ApiOnly or JIT, then the external id is + /// the identity provider's ID for this group. There is a database + /// constraint (`lookup_silo_group_by_silo`) that ensures this field must be + /// non-null for those provision types. + /// + /// For SCIM, this may be null, which would trigger the uniqueness + /// constraint if that wasn't limited to specific provision types. + pub external_id: Option, + + pub user_provision_type: UserProvisionType, } impl SiloGroup { - pub fn new(id: SiloGroupUuid, silo_id: Uuid, external_id: String) -> Self { - Self { identity: SiloGroupIdentity::new(id), silo_id, external_id } + pub fn new_api_only_group( + id: SiloGroupUuid, + silo_id: Uuid, + external_id: String, + ) -> Self { + Self { + identity: SiloGroupIdentity::new(id), + time_deleted: None, + silo_id, + user_provision_type: UserProvisionType::ApiOnly, + external_id: Some(external_id), + } + } + + pub fn new_jit_group( + id: SiloGroupUuid, + silo_id: Uuid, + external_id: String, + ) -> Self { + Self { + identity: SiloGroupIdentity::new(id), + time_deleted: None, + silo_id, + user_provision_type: UserProvisionType::Jit, + external_id: Some(external_id), + } } } @@ -53,14 +85,3 @@ impl SiloGroupMembership { } } } - -impl From for views::Group { - fn from(group: SiloGroup) -> Self { - Self { - id: group.id(), - // TODO the use of external_id as display_name is temporary - display_name: group.external_id, - silo_id: group.silo_id, - } - } -} diff --git a/nexus/db-model/src/silo_user.rs b/nexus/db-model/src/silo_user.rs index bcf31aeff48..7ff962244bc 100644 --- a/nexus/db-model/src/silo_user.rs +++ b/nexus/db-model/src/silo_user.rs @@ -2,10 +2,9 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use super::UserProvisionType; use db_macros::Asset; use nexus_db_schema::schema::silo_user; -use nexus_types::external_api::views; -use nexus_types::identity::Asset; use omicron_uuid_kinds::SiloUserUuid; use uuid::Uuid; @@ -15,17 +14,25 @@ use uuid::Uuid; #[asset(uuid_kind = SiloUserKind)] pub struct SiloUser { #[diesel(embed)] - identity: SiloUserIdentity, + pub identity: SiloUserIdentity, pub time_deleted: Option>, pub silo_id: Uuid, - /// The identity provider's ID for this user. - pub external_id: String, + /// If the user provision type is ApiOnly or JIT, then the external id is + /// the identity provider's ID for this user. There is a database constraint + /// (`lookup_silo_user_by_silo`) that ensures this field must be non-null + /// for those provision types. + /// + /// For SCIM, this may be null, which would trigger the uniqueness + /// constraint if that wasn't limited to specific provision types. + pub external_id: Option, + + pub user_provision_type: UserProvisionType, } impl SiloUser { - pub fn new( + pub fn new_api_only_user( silo_id: Uuid, user_id: SiloUserUuid, external_id: String, @@ -34,18 +41,22 @@ impl SiloUser { identity: SiloUserIdentity::new(user_id), time_deleted: None, silo_id, - external_id, + external_id: Some(external_id), + user_provision_type: UserProvisionType::ApiOnly, } } -} -impl From for views::User { - fn from(user: SiloUser) -> Self { + pub fn new_jit_user( + silo_id: Uuid, + user_id: SiloUserUuid, + external_id: String, + ) -> Self { Self { - id: user.id(), - // TODO the use of external_id as display_name is temporary - display_name: user.external_id, - silo_id: user.silo_id, + identity: SiloUserIdentity::new(user_id), + time_deleted: None, + silo_id, + external_id: Some(external_id), + user_provision_type: UserProvisionType::Jit, } } } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 6180ee8fb0b..eaff385e7e1 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -143,6 +143,14 @@ pub use region::RegionAllocationParameters; pub use region_snapshot_replacement::NewRegionVolumeId; pub use region_snapshot_replacement::OldSnapshotVolumeId; pub use silo::Discoverability; +pub use silo_group::SiloGroup; +pub use silo_group::SiloGroupApiOnly; +pub use silo_group::SiloGroupJit; +pub use silo_group::SiloGroupLookup; +pub use silo_user::SiloUser; +pub use silo_user::SiloUserApiOnly; +pub use silo_user::SiloUserJit; +pub use silo_user::SiloUserLookup; pub use sled::SledTransition; pub use sled::TransitionError; pub use support_bundle::SupportBundleExpungementReport; @@ -577,7 +585,7 @@ mod test { use crate::db::model::{ BlockSize, ConsoleSession, CrucibleDataset, ExternalIp, PhysicalDisk, PhysicalDiskKind, PhysicalDiskPolicy, PhysicalDiskState, Project, Rack, - Region, SiloUser, SshKey, Zpool, + Region, SshKey, Zpool, }; use crate::db::pub_test_utils::TestDatabase; use crate::db::pub_test_utils::helpers::SledUpdateBuilder; @@ -716,11 +724,12 @@ mod test { datastore .silo_user_create( &authz_silo, - SiloUser::new( + SiloUserApiOnly::new( authz_silo.id(), silo_user_id, "external_id".into(), - ), + ) + .into(), ) .await .unwrap(); @@ -1856,11 +1865,12 @@ mod test { datastore .silo_user_create( &authz_silo, - SiloUser::new( + SiloUserApiOnly::new( authz_silo.id(), silo_user_id, "external@id".into(), - ), + ) + .into(), ) .await .unwrap(); diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index e4e559ce755..d7f8e37e9ec 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -19,6 +19,7 @@ use crate::db::model::CrucibleDataset; use crate::db::model::IncompleteExternalIp; use crate::db::model::PhysicalDisk; use crate::db::model::Rack; +use crate::db::model::UserProvisionType; use crate::db::model::Zpool; use crate::db::pagination::paginated; use async_bb8_diesel::AsyncRunQueryDsl; @@ -442,6 +443,15 @@ impl DataStore { recovery_user_password_hash: omicron_passwords::PasswordHashString, dns_update: DnsVersionUpdateBuilder, ) -> Result<(), RackInitError> { + if !matches!( + &recovery_silo.identity_mode, + shared::SiloIdentityMode::LocalOnly + ) { + return Err(RackInitError::Silo(Error::invalid_request( + "recovery silo should only use identity mode LocalOnly", + ))); + } + let db_silo = self .silo_create_conn( conn, @@ -460,11 +470,19 @@ impl DataStore { // Create the first user in the initial Recovery Silo let silo_user_id = SiloUserUuid::new_v4(); - let silo_user = SiloUser::new( - db_silo.id(), - silo_user_id, - recovery_user_id.as_ref().to_owned(), - ); + + let silo_user = match &db_silo.user_provision_type { + UserProvisionType::ApiOnly => SiloUser::new_api_only_user( + db_silo.id(), + silo_user_id, + recovery_user_id.as_ref().to_owned(), + ), + + UserProvisionType::Jit => { + unreachable!("match at start of function should prevent this"); + } + }; + { use nexus_db_schema::schema::silo_user::dsl; diesel::insert_into(dsl::silo_user) @@ -1250,7 +1268,7 @@ mod test { .expect("failed to list users"); assert_eq!(silo_users.len(), 1); assert_eq!( - silo_users[0].external_id, + silo_users[0].external_id(), rack_init.recovery_user_id.as_ref() ); let authz_silo_user = authz::SiloUser::new( diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index a1babdd62f3..f282d75587d 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -192,15 +192,30 @@ impl DataStore { let silo_admin_group_ensure_query = if let Some(ref admin_group_name) = new_silo_params.admin_group_name { + let silo_admin_group = + match new_silo_params.identity_mode.user_provision_type() { + shared::UserProvisionType::ApiOnly => { + db::model::SiloGroup::new_api_only_group( + silo_group_id, + silo_id, + admin_group_name.clone(), + ) + } + + shared::UserProvisionType::Jit => { + db::model::SiloGroup::new_jit_group( + silo_group_id, + silo_id, + admin_group_name.clone(), + ) + } + }; + let silo_admin_group_ensure_query = DataStore::silo_group_ensure_query( &nexus_opctx, &authz_silo, - db::model::SiloGroup::new( - silo_group_id, - silo_id, - admin_group_name.clone(), - ), + silo_admin_group, ) .await?; @@ -505,6 +520,10 @@ impl DataStore { silo_user::dsl::silo_user .filter(silo_user::dsl::silo_id.eq(id)) .filter(silo_user::dsl::time_deleted.is_null()) + .filter( + silo_user::dsl::user_provision_type + .eq(db_silo.user_provision_type), + ) .select(silo_user::dsl::id), ), ) @@ -520,6 +539,10 @@ impl DataStore { let updated_rows = diesel::update(silo_user::dsl::silo_user) .filter(silo_user::dsl::silo_id.eq(id)) .filter(silo_user::dsl::time_deleted.is_null()) + .filter( + silo_user::dsl::user_provision_type + .eq(db_silo.user_provision_type), + ) .set(silo_user::dsl::time_deleted.eq(now)) .execute_async(&*conn) .await @@ -538,6 +561,10 @@ impl DataStore { silo_group::dsl::silo_group .filter(silo_group::dsl::silo_id.eq(id)) .filter(silo_group::dsl::time_deleted.is_null()) + .filter( + silo_group::dsl::user_provision_type + .eq(db_silo.user_provision_type), + ) .select(silo_group::dsl::id), ), ) @@ -556,6 +583,10 @@ impl DataStore { let updated_rows = diesel::update(silo_group::dsl::silo_group) .filter(silo_group::dsl::silo_id.eq(id)) .filter(silo_group::dsl::time_deleted.is_null()) + .filter( + silo_group::dsl::user_provision_type + .eq(db_silo.user_provision_type), + ) .set(silo_group::dsl::time_deleted.eq(now)) .execute_async(&*conn) .await diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index eb8f2438d35..7c185202bdf 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -7,19 +7,23 @@ use super::DataStore; use crate::authz; use crate::context::OpContext; -use crate::db; use crate::db::IncompleteOnConflictExt; use crate::db::datastore::RunnableQueryNoReturn; -use crate::db::model::SiloGroup; +use crate::db::model; +use crate::db::model::Silo; use crate::db::model::SiloGroupMembership; +use crate::db::model::UserProvisionType; use crate::db::model::to_db_typed_uuid; use crate::db::pagination::paginated; use async_bb8_diesel::AsyncRunQueryDsl; +use chrono::DateTime; use chrono::Utc; use diesel::prelude::*; use nexus_db_errors::ErrorHandler; use nexus_db_errors::TransactionError; use nexus_db_errors::public_error_from_diesel; +use nexus_types::external_api::views; +use nexus_types::identity::Asset; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; @@ -27,16 +31,242 @@ use omicron_common::api::external::Error; use omicron_common::api::external::InternalContext; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; +use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; +use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::SiloGroupUuid; use omicron_uuid_kinds::SiloUserUuid; use uuid::Uuid; +/// The datastore crate's SiloGroup is intended to provide type safety above the +/// database model, as the same database model is used to store semantically +/// different group types. Higher level parts of Nexus should use this type +/// instead. +#[derive(Debug, Clone)] +pub enum SiloGroup { + /// A Group created via the API + ApiOnly(SiloGroupApiOnly), + + /// A Group created during an authenticated SAML login + Jit(SiloGroupJit), +} + +impl SiloGroup { + pub fn id(&self) -> SiloGroupUuid { + match &self { + SiloGroup::ApiOnly(u) => u.id, + SiloGroup::Jit(u) => u.id, + } + } + + pub fn silo_id(&self) -> Uuid { + match &self { + SiloGroup::ApiOnly(u) => u.silo_id, + SiloGroup::Jit(u) => u.silo_id, + } + } + + pub fn external_id(&self) -> &str { + match &self { + SiloGroup::ApiOnly(u) => &u.external_id, + SiloGroup::Jit(u) => &u.external_id, + } + } +} + +impl From for SiloGroup { + fn from(record: model::SiloGroup) -> SiloGroup { + match record.user_provision_type { + UserProvisionType::ApiOnly => { + SiloGroup::ApiOnly(SiloGroupApiOnly { + id: record.id(), + time_created: record.time_created(), + time_modified: record.time_modified(), + time_deleted: record.time_deleted, + silo_id: record.silo_id, + // About the expect here: the mentioned database constraint + // prevents a group with provision type 'api_only' from + // having a null external id. + external_id: record + .external_id + .expect("constraint lookup_silo_group_by_silo exists"), + }) + } + + UserProvisionType::Jit => SiloGroup::Jit(SiloGroupJit { + id: record.id(), + time_created: record.time_created(), + time_modified: record.time_modified(), + time_deleted: record.time_deleted, + silo_id: record.silo_id, + // About the expect here: the mentioned database constraint + // prevents a group with provision type 'jit' from having a null + // external id + external_id: record + .external_id + .expect("constraint lookup_silo_group_by_silo exists"), + }), + } + } +} + +impl From for model::SiloGroup { + fn from(u: SiloGroup) -> model::SiloGroup { + match u { + SiloGroup::ApiOnly(u) => u.into(), + SiloGroup::Jit(u) => u.into(), + } + } +} + +impl From for views::Group { + fn from(u: SiloGroup) -> views::Group { + match u { + SiloGroup::ApiOnly(u) => u.into(), + SiloGroup::Jit(u) => u.into(), + } + } +} + +#[derive(Debug, Clone)] +pub struct SiloGroupApiOnly { + pub id: SiloGroupUuid, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, + pub silo_id: Uuid, + + /// The identity provider's ID for this group. + pub external_id: String, +} + +impl SiloGroupApiOnly { + pub fn new(silo_id: Uuid, id: SiloGroupUuid, external_id: String) -> Self { + Self { + id, + time_created: Utc::now(), + time_modified: Utc::now(), + time_deleted: None, + silo_id, + external_id, + } + } +} + +impl From for model::SiloGroup { + fn from(u: SiloGroupApiOnly) -> model::SiloGroup { + model::SiloGroup { + identity: model::SiloGroupIdentity { + id: u.id.into(), + time_created: u.time_created, + time_modified: u.time_modified, + }, + time_deleted: u.time_deleted, + silo_id: u.silo_id, + external_id: Some(u.external_id), + user_provision_type: UserProvisionType::ApiOnly, + } + } +} + +impl From for SiloGroup { + fn from(u: SiloGroupApiOnly) -> SiloGroup { + SiloGroup::ApiOnly(u) + } +} + +impl From for views::Group { + fn from(u: SiloGroupApiOnly) -> views::Group { + views::Group { + id: u.id, + // TODO the use of external_id as display_name is temporary + display_name: u.external_id, + silo_id: u.silo_id, + } + } +} + +#[derive(Debug, Clone)] +pub struct SiloGroupJit { + pub id: SiloGroupUuid, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, + pub silo_id: Uuid, + + /// The identity provider's ID for this user. + pub external_id: String, +} + +impl SiloGroupJit { + pub fn new(silo_id: Uuid, id: SiloGroupUuid, external_id: String) -> Self { + Self { + id, + time_created: Utc::now(), + time_modified: Utc::now(), + time_deleted: None, + silo_id, + external_id, + } + } +} + +impl From for model::SiloGroup { + fn from(u: SiloGroupJit) -> model::SiloGroup { + model::SiloGroup { + identity: model::SiloGroupIdentity { + id: u.id.into(), + time_created: u.time_created, + time_modified: u.time_modified, + }, + time_deleted: u.time_deleted, + silo_id: u.silo_id, + external_id: Some(u.external_id), + user_provision_type: UserProvisionType::Jit, + } + } +} + +impl From for SiloGroup { + fn from(u: SiloGroupJit) -> SiloGroup { + SiloGroup::Jit(u) + } +} + +impl From for views::Group { + fn from(u: SiloGroupJit) -> views::Group { + views::Group { + id: u.id, + // TODO the use of external_id as display_name is temporary + display_name: u.external_id, + silo_id: u.silo_id, + } + } +} + +/// Different types of group have different fields that are considered unique, +/// and therefore lookup needs to be typed as well. +#[derive(Debug)] +pub enum SiloGroupLookup<'a> { + ApiOnly { external_id: &'a str }, + + Jit { external_id: &'a str }, +} + +impl<'a> SiloGroupLookup<'a> { + pub fn user_provision_type(&self) -> UserProvisionType { + match &self { + SiloGroupLookup::ApiOnly { .. } => UserProvisionType::ApiOnly, + SiloGroupLookup::Jit { .. } => UserProvisionType::Jit, + } + } +} + impl DataStore { pub(super) async fn silo_group_ensure_query( opctx: &OpContext, authz_silo: &authz::Silo, - silo_group: SiloGroup, + silo_group: model::SiloGroup, ) -> Result { opctx.authorize(authz::Action::CreateChild, authz_silo).await?; @@ -54,39 +284,146 @@ impl DataStore { authz_silo: &authz::Silo, silo_group: SiloGroup, ) -> CreateResult { - let external_id = silo_group.external_id.clone(); + let conn = self.pool_connection_authorized(opctx).await?; + + let group_id = silo_group.id(); + let model: model::SiloGroup = silo_group.into(); + + // Check the user's provision type matches the silo + + let silo = { + use nexus_db_schema::schema::silo::dsl; + dsl::silo + .filter(dsl::id.eq(authz_silo.id())) + .select(model::Silo::as_select()) + .get_result_async::(&*conn) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource(authz_silo), + ) + })? + }; - DataStore::silo_group_ensure_query(opctx, authz_silo, silo_group) + if silo.user_provision_type != model.user_provision_type { + error!( + self.log, + "silo {} provision type {:?} does not match silo group {} \ + provision type {:?}", + authz_silo.id(), + silo.user_provision_type, + model.id(), + model.user_provision_type, + ); + + return Err(Error::invalid_request( + "user provision type of silo does not match silo group", + )); + } + + DataStore::silo_group_ensure_query(opctx, authz_silo, model) .await? - .execute_async(&*self.pool_connection_authorized(opctx).await?) + .execute_async(&*conn) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - Ok(self - .silo_group_optional_lookup(opctx, authz_silo, external_id) - .await? - .unwrap()) + let silo_group = { + use nexus_db_schema::schema::silo_group::dsl; + + let maybe_silo_group = dsl::silo_group + .filter(dsl::silo_id.eq(authz_silo.id())) + .filter(dsl::id.eq(to_db_typed_uuid(group_id))) + .select(model::SiloGroup::as_select()) + .first_async(&*conn) + .await + .optional() + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + // This function is _not_ transactional, meaning a delete could race + // between the ensure query and the lookup. + + let Some(silo_group) = maybe_silo_group else { + return Err(Error::not_found_by_id( + ResourceType::SiloGroup, + group_id.as_untyped_uuid(), + )); + }; + + silo_group + }; + + Ok(silo_group.into()) } - pub async fn silo_group_optional_lookup( + pub async fn silo_group_optional_lookup<'a>( &self, opctx: &OpContext, authz_silo: &authz::Silo, - external_id: String, - ) -> LookupResult> { + silo_group_lookup: &'a SiloGroupLookup<'a>, + ) -> LookupResult> { opctx.authorize(authz::Action::ListChildren, authz_silo).await?; + // Check the silo's provision type matches the lookup. + + let conn = self.pool_connection_authorized(opctx).await?; + + let silo = { + use nexus_db_schema::schema::silo::dsl; + dsl::silo + .filter(dsl::id.eq(authz_silo.id())) + .select(Silo::as_select()) + .get_result_async::(&*conn) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource(authz_silo), + ) + })? + }; + + let lookup_user_provision_type = + silo_group_lookup.user_provision_type(); + + if silo.user_provision_type != lookup_user_provision_type { + error!( + self.log, + "silo {} provision type {:?} does not match lookup provision \ + type {:?}", + authz_silo.id(), + silo.user_provision_type, + lookup_user_provision_type, + ); + + return Err(Error::internal_error(&format!( + "user provision type of silo ({:?}) does not match \ + lookup ({:?})", + silo.user_provision_type, lookup_user_provision_type, + ))); + } + use nexus_db_schema::schema::silo_group::dsl; - dsl::silo_group - .filter(dsl::silo_id.eq(authz_silo.id())) - .filter(dsl::external_id.eq(external_id)) - .filter(dsl::time_deleted.is_null()) - .select(db::model::SiloGroup::as_select()) - .first_async(&*self.pool_connection_authorized(opctx).await?) - .await - .optional() - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + let maybe_silo_group = match silo_group_lookup { + SiloGroupLookup::ApiOnly { external_id } + | SiloGroupLookup::Jit { external_id } => dsl::silo_group + .filter(dsl::silo_id.eq(authz_silo.id())) + .filter(dsl::user_provision_type.eq(lookup_user_provision_type)) + .filter(dsl::external_id.eq(external_id.to_string())) + .filter(dsl::time_deleted.is_null()) + .select(model::SiloGroup::as_select()) + .first_async(&*conn) + .await + .optional() + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?, + }; + + Ok(maybe_silo_group.map(|group| group.into())) } pub async fn silo_group_membership_for_user( @@ -106,6 +443,23 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + pub async fn silo_group_membership_for_group( + &self, + opctx: &OpContext, + authz_silo: &authz::Silo, + silo_group_id: SiloGroupUuid, + ) -> ListResultVec { + opctx.authorize(authz::Action::ListChildren, authz_silo).await?; + + use nexus_db_schema::schema::silo_group_membership::dsl; + dsl::silo_group_membership + .filter(dsl::silo_group_id.eq(to_db_typed_uuid(silo_group_id))) + .select(SiloGroupMembership::as_returning()) + .get_results_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + } + pub async fn silo_groups_for_self( &self, opctx: &OpContext, @@ -131,14 +485,20 @@ impl DataStore { use nexus_db_schema::schema::{ silo_group as sg, silo_group_membership as sgm, }; - paginated(sg::dsl::silo_group, sg::id, pagparams) + + let page = paginated(sg::dsl::silo_group, sg::id, pagparams) .inner_join(sgm::table.on(sgm::silo_group_id.eq(sg::id))) .filter(sgm::silo_user_id.eq(to_db_typed_uuid(silo_user_id))) .filter(sg::time_deleted.is_null()) - .select(SiloGroup::as_returning()) + .select(model::SiloGroup::as_returning()) .get_results_async(&*self.pool_connection_authorized(opctx).await?) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .into_iter() + .map(|group: model::SiloGroup| group.into()) + .collect::>(); + + Ok(page) } /// Update a silo user's group membership: @@ -178,10 +538,10 @@ impl DataStore { // Create new memberships for user let silo_group_memberships: Vec< - db::model::SiloGroupMembership, + model::SiloGroupMembership, > = silo_group_ids .iter() - .map(|group_id| db::model::SiloGroupMembership { + .map(|group_id| model::SiloGroupMembership { silo_group_id: to_db_typed_uuid(*group_id), silo_user_id: to_db_typed_uuid(silo_user_id), }) @@ -273,14 +633,36 @@ impl DataStore { use nexus_db_schema::schema::silo_group::dsl; opctx.authorize(authz::Action::Read, authz_silo).await?; - paginated(dsl::silo_group, dsl::id, pagparams) + + let conn = self.pool_connection_authorized(opctx).await?; + + let silo = { + use nexus_db_schema::schema::silo::dsl; + dsl::silo + .filter(dsl::id.eq(authz_silo.id())) + .select(model::Silo::as_select()) + .get_result_async::(&*conn) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource(authz_silo), + ) + })? + }; + + let page = paginated(dsl::silo_group, dsl::id, pagparams) .filter(dsl::silo_id.eq(authz_silo.id())) .filter(dsl::time_deleted.is_null()) - .select(SiloGroup::as_select()) - .load_async::( - &*self.pool_connection_authorized(opctx).await?, - ) + .filter(dsl::user_provision_type.eq(silo.user_provision_type)) + .select(model::SiloGroup::as_select()) + .load_async::(&*conn) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? + .into_iter() + .map(|group: model::SiloGroup| group.into()) + .collect::>(); + + Ok(page) } } diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index 0eb374af0bb..1d2500e2010 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -9,9 +9,9 @@ use crate::authn; use crate::authz; use crate::context::OpContext; use crate::db::datastore::IdentityMetadataCreateParams; +use crate::db::model; use crate::db::model::Name; use crate::db::model::Silo; -use crate::db::model::SiloUser; use crate::db::model::SiloUserPasswordHash; use crate::db::model::SiloUserPasswordUpdate; use crate::db::model::UserBuiltin; @@ -20,11 +20,13 @@ use crate::db::model::to_db_typed_uuid; use crate::db::pagination::paginated; use crate::db::update_and_check::UpdateAndCheck; use async_bb8_diesel::AsyncRunQueryDsl; +use chrono::DateTime; use chrono::Utc; use diesel::prelude::*; use nexus_db_errors::ErrorHandler; use nexus_db_errors::public_error_from_diesel; use nexus_types::external_api::params; +use nexus_types::external_api::views; use nexus_types::identity::Asset; use nexus_types::identity::Resource; use omicron_common::api::external::CreateResult; @@ -38,8 +40,229 @@ use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; use omicron_common::bail_unless; use omicron_uuid_kinds::GenericUuid; +use omicron_uuid_kinds::SiloUserUuid; use uuid::Uuid; +/// The datastore crate's SiloUser is intended to provide type safety above the +/// database model, as the same database model is used to store semantically +/// different user types. Higher level parts of Nexus should use this type +/// instead. +#[derive(Debug, Clone)] +pub enum SiloUser { + /// A User created via the API + ApiOnly(SiloUserApiOnly), + + /// A User created during an authenticated SAML login + Jit(SiloUserJit), +} + +impl SiloUser { + pub fn id(&self) -> SiloUserUuid { + match &self { + SiloUser::ApiOnly(u) => u.id, + SiloUser::Jit(u) => u.id, + } + } + + pub fn silo_id(&self) -> Uuid { + match &self { + SiloUser::ApiOnly(u) => u.silo_id, + SiloUser::Jit(u) => u.silo_id, + } + } + + pub fn external_id(&self) -> &str { + match &self { + SiloUser::ApiOnly(u) => &u.external_id, + SiloUser::Jit(u) => &u.external_id, + } + } +} + +impl From for SiloUser { + fn from(record: model::SiloUser) -> SiloUser { + match record.user_provision_type { + UserProvisionType::ApiOnly => SiloUser::ApiOnly(SiloUserApiOnly { + id: record.id(), + time_created: record.time_created(), + time_modified: record.time_modified(), + time_deleted: record.time_deleted, + silo_id: record.silo_id, + // SAFETY: there is a database constraint that prevents a user + // with provision type 'api_only' from having a null external id + external_id: record + .external_id + .expect("database constraint exists"), + }), + + UserProvisionType::Jit => SiloUser::Jit(SiloUserJit { + id: record.id(), + time_created: record.time_created(), + time_modified: record.time_modified(), + time_deleted: record.time_deleted, + silo_id: record.silo_id, + // SAFETY: there is a database constraint that prevents a user + // with provision type 'jit' from having a null external id + external_id: record + .external_id + .expect("database constraint exists"), + }), + } + } +} + +impl From for model::SiloUser { + fn from(u: SiloUser) -> model::SiloUser { + match u { + SiloUser::ApiOnly(u) => u.into(), + SiloUser::Jit(u) => u.into(), + } + } +} + +impl From for views::User { + fn from(u: SiloUser) -> views::User { + match u { + SiloUser::ApiOnly(u) => u.into(), + SiloUser::Jit(u) => u.into(), + } + } +} + +#[derive(Debug, Clone)] +pub struct SiloUserApiOnly { + pub id: SiloUserUuid, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, + pub silo_id: Uuid, + + /// The identity provider's ID for this user. + pub external_id: String, +} + +impl SiloUserApiOnly { + pub fn new(silo_id: Uuid, id: SiloUserUuid, external_id: String) -> Self { + Self { + id, + time_created: Utc::now(), + time_modified: Utc::now(), + time_deleted: None, + silo_id, + external_id, + } + } +} + +impl From for model::SiloUser { + fn from(u: SiloUserApiOnly) -> model::SiloUser { + model::SiloUser { + identity: model::SiloUserIdentity { + id: u.id.into(), + time_created: u.time_created, + time_modified: u.time_modified, + }, + time_deleted: u.time_deleted, + silo_id: u.silo_id, + external_id: Some(u.external_id), + user_provision_type: UserProvisionType::ApiOnly, + } + } +} + +impl From for SiloUser { + fn from(u: SiloUserApiOnly) -> SiloUser { + SiloUser::ApiOnly(u) + } +} + +impl From for views::User { + fn from(u: SiloUserApiOnly) -> views::User { + views::User { + id: u.id, + // TODO the use of external_id as display_name is temporary + display_name: u.external_id, + silo_id: u.silo_id, + } + } +} + +#[derive(Debug, Clone)] +pub struct SiloUserJit { + pub id: SiloUserUuid, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, + pub silo_id: Uuid, + + /// The identity provider's ID for this user. + pub external_id: String, +} + +impl SiloUserJit { + pub fn new(silo_id: Uuid, id: SiloUserUuid, external_id: String) -> Self { + Self { + id, + time_created: Utc::now(), + time_modified: Utc::now(), + time_deleted: None, + silo_id, + external_id, + } + } +} + +impl From for model::SiloUser { + fn from(u: SiloUserJit) -> model::SiloUser { + model::SiloUser { + identity: model::SiloUserIdentity { + id: u.id.into(), + time_created: u.time_created, + time_modified: u.time_modified, + }, + time_deleted: u.time_deleted, + silo_id: u.silo_id, + external_id: Some(u.external_id), + user_provision_type: UserProvisionType::Jit, + } + } +} + +impl From for SiloUser { + fn from(u: SiloUserJit) -> SiloUser { + SiloUser::Jit(u) + } +} + +impl From for views::User { + fn from(u: SiloUserJit) -> views::User { + views::User { + id: u.id, + // TODO the use of external_id as display_name is temporary + display_name: u.external_id, + silo_id: u.silo_id, + } + } +} + +/// Different types of user have different fields that are considered unique, +/// and therefore lookup needs to be typed as well. +#[derive(Debug)] +pub enum SiloUserLookup<'a> { + ApiOnly { external_id: &'a str }, + + Jit { external_id: &'a str }, +} + +impl<'a> SiloUserLookup<'a> { + pub fn user_provision_type(&self) -> UserProvisionType { + match &self { + SiloUserLookup::ApiOnly { .. } => UserProvisionType::ApiOnly, + SiloUserLookup::Jit { .. } => UserProvisionType::Jit, + } + } +} + impl DataStore { /// Create a silo user // TODO-security This function should take an OpContext and do an authz @@ -50,13 +273,51 @@ impl DataStore { silo_user: SiloUser, ) -> CreateResult<(authz::SiloUser, SiloUser)> { // TODO-security This needs an authz check. - use nexus_db_schema::schema::silo_user::dsl; - let silo_user_external_id = silo_user.external_id.clone(); + let silo_user_external_id = silo_user.external_id().to_string(); + let model: model::SiloUser = silo_user.into(); + let conn = self.pool_connection_unauthorized().await?; + + // Check the user's provision type matches the silo + + let silo = { + use nexus_db_schema::schema::silo::dsl; + dsl::silo + .filter(dsl::id.eq(authz_silo.id())) + .select(Silo::as_select()) + .get_result_async::(&*conn) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource(authz_silo), + ) + })? + }; + + if silo.user_provision_type != model.user_provision_type { + error!( + self.log, + "silo {} provision type {:?} does not match silo user {} \ + provision type {:?}", + authz_silo.id(), + silo.user_provision_type, + model.id(), + model.user_provision_type, + ); + + return Err(Error::internal_error(&format!( + "user provision type of silo ({:?}) does not match silo \ + user's ({:?})", + silo.user_provision_type, model.user_provision_type, + ))); + } + + use nexus_db_schema::schema::silo_user::dsl; diesel::insert_into(dsl::silo_user) - .values(silo_user) - .returning(SiloUser::as_returning()) + .values(model) + .returning(model::SiloUser::as_returning()) .get_result_async(&*conn) .await .map_err(|e| { @@ -68,14 +329,14 @@ impl DataStore { ), ) }) - .map(|db_silo_user: SiloUser| { + .map(|db_silo_user: model::SiloUser| { let silo_user_id = db_silo_user.id(); let authz_silo_user = authz::SiloUser::new( authz_silo.clone(), silo_user_id, LookupType::by_id(silo_user_id), ); - (authz_silo_user, db_silo_user) + (authz_silo_user, db_silo_user.into()) }) } @@ -105,7 +366,7 @@ impl DataStore { ) .filter(dsl::time_deleted.is_null()) .set(dsl::time_deleted.eq(Utc::now())) - .check_if_exists::( + .check_if_exists::( authz_silo_user_id.into_untyped_uuid(), ) .execute_and_check(&conn) @@ -174,40 +435,80 @@ impl DataStore { }) } - /// Given an external ID, return - /// - Ok(Some((authz::SiloUser, SiloUser))) if that external id refers to an + /// Given a silo user lookup, return + /// - Ok(Some((authz::SiloUser, SiloUser))) if that lookup refers to an /// existing silo user /// - Ok(None) if it does not /// - Err(...) if there was an error doing this lookup. - pub async fn silo_user_fetch_by_external_id( - &self, + pub async fn silo_user_fetch<'a>( + &'a self, opctx: &OpContext, authz_silo: &authz::Silo, - external_id: &str, + silo_user_lookup: &'a SiloUserLookup<'a>, ) -> Result, Error> { opctx.authorize(authz::Action::ListChildren, authz_silo).await?; use nexus_db_schema::schema::silo_user::dsl; - Ok(dsl::silo_user - .filter(dsl::silo_id.eq(authz_silo.id())) - .filter(dsl::external_id.eq(external_id.to_string())) - .filter(dsl::time_deleted.is_null()) - .select(SiloUser::as_select()) - .load_async::( - &*self.pool_connection_authorized(opctx).await?, - ) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? - .pop() - .map(|db_silo_user| { + let conn = self.pool_connection_authorized(opctx).await?; + + // Check the silo's provision type matches the lookup. + + let silo = { + use nexus_db_schema::schema::silo::dsl; + dsl::silo + .filter(dsl::id.eq(authz_silo.id())) + .select(Silo::as_select()) + .get_result_async::(&*conn) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource(authz_silo), + ) + })? + }; + + let lookup_user_provision_type = silo_user_lookup.user_provision_type(); + + if silo.user_provision_type != lookup_user_provision_type { + return Err(Error::invalid_request(format!( + "silo provision type {:?} does not match lookup {:?}", + silo.user_provision_type, lookup_user_provision_type, + ))); + } + + match silo_user_lookup { + SiloUserLookup::ApiOnly { external_id } + | SiloUserLookup::Jit { external_id } => { + let maybe_db_silo_user = dsl::silo_user + .filter(dsl::silo_id.eq(authz_silo.id())) + .filter( + dsl::user_provision_type.eq(lookup_user_provision_type), + ) + .filter(dsl::external_id.eq(external_id.to_string())) + .filter(dsl::time_deleted.is_null()) + .select(model::SiloUser::as_select()) + .get_result_async::(&*conn) + .await + .optional() + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + let Some(db_silo_user) = maybe_db_silo_user else { + return Ok(None); + }; + let authz_silo_user = authz::SiloUser::new( authz_silo.clone(), db_silo_user.id(), - LookupType::ByName(external_id.to_owned()), + LookupType::ByName(external_id.to_string()), ); - (authz_silo_user, db_silo_user) - })) + + Ok(Some((authz_silo_user, db_silo_user.into()))) + } + } } pub async fn silo_users_list( @@ -222,14 +523,33 @@ impl DataStore { .authorize(authz::Action::ListChildren, authz_silo_user_list) .await?; + let conn = self.pool_connection_authorized(opctx).await?; + + let db_silo = { + use nexus_db_schema::schema::silo::dsl; + dsl::silo + .filter(dsl::id.eq(authz_silo_user_list.silo().id())) + .select(Silo::as_select()) + .get_result_async::(&*conn) + .await + .map_err(|e| { + public_error_from_diesel( + e, + ErrorHandler::NotFoundByResource( + authz_silo_user_list.silo(), + ), + ) + })? + }; + paginated(silo_user, id, pagparams) .filter(silo_id.eq(authz_silo_user_list.silo().id())) + .filter(user_provision_type.eq(db_silo.user_provision_type)) .filter(time_deleted.is_null()) - .select(SiloUser::as_select()) - .load_async::( - &*self.pool_connection_authorized(opctx).await?, - ) + .select(model::SiloUser::as_select()) + .load_async::(&*conn) .await + .map(|list| list.into_iter().map(|user| user.into()).collect()) .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } @@ -259,11 +579,12 @@ impl DataStore { .eq(to_db_typed_uuid(authz_silo_group.id())), )), ) - .select(SiloUser::as_select()) - .load_async::( + .select(model::SiloUser::as_select()) + .load_async::( &*self.pool_connection_authorized(opctx).await?, ) .await + .map(|list| list.into_iter().map(|user| user.into()).collect()) .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } @@ -277,18 +598,18 @@ impl DataStore { opctx: &OpContext, db_silo: &Silo, authz_silo_user: &authz::SiloUser, - db_silo_user: &SiloUser, + silo_user: &SiloUserApiOnly, db_silo_user_password_hash: Option, ) -> UpdateResult<()> { opctx.authorize(authz::Action::Modify, authz_silo_user).await?; // Verify that the various objects we're given actually match up. // Failures here reflect bugs, not bad input. - bail_unless!(db_silo.id() == db_silo_user.silo_id); - bail_unless!(db_silo_user.id() == authz_silo_user.id()); + bail_unless!(db_silo.id() == silo_user.silo_id); + bail_unless!(silo_user.id == authz_silo_user.id()); if let Some(db_silo_user_password_hash) = &db_silo_user_password_hash { bail_unless!( - db_silo_user_password_hash.silo_user_id() == db_silo_user.id() + db_silo_user_password_hash.silo_user_id() == silo_user.id ); } diff --git a/nexus/db-queries/src/policy_test/mod.rs b/nexus/db-queries/src/policy_test/mod.rs index 93524c7da43..67879f61877 100644 --- a/nexus/db-queries/src/policy_test/mod.rs +++ b/nexus/db-queries/src/policy_test/mod.rs @@ -14,6 +14,10 @@ mod coverage; mod resource_builder; mod resources; +use crate::db::DataStore; +use crate::db::datastore::DnsVersionUpdateBuilder; +use crate::db::model::DnsGroup; +use crate::db::model::InitialDnsGroup; use crate::db::pub_test_utils::TestDatabase; use coverage::Coverage; use futures::StreamExt; @@ -22,11 +26,14 @@ use nexus_auth::authn::SiloAuthnPolicy; use nexus_auth::authn::USER_TEST_PRIVILEGED; use nexus_auth::authz; use nexus_auth::context::OpContext; +use nexus_types::external_api::params; use nexus_types::external_api::shared; use nexus_types::external_api::shared::FleetRole; use nexus_types::external_api::shared::SiloRole; use nexus_types::identity::Asset; +use nexus_types::identity::Resource; use omicron_common::api::external::Error; +use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::LookupType; use omicron_test_utils::dev; use resource_builder::DynAuthorizedResource; @@ -35,12 +42,90 @@ use resource_builder::ResourceSet; use slog::{o, trace}; use std::collections::BTreeMap; use std::collections::BTreeSet; +use std::collections::HashMap; use std::io::Cursor; use std::io::Write; use std::sync::Arc; use strum::IntoEnumIterator; use uuid::Uuid; +struct TestPrepOutput { + main_silo_id: Uuid, + main_silo: authz::Silo, +} + +async fn test_iam_prep( + opctx: &OpContext, + datastore: &DataStore, +) -> TestPrepOutput { + // Before we can create the resources, users, and role assignments that we + // need, we must grant the "test-privileged" user privileges to fetch and + // modify policies inside the "main" Silo (the one we create users in). + + let initial = InitialDnsGroup::new( + DnsGroup::External, + "dummy.oxide.test", + "test suite", + "test suite", + HashMap::new(), + ); + + { + let conn = datastore.pool_connection_for_tests().await.unwrap(); + DataStore::load_dns_data(&conn, initial) + .await + .expect("failed to load initial DNS zone"); + } + + let silo = datastore + .silo_create( + &opctx, + &opctx, + params::SiloCreate { + identity: IdentityMetadataCreateParams { + name: "main-silo".parse().unwrap(), + description: "".into(), + }, + quotas: params::SiloQuotasCreate::empty(), + discoverable: true, + identity_mode: shared::SiloIdentityMode::LocalOnly, + admin_group_name: None, + tls_certificates: vec![], + mapped_fleet_roles: Default::default(), + }, + &[], + DnsVersionUpdateBuilder::new( + DnsGroup::External, + String::from("test suite"), + String::from("test suite"), + ), + ) + .await + .expect("silo_create"); + + let main_silo_id = silo.id(); + + let main_silo = authz::Silo::new( + authz::FLEET, + main_silo_id, + LookupType::ById(main_silo_id), + ); + + datastore + .role_assignment_replace_visible( + &opctx, + &main_silo, + &[shared::RoleAssignment::for_silo_user( + USER_TEST_PRIVILEGED.id(), + SiloRole::Admin, + )], + ) + .await + .unwrap(); + + TestPrepOutput { main_silo_id, main_silo } +} + /// Verifies that all roles grant precisely the privileges that we expect them /// to /// @@ -64,26 +149,8 @@ async fn test_iam_roles_behavior() { let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); - // Before we can create the resources, users, and role assignments that we - // need, we must grant the "test-privileged" user privileges to fetch and - // modify policies inside the "main" Silo (the one we create users in). - let main_silo_id = Uuid::new_v4(); - let main_silo = authz::Silo::new( - authz::FLEET, - main_silo_id, - LookupType::ById(main_silo_id), - ); - datastore - .role_assignment_replace_visible( - &opctx, - &main_silo, - &[shared::RoleAssignment::for_silo_user( - USER_TEST_PRIVILEGED.id(), - SiloRole::Admin, - )], - ) - .await - .unwrap(); + let TestPrepOutput { main_silo_id, main_silo: _ } = + test_iam_prep(&opctx, &datastore).await; // Assemble the list of resources that we'll use for testing. As we create // these resources, create the users and role assignments needed for the @@ -329,26 +396,8 @@ async fn test_conferred_roles() { let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); - // Before we can create the resources, users, and role assignments that we - // need, we must grant the "test-privileged" user privileges to fetch and - // modify policies inside the "main" Silo (the one we create users in). - let main_silo_id = Uuid::new_v4(); - let main_silo = authz::Silo::new( - authz::FLEET, - main_silo_id, - LookupType::ById(main_silo_id), - ); - datastore - .role_assignment_replace_visible( - &opctx, - &main_silo, - &[shared::RoleAssignment::for_silo_user( - USER_TEST_PRIVILEGED.id(), - SiloRole::Admin, - )], - ) - .await - .unwrap(); + let TestPrepOutput { main_silo_id, main_silo } = + test_iam_prep(&opctx, &datastore).await; let exemptions = resources::exempted_authz_classes(); let mut coverage = Coverage::new(&logctx.log, exemptions); diff --git a/nexus/db-queries/src/policy_test/resource_builder.rs b/nexus/db-queries/src/policy_test/resource_builder.rs index f648810ff2f..e7c7da87b3d 100644 --- a/nexus/db-queries/src/policy_test/resource_builder.rs +++ b/nexus/db-queries/src/policy_test/resource_builder.rs @@ -7,6 +7,7 @@ use super::coverage::Coverage; use crate::db; +use crate::db::datastore::SiloUserApiOnly; use authz::ApiResource; use futures::FutureExt; use futures::future::BoxFuture; @@ -112,10 +113,9 @@ impl<'a> ResourceBuilder<'a> { silo_id, LookupType::ById(silo_id), ); - let silo_user = - db::model::SiloUser::new(silo_id, user_id, username); + let silo_user = SiloUserApiOnly::new(silo_id, user_id, username); datastore - .silo_user_create(&authz_silo, silo_user) + .silo_user_create(&authz_silo, silo_user.into()) .await .expect("failed to create silo user"); diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index ad191dfe067..361ed15dc62 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -766,7 +766,8 @@ table! { time_deleted -> Nullable, silo_id -> Uuid, - external_id -> Text, + external_id -> Nullable, + user_provision_type -> crate::enums::UserProvisionTypeEnum, } } @@ -786,7 +787,8 @@ table! { time_deleted -> Nullable, silo_id -> Uuid, - external_id -> Text, + external_id -> Nullable, + user_provision_type -> crate::enums::UserProvisionTypeEnum, } } @@ -803,6 +805,8 @@ allow_tables_to_appear_in_same_query!( silo_group_membership, silo_user, ); +allow_tables_to_appear_in_same_query!(silo_group, silo); +allow_tables_to_appear_in_same_query!(silo_user, silo); allow_tables_to_appear_in_same_query!(role_assignment, silo_group_membership); table! { diff --git a/nexus/src/app/iam.rs b/nexus/src/app/iam.rs index d29c86fcb0b..aa1b7e99364 100644 --- a/nexus/src/app/iam.rs +++ b/nexus/src/app/iam.rs @@ -11,6 +11,8 @@ use nexus_db_lookup::lookup; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; +use nexus_db_queries::db::datastore::SiloGroup; +use nexus_db_queries::db::datastore::SiloUser; use nexus_db_queries::db::model::Name; use nexus_types::external_api::params; use omicron_common::api::external::DataPageParams; @@ -70,7 +72,7 @@ impl super::Nexus { &self, opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { + ) -> ListResultVec { let authz_silo = opctx .authn .silo_required() @@ -88,7 +90,7 @@ impl super::Nexus { opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, group_id: &SiloGroupUuid, - ) -> ListResultVec { + ) -> ListResultVec { let authz_silo = opctx .authn .silo_required() @@ -115,7 +117,7 @@ impl super::Nexus { pub(crate) async fn silo_user_fetch_self( &self, opctx: &OpContext, - ) -> LookupResult { + ) -> LookupResult { let &actor = opctx .authn .actor_required() @@ -124,14 +126,14 @@ impl super::Nexus { .silo_user_actor(&actor)? .fetch() .await?; - Ok(db_silo_user) + Ok(db_silo_user.into()) } pub(crate) async fn silo_user_fetch_groups_for_self( &self, opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { + ) -> ListResultVec { self.db_datastore.silo_groups_for_self(opctx, pagparams).await } @@ -141,7 +143,7 @@ impl super::Nexus { &self, opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { + ) -> ListResultVec { let authz_silo = opctx .authn .silo_required() diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index 53a6204b536..9e817e39471 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -11,7 +11,7 @@ use nexus_db_queries::authn::Reason; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; -use nexus_db_queries::db::identity::Asset; +use nexus_db_queries::db::datastore::SiloUser; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; @@ -41,7 +41,7 @@ impl super::Nexus { pub(crate) async fn session_create( &self, opctx: &OpContext, - user: &db::model::SiloUser, + user: &SiloUser, ) -> CreateResult { let session = db::model::ConsoleSession::new(generate_session_token(), user.id()); diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 6d0cf94ea32..e8bdd48418d 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -17,7 +17,14 @@ use nexus_db_queries::context::OpContext; use nexus_db_queries::db; use nexus_db_queries::db::datastore::Discoverability; use nexus_db_queries::db::datastore::DnsVersionUpdateBuilder; -use nexus_db_queries::db::identity::{Asset, Resource}; +use nexus_db_queries::db::datastore::SiloGroup; +use nexus_db_queries::db::datastore::SiloGroupJit; +use nexus_db_queries::db::datastore::SiloGroupLookup; +use nexus_db_queries::db::datastore::SiloUser; +use nexus_db_queries::db::datastore::SiloUserApiOnly; +use nexus_db_queries::db::datastore::SiloUserJit; +use nexus_db_queries::db::datastore::SiloUserLookup; +use nexus_db_queries::db::identity::Resource; use nexus_db_queries::{authn, authz}; use nexus_types::deployment::execution::blueprint_nexus_external_ips; use nexus_types::internal_api::params::DnsRecord; @@ -30,7 +37,6 @@ use omicron_common::api::external::{CreateResult, LookupType}; use omicron_common::api::external::{DataPageParams, ResourceType}; use omicron_common::api::external::{DeleteResult, NameOrId}; use omicron_common::api::external::{Error, InternalContext}; -use omicron_common::bail_unless; use omicron_uuid_kinds::SiloGroupUuid; use omicron_uuid_kinds::SiloUserUuid; use std::net::IpAddr; @@ -276,17 +282,18 @@ impl super::Nexus { authz_silo: &authz::Silo, silo_user_id: SiloUserUuid, action: authz::Action, - ) -> LookupResult<(authz::SiloUser, db::model::SiloUser)> { + ) -> LookupResult<(authz::SiloUser, 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)) + Ok((authz_silo_user, db_silo_user.into())) } /// List the users in a Silo @@ -295,7 +302,7 @@ impl super::Nexus { opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, pagparams: &DataPageParams<'_, Uuid>, - ) -> ListResultVec { + ) -> ListResultVec { let (authz_silo,) = silo_lookup.lookup_for(authz::Action::Read).await?; let authz_silo_user_list = authz::SiloUserList::new(authz_silo); self.db_datastore @@ -309,9 +316,9 @@ impl super::Nexus { opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, silo_user_id: SiloUserUuid, - ) -> LookupResult { + ) -> LookupResult { let (authz_silo,) = silo_lookup.lookup_for(authz::Action::Read).await?; - let (_, db_silo_user) = self + let (_, silo_user) = self .silo_user_lookup_by_id( opctx, &authz_silo, @@ -319,7 +326,7 @@ impl super::Nexus { authz::Action::Read, ) .await?; - Ok(db_silo_user) + Ok(silo_user) } /// Delete all of user's tokens and sessions @@ -363,14 +370,14 @@ impl super::Nexus { &self, opctx: &OpContext, silo_user_id: SiloUserUuid, - ) -> LookupResult<(authz::SiloUser, db::model::SiloUser)> { + ) -> LookupResult<(authz::SiloUser, SiloUser)> { let (_, authz_silo_user, db_silo_user) = LookupPath::new(opctx, self.datastore()) .silo_user_id(silo_user_id) .fetch() .await?; - Ok((authz_silo_user, db_silo_user)) + Ok((authz_silo_user, db_silo_user.into())) } /// List device access tokens for a user in a Silo @@ -450,36 +457,43 @@ impl super::Nexus { opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, new_user_params: params::UserCreate, - ) -> CreateResult { + ) -> CreateResult { let (authz_silo, db_silo) = self.local_idp_fetch_silo(silo_lookup).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( + + let silo_user = SiloUserApiOnly::new( authz_silo.id(), SiloUserUuid::new_v4(), new_user_params.external_id.as_ref().to_owned(), ); + // TODO These two steps should happen in a transaction. - let (_, db_silo_user) = - self.datastore().silo_user_create(&authz_silo, silo_user).await?; + self.datastore() + .silo_user_create(&authz_silo, silo_user.clone().into()) + .await?; + let authz_silo_user = authz::SiloUser::new( authz_silo.clone(), - db_silo_user.id(), - LookupType::by_id(db_silo_user.id()), + silo_user.id, + LookupType::by_id(silo_user.id), ); + self.silo_user_password_set_internal( opctx, &db_silo, &authz_silo_user, - &db_silo_user, + &silo_user, new_user_params.password, ) .await?; - Ok(db_silo_user) + + Ok(silo_user.into()) } /// Delete a user in a Silo's local identity provider @@ -509,52 +523,63 @@ impl super::Nexus { authz_silo: &authz::Silo, db_silo: &db::model::Silo, authenticated_subject: &authn::silos::AuthenticatedSubject, - ) -> LookupResult { + ) -> LookupResult { // XXX create user permission? opctx.authorize(authz::Action::CreateChild, authz_silo).await?; opctx.authorize(authz::Action::ListChildren, authz_silo).await?; let fetch_result = self .datastore() - .silo_user_fetch_by_external_id( + .silo_user_fetch( opctx, &authz_silo, - &authenticated_subject.external_id, + &match db_silo.user_provision_type { + db::model::UserProvisionType::ApiOnly => { + SiloUserLookup::ApiOnly { + external_id: &authenticated_subject.external_id, + } + } + + db::model::UserProvisionType::Jit => SiloUserLookup::Jit { + external_id: &authenticated_subject.external_id, + }, + }, ) .await?; - let (authz_silo_user, db_silo_user) = if let Some(existing_silo_user) = - fetch_result - { - existing_silo_user - } else { - // In this branch, no user exists for the authenticated subject - // external id. The next action depends on the silo's user - // provision type. - match db_silo.user_provision_type { - // If the user provision type is ApiOnly, do not create a - // new user if one does not exist. - db::model::UserProvisionType::ApiOnly => { - return Err(Error::Unauthenticated { - internal_message: "User must exist before login when user provision type is ApiOnly".to_string(), - }); - } + let (authz_silo_user, silo_user) = + if let Some(existing_silo_user) = fetch_result { + existing_silo_user + } else { + // In this branch, no user exists for the authenticated subject + // external id. The next action depends on the silo's user + // provision type. + match db_silo.user_provision_type { + // If the user provision type is ApiOnly, do not create a + // new user if one does not exist. + db::model::UserProvisionType::ApiOnly => { + return Err(Error::Unauthenticated { + internal_message: "User must exist before login \ + when user provision type is ApiOnly" + .to_string(), + }); + } - // If the user provision type is JIT, then create the user if - // one does not exist - db::model::UserProvisionType::Jit => { - let silo_user = db::model::SiloUser::new( - authz_silo.id(), - SiloUserUuid::new_v4(), - authenticated_subject.external_id.clone(), - ); + // If the user provision type is JIT, then create the user + // if one does not exist + db::model::UserProvisionType::Jit => { + let silo_user = SiloUserJit::new( + authz_silo.id(), + SiloUserUuid::new_v4(), + authenticated_subject.external_id.clone(), + ); - self.db_datastore - .silo_user_create(&authz_silo, silo_user) - .await? + self.db_datastore + .silo_user_create(&authz_silo, silo_user.into()) + .await? + } } - } - }; + }; // Gather a list of groups that the user is part of based on what the // IdP sent us. Also, if the silo user provision type is Jit, create @@ -564,13 +589,13 @@ impl super::Nexus { Vec::with_capacity(authenticated_subject.groups.len()); for group in &authenticated_subject.groups { - let silo_group = match db_silo.user_provision_type { + let silo_group = match &db_silo.user_provision_type { db::model::UserProvisionType::ApiOnly => { self.db_datastore .silo_group_optional_lookup( opctx, &authz_silo, - group.clone(), + &SiloGroupLookup::ApiOnly { external_id: &group }, ) .await? } @@ -580,7 +605,7 @@ impl super::Nexus { .silo_group_lookup_or_create_by_name( opctx, &authz_silo, - &group, + &SiloGroupLookup::Jit { external_id: &group }, ) .await?; @@ -603,7 +628,7 @@ impl super::Nexus { ) .await?; - Ok(db_silo_user) + Ok(silo_user) } // Silo user passwords @@ -622,7 +647,8 @@ impl super::Nexus { ) -> UpdateResult<()> { let (authz_silo, db_silo) = self.local_idp_fetch_silo(silo_lookup).await?; - let (authz_silo_user, db_silo_user) = self + + let (authz_silo_user, silo_user) = self .silo_user_lookup_by_id( opctx, &authz_silo, @@ -630,11 +656,21 @@ impl super::Nexus { authz::Action::Modify, ) .await?; + + let silo_user = match &silo_user { + SiloUser::ApiOnly(user) => user, + SiloUser::Jit(_) => { + return Err(Error::invalid_request( + "invalid user type for password set", + )); + } + }; + self.silo_user_password_set_internal( opctx, &db_silo, &authz_silo_user, - &db_silo_user, + silo_user, password_value, ) .await @@ -649,7 +685,7 @@ impl super::Nexus { opctx: &OpContext, db_silo: &db::model::Silo, authz_silo_user: &authz::SiloUser, - db_silo_user: &db::model::SiloUser, + silo_user: &SiloUserApiOnly, password_value: params::UserPassword, ) -> UpdateResult<()> { let password_hash = match password_value { @@ -673,7 +709,7 @@ impl super::Nexus { opctx, db_silo, authz_silo_user, - db_silo_user, + silo_user, password_hash, ) .await @@ -730,7 +766,7 @@ impl super::Nexus { opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, credentials: params::UsernamePasswordCredentials, - ) -> Result { + ) -> Result { let (authz_silo, _) = self.local_idp_fetch_silo(silo_lookup).await?; // NOTE: It's very important that we not bail out early if we fail to @@ -741,10 +777,12 @@ impl super::Nexus { // exist. Rate limiting might help. See omicron#2184. let fetch_user = self .datastore() - .silo_user_fetch_by_external_id( + .silo_user_fetch( opctx, &authz_silo, - credentials.username.as_ref(), + &SiloUserLookup::ApiOnly { + external_id: credentials.username.as_ref(), + }, ) .await?; let verified = self @@ -755,12 +793,13 @@ impl super::Nexus { ) .await?; if verified { - bail_unless!( - fetch_user.is_some(), - "passed password verification without a valid user" - ); - let db_user = fetch_user.unwrap().1; - Ok(db_user) + if let Some((_, user)) = fetch_user { + Ok(user) + } else { + Err(Error::internal_error( + "passed password verification without a valid user", + )) + } } else { Err(Error::Unauthenticated { internal_message: "Failed password verification".to_string(), @@ -770,15 +809,37 @@ impl super::Nexus { // Silo groups - pub async fn silo_group_lookup_or_create_by_name( + pub async fn silo_group_lookup_or_create_by_name<'a>( &self, opctx: &OpContext, authz_silo: &authz::Silo, - external_id: &String, - ) -> LookupResult { + silo_group_lookup: &'a SiloGroupLookup<'a>, + ) -> LookupResult { + // Traditionally the only group created for ApiOnly silos is the group + // with the silo admin group name that is created during silo creation. + // This function's "lookup or create" behaviour should be restricted to + // JIT silos to prevent accidentally creating a group in an ApiOnly + // silo. + // + // Note: as of this writing there's currently no API call for creating + // and deleting groups for ApiOnly silos. There's also no API call for + // creating and deleting groups for JIT silos, but this "lookup or + // create" behaviour is how they're supposed to work anyway! + + let SiloGroupLookup::Jit { external_id } = silo_group_lookup else { + return Err(Error::invalid_request(format!( + "cannot create group by name for {:?} provision type", + silo_group_lookup.user_provision_type(), + ))); + }; + match self .db_datastore - .silo_group_optional_lookup(opctx, authz_silo, external_id.clone()) + .silo_group_optional_lookup( + opctx, + authz_silo, + &SiloGroupLookup::Jit { external_id }, + ) .await? { Some(silo_group) => Ok(silo_group), @@ -788,11 +849,12 @@ impl super::Nexus { .silo_group_ensure( opctx, authz_silo, - db::model::SiloGroup::new( - SiloGroupUuid::new_v4(), + SiloGroupJit::new( authz_silo.id(), - external_id.clone(), - ), + SiloGroupUuid::new_v4(), + external_id.to_string(), + ) + .into(), ) .await } @@ -1036,11 +1098,16 @@ impl super::Nexus { .await } - pub fn silo_group_lookup<'a>( - &'a self, - opctx: &'a OpContext, - group_id: &'a SiloGroupUuid, - ) -> lookup::SiloGroup<'a> { - LookupPath::new(opctx, &self.db_datastore).silo_group_id(*group_id) + pub async fn silo_group_lookup( + &self, + opctx: &OpContext, + group_id: &SiloGroupUuid, + ) -> LookupResult { + let (.., db_silo_group) = LookupPath::new(opctx, &self.db_datastore) + .silo_group_id(*group_id) + .fetch() + .await?; + + Ok(db_silo_group.into()) } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index a569203d8c4..4ae16603151 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7095,8 +7095,7 @@ impl NexusExternalApi for NexusExternalApiImpl { let path = path_params.into_inner(); let opctx = crate::context::op_context_for_external_api(&rqctx).await?; - let (.., group) = - nexus.silo_group_lookup(&opctx, &path.group_id).fetch().await?; + let group = nexus.silo_group_lookup(&opctx, &path.group_id).await?; Ok(HttpResponseOk(group.into())) }; apictx diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index eb6d29408be..8e3375b1a8b 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -438,7 +438,7 @@ async fn test_session_me(cptestctx: &ControlPlaneTestContext) { views::CurrentUser { user: views::User { id: USER_TEST_PRIVILEGED.id(), - display_name: USER_TEST_PRIVILEGED.external_id.clone(), + display_name: USER_TEST_PRIVILEGED.external_id.clone().unwrap(), silo_id: DEFAULT_SILO.id(), }, silo_name: DEFAULT_SILO.name().clone(), @@ -457,7 +457,10 @@ async fn test_session_me(cptestctx: &ControlPlaneTestContext) { views::CurrentUser { user: views::User { id: USER_TEST_UNPRIVILEGED.id(), - display_name: USER_TEST_UNPRIVILEGED.external_id.clone(), + display_name: USER_TEST_UNPRIVILEGED + .external_id + .clone() + .unwrap(), silo_id: DEFAULT_SILO.id(), }, silo_name: DEFAULT_SILO.name().clone(), diff --git a/nexus/tests/integration_tests/silo_users.rs b/nexus/tests/integration_tests/silo_users.rs index d1c11f83f74..564e58a7301 100644 --- a/nexus/tests/integration_tests/silo_users.rs +++ b/nexus/tests/integration_tests/silo_users.rs @@ -7,6 +7,7 @@ use http::{StatusCode, method::Method}; use nexus_db_queries::authn::USER_TEST_UNPRIVILEGED; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; +use nexus_db_queries::db::datastore::SiloGroupApiOnly; use nexus_test_utils::assert_same_items; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; use nexus_test_utils::resource_helpers::objects_list_page_authz; @@ -15,6 +16,7 @@ use nexus_types::external_api::views; use nexus_types::identity::Asset; use nexus_types::silo::DEFAULT_SILO_ID; use omicron_common::api::external::LookupType; +use omicron_uuid_kinds::SiloGroupUuid; use uuid::Uuid; type ControlPlaneTestContext = @@ -53,7 +55,17 @@ async fn test_silo_group_users(cptestctx: &ControlPlaneTestContext) { // create a group let group_name = "group1".to_string(); nexus - .silo_group_lookup_or_create_by_name(&opctx, &authz_silo, &group_name) + .datastore() + .silo_group_ensure( + &opctx, + &authz_silo, + SiloGroupApiOnly::new( + authz_silo.id(), + SiloGroupUuid::new_v4(), + group_name.clone(), + ) + .into(), + ) .await .expect("Group created"); diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index 2aa271dc8f6..dd682464fe7 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -10,6 +10,9 @@ use nexus_db_queries::authn::{USER_TEST_PRIVILEGED, USER_TEST_UNPRIVILEGED}; use nexus_db_queries::authz::{self}; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; +use nexus_db_queries::db::datastore::SiloGroupLookup; +use nexus_db_queries::db::datastore::SiloUserJit; +use nexus_db_queries::db::datastore::SiloUserLookup; use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO; use nexus_db_queries::db::identity::Asset; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder}; @@ -27,7 +30,7 @@ use nexus_types::external_api::{params, shared}; use nexus_types::silo::DEFAULT_SILO_ID; use omicron_common::address::{IpRange, Ipv4Range}; use omicron_common::api::external::{ - IdentityMetadataCreateParams, LookupType, Name, + DataPageParams, IdentityMetadataCreateParams, LookupType, Name, }; use omicron_common::api::external::{ObjectIdentity, UserId}; use omicron_test_utils::certificates::CertificateChain; @@ -312,7 +315,7 @@ async fn test_silo_admin_group(cptestctx: &ControlPlaneTestContext) { .silo_group_optional_lookup( &authn_opctx, &authz_silo, - "administrator".into(), + &SiloGroupLookup::Jit { external_id: "administrator" }, ) .await .unwrap() @@ -879,10 +882,10 @@ async fn test_silo_user_fetch_by_external_id( // Fetching by external id that's not in the db should be Ok(None) let result = nexus .datastore() - .silo_user_fetch_by_external_id( + .silo_user_fetch( &opctx_external_authn, &authz_silo, - "123", + &SiloUserLookup::ApiOnly { external_id: "123" }, ) .await; assert!(result.is_ok()); @@ -891,10 +894,12 @@ async fn test_silo_user_fetch_by_external_id( // Fetching by external id that is should be Ok(Some) let result = nexus .datastore() - .silo_user_fetch_by_external_id( + .silo_user_fetch( &opctx_external_authn, &authz_silo, - "f5513e049dac9468de5bdff36ab17d04f", + &SiloUserLookup::ApiOnly { + external_id: "f5513e049dac9468de5bdff36ab17d04f", + }, ) .await; assert!(result.is_ok()); @@ -918,12 +923,15 @@ async fn test_silo_users_list(cptestctx: &ControlPlaneTestContext) { vec![ views::User { id: USER_TEST_PRIVILEGED.id(), - display_name: USER_TEST_PRIVILEGED.external_id.clone(), + display_name: USER_TEST_PRIVILEGED.external_id.clone().unwrap(), silo_id: DEFAULT_SILO_ID, }, views::User { id: USER_TEST_UNPRIVILEGED.id(), - display_name: USER_TEST_UNPRIVILEGED.external_id.clone(), + display_name: USER_TEST_UNPRIVILEGED + .external_id + .clone() + .unwrap(), silo_id: DEFAULT_SILO_ID, }, ] @@ -957,12 +965,15 @@ async fn test_silo_users_list(cptestctx: &ControlPlaneTestContext) { }, views::User { id: USER_TEST_PRIVILEGED.id(), - display_name: USER_TEST_PRIVILEGED.external_id.clone(), + display_name: USER_TEST_PRIVILEGED.external_id.clone().unwrap(), silo_id: DEFAULT_SILO_ID, }, views::User { id: USER_TEST_UNPRIVILEGED.id(), - display_name: USER_TEST_UNPRIVILEGED.external_id.clone(), + display_name: USER_TEST_UNPRIVILEGED + .external_id + .clone() + .unwrap(), silo_id: DEFAULT_SILO_ID, }, ] @@ -1085,7 +1096,8 @@ async fn test_silo_groups_jit(cptestctx: &ControlPlaneTestContext) { .await .unwrap(); - group_names.push(db_group.external_id); + // expect groups to have non-None external ids for JIT silos + group_names.push(db_group.external_id.unwrap()); } assert!(group_names.contains(&"a-group".to_string())); @@ -1214,7 +1226,8 @@ async fn test_silo_groups_remove_from_one_group( .await .unwrap(); - group_names.push(db_group.external_id); + // expect groups to have non-None external ids for JIT silos + group_names.push(db_group.external_id.unwrap()); } assert!(group_names.contains(&"a-group".to_string())); @@ -1255,7 +1268,8 @@ async fn test_silo_groups_remove_from_one_group( .await .unwrap(); - group_names.push(db_group.external_id); + // expect groups to have non-None external ids for JIT silos + group_names.push(db_group.external_id.unwrap()); } assert!(group_names.contains(&"b-group".to_string())); @@ -1325,7 +1339,8 @@ async fn test_silo_groups_remove_from_both_groups( .await .unwrap(); - group_names.push(db_group.external_id); + // expect groups to have non-None external ids for JIT silos + group_names.push(db_group.external_id.unwrap()); } assert!(group_names.contains(&"a-group".to_string())); @@ -1366,7 +1381,8 @@ async fn test_silo_groups_remove_from_both_groups( .await .unwrap(); - group_names.push(db_group.external_id); + // expect groups to have non-None external ids for JIT silos + group_names.push(db_group.external_id.unwrap()); } assert!(group_names.contains(&"c-group".to_string())); @@ -1427,7 +1443,7 @@ async fn test_silo_delete_clean_up_groups(cptestctx: &ControlPlaneTestContext) { .silo_group_optional_lookup( &opctx_external_authn, &authz_silo, - "a-group".into(), + &SiloGroupLookup::Jit { external_id: "a-group" }, ) .await .expect("silo_group_optional_lookup") @@ -1475,6 +1491,17 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) { nexus.datastore().clone(), ); + // Grant this user admin privileges on the new silo + + grant_iam( + client, + "/v1/system/silos/test-silo", + SiloRole::Admin, + opctx.authn.actor().unwrap().silo_user_id().unwrap(), + AuthnMode::PrivilegedUser, + ) + .await; + let (authz_silo, db_silo) = LookupPath::new(&opctx, nexus.datastore()) .silo_name(&silo.identity.name.into()) .fetch() @@ -1495,7 +1522,7 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) { .await .expect("silo_user_from_authenticated_subject 1"); - // Add the first user with a group membership + // Add the second user with the same group membership let _silo_user_2 = nexus .silo_user_from_authenticated_subject( &nexus.opctx_external_authn(), @@ -1509,7 +1536,42 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) { .await .expect("silo_user_from_authenticated_subject 2"); - // TODO-coverage were we intending to verify something here? + // Validate only one group was created + + let page = nexus + .datastore() + .silo_groups_list_by_id( + &opctx, + &authz_silo, + &DataPageParams::max_page(), + ) + .await + .expect("silo_groups_list_by_id"); + + assert_eq!(page.len(), 1); + + // Validate that the "sre" group was created + + let sre_group = nexus + .datastore() + .silo_group_optional_lookup( + &opctx, + &authz_silo, + &SiloGroupLookup::Jit { external_id: "sre" }, + ) + .await + .expect("silo_group_optional_lookup") + .expect("sre group exists"); + + // Validate that the "sre" group has two members + + let members = nexus + .datastore() + .silo_group_membership_for_group(&opctx, &authz_silo, sre_group.id()) + .await + .expect("silo_group_membership_for_group"); + + assert_eq!(members.len(), 2); } /// Tests the behavior of the per-Silo "list users" and "fetch user" endpoints. @@ -1724,9 +1786,9 @@ async fn create_jit_user( let authz_silo = authz::Silo::new(authz::FLEET, silo_id, LookupType::ById(silo_id)); let silo_user = - db::model::SiloUser::new(silo_id, silo_user_id, external_id.to_owned()); + SiloUserJit::new(silo_id, silo_user_id, external_id.to_owned()); datastore - .silo_user_create(&authz_silo, silo_user) + .silo_user_create(&authz_silo, silo_user.into()) .await .expect("failed to create user in SamlJit Silo") .1 diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 2407b7d7ca5..23b99aa0070 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -895,15 +895,36 @@ CREATE TABLE IF NOT EXISTS omicron.public.silo_user ( time_deleted TIMESTAMPTZ, silo_id UUID NOT NULL, - external_id TEXT NOT NULL + + -- if the user provision type is 'api_only' or 'jit', then this field must + -- contain a value + external_id TEXT, + + user_provision_type omicron.public.user_provision_type, + + CONSTRAINT user_provision_type_required_for_non_deleted CHECK ( + (user_provision_type IS NOT NULL AND time_deleted IS NULL) + OR (time_deleted IS NOT NULL) + ), + + CONSTRAINT external_id_consistency CHECK ( + CASE user_provision_type + WHEN 'api_only' THEN external_id IS NOT NULL + WHEN 'jit' THEN external_id IS NOT NULL + END + ) ); -/* This index lets us quickly find users for a given silo. */ -CREATE UNIQUE INDEX IF NOT EXISTS lookup_silo_user_by_silo ON omicron.public.silo_user ( - silo_id, - external_id -) WHERE - time_deleted IS NULL; +/* This index lets us quickly find users for a given silo, and prevents + multiple users from having the same external id (for certain provision + types). */ +CREATE UNIQUE INDEX IF NOT EXISTS + lookup_silo_user_by_silo +ON + omicron.public.silo_user (silo_id, external_id) +WHERE + time_deleted IS NULL AND + (user_provision_type = 'api_only' OR user_provision_type = 'jit'); CREATE TABLE IF NOT EXISTS omicron.public.silo_user_password_hash ( silo_user_id UUID NOT NULL, @@ -924,14 +945,33 @@ CREATE TABLE IF NOT EXISTS omicron.public.silo_group ( time_deleted TIMESTAMPTZ, silo_id UUID NOT NULL, - external_id TEXT NOT NULL + + -- if the user provision type is 'api_only' or 'jit', then this field must + -- contain a value + external_id TEXT, + + user_provision_type omicron.public.user_provision_type, + + CONSTRAINT user_provision_type_required_for_non_deleted CHECK ( + (user_provision_type IS NOT NULL AND time_deleted IS NULL) + OR (time_deleted IS NOT NULL) + ), + + CONSTRAINT external_id_consistency CHECK ( + CASE user_provision_type + WHEN 'api_only' THEN external_id IS NOT NULL + WHEN 'jit' THEN external_id IS NOT NULL + END + ) ); -CREATE UNIQUE INDEX IF NOT EXISTS lookup_silo_group_by_silo ON omicron.public.silo_group ( - silo_id, - external_id -) WHERE - time_deleted IS NULL; +CREATE UNIQUE INDEX IF NOT EXISTS + lookup_silo_group_by_silo +ON + omicron.public.silo_group (silo_id, external_id) +WHERE + time_deleted IS NULL and + (user_provision_type = 'api_only' OR user_provision_type = 'jit'); /* * Silo group membership @@ -6695,7 +6735,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '195.0.0', NULL) + (TRUE, NOW(), NOW(), '196.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up01.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up01.sql new file mode 100644 index 00000000000..ad313cbcf5d --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up01.sql @@ -0,0 +1,5 @@ +ALTER TABLE + omicron.public.silo_user +ADD COLUMN IF NOT EXISTS + user_provision_type omicron.public.user_provision_type +DEFAULT NULL; diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up02.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up02.sql new file mode 100644 index 00000000000..b7d1e4f382b --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up02.sql @@ -0,0 +1,8 @@ +UPDATE + omicron.public.silo_user +SET + user_provision_type = silo.user_provision_type +FROM + silo +WHERE + silo.id = silo_user.silo_id AND silo_user.time_deleted is null; diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up03.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up03.sql new file mode 100644 index 00000000000..df5ebe446d5 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up03.sql @@ -0,0 +1,6 @@ +ALTER TABLE + omicron.public.silo_user +ADD CONSTRAINT IF NOT EXISTS user_provision_type_required_for_non_deleted CHECK ( + (user_provision_type IS NOT NULL AND time_deleted IS NULL) + OR (time_deleted IS NOT NULL) +) diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up04.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up04.sql new file mode 100644 index 00000000000..b22f06fdf46 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up04.sql @@ -0,0 +1,5 @@ +ALTER TABLE + omicron.public.silo_user +ALTER COLUMN + external_id +DROP NOT NULL diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up05.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up05.sql new file mode 100644 index 00000000000..eececff09d0 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up05.sql @@ -0,0 +1,10 @@ +ALTER TABLE + omicron.public.silo_user +ADD CONSTRAINT IF NOT EXISTS + external_id_consistency +CHECK ( + CASE user_provision_type + WHEN 'api_only' THEN external_id IS NOT NULL + WHEN 'jit' THEN external_id IS NOT NULL + END +) diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up06.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up06.sql new file mode 100644 index 00000000000..a61689dc176 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up06.sql @@ -0,0 +1 @@ +DROP INDEX IF EXISTS lookup_silo_user_by_silo; diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up07.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up07.sql new file mode 100644 index 00000000000..291a17c3373 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up07.sql @@ -0,0 +1,7 @@ +CREATE UNIQUE INDEX IF NOT EXISTS + lookup_silo_user_by_silo +ON + omicron.public.silo_user (silo_id, external_id) +WHERE + time_deleted IS NULL AND + (user_provision_type = 'api_only' OR user_provision_type = 'jit'); diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up08.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up08.sql new file mode 100644 index 00000000000..19f94694f54 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up08.sql @@ -0,0 +1,5 @@ +ALTER TABLE + omicron.public.silo_group +ADD COLUMN IF NOT EXISTS + user_provision_type omicron.public.user_provision_type +DEFAULT NULL; diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up09.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up09.sql new file mode 100644 index 00000000000..9421dca95b3 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up09.sql @@ -0,0 +1,8 @@ +UPDATE + omicron.public.silo_group +SET + user_provision_type = silo.user_provision_type +FROM + silo +WHERE + silo.id = silo_group.silo_id AND silo_group.time_deleted is null; diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up10.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up10.sql new file mode 100644 index 00000000000..538821e5984 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up10.sql @@ -0,0 +1,6 @@ +ALTER TABLE + omicron.public.silo_group +ADD CONSTRAINT IF NOT EXISTS user_provision_type_required_for_non_deleted CHECK ( + (user_provision_type IS NOT NULL AND time_deleted IS NULL) + OR (time_deleted IS NOT NULL) +) diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up11.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up11.sql new file mode 100644 index 00000000000..06fd66cb66a --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up11.sql @@ -0,0 +1,5 @@ +ALTER TABLE + omicron.public.silo_group +ALTER COLUMN + external_id +DROP NOT NULL diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up12.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up12.sql new file mode 100644 index 00000000000..6d9abc9de1c --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up12.sql @@ -0,0 +1,10 @@ +ALTER TABLE + omicron.public.silo_group +ADD CONSTRAINT IF NOT EXISTS + external_id_consistency +CHECK ( + CASE user_provision_type + WHEN 'api_only' THEN external_id IS NOT NULL + WHEN 'jit' THEN external_id IS NOT NULL + END +) diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up13.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up13.sql new file mode 100644 index 00000000000..981120de5e6 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up13.sql @@ -0,0 +1 @@ +DROP INDEX IF EXISTS lookup_silo_group_by_silo; diff --git a/schema/crdb/user-provision-type-for-silo-user-and-group/up14.sql b/schema/crdb/user-provision-type-for-silo-user-and-group/up14.sql new file mode 100644 index 00000000000..edf08223dd7 --- /dev/null +++ b/schema/crdb/user-provision-type-for-silo-user-and-group/up14.sql @@ -0,0 +1,7 @@ +CREATE UNIQUE INDEX IF NOT EXISTS + lookup_silo_group_by_silo +ON + omicron.public.silo_group (silo_id, external_id) +WHERE + time_deleted IS NULL AND + (user_provision_type = 'api_only' OR user_provision_type = 'jit');