-
Notifications
You must be signed in to change notification settings - Fork 58
Add a user provision type to silo user and group #8981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4405524
d41807a
e5b315b
5690907
e0f74d6
1aee591
683ad8e
87d5a66
7d2e958
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,11 @@ | |
// License, v. 2.0. If a copy of the MPL was not distributed with this | ||
// file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
||
use super::UserProvisionType; | ||
use crate::DbTypedUuid; | ||
use crate::to_db_typed_uuid; | ||
use db_macros::Asset; | ||
use nexus_db_schema::schema::{silo_group, silo_group_membership}; | ||
use nexus_types::external_api::views; | ||
use nexus_types::identity::Asset; | ||
use omicron_uuid_kinds::SiloGroupKind; | ||
use omicron_uuid_kinds::SiloGroupUuid; | ||
use omicron_uuid_kinds::SiloUserKind; | ||
|
@@ -20,17 +19,50 @@ use uuid::Uuid; | |
#[asset(uuid_kind = SiloGroupKind)] | ||
pub struct SiloGroup { | ||
#[diesel(embed)] | ||
identity: SiloGroupIdentity, | ||
pub identity: SiloGroupIdentity, | ||
pub time_deleted: Option<chrono::DateTime<chrono::Utc>>, | ||
|
||
pub silo_id: Uuid, | ||
|
||
/// The identity provider's name for this group. | ||
pub external_id: String, | ||
/// If the user provision type is ApiOnly or JIT, then the external id is | ||
/// the identity provider's ID for this group. There is a database | ||
/// constraint (`lookup_silo_group_by_silo`) that ensures this field must be | ||
/// non-null for those provision types. | ||
/// | ||
/// For SCIM, this may be null, which would trigger the uniqueness | ||
/// constraint if that wasn't limited to specific provision types. | ||
pub external_id: Option<String>, | ||
|
||
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 { | ||
Comment on lines
+40
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why two separate constructors instead of taking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right yeah, and specifically the |
||
Self { | ||
identity: SiloGroupIdentity::new(id), | ||
time_deleted: None, | ||
silo_id, | ||
user_provision_type: UserProvisionType::Jit, | ||
external_id: Some(external_id), | ||
} | ||
} | ||
} | ||
|
||
|
@@ -53,14 +85,3 @@ impl SiloGroupMembership { | |
} | ||
} | ||
} | ||
|
||
impl From<SiloGroup> 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, | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(), | ||
) | ||
} | ||
}; | ||
Comment on lines
+195
to
+212
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this sure seems like it would be a lot simpler if we just had one constructor that you pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's additional behaviour changes required in this function (and others) based on the user provision type. Hopefully the additional "add the scim user provision type" PR will make this make more sense, I just wanted to break out the changes in the smallest parts that made sense (maybe this one was too small!). |
||
|
||
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need to be fully pub or can we make them
pub(crate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these need to be fully pub as the DataStore structs need to access those fields.