editoast: authz: add remove_roles to v2

Add a new function remove_roles to replaces revoke_user_roles and revoke_group_roles.
Remove the old revoke_user_roles and revoke_group_roles functions.
add tests for remove_roles.

Signed-off-by: Simon <sim.gaubert.sg@gmail.com>

Author: SaumonDesMers

editoast/authz/src/authorizer.rs

@@ -217,10 +217,15 @@ mod tests {
 
         // remove role
         {
-            regulator()
-                .revoke_user_roles(&User(user_id), HashSet::from([Role::OperationalStudies]))
-                .await
-                .expect("roles should be stripped");
+            v2::remove_roles(
+                Subject::user(user_id),
+                HashSet::from([Role::OperationalStudies]),
+            )
+            .authorize(&v2::test_authorizers::Authorize(regulator().openfga()))
+            .await
+            .expect("roles should be stripped")
+            .unwrap_authorized()
+            .await;
         }
 
         assert!(

editoast/authz/src/regulator.rs

@@ -199,42 +199,6 @@ impl<S: StorageDriver> Regulator<S> {
         Ok(roles.into_iter().collect())
     }
 
-    #[tracing::instrument(skip_all, fields(user, ?roles), ret(level = Level::DEBUG), err)]
-    pub async fn revoke_user_roles(
-        &self,
-        user: &User,
-        roles: HashSet<Role>,
-    ) -> Result<(), Error<S::Error>> {
-        if !self.user_exists(user.0).await? {
-            return Err(Error::UnknownSubject(user.0));
-        }
-        let mut deletes = self.openfga.prepare_deletes();
-        let existing_roles = self.user_roles(user).await?;
-        for role in roles.intersection(&existing_roles) {
-            deletes.push(&User::role().tuple(role, user));
-        }
-        deletes.execute().await?;
-        Ok(())
-    }
-
-    #[tracing::instrument(skip_all, fields(group, ?roles), ret(level = Level::DEBUG), err)]
-    pub async fn revoke_group_roles(
-        &self,
-        group: &Group,
-        roles: HashSet<Role>,
-    ) -> Result<(), Error<S::Error>> {
-        if !self.group_exists(group.0).await? {
-            return Err(Error::UnknownSubject(group.0));
-        }
-        let mut deletes = self.openfga.prepare_deletes();
-        let existing_roles = self.group_roles(group).await?;
-        for role in roles.intersection(&existing_roles) {
-            deletes.push(&Group::role().tuple(role, group));
-        }
-        deletes.execute().await?;
-        Ok(())
-    }
-
     #[tracing::instrument(skip(self), fields(user, ?roles), ret(level = Level::DEBUG), err)]
     pub async fn check_roles(
         &self,

editoast/authz/src/v2.rs

@@ -256,7 +256,6 @@ pub fn remove_members(group: Group, members: HashSet<User>) -> Protected<'static
                 writes.push(&User::group().tuple(&group, user));
             }
             writes.execute().await?;
-
             Ok(())
         }
         .boxed()
@@ -266,6 +265,45 @@ pub fn remove_members(group: Group, members: HashSet<User>) -> Protected<'static
     .with_guardrail(Guardrail::IssuerHasRole(Role::Admin))
 }
 
+// TODO: move somewhere more appropriate
+/// Removes the specified roles from the subject
+///
+/// Idempotent but not atomic due to the lack of transactions in OpenFGA.
+pub fn remove_roles(subject: Subject, roles: HashSet<Role>) -> Protected<'static, ()> {
+    Protected::new(move |openfga| {
+        async move {
+            let existing_roles = match &subject {
+                Subject::User(user) => Role::list_roles(openfga, User::role(), user).await?,
+                Subject::Group(group) => Role::list_roles(openfga, Group::role(), group).await?,
+            };
+
+            let existing_roles = HashSet::from_iter(existing_roles);
+            let removed_roles = roles.intersection(&existing_roles);
+            let mut writes = openfga.prepare_deletes();
+            match subject {
+                Subject::User(user) => {
+                    for role in removed_roles {
+                        writes.push(&User::role().tuple(role, &user));
+                    }
+                }
+                Subject::Group(group) => {
+                    for role in removed_roles {
+                        writes.push(&Group::role().tuple(role, &group));
+                    }
+                }
+            }
+            writes.execute().await?;
+            Ok(())
+        }
+        .boxed()
+    })
+    .with_guardrail(Guardrail::IssuerHasRole(Role::Admin))
+    .with_check(match &subject {
+        Subject::User(user) => SanityCheck::UserExists(*user),
+        Subject::Group(group) => SanityCheck::GroupExists(*group),
+    })
+}
+
 pub mod test_authorizers {
     use std::convert::Infallible;
 
@@ -539,4 +577,100 @@ mod tests {
             HashSet::from_iter([Role::Admin, Role::Stdcm, Role::OperationalStudies])
         );
     }
