V1 APIs for Silos, Identity Providers, and Users #2442
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,30 +6,49 @@ | |||||
|
|
||||||
| use crate::authz::ApiResource; | ||||||
| use crate::context::OpContext; | ||||||
| use crate::db::identity::{Asset, Resource}; | ||||||
| use crate::db::lookup::LookupPath; | ||||||
| use crate::db::model::Name; | ||||||
| use crate::db::model::SshKey; | ||||||
| use crate::db::{self, lookup}; | ||||||
| use crate::external_api::params; | ||||||
| use crate::external_api::shared; | ||||||
| use crate::{authn, authz}; | ||||||
| use anyhow::Context; | ||||||
| use nexus_db_model::UserProvisionType; | ||||||
| use omicron_common::api::external::http_pagination::PaginatedBy; | ||||||
| use omicron_common::api::external::Error; | ||||||
| use omicron_common::api::external::ListResultVec; | ||||||
| use omicron_common::api::external::LookupResult; | ||||||
| use omicron_common::api::external::UpdateResult; | ||||||
| use omicron_common::api::external::{CreateResult, LookupType}; | ||||||
| use omicron_common::api::external::{DataPageParams, ResourceType}; | ||||||
| use omicron_common::api::external::{DeleteResult, NameOrId}; | ||||||
| use omicron_common::bail_unless; | ||||||
| use ref_cast::RefCast; | ||||||
| use std::str::FromStr; | ||||||
| use uuid::Uuid; | ||||||
|
|
||||||
| impl super::Nexus { | ||||||
| // Silos | ||||||
| pub fn silo_lookup<'a>( | ||||||
| &'a self, | ||||||
| opctx: &'a OpContext, | ||||||
| silo: &'a NameOrId, | ||||||
| ) -> LookupResult<lookup::Silo<'a>> { | ||||||
| match silo { | ||||||
| NameOrId::Id(id) => { | ||||||
| let silo = | ||||||
| LookupPath::new(opctx, &self.db_datastore).silo_id(*id); | ||||||
| Ok(silo) | ||||||
| } | ||||||
| NameOrId::Name(name) => { | ||||||
| let silo = LookupPath::new(opctx, &self.db_datastore) | ||||||
| .silo_name(Name::ref_cast(name)); | ||||||
| Ok(silo) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| pub async fn silo_create( | ||||||
| &self, | ||||||
|
|
@@ -54,40 +73,13 @@ impl super::Nexus { | |||||
| self.db_datastore.silos_list(opctx, pagparams).await | ||||||
| } | ||||||
|
|
||||||
| pub async fn silo_delete( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| ) -> DeleteResult { | ||||||
| let (.., authz_silo, db_silo) = | ||||||
| silo_lookup.fetch_for(authz::Action::Delete).await?; | ||||||
| self.db_datastore.silo_delete(opctx, &authz_silo, &db_silo).await | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -96,7 +88,7 @@ impl super::Nexus { | |||||
| pub async fn silo_fetch_policy( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| ) -> LookupResult<shared::Policy<authz::SiloRole>> { | ||||||
| let (.., authz_silo) = | ||||||
| silo_lookup.lookup_for(authz::Action::ReadPolicy).await?; | ||||||
|
|
@@ -114,7 +106,7 @@ impl super::Nexus { | |||||
| pub async fn silo_update_policy( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| policy: &shared::Policy<authz::SiloRole>, | ||||||
| ) -> UpdateResult<shared::Policy<authz::SiloRole>> { | ||||||
| let (.., authz_silo) = | ||||||
|
|
@@ -164,13 +156,10 @@ impl super::Nexus { | |||||
| pub async fn silo_list_users( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| pagparams: &DataPageParams<'_, Uuid>, | ||||||
| ) -> ListResultVec<db::model::SiloUser> { | ||||||
| let (authz_silo,) = silo_lookup.lookup_for(authz::Action::Read).await?; | ||||||
| let authz_silo_user_list = authz::SiloUserList::new(authz_silo); | ||||||
| self.db_datastore | ||||||
| .silo_users_list(opctx, &authz_silo_user_list, pagparams) | ||||||
|
|
@@ -181,13 +170,10 @@ impl super::Nexus { | |||||
| pub async fn silo_user_fetch( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| silo_user_id: Uuid, | ||||||
| ) -> LookupResult<db::model::SiloUser> { | ||||||
| let (authz_silo,) = silo_lookup.lookup_for(authz::Action::Read).await?; | ||||||
|
Contributor
There was a problem hiding this comment. As discussed in chat with @zephraph, we considered the possibility that this would succeed at looking up a user by ID as long as the specified silo exists, even if the silo doesn't match the user's ID (because the silo is technically not needed for the lookup if we have a user ID to look up directly). But there is an explicit check that the retrieved user's silo ID matches the one passed in: Lines 156 to 158 in 7be926a |
||||||
| let (_, db_silo_user) = self | ||||||
| .silo_user_lookup_by_id( | ||||||
| opctx, | ||||||
|
|
@@ -208,13 +194,9 @@ impl super::Nexus { | |||||
| /// provider. | ||||||
| async fn local_idp_fetch_silo( | ||||||
| &self, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| ) -> LookupResult<(authz::Silo, db::model::Silo)> { | ||||||
| let (authz_silo, db_silo) = silo_lookup.fetch().await?; | ||||||
| if db_silo.user_provision_type != UserProvisionType::ApiOnly { | ||||||
| return Err(Error::not_found_by_name( | ||||||
| ResourceType::IdentityProvider, | ||||||
|
|
@@ -229,11 +211,11 @@ impl super::Nexus { | |||||
| pub async fn local_idp_create_user( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| new_user_params: params::UserCreate, | ||||||
| ) -> CreateResult<db::model::SiloUser> { | ||||||
| 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 | ||||||
|
|
@@ -267,11 +249,11 @@ impl super::Nexus { | |||||
| pub async fn local_idp_delete_user( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| silo_user_id: Uuid, | ||||||
| ) -> DeleteResult { | ||||||
| let (authz_silo, _) = self.local_idp_fetch_silo(silo_lookup).await?; | ||||||
|
|
||||||
| let (authz_silo_user, _) = self | ||||||
| .silo_user_lookup_by_id( | ||||||
| opctx, | ||||||
|
|
@@ -394,12 +376,12 @@ impl super::Nexus { | |||||
| pub async fn local_idp_user_set_password( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| silo_user_id: Uuid, | ||||||
| password_value: params::UserPassword, | ||||||
| ) -> UpdateResult<()> { | ||||||
| let (authz_silo, db_silo) = | ||||||
| self.local_idp_fetch_silo(silo_lookup).await?; | ||||||
| let (authz_silo_user, db_silo_user) = self | ||||||
| .silo_user_lookup_by_id( | ||||||
| opctx, | ||||||
|
|
@@ -506,11 +488,10 @@ impl super::Nexus { | |||||
| pub async fn login_local( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| credentials: params::UsernamePasswordCredentials, | ||||||
| ) -> Result<Option<db::model::SiloUser>, Error> { | ||||||
| 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 | ||||||
| // find a user with this external id. See the note in | ||||||
|
|
@@ -640,16 +621,46 @@ impl super::Nexus { | |||||
|
|
||||||
| // identity providers | ||||||
|
|
||||||
| pub fn saml_identity_provider_lookup<'a>( | ||||||
| &'a self, | ||||||
| opctx: &'a OpContext, | ||||||
| saml_identity_provider_selector: &'a params::SamlIdentityProviderSelector, | ||||||
| ) -> LookupResult<lookup::SamlIdentityProvider<'a>> { | ||||||
| match saml_identity_provider_selector { | ||||||
| params::SamlIdentityProviderSelector { | ||||||
| saml_identity_provider: NameOrId::Id(id), | ||||||
| silo_selector: None, | ||||||
| } => { | ||||||
|
|
||||||
| let saml_provider = LookupPath::new(opctx, &self.db_datastore) | ||||||
| .saml_identity_provider_id(*id); | ||||||
| Ok(saml_provider) | ||||||
| } | ||||||
| params::SamlIdentityProviderSelector { | ||||||
| saml_identity_provider: NameOrId::Name(name), | ||||||
| silo_selector: Some(silo_selector), | ||||||
| } => { | ||||||
| let saml_provider = self | ||||||
| .silo_lookup(opctx, &silo_selector.silo)? | ||||||
| .saml_identity_provider_name(Name::ref_cast(name)); | ||||||
| Ok(saml_provider) | ||||||
| } | ||||||
| params::SamlIdentityProviderSelector { | ||||||
| saml_identity_provider: NameOrId::Id(_), | ||||||
| silo_selector: Some(_), | ||||||
| } => Err(Error::invalid_request("when providing provider as an ID, silo should not be specified")), | ||||||
| _ => Err(Error::invalid_request("provider should either be a UUID or silo should be specified")) | ||||||
|
Contributor
There was a problem hiding this comment. nittiest of nits
Suggested change
Contributor
Author
There was a problem hiding this comment. I'm 👍 to this, but the message is consistent between all the other lookup functions. Maybe we should make this change across all of them at the same time? Also makes me wonder if the should just be a lookup error that wraps around these cases 🤔
Contributor
There was a problem hiding this comment. Yeah, probably do a followup to change them all. |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| pub async fn identity_provider_list( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| pagparams: &PaginatedBy<'_>, | ||||||
| ) -> ListResultVec<db::model::IdentityProvider> { | ||||||
| // TODO-security: This should likely be lookup_for ListChildren on the silo | ||||||
| let (.., authz_silo, _) = silo_lookup.fetch().await?; | ||||||
| let authz_idp_list = authz::SiloIdentityProviderList::new(authz_silo); | ||||||
| self.db_datastore | ||||||
| .identity_provider_list(opctx, &authz_idp_list, pagparams) | ||||||
|
|
@@ -661,13 +672,11 @@ impl super::Nexus { | |||||
| pub async fn saml_identity_provider_create( | ||||||
| &self, | ||||||
| opctx: &OpContext, | ||||||
| silo_lookup: &lookup::Silo<'_>, | ||||||
| params: params::SamlIdentityProviderCreate, | ||||||
| ) -> CreateResult<db::model::SamlIdentityProvider> { | ||||||
| // TODO-security: This should likely be fetch_for CreateChild on the silo | ||||||
| let (authz_silo, db_silo) = silo_lookup.fetch().await?; | ||||||
|
Contributor
There was a problem hiding this comment. wouldn't that auth check get done when you actually do the creating?
Contributor
Author
There was a problem hiding this comment. This is a bit of an imperfect process across the API. We do a lot of the authz calls twice. Once at the entrance of the nexus layer and again at the datastore layer. Sometimes the nexus layer has more authz checks because there are logical permissions that must be validated beyond just what the datastore layer would check. @davepacheco has mentioned potentially caching duplicatively authz queries. The pattern here is fairly typical though. Do an authz check at the entrance of the nexus function. |
||||||
| let authz_idp_list = authz::SiloIdentityProviderList::new(authz_silo); | ||||||
|
|
||||||
| if db_silo.user_provision_type != UserProvisionType::Jit { | ||||||
|
|
@@ -792,21 +801,6 @@ impl super::Nexus { | |||||
| .await | ||||||
| } | ||||||
|
|
||||||
| pub fn silo_group_lookup<'a>( | ||||||
| &'a self, | ||||||
| opctx: &'a OpContext, | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This lookup mechanism differs from the normal lookups in that it doesn't take a selector. Once we've moved over to the V1 API I fully expect we'll simplify the selector implementations anyway so I think it's fine that this one case is slightly different.
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.
I'm doing something similar here:
https://github.com/oxidecomputer/omicron/pull/2455/files#diff-882861bfb859d6d0519f20dfcb9abad82e23d52a6963f921fb144a8d9fc2f273R810