diff --git a/nexus/src/app/login.rs b/nexus/src/app/login.rs index f47ae264d99..712267cbfee 100644 --- a/nexus/src/app/login.rs +++ b/nexus/src/app/login.rs @@ -6,9 +6,7 @@ use dropshot::{HttpError, HttpResponseFound, http_response_found}; use nexus_auth::context::OpContext; use nexus_db_model::{ConsoleSession, Name}; use nexus_db_queries::authn::silos::IdentityProviderType; -use nexus_db_queries::db::identity::Asset; use nexus_types::external_api::{params::RelativeUri, shared::RelayState}; -use omicron_common::api::external::Error; impl super::Nexus { pub(crate) async fn login_saml_redirect( @@ -77,28 +75,11 @@ impl super::Nexus { &authenticated_subject, ) .await?; - let session = self.create_session(opctx, user).await?; + let session = self.session_create(opctx, &user).await?; let next_url = relay_state .and_then(|r| r.redirect_uri) .map(|u| u.to_string()) .unwrap_or_else(|| "/".to_string()); Ok((session, next_url)) } - - // TODO: move this logic, it's weird - pub(crate) async fn create_session( - &self, - opctx: &OpContext, - user: Option, - ) -> Result { - let session = match user { - Some(user) => self.session_create(&opctx, user.id()).await?, - None => Err(Error::Unauthenticated { - internal_message: String::from( - "no matching user found or credentials were not valid", - ), - })?, - }; - Ok(session) - } } diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index d043fc6474d..30489112b35 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -10,6 +10,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::lookup::LookupPath; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DeleteResult; @@ -34,40 +35,13 @@ fn generate_session_token() -> String { } impl super::Nexus { - async fn login_allowed( - &self, - opctx: &OpContext, - silo_user_id: Uuid, - ) -> Result { - // Was this silo user deleted? - let fetch_result = LookupPath::new(opctx, &self.db_datastore) - .silo_user_id(silo_user_id) - .fetch() - .await; - - match fetch_result { - // if the silo user was deleted, they're not allowed to log in :) - Err(Error::ObjectNotFound { .. }) => Ok(false), - Err(e) => Err(e), - // they're allowed - Ok(_) => Ok(true), - } - } - pub(crate) async fn session_create( &self, opctx: &OpContext, - user_id: Uuid, + user: &db::model::SiloUser, ) -> CreateResult { - if !self.login_allowed(opctx, user_id).await? { - return Err(Error::Unauthenticated { - internal_message: "User not allowed to login".to_string(), - }); - } - let session = - db::model::ConsoleSession::new(generate_session_token(), user_id); - + db::model::ConsoleSession::new(generate_session_token(), user.id()); self.db_datastore.session_create(opctx, session).await } diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index 39d70a15eaf..2a28b005215 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -369,7 +369,7 @@ 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?; @@ -383,35 +383,38 @@ impl super::Nexus { ) .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 Ok(None); - } + 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(), + }); + } - // 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(), - Uuid::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 = db::model::SiloUser::new( + authz_silo.id(), + Uuid::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) + .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 @@ -460,7 +463,7 @@ impl super::Nexus { ) .await?; - Ok(Some(db_silo_user)) + Ok(db_silo_user) } // Silo user passwords @@ -587,7 +590,7 @@ impl super::Nexus { opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, credentials: params::UsernamePasswordCredentials, - ) -> Result, Error> { + ) -> 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 @@ -617,9 +620,11 @@ impl super::Nexus { "passed password verification without a valid user" ); let db_user = fetch_user.unwrap().1; - Ok(Some(db_user)) + Ok(db_user) } else { - Ok(None) + Err(Error::Unauthenticated { + internal_message: "Failed password verification".to_string(), + }) } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 40da27505e7..e28c2c3e589 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7399,7 +7399,7 @@ impl NexusExternalApi for NexusExternalApiImpl { let user = nexus.login_local(&opctx, &silo_lookup, credentials).await?; - let session = nexus.create_session(opctx, user).await?; + let session = nexus.session_create(opctx, &user).await?; let mut response = HttpResponseHeaders::new_unnamed( HttpResponseUpdatedNoContent(), ); diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index 32a90dc6721..587c4b69387 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -330,7 +330,6 @@ async fn test_silo_admin_group(cptestctx: &ControlPlaneTestContext) { }, ) .await - .unwrap() .unwrap(); let group_memberships = nexus @@ -824,13 +823,12 @@ async fn test_silo_user_provision_types(cptestctx: &ControlPlaneTestContext) { groups: vec![], }, ) - .await - .unwrap(); + .await; if test_case.expect_user { - assert!(existing_silo_user.is_some()); + assert!(existing_silo_user.is_ok()); } else { - assert!(existing_silo_user.is_none()); + assert!(existing_silo_user.is_err()); } NexusRequest::object_delete(&client, &"/v1/system/silos/test-silo") @@ -1063,7 +1061,6 @@ async fn test_silo_groups_jit(cptestctx: &ControlPlaneTestContext) { }, ) .await - .unwrap() .unwrap(); let group_memberships = nexus @@ -1137,7 +1134,6 @@ async fn test_silo_groups_fixed(cptestctx: &ControlPlaneTestContext) { }, ) .await - .unwrap() .unwrap(); let group_memberships = nexus @@ -1193,7 +1189,6 @@ async fn test_silo_groups_remove_from_one_group( }, ) .await - .unwrap() .unwrap(); // Check those groups were created and the user was added @@ -1236,7 +1231,6 @@ async fn test_silo_groups_remove_from_one_group( }, ) .await - .unwrap() .unwrap(); let group_memberships = nexus @@ -1306,7 +1300,6 @@ async fn test_silo_groups_remove_from_both_groups( }, ) .await - .unwrap() .unwrap(); // Check those groups were created and the user was added @@ -1349,7 +1342,6 @@ async fn test_silo_groups_remove_from_both_groups( }, ) .await - .unwrap() .unwrap(); let group_memberships = nexus @@ -1418,8 +1410,7 @@ async fn test_silo_delete_clean_up_groups(cptestctx: &ControlPlaneTestContext) { }, ) .await - .expect("silo_user_from_authenticated_subject") - .unwrap(); + .expect("silo_user_from_authenticated_subject"); // Delete the silo NexusRequest::object_delete(&client, &"/v1/system/silos/test-silo") @@ -1501,8 +1492,7 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) { }, ) .await - .expect("silo_user_from_authenticated_subject 1") - .unwrap(); + .expect("silo_user_from_authenticated_subject 1"); // Add the first user with a group membership let _silo_user_2 = nexus @@ -1516,8 +1506,7 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) { }, ) .await - .expect("silo_user_from_authenticated_subject 2") - .unwrap(); + .expect("silo_user_from_authenticated_subject 2"); // TODO-coverage were we intending to verify something here? }