+
+    #[tokio::test]
+    async fn remove_roles_idempotent() {
+        let openfga = crate::authz_client!();
+        let authorize = Authorize(&openfga);
+
+        add_roles(
+            Subject::user(1),
+            HashSet::from_iter([Role::Admin, Role::Stdcm]),
+        )
+        .authorize(&authorize)
+        .await
+        .unwrap()
+        .unwrap_authorized()
+        .await;
+        assert_eq!(
+            openfga.subject_roles(&Subject::user(1)).await,
+            HashSet::from_iter([Role::Admin, Role::Stdcm])
+        );
+
+        remove_roles(
+            Subject::user(1),
+            HashSet::from_iter([Role::Admin, Role::Stdcm]),
+        )
+        .authorize(&authorize)
+        .await
+        .unwrap()
+        .unwrap_authorized()
+        .await;
+        assert_eq!(
+            openfga.subject_roles(&Subject::user(1)).await,
+            HashSet::from_iter([])
+        );
+
+        remove_roles(
+            Subject::user(1),
+            HashSet::from_iter([Role::Admin, Role::Stdcm]),
+        )
+        .authorize(&authorize)
+        .await
+        .unwrap()
+        .unwrap_authorized()
+        .await;
+        assert_eq!(
+            openfga.subject_roles(&Subject::user(1)).await,
+            HashSet::from_iter([])
+        );
+    }
+
+    #[tokio::test]
+    async fn remove_roles_intersecting_calls() {
+        let openfga = crate::authz_client!();
+        let authorize = Authorize(&openfga);
+
+        add_roles(
+            Subject::user(1),
+            HashSet::from_iter([Role::Admin, Role::Stdcm, Role::OperationalStudies]),
+        )
+        .authorize(&authorize)
+        .await
+        .unwrap()
+        .unwrap_authorized()
+        .await;
+        assert_eq!(
+            openfga.subject_roles(&Subject::user(1)).await,
+            HashSet::from_iter([Role::Admin, Role::Stdcm, Role::OperationalStudies])
+        );
+
+        remove_roles(
+            Subject::user(1),
+            HashSet::from_iter([Role::Admin, Role::Stdcm]),
+        )
+        .authorize(&authorize)
+        .await
+        .unwrap()
+        .unwrap_authorized()
+        .await;
+        assert_eq!(
+            openfga.subject_roles(&Subject::user(1)).await,
+            HashSet::from_iter([Role::OperationalStudies])
+        );
+
+        remove_roles(
+            Subject::user(1),
+            HashSet::from_iter([Role::Admin, Role::OperationalStudies]),
+        )
+        .authorize(&authorize)
+        .await
+        .unwrap()
+        .unwrap_authorized()
+        .await;
+        assert_eq!(
+            openfga.subject_roles(&Subject::user(1)).await,
+            HashSet::from_iter([])
+        );
+    }
 }

editoast/src/client/roles.rs

@@ -206,7 +206,12 @@ pub async fn remove_roles(
     pool: Arc<DbConnectionPoolV2>,
     openfga_config: OpenfgaConfig,
 ) -> anyhow::Result<()> {
-    let regulator = openfga_config.into_regulator(pool).await?;
+    let openfga = &openfga_config.into_client().await?;
+    let system = SystemAuthorizer {
+        openfga,
+        conn: pool.get().await?,
+    };
+
     let roles = roles
         .iter()
         .map(String::as_str)
@@ -220,19 +225,12 @@ pub async fn remove_roles(
             .collect_vec()
             .join(", "),
     );
-    match parse_and_fetch_subject(&subject, regulator.driver()).await? {
-        Subject {
-            id,
-            info: SubjectInfo::User(_),
-        } => regulator.revoke_user_roles(&authz::User(id), roles).await?,
-        Subject {
-            id,
-            info: SubjectInfo::Group(_),
-        } => {
-            regulator
-                .revoke_group_roles(&authz::Group(id), roles)
-                .await?
+    let subject = parse_and_fetch_subject(&subject, &PgAuthDriver::new(pool)).await?;
+    let remove_roles = authz::v2::remove_roles(subject.into_authz(), roles);
+    match system.authorize(remove_roles).await?.access().await? {
+        Ok(()) => Ok(()),
+        Err(Rejection::NoSuchUser(_)) | Err(Rejection::NoSuchGroup(_)) => {
+            unreachable!("checked by parse_and_fetch_subject")
         }
     }
-    Ok(())
 }