minor: refactor session create methods (#7827)

There should be no functional changes here, though the internal error
messages are now slightly different between saml login and local login,
where before they were the same. Ran into this while working #7339. This
logic (which I wrote originally and shuffled around in #7374) never
really made sense, and I figured it's good prep for #7818 and friends to
clean it up.

The core of the change here is updating two existing functions that
returned `Result<Option<User>, Error>` to just return `Result<User,
Error>` and move the logic about what we do when the user was `None`
inside each function. In both cases, when the user was `None` we ended
up with an `Error::Unauthenticated` anyway, so we can just do that a
moment earlier and eliminate a lot of misdirection.

Author: david-crespo

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<nexus_db_queries::db::model::SiloUser>,
-    ) -> Result<ConsoleSession, Error> {
-        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)
-    }
 }

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<bool, Error> {
-        // 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<db::model::ConsoleSession> {
-        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
     }
 

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<Option<db::model::SiloUser>> {
+    ) -> LookupResult<db::model::SiloUser> {
         // 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<Option<db::model::SiloUser>, Error> {
+    ) -> Result<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
@@ -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(),
+            })
         }
     }
 

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(),
             );

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?
 }