diff --git a/editoast/Cargo.lock b/editoast/Cargo.lock index 2103747545b..01945449d8d 100644 --- a/editoast/Cargo.lock +++ b/editoast/Cargo.lock @@ -325,6 +325,7 @@ dependencies = [ "futures 0.3.32", "itertools", "pretty_assertions", + "rstest", "serde", "stdext", "strum", diff --git a/editoast/authz/Cargo.toml b/editoast/authz/Cargo.toml index 6f5c1b98619..5b48e217ac3 100644 --- a/editoast/authz/Cargo.toml +++ b/editoast/authz/Cargo.toml @@ -22,6 +22,7 @@ utoipa.workspace = true common.workspace = true fga = { workspace = true, features = ["test_utilities"] } pretty_assertions.workspace = true +rstest.workspace = true stdext.workspace = true tokio.workspace = true diff --git a/editoast/authz/src/authorizer.rs b/editoast/authz/src/authorizer.rs index a728531e5a7..d08b52b695e 100644 --- a/editoast/authz/src/authorizer.rs +++ b/editoast/authz/src/authorizer.rs @@ -93,16 +93,6 @@ impl Authorizer { .give_infra_grant(&User(self.user_id), subject, infra, grant) .await } - - pub async fn revoke_infra_grants( - &self, - subject: &Subject, - infra: &Infra, - ) -> Result, Error> { - self.regulator - .revoke_infra_grants(&User(self.user_id), subject, infra) - .await - } } impl std::fmt::Debug for Authorizer { diff --git a/editoast/authz/src/model.rs b/editoast/authz/src/model.rs index 549ef84f578..3fa586f91cd 100644 --- a/editoast/authz/src/model.rs +++ b/editoast/authz/src/model.rs @@ -38,22 +38,6 @@ impl Subject { Subject::Group(group) => group.0, } } - - pub(crate) async fn fetch<'a, T, E, Ureq, Greq>( - &'a self, - client: &fga::Client, - u: impl FnOnce(&'a User) -> Ureq, - g: impl FnOnce(&'a Group) -> Greq, - ) -> Result - where - Ureq: fga::client::Request, - Greq: fga::client::Request, - { - match self { - Subject::User(user) => u(user).fetch(client).await, - Subject::Group(group) => g(group).fetch(client).await, - } - } } #[cfg(test)] diff --git a/editoast/authz/src/regulator.rs b/editoast/authz/src/regulator.rs index bead88f2baf..9141c7d5164 100644 --- a/editoast/authz/src/regulator.rs +++ b/editoast/authz/src/regulator.rs @@ -275,7 +275,10 @@ impl Regulator { } // Remove existing grants before adding the new one - self.revoke_infra_grants_unchecked(subject, infra).await?; + let authorize = v2::special_authorizers::Authorize(&self.openfga); + authorize + .access_value(v2::infra_revoke_grant(*subject, *infra)) + .await?; // Grant the new one let mut writes = self.openfga.prepare_writes(); @@ -387,132 +390,6 @@ impl Regulator { }) .await } - - #[tracing::instrument(skip(self), ret(level = Level::DEBUG), err)] - pub async fn revoke_infra_grants( - &self, - issuer: &User, - subject: &Subject, - infra: &Infra, - ) -> Result, Error> { - // TODO: add a can_revoke privilege in the authorization model - - // Revoking rules: - // 1. Only owners (and admins) can fully revoke grants - // 2. The last owner of a resource cannot be revoked, even by admins - // 3. An owner cannot revoke another owner - - let is_subject_owner = match subject { - Subject::User(user) => { - self.openfga - .check(Infra::owner().check(user, infra)) - .await? - } - Subject::Group(group) => { - self.openfga - .check(Infra::owner().check(Group::member().userset(group), infra)) - .await? - } - }; - - if is_subject_owner { - let authorize = crate::v2::special_authorizers::Authorize(&self.openfga); - let current_owners = authorize - .access_value(crate::v2::infra_granted_subjects(*infra, InfraGrant::Owner)) - .await?; - if current_owners.len() == 1 && current_owners.contains(subject) { - return Ok(Authorization::Denied { - reason: "cannot remove the last owner from infrastructure", - }); - } - } - - if !self.is_admin(issuer).await? { - let is_issuer_owner = self - .openfga - .check(Infra::owner().check(issuer, infra)) - .await?; - - if !is_issuer_owner { - return Ok(Authorization::Denied { - reason: "only owners can revoke grants", - }); - } - - // Rule 3: An owner cannot revoke another owner (only admins can) - if is_issuer_owner && is_subject_owner { - return Ok(Authorization::Denied { - reason: "owner cannot revoke another owner", - }); - } - } - - self.revoke_infra_grants_unchecked(subject, infra).await?; - Ok(Authorization::Granted(())) - } - - #[tracing::instrument(skip(self), ret(level = Level::DEBUG), err)] - pub async fn revoke_infra_grants_unchecked( - &self, - subject: &Subject, - infra: &Infra, - ) -> Result<(), Error> { - // No need to check if the infra exists. If it doesn't, there won't be any tuples in OpenFGA. - // And even if there is, we're about to remove them anyway. - // Likewise about both users. - - let mut delete = self.openfga.prepare_deletes(); - - if subject - .fetch( - &self.openfga, - |user| Infra::reader().tuple(user, infra), - |group| Infra::reader().tuple(Group::member().userset(group), infra), - ) - .await? - { - match subject { - Subject::User(user) => delete.push(&Infra::reader().tuple(user, infra)), - Subject::Group(group) => { - delete.push(&Infra::reader().tuple(Group::member().userset(group), infra)) - } - } - } - - if subject - .fetch( - &self.openfga, - |user| Infra::writer().tuple(user, infra), - |group| Infra::writer().tuple(Group::member().userset(group), infra), - ) - .await? - { - match subject { - Subject::User(user) => delete.push(&Infra::writer().tuple(user, infra)), - Subject::Group(group) => { - delete.push(&Infra::writer().tuple(Group::member().userset(group), infra)) - } - } - } - - if subject - .fetch( - &self.openfga, - |user| Infra::owner().tuple(user, infra), - |group| Infra::owner().tuple(Group::member().userset(group), infra), - ) - .await? - { - match subject { - Subject::User(user) => delete.push(&Infra::owner().tuple(user, infra)), - Subject::Group(group) => { - delete.push(&Infra::owner().tuple(Group::member().userset(group), infra)) - } - } - } - delete.execute().await?; - Ok(()) - } } #[cfg(test)] @@ -733,98 +610,4 @@ mod tests { .assert_infra_grant_eq(bob, infra, Some(InfraGrant::Reader)) .await; } - - // REVOKING TESTS - - #[tokio::test(flavor = "multi_thread")] - async fn only_owners_can_revoke() { - let regulator = Regulator::new(crate::authz_client!(), MockAuthDriver::default()); - let infra = Infra(1); - let alice = regulator.alice().await; - let bob = regulator.bob().await; - - regulator - .set_infra_grant(alice, infra, InfraGrant::Writer) - .await; - regulator - .set_infra_grant(bob, infra, InfraGrant::Reader) - .await; - - regulator - .revoke_infra_grants(&alice, &bob.into(), &infra) - .await - .expect("revoke operation should complete") - .expect_denied("only owners can revoke grants"); - - regulator - .assert_infra_grant_eq(bob, infra, Some(InfraGrant::Reader)) - .await; - } - - #[tokio::test(flavor = "multi_thread")] - async fn admins_can_revoke() { - let regulator = Regulator::new(crate::authz_client!(), MockAuthDriver::default()); - let infra = Infra(1); - let alice = regulator.alice().await; - let walter = regulator.walter().await; - - regulator - .set_infra_grant(alice, infra, InfraGrant::Reader) - .await; - - regulator - .revoke_infra_grants(&walter, &alice.into(), &infra) - .await - .expect("revoke operation should complete") - .expect_allowed("admin can revoke grants"); - - regulator.assert_infra_grant_eq(alice, infra, None).await; - } - - #[tokio::test(flavor = "multi_thread")] - async fn last_owner_cannot_be_revoked_by_admin() { - let regulator = Regulator::new(crate::authz_client!(), MockAuthDriver::default()); - let infra = Infra(1); - let alice = regulator.alice().await; - let walter = regulator.walter().await; - - regulator - .set_infra_grant(alice, infra, InfraGrant::Owner) - .await; - - regulator - .revoke_infra_grants(&walter, &alice.into(), &infra) - .await - .expect("revoke operation should complete") - .expect_denied("last owner cannot be revoked"); - - regulator - .assert_infra_grant_eq(alice, infra, Some(InfraGrant::Owner)) - .await; - } - - #[tokio::test(flavor = "multi_thread")] - async fn owner_cannot_revoke_another_owner() { - let regulator = Regulator::new(crate::authz_client!(), MockAuthDriver::default()); - let infra = Infra(1); - let alice = regulator.alice().await; - let bob = regulator.bob().await; - - regulator - .set_infra_grant(alice, infra, InfraGrant::Owner) - .await; - regulator - .set_infra_grant(bob, infra, InfraGrant::Owner) - .await; - - regulator - .revoke_infra_grants(&alice, &bob.into(), &infra) - .await - .expect("revoke operation should complete") - .expect_denied("owner cannot revoke another owner"); - - regulator - .assert_infra_grant_eq(bob, infra, Some(InfraGrant::Owner)) - .await; - } } diff --git a/editoast/authz/src/v2.rs b/editoast/authz/src/v2.rs index 9eebd0aa868..4643cf302bc 100644 --- a/editoast/authz/src/v2.rs +++ b/editoast/authz/src/v2.rs @@ -15,6 +15,7 @@ use futures::future::BoxFuture; use crate::Group; use crate::Infra; +use crate::InfraGrant; use crate::InfraPrivilege; use crate::Role; use crate::Subject; @@ -59,6 +60,10 @@ pub enum Check { HasRole(Actor, Role), /// The actor needs an infra privilege to perform the operation HasInfraPrivilege(Actor, InfraPrivilege, Infra), + /// The subject must not have the specified effective infra grant + SubjectEffectiveInfraGrantIsNot(InfraGrant, Subject, Infra), + /// The subject must not be the last direct owner of the infra + IsntLastInfraOwner(Subject, Infra), /// The subject must exist in PostgreSQL SubjectExists(Subject), /// The infra must exist in PostgreSQL @@ -317,3 +322,22 @@ pub mod special_authorizers { } } } + +impl Authorizer for itertools::Either +where + L: Authorizer, + R: Authorizer, +{ + type Rejection = Rejection; + type Error = Error; + + async fn authorize<'a, T>( + &'a self, + data: Protected, + ) -> Result, Self::Error> { + match self { + itertools::Either::Left(l) => l.authorize(data).await, + itertools::Either::Right(r) => r.authorize(data).await, + } + } +} diff --git a/editoast/authz/src/v2/infra.rs b/editoast/authz/src/v2/infra.rs index 0d4dcf68968..4e25518444b 100644 --- a/editoast/authz/src/v2/infra.rs +++ b/editoast/authz/src/v2/infra.rs @@ -140,6 +140,61 @@ pub fn infra_effective_grant(subject: Subject, infra: Infra) -> Protected