From cb37583a59a566fe0492752eb46089c0752aefe2 Mon Sep 17 00:00:00 2001 From: LucaCappelletti94 Date: Wed, 28 Oct 2026 22:40:58 +0100 Subject: [PATCH 1/3] Follow sqlparser main to the REVOKE role parser --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e0cf6395a..5e777b705 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -871,7 +871,7 @@ dependencies = [ [[package]] name = "sqlparser" version = "0.63.0" -source = "git+https://github.com/apache/datafusion-sqlparser-rs?branch=main#5989f9ce1582bd859f97180e060e4955ab86d9be" +source = "git+https://github.com/apache/datafusion-sqlparser-rs?branch=main#a75e736aa07d5b17a575a9c2462e57325ffe1bdc" dependencies = [ "log", "recursive", @@ -881,7 +881,7 @@ dependencies = [ [[package]] name = "sqlparser_derive" version = "0.6.0" -source = "git+https://github.com/apache/datafusion-sqlparser-rs?branch=main#5989f9ce1582bd859f97180e060e4955ab86d9be" +source = "git+https://github.com/apache/datafusion-sqlparser-rs?branch=main#a75e736aa07d5b17a575a9c2462e57325ffe1bdc" dependencies = [ "proc-macro2", "quote", From e387087c87bd4b3597cad328cc04d2f8bd7ab805 Mon Sep 17 00:00:00 2001 From: LucaCappelletti94 Date: Wed, 28 Oct 2026 22:40:58 +0100 Subject: [PATCH 2/3] Remove role memberships on REVOKE role FROM grantee --- src/errors.rs | 27 +- src/structs/generic_db/sqlparser.rs | 3 + .../generic_db/sqlparser/role_memberships.rs | 272 ++++++++++++++++-- .../generic_db/sqlparser/transactions.rs | 1 + src/traits/database.rs | 3 +- src/traits/role.rs | 2 +- tests/role_memberships.rs | 239 ++++++++++++++- 7 files changed, 505 insertions(+), 42 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index bd7d8da90..abe4c3faf 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -799,9 +799,9 @@ pub enum Error { /// Error indicating that a role membership names a role that does not /// exist, as the granted role or as the member. /// - /// Covers `GRANT role TO role` and the `IN ROLE`, `IN GROUP`, `ROLE`, - /// `USER` and `ADMIN` clauses of `CREATE ROLE`. Like the other role - /// checks this one follows + /// Covers `GRANT role TO role`, `REVOKE role FROM role` and the + /// `IN ROLE`, `IN GROUP`, `ROLE`, `USER` and `ADMIN` clauses of + /// `CREATE ROLE`. Like the other role checks this one follows /// [`AccessResolution`](crate::structs::AccessResolution), except for /// `PUBLIC`, which names no role under either setting and which /// PostgreSQL refuses as `role "public" does not exist`. @@ -809,13 +809,28 @@ pub enum Error { /// Name of the undefined role. role_name: String, }, - #[error("Unrecognized role option `{option}` in a role membership grant.")] - /// Error indicating that `GRANT role TO role ... WITH` names an option - /// other than `ADMIN`, `INHERIT` and `SET`, which PostgreSQL refuses. + #[error("Unrecognized role option `{option}` in a role membership grant or revoke.")] + /// Error indicating that `GRANT role TO role ... WITH` or + /// `REVOKE option OPTION FOR role FROM role` names an option other than + /// `ADMIN`, `INHERIT` and `SET`, which PostgreSQL refuses. UnrecognizedRoleOption { /// The option as written. option: String, }, + #[error( + "Revoking role `{role_name}` from `{member_name}` strands the memberships `{member_name}` \ + granted with its ADMIN option." + )] + /// Error indicating that `REVOKE role FROM role` without `CASCADE` takes + /// away the last `ADMIN` option the member holds on the role while + /// memberships it granted depend on it, which PostgreSQL refuses with + /// `dependent privileges exist`. + DependentRoleMemberships { + /// Name of the revoked role, as written. + role_name: String, + /// Name of the member losing the `ADMIN` option, as written. + member_name: String, + }, #[error("Table `{table_name}` not found for a grant or revoke.")] /// Error indicating that a grant or a revoke names a table that does not /// exist. diff --git a/src/structs/generic_db/sqlparser.rs b/src/structs/generic_db/sqlparser.rs index 601feb403..8a83b6f64 100644 --- a/src/structs/generic_db/sqlparser.rs +++ b/src/structs/generic_db/sqlparser.rs @@ -7757,6 +7757,9 @@ impl ParserDB { &role_alterations, )?; } + Statement::RevokeRole(revoke) => { + role_memberships::revoke_role(&mut builder, &revoke, access_resolution)?; + } Statement::CreateSchema { schema_name, if_not_exists, .. } => { if let SchemaName::Simple(name) | SchemaName::NamedAuthorization(name, _) = &schema_name diff --git a/src/structs/generic_db/sqlparser/role_memberships.rs b/src/structs/generic_db/sqlparser/role_memberships.rs index c75c72033..638e29a2a 100644 --- a/src/structs/generic_db/sqlparser/role_memberships.rs +++ b/src/structs/generic_db/sqlparser/role_memberships.rs @@ -1,5 +1,6 @@ //! Role memberships, from `GRANT role TO role` and from the `IN ROLE`, -//! `IN GROUP`, `ROLE`, `USER` and `ADMIN` clauses of `CREATE ROLE`. +//! `IN GROUP`, `ROLE`, `USER` and `ADMIN` clauses of `CREATE ROLE`, and their +//! removal by `REVOKE role FROM role`. //! //! PostgreSQL keeps one membership row for each granted role, member and //! grantor. A repeated grant from the same grantor changes only the options @@ -10,8 +11,8 @@ use alloc::{string::ToString, sync::Arc, vec::Vec}; use core::mem::take; use sqlparser::ast::{ - CreateRole, GrantRole, Grantee, GranteeName, GranteesType, Ident, RoleGrantOption, - RoleGrantOptionValue, RoleOption, + CascadeOption, CreateRole, GrantRole, Grantee, GranteeName, GranteesType, Ident, RevokeRole, + RoleGrantOption, RoleGrantOptionValue, RoleOption, }; use super::{ @@ -25,6 +26,26 @@ use crate::{ utils::identifier_resolution::{is_public_pseudo_role, normalize_identifier}, }; +/// One of the options a membership grant carries. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum MembershipOption { + Admin, + Inherit, + Set, +} + +impl MembershipOption { + /// Reads an option name as PostgreSQL does, refusing any other name. + fn read(name: &Ident) -> Result { + match normalize_identifier(&name.value, name.quote_style.is_some()).as_ref() { + "admin" => Ok(Self::Admin), + "inherit" => Ok(Self::Inherit), + "set" => Ok(Self::Set), + _ => Err(Error::UnrecognizedRoleOption { option: name.value.clone() }), + } + } +} + /// The options one grant writes, each `None` when the grant leaves it out. #[derive(Debug, Clone, Copy, Default)] struct WrittenOptions { @@ -42,16 +63,10 @@ impl WrittenOptions { fn read(options: &[RoleGrantOption]) -> Result { let mut written = Self::default(); for option in options { - let name = normalize_identifier(&option.name.value, option.name.quote_style.is_some()); - let slot = match name.as_ref() { - "admin" => &mut written.admin, - "inherit" => &mut written.inherit, - "set" => &mut written.set, - _ => { - return Err(Error::UnrecognizedRoleOption { - option: option.name.value.clone(), - }); - } + let slot = match MembershipOption::read(&option.name)? { + MembershipOption::Admin => &mut written.admin, + MembershipOption::Inherit => &mut written.inherit, + MembershipOption::Set => &mut written.set, }; *slot = Some(option.value != RoleGrantOptionValue::False); } @@ -135,15 +150,7 @@ pub(super) fn grant_role( role_alterations: &[UndeclaredRoleAlteration], ) -> Result<(), Error> { let written = WrittenOptions::read(&grant.options)?; - let mut members = Vec::with_capacity(grant.grantees.len()); - for grantee in &grant.grantees { - members.extend(grantee_member(grantee)?); - } - for role in grant.roles.iter().chain(members.iter().copied()) { - refuse_public(role)?; - require_role(builder, access_resolution, role)?; - } - + let members = named_members(&grant.roles, &grant.grantees, builder, access_resolution)?; for role in &grant.roles { for member in &members { let inherits = member_inherits(builder, role_alterations, member); @@ -153,6 +160,216 @@ pub(super) fn grant_role( Ok(()) } +/// Applies `REVOKE [option OPTION FOR] role [, ...] FROM grantee [, ...]` to +/// the grant each named member holds from the grantor in `GRANTED BY`, or +/// from no written grantor, one role after another as PostgreSQL does. +/// +/// A revoke reaching no grant changes nothing, as PostgreSQL only warns. +/// Taking away a member's last `ADMIN` option strands the grants it made, +/// which only `CASCADE` revokes. +pub(super) fn revoke_role( + builder: &mut ParserDBBuilder, + revoke: &RevokeRole, + access_resolution: AccessResolution, +) -> Result<(), Error> { + let option = revoke.option_for.as_ref().map(MembershipOption::read).transpose()?; + let members = named_members(&revoke.roles, &revoke.grantees, builder, access_resolution)?; + let cascade = revoke.cascade == Some(CascadeOption::Cascade); + for role in &revoke.roles { + let rows = plan_revoke( + builder.role_memberships(), + role, + &members, + revoke.granted_by.as_ref(), + option, + cascade, + )?; + apply_revoke(builder, &rows); + } + Ok(()) +} + +/// What a revoke does to one grant of the revoked role. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Revocation { + Keep, + Clear(MembershipOption), + Delete, +} + +/// One grant of the revoked role, by its membership's position in the +/// builder and its own position in that membership. +#[derive(Debug, Clone, Copy)] +struct PlannedGrant { + membership: usize, + grant: usize, + revocation: Revocation, +} + +/// Plans the revoke of `role` from each of `members` over every grant of +/// `role`, following PostgreSQL's `plan_single_revoke`. +fn plan_revoke( + memberships: &[(Arc, ())], + role: &Ident, + members: &[&Ident], + grantor: Option<&Ident>, + option: Option, + cascade: bool, +) -> Result, Error> { + let mut rows: Vec = memberships + .iter() + .enumerate() + .filter(|(_, (membership, ()))| idents_match(&membership.role, role)) + .flat_map(|(membership, (granted, ()))| { + (0..granted.grants.len()) + .map(move |grant| PlannedGrant { membership, grant, revocation: Revocation::Keep }) + }) + .collect(); + for member in members { + let Some(index) = rows.iter().position(|row| { + let (membership, grant) = planned(memberships, row); + idents_match(&membership.member, member) + && same_grantor(grant.grantor.as_ref(), grantor) + }) else { + continue; + }; + match option { + Some(cleared @ (MembershipOption::Inherit | MembershipOption::Set)) => { + rows[index].revocation = Revocation::Clear(cleared); + } + Some(MembershipOption::Admin) => { + plan_recursive_revoke(memberships, &mut rows, index, true, cascade, role)?; + } + None => plan_recursive_revoke(memberships, &mut rows, index, false, cascade, role)?, + } + } + Ok(rows) +} + +/// Plans taking away the grant at `index`, or only its `ADMIN` option, and +/// with it the grants its member can no longer back, following +/// PostgreSQL's `plan_recursive_revoke`. +fn plan_recursive_revoke( + memberships: &[(Arc, ())], + rows: &mut [PlannedGrant], + index: usize, + admin_only: bool, + cascade: bool, + role: &Ident, +) -> Result<(), Error> { + match rows[index].revocation { + Revocation::Delete => return Ok(()), + Revocation::Clear(MembershipOption::Admin) if admin_only => return Ok(()), + _ => {} + } + let (membership, grant) = planned(memberships, &rows[index]); + if !admin_only { + rows[index].revocation = Revocation::Delete; + } + if !grant.admin { + return Ok(()); + } + if admin_only { + rows[index].revocation = Revocation::Clear(MembershipOption::Admin); + } + + let member = &membership.member; + let still_admin = rows.iter().any(|row| { + let (other, other_grant) = planned(memberships, row); + row.revocation == Revocation::Keep + && other_grant.admin + && idents_match(&other.member, member) + }); + if still_admin { + return Ok(()); + } + // Indexed because each recursion replans rows this loop has yet to reach. + for dependent in 0..rows.len() { + let (_, dependent_grant) = planned(memberships, &rows[dependent]); + if rows[dependent].revocation == Revocation::Delete + || !dependent_grant + .grantor + .as_ref() + .is_some_and(|grantor| idents_match(grantor, member)) + { + continue; + } + if !cascade { + return Err(Error::DependentRoleMemberships { + role_name: role.value.clone(), + member_name: member.value.clone(), + }); + } + plan_recursive_revoke(memberships, rows, dependent, false, cascade, role)?; + } + Ok(()) +} + +/// The membership and grant a planned row stands for. +fn planned<'a>( + memberships: &'a [(Arc, ())], + row: &PlannedGrant, +) -> (&'a RoleMembership, &'a MembershipGrant) { + let membership = memberships[row.membership].0.as_ref(); + (membership, &membership.grants[row.grant]) +} + +/// Applies a planned revoke, dropping the memberships it leaves without a +/// grant. +fn apply_revoke(builder: &mut ParserDBBuilder, rows: &[PlannedGrant]) { + let memberships = builder.role_memberships_mut(); + let mut emptied = false; + // Rows run in ascending grant order within a membership, so walking them + // backwards removes each grant before the ones ahead of it shift. + for row in rows.iter().rev().filter(|row| row.revocation != Revocation::Keep) { + let membership = Arc::make_mut(&mut memberships[row.membership].0); + let grant = &mut membership.grants[row.grant]; + match row.revocation { + Revocation::Keep => {} + Revocation::Clear(MembershipOption::Admin) => grant.admin = false, + Revocation::Clear(MembershipOption::Inherit) => grant.inherit = false, + Revocation::Clear(MembershipOption::Set) => grant.set = false, + Revocation::Delete => { + membership.grants.remove(row.grant); + emptied |= membership.grants.is_empty(); + } + } + } + if emptied { + memberships.retain(|(membership, ())| !membership.grants.is_empty()); + } +} + +/// Refuses `PUBLIC` and, under [`AccessResolution::ClosedWorld`], a role the +/// input has not created, among the roles and grantees of a membership +/// statement, and returns the members the grantees name. +fn named_members<'a>( + roles: &[Ident], + grantees: &'a [Grantee], + builder: &ParserDBBuilder, + access_resolution: AccessResolution, +) -> Result, Error> { + let mut members = Vec::with_capacity(grantees.len()); + for grantee in grantees { + members.extend(grantee_member(grantee)?); + } + for role in roles.iter().chain(members.iter().copied()) { + refuse_public(role)?; + require_role(builder, access_resolution, role)?; + } + Ok(members) +} + +/// Whether a grant's grantor is the one a statement names, a grant written +/// without `GRANTED BY` matching only a statement written without it. +fn same_grantor(granted_by: Option<&Ident>, grantor: Option<&Ident>) -> bool { + match (granted_by, grantor) { + (None, None) => true, + (Some(granted_by), Some(grantor)) => idents_match(granted_by, grantor), + _ => false, + } +} + /// Repoints every membership naming `previous`, as the granted role, the /// member or a grantor, to `replacement`. pub(super) fn rename_role(builder: &mut ParserDBBuilder, previous: &Ident, replacement: &Ident) { @@ -288,14 +505,9 @@ fn record( }); let membership = Arc::make_mut(&mut memberships[position].0); - let same_grantor = |grant: &&mut MembershipGrant| { - match (&grant.grantor, grantor) { - (None, None) => true, - (Some(granted_by), Some(grantor)) => idents_match(granted_by, grantor), - _ => false, - } - }; - if let Some(grant) = membership.grants.iter_mut().find(same_grantor) { + if let Some(grant) = + membership.grants.iter_mut().find(|grant| same_grantor(grant.grantor.as_ref(), grantor)) + { if let Some(admin) = written.admin { grant.admin = admin; } diff --git a/src/structs/generic_db/sqlparser/transactions.rs b/src/structs/generic_db/sqlparser/transactions.rs index 914059c3a..74a3aa581 100644 --- a/src/structs/generic_db/sqlparser/transactions.rs +++ b/src/structs/generic_db/sqlparser/transactions.rs @@ -643,6 +643,7 @@ fn defines_schema(statement: &Statement) -> bool { | Statement::GrantRole(_) | Statement::Deny(_) | Statement::Revoke(_) + | Statement::RevokeRole(_) | Statement::CreateSequence { .. } | Statement::CreateDomain(_) | Statement::CreateType { .. } diff --git a/src/traits/database.rs b/src/traits/database.rs index f315ec8ab..0ecc1fbcb 100644 --- a/src/traits/database.rs +++ b/src/traits/database.rs @@ -1254,7 +1254,8 @@ pub trait DatabaseLike: Clone + Debug + Send + Sync { /// Iterates over the role memberships recorded in the database, from /// `GRANT role TO role` and from the membership clauses of `CREATE ROLE`, - /// one for each granted role and member. + /// one for each granted role and member that `REVOKE role FROM role` has + /// not taken away. /// /// # Example /// diff --git a/src/traits/role.rs b/src/traits/role.rs index 180f161d3..1e2da484e 100644 --- a/src/traits/role.rs +++ b/src/traits/role.rs @@ -254,7 +254,7 @@ pub trait RoleLike: Debug + Clone + Ord + Eq + Metadata + Send + Sync { /// Returns the memberships this role holds, one for each role it was /// granted, whether by `GRANT role TO role` or by a membership clause of - /// `CREATE ROLE`. + /// `CREATE ROLE`, and not since revoked by `REVOKE role FROM role`. /// /// A membership may name a granted role the database does not hold, as a /// schema checked under an open world may. diff --git a/tests/role_memberships.rs b/tests/role_memberships.rs index 0534f8e7d..eca0d3077 100644 --- a/tests/role_memberships.rs +++ b/tests/role_memberships.rs @@ -1,5 +1,6 @@ //! Role memberships, from `GRANT role TO role` and from the membership -//! clauses of `CREATE ROLE`, with the options PostgreSQL 16 gives each one. +//! clauses of `CREATE ROLE`, with the options PostgreSQL 16 gives each one, +//! and their removal by `REVOKE role FROM role`. #![allow(clippy::expect_used)] use sql_traits::{errors::Error, prelude::*}; @@ -18,8 +19,11 @@ fn open_world(sql: &str) -> Result { .parse::(sql) } -/// The memberships `member` holds, as `(role, admin, inherit, set)`. -fn memberships_of(database: &ParserDB, member: &str) -> Vec<(String, bool, bool, bool)> { +/// A membership as `(role, admin, inherit, set)`. +type Membership = (String, bool, bool, bool); + +/// The memberships `member` holds. +fn memberships_of(database: &ParserDB, member: &str) -> Vec { database .role(member) .expect("the member resolves") @@ -35,7 +39,7 @@ fn memberships_of(database: &ParserDB, member: &str) -> Vec<(String, bool, bool, .collect() } -fn membership(role: &str, admin: bool, inherit: bool, set: bool) -> (String, bool, bool, bool) { +fn membership(role: &str, admin: bool, inherit: bool, set: bool) -> Membership { (role.to_owned(), admin, inherit, set) } @@ -603,3 +607,230 @@ fn mysql_account_grantee_records_nothing() { assert_eq!(database.role_memberships().count(), 0); } + +/// Measured on PostgreSQL 18 through `pg_auth_members`. +#[test] +fn revoke_removes_every_named_membership() { + let database = parse( + "CREATE ROLE a; + CREATE ROLE b; + CREATE ROLE kept; + CREATE ROLE x; + CREATE ROLE y; + GRANT a, b, kept TO x, y; + REVOKE a, b FROM x, y;", + ) + .expect("schema builds"); + + assert_eq!(memberships_of(&database, "x"), [membership("kept", false, true, true)]); + assert_eq!(memberships_of(&database, "y"), [membership("kept", false, true, true)]); +} + +/// `OPTION FOR` clears that option and keeps the membership, as measured on +/// PostgreSQL 18. +#[test] +fn revoke_option_for_clears_only_that_option() { + for (option, expected) in [ + ("ADMIN", membership("parent", false, true, true)), + ("INHERIT", membership("parent", true, false, true)), + ("set", membership("parent", true, true, false)), + ] { + let database = parse(&format!( + "CREATE ROLE parent; + CREATE ROLE child; + GRANT parent TO child WITH ADMIN TRUE; + REVOKE {option} OPTION FOR parent FROM child;" + )) + .expect("schema builds"); + + assert_eq!(memberships_of(&database, "child"), [expected], "{option} OPTION FOR"); + } +} + +/// `GRANTED BY` revokes that grantor's grant and leaves the others, as +/// measured on PostgreSQL 18. +#[test] +fn revoke_granted_by_removes_that_grantors_grant() { + let database = parse( + "CREATE ROLE parent; + CREATE ROLE child; + CREATE ROLE g; + GRANT parent TO g WITH ADMIN TRUE; + GRANT parent TO child WITH ADMIN TRUE GRANTED BY g; + GRANT parent TO child WITH SET FALSE; + REVOKE parent FROM child GRANTED BY G;", + ) + .expect("schema builds"); + assert_eq!(memberships_of(&database, "child"), [membership("parent", false, true, false)]); + + let database = parse( + "CREATE ROLE parent; + CREATE ROLE child; + CREATE ROLE g; + GRANT parent TO g WITH ADMIN TRUE; + GRANT parent TO child GRANTED BY g; + REVOKE parent FROM child;", + ) + .expect("schema builds"); + assert_eq!(memberships_of(&database, "child"), [membership("parent", false, true, true)]); +} + +/// PostgreSQL 18 answers a revoke naming no recorded grant with a warning +/// and changes nothing. +#[test] +fn revoke_of_an_absent_grant_changes_nothing() { + let database = parse( + "CREATE ROLE parent; + CREATE ROLE child; + CREATE ROLE other; + REVOKE parent FROM child; + GRANT parent TO child; + REVOKE parent FROM other; + REVOKE ADMIN OPTION FOR parent FROM other;", + ) + .expect("schema builds"); + + assert_eq!(memberships_of(&database, "child"), [membership("parent", false, true, true)]); + assert_eq!(memberships_of(&database, "other"), [] as [Membership; 0]); +} + +/// A grant the member made with its `ADMIN` option depends on it, and +/// PostgreSQL 18 refuses to take that option away without `CASCADE`, with +/// `dependent privileges exist`. +#[test] +fn revoke_refuses_to_strand_dependent_grants() { + for revoke in [ + "REVOKE parent FROM admin", + "REVOKE parent FROM admin RESTRICT", + "REVOKE ADMIN OPTION FOR parent FROM admin", + ] { + let error = parse(&format!( + "CREATE ROLE parent; + CREATE ROLE admin; + CREATE ROLE child; + GRANT parent TO admin WITH ADMIN TRUE; + GRANT parent TO child GRANTED BY admin; + {revoke};" + )) + .expect_err("child's grant depends on admin's"); + + assert!( + matches!( + &error, + Error::DependentRoleMemberships { role_name, member_name } + if role_name == "parent" && member_name == "admin" + ), + "{revoke}: {error:?}" + ); + } +} + +/// `CASCADE` follows the dependent grants down the chain of grantors, as +/// measured on PostgreSQL 18. +#[test] +fn revoke_cascade_removes_dependent_grants() { + let database = parse( + "CREATE ROLE parent; + CREATE ROLE admin; + CREATE ROLE deputy; + CREATE ROLE child; + CREATE ROLE other; + GRANT parent TO admin WITH ADMIN TRUE; + GRANT parent TO other WITH ADMIN TRUE; + GRANT parent TO deputy WITH ADMIN TRUE GRANTED BY admin; + GRANT parent TO child GRANTED BY deputy; + GRANT parent TO child GRANTED BY other; + REVOKE ADMIN OPTION FOR parent FROM admin CASCADE;", + ) + .expect("schema builds"); + + assert_eq!(memberships_of(&database, "admin"), [membership("parent", false, true, true)]); + assert_eq!(memberships_of(&database, "deputy"), [] as [Membership; 0]); + assert_eq!(memberships_of(&database, "child"), [membership("parent", false, true, true)]); + assert_eq!(memberships_of(&database, "other"), [membership("parent", true, true, true)]); +} + +/// A member keeping `ADMIN` through another grantor's grant strands nothing, +/// and `INHERIT` and `SET` back no grant, as measured on PostgreSQL 18. +#[test] +fn revoke_keeps_grants_still_backed_by_admin() { + let database = parse( + "CREATE ROLE parent; + CREATE ROLE admin; + CREATE ROLE child; + CREATE ROLE g; + GRANT parent TO g WITH ADMIN TRUE; + GRANT parent TO admin WITH ADMIN TRUE; + GRANT parent TO admin WITH ADMIN TRUE GRANTED BY g; + GRANT parent TO child GRANTED BY admin; + REVOKE INHERIT OPTION FOR parent FROM admin GRANTED BY g; + REVOKE SET OPTION FOR parent FROM admin GRANTED BY g; + REVOKE parent FROM admin;", + ) + .expect("schema builds"); + + assert_eq!(memberships_of(&database, "admin"), [membership("parent", true, false, false)]); + assert_eq!(memberships_of(&database, "child"), [membership("parent", false, true, true)]); +} + +/// PostgreSQL 18 refuses an unknown option, an absent role and `PUBLIC` as it +/// does in a grant. +#[test] +fn revoke_refuses_what_a_grant_refuses() { + let error = parse( + "CREATE ROLE parent; + CREATE ROLE child; + GRANT parent TO child; + REVOKE FOO OPTION FOR parent FROM child;", + ) + .expect_err("FOO is no role option"); + assert!(matches!(error, Error::UnrecognizedRoleOption { option } if option == "FOO")); + + for (revoke, absent) in [ + ("REVOKE nobody FROM child", "nobody"), + ("REVOKE parent FROM nobody", "nobody"), + ("REVOKE parent FROM PUBLIC", "public"), + ("REVOKE public FROM child", "public"), + ] { + let error = parse(&format!("CREATE ROLE parent; CREATE ROLE child; {revoke};")) + .expect_err("the revoke names no role"); + assert!( + matches!(&error, Error::RoleNotFoundForMembership { role_name } if role_name == absent), + "{revoke}: {error:?}" + ); + } +} + +#[test] +fn open_world_revoke_removes_memberships_naming_undeclared_roles() { + let database = open_world( + "GRANT parent TO child; + GRANT other TO child; + REVOKE parent FROM child; + REVOKE parent FROM nobody;", + ) + .expect("schema builds"); + + let recorded: Vec<_> = database + .role_memberships() + .map(|membership| (membership.role_name().to_owned(), membership.member_name().to_owned())) + .collect(); + assert_eq!(recorded, [("other".to_owned(), "child".to_owned())]); +} + +/// MySQL commits the open transaction before a definition, so a rollback +/// leaves the membership revoked. +#[test] +fn mysql_role_revoke_commits_the_open_transaction() { + let database = ParserDB::parse::( + "CREATE ROLE parent; + CREATE ROLE child; + GRANT parent TO child; + START TRANSACTION; + REVOKE parent FROM child; + ROLLBACK;", + ) + .expect("schema builds"); + + assert_eq!(memberships_of(&database, "child"), [] as [Membership; 0]); +} From c54339d930aad7efb6896c099dbf38b2e15ca349 Mon Sep 17 00:00:00 2001 From: LucaCappelletti94 Date: Wed, 28 Oct 2026 22:57:27 +0100 Subject: [PATCH 3/3] Keep a cascaded revoke deletion final and drop the unreachable keep arm --- .../generic_db/sqlparser/role_memberships.rs | 44 +++++++++---------- tests/role_memberships.rs | 18 ++++++++ 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/src/structs/generic_db/sqlparser/role_memberships.rs b/src/structs/generic_db/sqlparser/role_memberships.rs index 638e29a2a..373acce76 100644 --- a/src/structs/generic_db/sqlparser/role_memberships.rs +++ b/src/structs/generic_db/sqlparser/role_memberships.rs @@ -189,21 +189,21 @@ pub(super) fn revoke_role( Ok(()) } -/// What a revoke does to one grant of the revoked role. +/// What a revoke changes in one grant of the revoked role. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum Revocation { - Keep, +enum Change { Clear(MembershipOption), Delete, } /// One grant of the revoked role, by its membership's position in the -/// builder and its own position in that membership. +/// builder and its own position in that membership, with the change planned +/// for it, if any. #[derive(Debug, Clone, Copy)] struct PlannedGrant { membership: usize, grant: usize, - revocation: Revocation, + change: Option, } /// Plans the revoke of `role` from each of `members` over every grant of @@ -222,7 +222,7 @@ fn plan_revoke( .filter(|(_, (membership, ()))| idents_match(&membership.role, role)) .flat_map(|(membership, (granted, ()))| { (0..granted.grants.len()) - .map(move |grant| PlannedGrant { membership, grant, revocation: Revocation::Keep }) + .map(move |grant| PlannedGrant { membership, grant, change: None }) }) .collect(); for member in members { @@ -235,7 +235,7 @@ fn plan_revoke( }; match option { Some(cleared @ (MembershipOption::Inherit | MembershipOption::Set)) => { - rows[index].revocation = Revocation::Clear(cleared); + rows[index].change = Some(Change::Clear(cleared)); } Some(MembershipOption::Admin) => { plan_recursive_revoke(memberships, &mut rows, index, true, cascade, role)?; @@ -257,28 +257,25 @@ fn plan_recursive_revoke( cascade: bool, role: &Ident, ) -> Result<(), Error> { - match rows[index].revocation { - Revocation::Delete => return Ok(()), - Revocation::Clear(MembershipOption::Admin) if admin_only => return Ok(()), - _ => {} + // A deletion is final, even where a later member's visit asks for less. + if rows[index].change == Some(Change::Delete) { + return Ok(()); } let (membership, grant) = planned(memberships, &rows[index]); if !admin_only { - rows[index].revocation = Revocation::Delete; + rows[index].change = Some(Change::Delete); } if !grant.admin { return Ok(()); } if admin_only { - rows[index].revocation = Revocation::Clear(MembershipOption::Admin); + rows[index].change = Some(Change::Clear(MembershipOption::Admin)); } let member = &membership.member; let still_admin = rows.iter().any(|row| { let (other, other_grant) = planned(memberships, row); - row.revocation == Revocation::Keep - && other_grant.admin - && idents_match(&other.member, member) + row.change.is_none() && other_grant.admin && idents_match(&other.member, member) }); if still_admin { return Ok(()); @@ -286,7 +283,7 @@ fn plan_recursive_revoke( // Indexed because each recursion replans rows this loop has yet to reach. for dependent in 0..rows.len() { let (_, dependent_grant) = planned(memberships, &rows[dependent]); - if rows[dependent].revocation == Revocation::Delete + if rows[dependent].change == Some(Change::Delete) || !dependent_grant .grantor .as_ref() @@ -321,15 +318,14 @@ fn apply_revoke(builder: &mut ParserDBBuilder, rows: &[PlannedGrant]) { let mut emptied = false; // Rows run in ascending grant order within a membership, so walking them // backwards removes each grant before the ones ahead of it shift. - for row in rows.iter().rev().filter(|row| row.revocation != Revocation::Keep) { + for (row, change) in rows.iter().rev().filter_map(|row| Some((row, row.change?))) { let membership = Arc::make_mut(&mut memberships[row.membership].0); let grant = &mut membership.grants[row.grant]; - match row.revocation { - Revocation::Keep => {} - Revocation::Clear(MembershipOption::Admin) => grant.admin = false, - Revocation::Clear(MembershipOption::Inherit) => grant.inherit = false, - Revocation::Clear(MembershipOption::Set) => grant.set = false, - Revocation::Delete => { + match change { + Change::Clear(MembershipOption::Admin) => grant.admin = false, + Change::Clear(MembershipOption::Inherit) => grant.inherit = false, + Change::Clear(MembershipOption::Set) => grant.set = false, + Change::Delete => { membership.grants.remove(row.grant); emptied |= membership.grants.is_empty(); } diff --git a/tests/role_memberships.rs b/tests/role_memberships.rs index eca0d3077..46f3dbe9c 100644 --- a/tests/role_memberships.rs +++ b/tests/role_memberships.rs @@ -750,6 +750,24 @@ fn revoke_cascade_removes_dependent_grants() { assert_eq!(memberships_of(&database, "other"), [membership("parent", true, true, true)]); } +/// A grant `CASCADE` deletes stays deleted when the statement also names its +/// member for less, as PostgreSQL's revoke planning keeps it, over a +/// self-granted `ADMIN` that only this model accepts. +#[test] +fn revoke_cascade_deletion_outlasts_a_later_option_revoke() { + let database = parse( + "CREATE ROLE parent; + CREATE ROLE g; + CREATE ROLE b; + GRANT parent TO g WITH ADMIN TRUE GRANTED BY g; + GRANT parent TO b WITH ADMIN TRUE GRANTED BY g; + REVOKE ADMIN OPTION FOR parent FROM g, b GRANTED BY g CASCADE;", + ) + .expect("schema builds"); + + assert_eq!(database.role_memberships().count(), 0); +} + /// A member keeping `ADMIN` through another grantor's grant strands nothing, /// and `INHERIT` and `SET` back no grant, as measured on PostgreSQL 18. #[test]