editoast: fix a couple issues in new authentication middlewares

editoast/src/authentication.rs

@@ -18,9 +18,7 @@ pub enum Authentication {
         impersonated_identity: String,
     },
     Skip {
-        #[expect(unused)]
         identity: Option<String>,
-        #[expect(unused)]
         name: Option<String>,
     },
 }
@@ -75,16 +73,4 @@ impl Authentication {
         };
         Ok(authn)
     }
-
-    pub fn origin(&self) -> Option<(&str, &str)> {
-        match self {
-            Authentication::Authenticated { identity, name }
-            | Authentication::Impersonating {
-                impersonator_identity: identity,
-                impersonator_name: name,
-                impersonated_identity: _,
-            } => Some((identity.as_str(), name.as_str())),
-            Authentication::Unauthenticated | Authentication::Skip { .. } => None,
-        }
-    }
 }

editoast/src/views/authz.rs

@@ -1379,6 +1379,66 @@ mod tests {
         );
     }
 
+    #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+    async fn whoami_impersonation() {
+        let app = test_app!().enable_authorization(true).build();
+        let impersonator = app
+            .user("impersonator", "Impersonator")
+            .with_roles([Role::Admin])
+            .create()
+            .await;
+        let impersonated = app
+            .user("impersonated", "Impersonated")
+            .with_roles([Role::Stdcm])
+            .create()
+            .await;
+
+        let request = app
+            .get("/authz/me")
+            .by_user(&impersonator)
+            .impersonate(&impersonated);
+        let user_data = app
+            .fetch(request)
+            .await
+            .assert_status(StatusCode::OK)
+            .json_into::<WhoamiResponse>();
+
+        assert_eq!(
+            user_data,
+            WhoamiResponse {
+                id: impersonated.id,
+                name: "Impersonated".to_string(),
+                roles: vec![Role::Stdcm],
+            }
+        );
+    }
+
+    #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+    async fn whoami_skip_with_user_info() {
+        let app = test_app!().enable_authorization(true).build();
+        let user = app
+            .user("bob", "Bob")
+            .with_roles([Role::Admin])
+            .create()
+            .await;
+
+        let request = app.get("/authz/me").by_user(&user).skip_authz();
+        let user_data = app
+            .fetch(request)
+            .await
+            .assert_status(StatusCode::OK)
+            .json_into::<WhoamiResponse>();
+
+        assert_eq!(
+            user_data,
+            WhoamiResponse {
+                id: user.id,
+                name: "Bob".to_string(),
+                roles: vec![Role::Admin],
+            }
+        );
+    }
+
     #[tokio::test(flavor = "multi_thread", worker_threads = 1)]
     async fn whoami_authorization_disabled() {
         let app = test_app!()

editoast/src/views/server/middlewares.rs

@@ -1,3 +1,5 @@
+use std::convert::Infallible;
+
 use authz::Authorizer;
 use authz::Role;
 use authz::StorageDriver as _;
@@ -95,30 +97,29 @@ pub(in crate::views) async fn authentication_validation_middleware(
     mut req: Request,
     next: Next,
 ) -> Result<Response> {
-    let is_impersonation = matches!(
-        authn,
-        crate::authentication::Authentication::Impersonating { .. }
-    );
-
-    fn origin_user(
-        user: Option<editoast_models::User>,
+    fn warn_mismatched_name(
+        user: &Option<editoast_models::User>,
         identity: &str,
         header_name: &str,
-    ) -> Option<(editoast_models::User, ::authz::v2::Protected<Vec<Role>>)> {
-        user.inspect(|user| {
-            if user.name != header_name {
-                tracing::warn!(
-                    identity,
-                    header = header_name,
-                    stored = user.name,
-                    "provided name for identity differ from stored",
-                );
-            }
-        })
-        .map(|user| {
-            let authz_user = ::authz::Subject::user(user.id);
-            (user, ::authz::v2::subject_roles(authz_user))
-        })
+    ) {
+        if user.as_ref().is_some_and(|u| u.name != header_name) {
+            tracing::warn!(
+                identity,
+                header = header_name,
+                stored = user.as_ref().map(|u| u.name.clone()),
+                "provided name for identity differ from stored",
+            );
+        }
+    }
+
+    fn zip_roles(
+        user: editoast_models::User,
+    ) -> (
+        Option<editoast_models::User>,
+        ::authz::v2::Protected<Vec<Role>>,
+    ) {
+        let subject = ::authz::Subject::user(user.id);
+        (Some(user), ::authz::v2::subject_roles(subject))
     }
 
     async fn register_origin_user(
@@ -139,7 +140,7 @@ pub(in crate::views) async fn authentication_validation_middleware(
             Ok(user) => user,
             Err(AddIdentitiesError::DuplicateIdentity(_)) => {
                 unreachable!(
-                    "the current function is only called when the user don't exists, and the `.register()`\n\
+                    "the current function is only called when the user doesn't exists, and the `.register()`\n\
                     operation above only creates the user with one unique identity"
                 );
             }
@@ -149,54 +150,83 @@ pub(in crate::views) async fn authentication_validation_middleware(
         Ok((Some(user), ::authz::v2::subject_roles(authz_user)))
     }
 
-    let (user, roles_prot) = if let Some(req_origin) = authn.origin() {
-        let conn = db_pool.get().await?;
-        conn.transaction(async |conn| {
-            let origin = match &authn {
-                crate::authentication::Authentication::Authenticated { identity, name } => {
-                    let user =
-                        editoast_models::User::retrieve_by_identity(identity, conn.clone()).await?;
-                    origin_user(user, identity, name)
+    async fn check_impersonation_privilege(
+        openfga: &fga::Client,
+        user: &editoast_models::User,
+    ) -> Result<()> {
+        let Ok(roles) = ::authz::v2::subject_roles(::authz::Subject::user(user.id))
+            .access_authorized::<Infallible>(openfga)
+            .access()
+            .await?;
+        if roles.contains(&Role::Admin) {
+            Ok(())
+        } else {
+            Err(AuthorizationError::ForbiddenImpersonation.into())
+        }
+    }
+
+    let (user, roles_prot) = match &authn {
+        crate::authentication::Authentication::Authenticated { identity, name }
+        | crate::authentication::Authentication::Skip {
+            identity: Some(identity),
+            name: Some(name),
+        } => {
+            let conn = db_pool.get().await?;
+            conn.transaction(async |conn| {
+                let user =
+                    editoast_models::User::retrieve_by_identity(identity, conn.clone()).await?;
+                warn_mismatched_name(&user, identity, name);
+                Ok::<_, crate::error::InternalError>(if let Some(user) = user {
+                    zip_roles(user)
+                } else {
+                    register_origin_user(conn.clone(), (identity, name)).await?
+                })
+            })
+            .await?
+        }
+        crate::authentication::Authentication::Impersonating {
+            impersonator_identity,
+            impersonator_name,
+            impersonated_identity,
+        } => {
+            let conn = db_pool.get().await?;
+            conn.transaction(async |conn| {
+                let (impersonator, impersonated) = tokio::try_join!(
+                    // The batching API is annoying, that's the best I can do concisely for now. We should
+                    // work on the DB user management that got worse since we added multiple identities support.
+                    editoast_models::User::retrieve_by_identity(
+                        impersonator_identity,
+                        conn.clone()
+                    ),
+                    editoast_models::User::retrieve_by_identity(
+                        impersonated_identity,
+                        conn.clone()
+                    )
+                )?;
+                warn_mismatched_name(&impersonator, impersonator_identity, impersonator_name);
+                if let Some(impersonator) = impersonator {
+                    check_impersonation_privilege(regulator.openfga(), &impersonator).await?;
+                } else {
+                    register_origin_user(conn.clone(), (impersonator_identity, impersonator_name))
+                        .await?;
                 }
-                crate::authentication::Authentication::Impersonating {
-                    impersonator_identity,
-                    impersonator_name,
-                    impersonated_identity,
-                } => {
-                    let (impersonator, impersonated) = tokio::try_join!(
-                        // The batching API is annoying, that's the best I can do concisely for now. We should
-                        // work on the DB user management that got worse since we added multiple identities support.
-                        editoast_models::User::retrieve_by_identity(
-                            impersonator_identity,
-                            conn.clone()
-                        ),
-                        editoast_models::User::retrieve_by_identity(
-                            impersonated_identity,
-                            conn.clone()
-                        )
-                    )?;
-                    if impersonated.is_none() {
-                        return Err::<_, crate::error::InternalError>(
-                            AuthorizationError::ImpersonatedUserNotFound {
-                                identity: impersonated_identity.to_owned(),
-                            }
-                            .into(),
-                        );
-                    }
-                    origin_user(impersonator, impersonator_identity, impersonator_name)
+                if let Some(impersonated) = impersonated {
+                    Ok(zip_roles(impersonated))
+                } else {
+                    Err::<_, crate::error::InternalError>(
+                        AuthorizationError::ImpersonatedUserNotFound {
+                            identity: impersonated_identity.to_owned(),
+                        }
+                        .into(),
+                    )
                 }
-                crate::authentication::Authentication::Unauthenticated
-                | crate::authentication::Authentication::Skip { .. } => None,
-            };
-
-            Ok(match origin {
-                Some((user, roles_prot)) => (Some(user), roles_prot),
-                None => register_origin_user(conn, req_origin).await?,
             })
-        })
-        .await?
-    } else {
-        (None, ::authz::v2::Protected::default())
+            .await?
+        }
+        crate::authentication::Authentication::Unauthenticated
+        | crate::authentication::Authentication::Skip { .. } => {
+            (None, ::authz::v2::Protected::default())
+        }
     };
 
     // A failed OpenFGA request does not invalidate the creation of a new user
@@ -206,14 +236,6 @@ pub(in crate::views) async fn authentication_validation_middleware(
         .await
         .map_err(AuthorizationError::from)?;
 
-    if is_impersonation {
-        if !roles.contains(&Role::Admin) {
-            return Err(AuthorizationError::ForbiddenImpersonation.into());
-        } else {
-            tracing::info!("impersonation enabled");
-        }
-    }
-
     let span = tracing::Span::current();
     if let Some(user) = &user {
         span.record("user.id", user.id);

editoast/src/views/test_app.rs

@@ -628,12 +628,13 @@ impl<'a> GroupBuilder<'a> {
 }
 
 pub trait TestRequestExt {
-    fn by_user(self, user: &impl AsRef<UserInfo>) -> Self;
+    fn by_user(self, user: impl AsRef<UserInfo>) -> Self;
+    fn impersonate(self, impersonated: impl AsRef<UserInfo>) -> Self;
     fn skip_authz(self) -> Self;
 }
 
 impl TestRequestExt for TestRequest {
-    fn by_user(self, user: &impl AsRef<UserInfo>) -> Self {
+    fn by_user(self, user: impl AsRef<UserInfo>) -> Self {
         let UserInfo { identities, name } = user.as_ref();
         self.add_header(
             "x-remote-user-identity",
@@ -642,6 +643,17 @@ impl TestRequestExt for TestRequest {
         .add_header("x-remote-user-name", name)
     }
 
+    fn impersonate(self, impersonated: impl AsRef<UserInfo>) -> Self {
+        self.add_header(
+            "x-impersonate",
+            impersonated
+                .as_ref()
+                .identities
+                .first()
+                .expect("existing user must have an identity"),
+        )
+    }
+
     fn skip_authz(self) -> Self {
         self.add_header("x-osrd-skip-authz", "true")
     }