From 440552410625bc08f73ce4f03f8cb363cad3daa1 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Tue, 19 Aug 2025 17:40:29 +0000 Subject: [PATCH 01/10] Add a user provision type to silo user and group Silo users and groups have traditionally defined "external id" as whatever the silo's identity provider (or in the case of ApiOnly silos, an operator) assigns for them: this could be username, email, a random id, etc. SCIM throws one of many wrenches at us because the specification defines an external id attribute for users and groups that is distinct from other related attributes that could be sent, including username and email. Importantly the SCIM client that is creating a user or group could send a NULL external ID, which is not supported by the current field's database type. The first step to supporting SCIM's extra attributes is to add the silo's user provision field to our user and group models, and use this field to change the semantics of the external id field depending on what provision type is used. For the existing ApiOnly and JIT silos this will stay the same. The second step that will help with type safety is to use this new user provision type field to separate the silo user and group database models into distinct silo user and group types at the datastore level, and have separate enum variants for each type that wrap an inner typed object: ```rust pub enum SiloUser { /// A User created via the API ApiOnly(SiloUserApiOnly), /// A User created during an authenticated SAML login Jit(SiloUserJit), } ``` Call sites now have to use either the ApiOnly or Jit variant of this new higher level silo user type. With the addition of user provision type to the silo user model, Nexus can also now check that the provision type on the user matches the silo, or (like with some of the local idp functions) accept only one specific variant. The third step that will also help with type safety is to have typed lookups for silo users and groups, as different user provision types will have different fields that they enforce uniqueness for: for example, only search for ApiOnly and JIT users using external_id, but for SCIM use username. One possible bug that this commit addresses is that the creation of the recovery silo did not have a check to ensure that the created silo was a "LocalOnly" identity type. The code prior to this commit could create a SamlJit silo, which is not ideal for a recovery silo! Another possible bug potential addressed is a mismatch in the silo's user provision type, and the users within that silo. This can be enforced via functions that only accept one variant of the new higher level enum, and is checked at the lower layers. Another bug addressed: in `silo_group_ensure` there was a small window between `silo_group_ensure_query` and unwrapping the result of `silo_group_optional_lookup` where, if it were possible to delete groups, a racing delete would cause the unwrap to panic. Also! Fill in `test_ensure_same_silo_group`, why that was left truncated is beyond me, but don't look at the git blame please. --- nexus/db-fixed-data/src/silo_user.rs | 7 +- nexus/db-model/src/schema_versions.rs | 3 +- nexus/db-model/src/silo_group.rs | 56 ++- nexus/db-model/src/silo_user.rs | 38 +- nexus/db-queries/src/db/datastore/mod.rs | 20 +- nexus/db-queries/src/db/datastore/rack.rs | 33 +- nexus/db-queries/src/db/datastore/silo.rs | 41 +- .../db-queries/src/db/datastore/silo_group.rs | 431 ++++++++++++++++-- .../db-queries/src/db/datastore/silo_user.rs | 390 ++++++++++++++-- nexus/db-queries/src/policy_test/mod.rs | 129 ++++-- .../src/policy_test/resource_builder.rs | 6 +- nexus/db-schema/src/schema.rs | 8 +- nexus/src/app/iam.rs | 14 +- nexus/src/app/session.rs | 4 +- nexus/src/app/silo.rs | 229 ++++++---- nexus/src/external_api/http_entrypoints.rs | 3 +- nexus/tests/integration_tests/console_api.rs | 7 +- nexus/tests/integration_tests/silo_users.rs | 14 +- nexus/tests/integration_tests/silos.rs | 102 ++++- schema/crdb/dbinit.sql | 68 ++- .../up01.sql | 5 + .../up02.sql | 8 + .../up03.sql | 6 + .../up04.sql | 5 + .../up05.sql | 10 + .../up06.sql | 1 + .../up07.sql | 7 + .../up08.sql | 5 + .../up09.sql | 8 + .../up10.sql | 6 + .../up11.sql | 5 + .../up12.sql | 10 + .../up13.sql | 1 + .../up14.sql | 7 + 34 files changed, 1391 insertions(+), 296 deletions(-) create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up01.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up02.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up03.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up04.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up05.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up06.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up07.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up08.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up09.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up10.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up11.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up12.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up13.sql create mode 100644 schema/crdb/user-provision-type-for-silo-user-and-group/up14.sql 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 5b259c0d7e3..c938aca78ed 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(186, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(187, 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(187, "user-provision-type-for-silo-user-and-group"), KnownVersion::new(186, "nexus-generation"), KnownVersion::new(185, "populate-db-metadata-nexus"), KnownVersion::new(184, "store-silo-admin-group-name"), diff --git a/nexus/db-model/src/silo_group.rs b/nexus/db-model/src/silo_group.rs index 384b74edf53..1918fe9b5d3 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,49 @@ 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 that 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 +84,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..857542c4e97 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,24 @@ 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 + /// that 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 +40,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 76862d92769..6129aa85a67 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -138,6 +138,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; @@ -483,7 +491,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; @@ -621,11 +629,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(); @@ -1760,11 +1769,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 8b090bd2c69..c9a7934d7d7 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; @@ -435,6 +436,18 @@ impl DataStore { recovery_user_password_hash: omicron_passwords::PasswordHashString, dns_update: DnsVersionUpdateBuilder, ) -> Result<(), RackInitError> { + match &recovery_silo.identity_mode { + shared::SiloIdentityMode::LocalOnly => { + // Ok + } + + shared::SiloIdentityMode::SamlJit => { + return Err(RackInitError::Silo(Error::invalid_request( + "recovery silo should only use identity mode LocalOnly", + ))); + } + } + let db_silo = self .silo_create_conn( conn, @@ -453,11 +466,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 +1271,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..3d9c28394b7 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -7,11 +7,12 @@ 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; @@ -20,6 +21,8 @@ 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 +30,236 @@ 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, + // SAFETY: there is a database constraint that prevents a group + // with provision type 'api_only' from having a null external id + external_id: record.external_id.unwrap(), + }) + } + + 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, + // SAFETY: there is a database constraint that prevents a group + // with provision type 'jit' from having a null external id + external_id: record.external_id.unwrap(), + }), + } + } +} + +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: chrono::DateTime, + pub time_modified: chrono::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: chrono::Utc::now(), + time_modified: chrono::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: chrono::DateTime, + pub time_modified: chrono::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: chrono::Utc::now(), + time_modified: chrono::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 +277,136 @@ 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 { + return Err(Error::invalid_request(format!( + "silo provision type {:?} 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 +426,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 +468,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 +521,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 +616,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..a6292e29b8e 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; @@ -25,6 +25,7 @@ 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 +39,225 @@ 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.unwrap(), + }), + + 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.unwrap(), + }), + } + } +} + +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: chrono::DateTime, + pub time_modified: chrono::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: chrono::Utc::now(), + time_modified: chrono::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: chrono::DateTime, + pub time_modified: chrono::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: chrono::Utc::now(), + time_modified: chrono::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 +268,49 @@ 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::invalid_request( + "user provision type of silo does not match silo user", + )); + } + + 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 +322,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 +359,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 +428,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 +516,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 +572,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 +591,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 23d4aa089e5..734fe3352a6 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -764,7 +764,8 @@ table! { time_deleted -> Nullable, silo_id -> Uuid, - external_id -> Text, + external_id -> Nullable, + user_provision_type -> crate::enums::UserProvisionTypeEnum, } } @@ -784,7 +785,8 @@ table! { time_deleted -> Nullable, silo_id -> Uuid, - external_id -> Text, + external_id -> Nullable, + user_provision_type -> crate::enums::UserProvisionTypeEnum, } } @@ -801,6 +803,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 411ae7b613d..59a19393d7b 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; @@ -270,17 +276,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 @@ -289,7 +296,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 @@ -303,9 +310,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, @@ -313,7 +320,7 @@ impl super::Nexus { authz::Action::Read, ) .await?; - Ok(db_silo_user) + Ok(silo_user) } /// Delete all of user's tokens and sessions @@ -357,14 +364,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 @@ -444,36 +451,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 @@ -503,52 +517,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 @@ -558,13 +583,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? } @@ -574,7 +599,7 @@ impl super::Nexus { .silo_group_lookup_or_create_by_name( opctx, &authz_silo, - &group, + &SiloGroupLookup::Jit { external_id: &group }, ) .await?; @@ -597,7 +622,7 @@ impl super::Nexus { ) .await?; - Ok(db_silo_user) + Ok(silo_user) } // Silo user passwords @@ -616,7 +641,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, @@ -624,11 +650,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 @@ -643,7 +679,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 { @@ -667,7 +703,7 @@ impl super::Nexus { opctx, db_silo, authz_silo_user, - db_silo_user, + silo_user, password_hash, ) .await @@ -724,7 +760,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 @@ -735,10 +771,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 @@ -749,12 +787,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(), @@ -764,15 +803,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), @@ -782,11 +843,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 } @@ -1030,11 +1092,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 c0768d1a8d5..8b8a59af114 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7071,8 +7071,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 552d3cf3d0e..8941d88d1ed 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 @@ -6602,7 +6642,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '186.0.0', NULL) + (TRUE, NOW(), NOW(), '187.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'); From e5b315bbc08a524e3415f035e391d1074aa4ea82 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 24 Sep 2025 12:58:21 +0000 Subject: [PATCH 02/10] match only one silo mode --- nexus/db-queries/src/db/datastore/rack.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 337288e9e68..8b629459a99 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -436,16 +436,13 @@ impl DataStore { recovery_user_password_hash: omicron_passwords::PasswordHashString, dns_update: DnsVersionUpdateBuilder, ) -> Result<(), RackInitError> { - match &recovery_silo.identity_mode { - shared::SiloIdentityMode::LocalOnly => { - // Ok - } - - shared::SiloIdentityMode::SamlJit => { - return Err(RackInitError::Silo(Error::invalid_request( - "recovery silo should only use identity mode LocalOnly", - ))); - } + 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 From 5690907a2539ff116d922ce7a36e3fda57ed24f9 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Wed, 24 Sep 2025 13:02:44 +0000 Subject: [PATCH 03/10] expect database constraint exists --- nexus/db-queries/src/db/datastore/silo_group.rs | 8 ++++++-- nexus/db-queries/src/db/datastore/silo_user.rs | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index 3d9c28394b7..ac2857fb866 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -85,7 +85,9 @@ impl From for SiloGroup { silo_id: record.silo_id, // SAFETY: there is a database constraint that prevents a group // with provision type 'api_only' from having a null external id - external_id: record.external_id.unwrap(), + external_id: record + .external_id + .expect("database constraint exists"), }) } @@ -97,7 +99,9 @@ impl From for SiloGroup { silo_id: record.silo_id, // SAFETY: there is a database constraint that prevents a group // with provision type 'jit' from having a null external id - external_id: record.external_id.unwrap(), + external_id: record + .external_id + .expect("database constraint exists"), }), } } diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index a6292e29b8e..8eac0c40984 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -89,7 +89,9 @@ impl From for SiloUser { 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.unwrap(), + external_id: record + .external_id + .expect("database constraint exists"), }), UserProvisionType::Jit => SiloUser::Jit(SiloUserJit { @@ -100,7 +102,9 @@ impl From for SiloUser { 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.unwrap(), + external_id: record + .external_id + .expect("database constraint exists"), }), } } From 1aee59107f82cf06b9ee40f92743a3b23385edbb Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Fri, 3 Oct 2025 14:53:57 +0000 Subject: [PATCH 04/10] mention the referenced constriant, and drop the SAFETY, it doesn't fit --- nexus/db-model/src/silo_group.rs | 3 ++- nexus/db-model/src/silo_user.rs | 3 ++- nexus/db-queries/src/db/datastore/silo_group.rs | 14 ++++++++------ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/nexus/db-model/src/silo_group.rs b/nexus/db-model/src/silo_group.rs index 1918fe9b5d3..793eb3ef921 100644 --- a/nexus/db-model/src/silo_group.rs +++ b/nexus/db-model/src/silo_group.rs @@ -26,7 +26,8 @@ pub struct SiloGroup { /// 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 that this field must be non-null for those provision types. + /// 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. diff --git a/nexus/db-model/src/silo_user.rs b/nexus/db-model/src/silo_user.rs index 857542c4e97..7ff962244bc 100644 --- a/nexus/db-model/src/silo_user.rs +++ b/nexus/db-model/src/silo_user.rs @@ -21,7 +21,8 @@ pub struct SiloUser { /// 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 - /// that this field must be non-null for those provision types. + /// (`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. diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index ac2857fb866..ba73c19edb6 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -83,11 +83,12 @@ impl From for SiloGroup { time_modified: record.time_modified(), time_deleted: record.time_deleted, silo_id: record.silo_id, - // SAFETY: there is a database constraint that prevents a group - // with provision type 'api_only' from having a null external 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("database constraint exists"), + .expect("constraint lookup_silo_group_by_silo exists"), }) } @@ -97,11 +98,12 @@ impl From for SiloGroup { time_modified: record.time_modified(), time_deleted: record.time_deleted, silo_id: record.silo_id, - // SAFETY: there is a database constraint that prevents a group - // with provision type 'jit' from having a null external 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("database constraint exists"), + .expect("constraint lookup_silo_group_by_silo exists"), }), } } From 683ad8ec0765f6c636910469334cf2b9aa6bd6d9 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Fri, 3 Oct 2025 15:02:40 +0000 Subject: [PATCH 05/10] just import chrono already haha --- .../db-queries/src/db/datastore/silo_group.rs | 21 ++++++++++--------- .../db-queries/src/db/datastore/silo_user.rs | 21 ++++++++++--------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index ba73c19edb6..b0a908cbae5 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -16,6 +16,7 @@ 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; @@ -130,9 +131,9 @@ impl From for views::Group { #[derive(Debug, Clone)] pub struct SiloGroupApiOnly { pub id: SiloGroupUuid, - pub time_created: chrono::DateTime, - pub time_modified: chrono::DateTime, - pub time_deleted: Option>, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, pub silo_id: Uuid, /// The identity provider's ID for this group. @@ -143,8 +144,8 @@ impl SiloGroupApiOnly { pub fn new(silo_id: Uuid, id: SiloGroupUuid, external_id: String) -> Self { Self { id, - time_created: chrono::Utc::now(), - time_modified: chrono::Utc::now(), + time_created: Utc::now(), + time_modified: Utc::now(), time_deleted: None, silo_id, external_id, @@ -188,9 +189,9 @@ impl From for views::Group { #[derive(Debug, Clone)] pub struct SiloGroupJit { pub id: SiloGroupUuid, - pub time_created: chrono::DateTime, - pub time_modified: chrono::DateTime, - pub time_deleted: Option>, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, pub silo_id: Uuid, /// The identity provider's ID for this user. @@ -201,8 +202,8 @@ impl SiloGroupJit { pub fn new(silo_id: Uuid, id: SiloGroupUuid, external_id: String) -> Self { Self { id, - time_created: chrono::Utc::now(), - time_modified: chrono::Utc::now(), + time_created: Utc::now(), + time_modified: Utc::now(), time_deleted: None, silo_id, external_id, diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index 8eac0c40984..1aed1533ab5 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -20,6 +20,7 @@ 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; @@ -131,9 +132,9 @@ impl From for views::User { #[derive(Debug, Clone)] pub struct SiloUserApiOnly { pub id: SiloUserUuid, - pub time_created: chrono::DateTime, - pub time_modified: chrono::DateTime, - pub time_deleted: Option>, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, pub silo_id: Uuid, /// The identity provider's ID for this user. @@ -144,8 +145,8 @@ impl SiloUserApiOnly { pub fn new(silo_id: Uuid, id: SiloUserUuid, external_id: String) -> Self { Self { id, - time_created: chrono::Utc::now(), - time_modified: chrono::Utc::now(), + time_created: Utc::now(), + time_modified: Utc::now(), time_deleted: None, silo_id, external_id, @@ -189,9 +190,9 @@ impl From for views::User { #[derive(Debug, Clone)] pub struct SiloUserJit { pub id: SiloUserUuid, - pub time_created: chrono::DateTime, - pub time_modified: chrono::DateTime, - pub time_deleted: Option>, + pub time_created: DateTime, + pub time_modified: DateTime, + pub time_deleted: Option>, pub silo_id: Uuid, /// The identity provider's ID for this user. @@ -202,8 +203,8 @@ impl SiloUserJit { pub fn new(silo_id: Uuid, id: SiloUserUuid, external_id: String) -> Self { Self { id, - time_created: chrono::Utc::now(), - time_modified: chrono::Utc::now(), + time_created: Utc::now(), + time_modified: Utc::now(), time_deleted: None, silo_id, external_id, From 87d5a66c2ba809cd736bfced08ea96b2dcf27c39 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Fri, 3 Oct 2025 15:17:32 +0000 Subject: [PATCH 06/10] should be an internal error --- nexus/db-queries/src/db/datastore/silo_group.rs | 14 ++++++++++++-- nexus/db-queries/src/db/datastore/silo_user.rs | 8 +++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index b0a908cbae5..7c185202bdf 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -389,8 +389,18 @@ impl DataStore { silo_group_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 {:?}", + 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, ))); } diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index 1aed1533ab5..1d2500e2010 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -307,9 +307,11 @@ impl DataStore { model.user_provision_type, ); - return Err(Error::invalid_request( - "user provision type of silo does not match silo user", - )); + 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; From 8464f970d365e6143007b9f3d34dfe94e436eb22 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Mon, 6 Oct 2025 19:35:19 +0000 Subject: [PATCH 07/10] how did I miss these invalid_requests --- nexus/db-queries/src/db/datastore/silo_group.rs | 8 +++++--- nexus/db-queries/src/db/datastore/silo_user.rs | 11 ++++++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index 7c185202bdf..d84fb25a756 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -317,9 +317,11 @@ impl DataStore { model.user_provision_type, ); - return Err(Error::invalid_request( - "user provision type of silo does not match silo group", - )); + return Err(Error::internal_error(&format!( + "user provision type of silo ({:?}) does not match \ + silo group ({:?})", + silo.user_provision_type, model.user_provision_type, + ))); } DataStore::silo_group_ensure_query(opctx, authz_silo, model) diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index 1d2500e2010..33e3949a351 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -472,7 +472,16 @@ impl DataStore { 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!( + 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!( "silo provision type {:?} does not match lookup {:?}", silo.user_provision_type, lookup_user_provision_type, ))); From 7a488eb0ce8ff3a0265df4ae54bf421538f5b042 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Mon, 6 Oct 2025 19:41:21 +0000 Subject: [PATCH 08/10] i gapped out here --- nexus/db-queries/src/db/datastore/silo_user.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/silo_user.rs b/nexus/db-queries/src/db/datastore/silo_user.rs index 33e3949a351..199a6a1d8da 100644 --- a/nexus/db-queries/src/db/datastore/silo_user.rs +++ b/nexus/db-queries/src/db/datastore/silo_user.rs @@ -88,11 +88,12 @@ impl From for SiloUser { 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 + // About the expect here: the mentioned database constraint + // prevents a user with provision type 'api_only' from having a + // null external id. external_id: record .external_id - .expect("database constraint exists"), + .expect("constraint lookup_silo_user_by_silo exists"), }), UserProvisionType::Jit => SiloUser::Jit(SiloUserJit { @@ -101,11 +102,12 @@ impl From for SiloUser { 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 + // About the expect here: the mentioned database constraint + // prevents a user with provision type 'jit' from having a null + // external id. external_id: record .external_id - .expect("database constraint exists"), + .expect("constraint lookup_silo_user_by_silo exists"), }), } } From e695d3b8ce61ee7e63dddb6f298c4d85b39dc9d4 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Mon, 6 Oct 2025 19:42:14 +0000 Subject: [PATCH 09/10] mention invalid user provision type --- nexus/src/app/silo.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index e8bdd48418d..b7d368ff567 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -661,7 +661,7 @@ impl super::Nexus { SiloUser::ApiOnly(user) => user, SiloUser::Jit(_) => { return Err(Error::invalid_request( - "invalid user type for password set", + "invalid user type (JIT) for password set", )); } }; From 93b033ba54593c6198d0b66098cfe4c297229b2d Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Mon, 6 Oct 2025 20:14:47 +0000 Subject: [PATCH 10/10] return conflict instead of not found --- .../db-queries/src/db/datastore/silo_group.rs | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/silo_group.rs b/nexus/db-queries/src/db/datastore/silo_group.rs index d84fb25a756..119ce564f02 100644 --- a/nexus/db-queries/src/db/datastore/silo_group.rs +++ b/nexus/db-queries/src/db/datastore/silo_group.rs @@ -31,9 +31,7 @@ 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; @@ -344,14 +342,22 @@ impl DataStore { public_error_from_diesel(e, ErrorHandler::Server) })?; - // This function is _not_ transactional, meaning a delete could race - // between the ensure query and the lookup. + // This function is _not_ transactional, meaning two types of delete + // could race between the ensure query and the lookup: + // + // - a silo delete could win the race, which will delete all the + // groups in that silo (including this newly created one). + // + // - a silo group delete (probably preceded by a silo group lookup + // by name) could win the race, which will delete this specific + // group. + // + // If either deletes win, return a conflict instead of a not found. let Some(silo_group) = maybe_silo_group else { - return Err(Error::not_found_by_id( - ResourceType::SiloGroup, - group_id.as_untyped_uuid(), - )); + return Err(Error::conflict(format!( + "group {group_id} deleted" + ))); }; silo_group