diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bdd8de..c385a51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,16 +33,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Approvals are bound to the database they were reviewed against, not just the Secret that reaches it.** Every plan records the server's physical identity (`pg_control_system().system_identifier`, the storage lineage) and a logical fingerprint of the resolved host, port and database, and both are part of the approval digest. If either changes between approval and execution — or the physical identifier was readable at approval and is not at execution — the plan is superseded instead of executed. Set `spec.connection.requirePhysicalIdentity: true` to stop reconciliation entirely (`TargetIdentityBlocked`) when the identifier cannot be read, e.g. on engines that only speak the PostgreSQL protocol. (#180, #173) +- **Owner-wide default privileges.** `default_privileges` entries accept `scope: {type: global}` beside the existing `schema:` shorthand, emitting `ALTER DEFAULT PRIVILEGES FOR ROLE ...` with no `IN SCHEMA` clause. PostgreSQL keeps default privileges in two layers, and only the global one applies to every schema an owner creates objects in — including schemas no policy manages. Inspection reads the global layer for exactly the `(owner, object type)` pairs a manifest declares, reporting the *effective* default so a database with no explicit `pg_default_acl` row still compares against what PostgreSQL will apply. Owner self-entries are excluded, because every `ALTER DEFAULT PRIVILEGES` materializes the owner's implicit self-grant into the stored row and reporting it would make authoritative mode revoke the owner's own default on the next reconcile. Global changes are counted on their own line in `diff` output, and in a bundle only the document owning the owner role may declare them. See [default privileges](https://hardbyte.github.io/pgroles/docs/default-privileges/). + +- **`ensure: absent` and a typed `PUBLIC` grantee.** PostgreSQL grants `EXECUTE` on every function to `PUBLIC` without writing an ACL entry, so no combination of positive grants could take it away — a `SECURITY DEFINER` routine stayed callable by every role. Grant entries and default-privilege entries now accept `ensure: absent`, which revokes a privilege where it is held, and `role: PUBLIC` addresses the pseudo-role (rendered unquoted, never as the identifier `"PUBLIC"`). Inspection reports PUBLIC's *effective* privileges, synthesizing `acldefault(...)` where the ACL is still NULL, so a fresh database plans the revoke it needs. Pair an object-level absence rule with a global default-privilege one to cover both today's objects and tomorrow's. **PUBLIC is reconciled only where a rule names it**, in every mode: a PUBLIC privilege no rule mentions is never revoked, and deleting a `present` PUBLIC rule does not revoke it — switch the rule to `ensure: absent`. `additive` silently ignores absence assertions, since it never revokes; `adopt` and `authoritative` apply them. Preflight warns on `diff` and dry runs, and blocks a real apply, when the executor cannot act as a default-privilege owner or cannot revoke on objects it does not own — a PUBLIC revoke without that authority silently changes nothing and would otherwise re-plan forever. See [grants](https://hardbyte.github.io/pgroles/docs/grants/) and [default privileges](https://hardbyte.github.io/pgroles/docs/default-privileges/). + ### Changed +- **Bundle plan JSON is now `pgroles.bundle_plan.v2`.** Default-privilege changes and their ownership keys carry a tagged `scope` (`{"type": "schema", "schema": "app"}` or `{"type": "global"}`) in place of the bare `schema` string, which could not express a global rule. **Migration:** read `scope.schema` where you read `schema`, and handle `scope.type == "global"` entries having no schema at all. + +- **`diff --format json` carries the same tagged `scope` on default-privilege changes.** Unlike bundle output it has no `schema_version` field to bump, so nothing announces the change in the payload itself. **Migration:** the same one as above — read `scope.schema` where you read `schema`, and handle `scope.type == "global"` entries having no schema. This output is a bare array of changes and stays unversioned for now, so treat its shape as unstable and pin the pgroles version if you parse it. + - **`spec.mode: plan` is renamed to `spec.mode: observe`, with a deprecation window.** "Plan" now names exactly one thing, the `PostgresPolicyPlan` resource; the `ApprovalIgnored` reason `PlanModeNeverExecutes` is now `ObserveModeNeverExecutes`. The old value keeps working: `mode: plan` stays an accepted schema value with identical behaviour, so a GitOps controller re-applying an existing manifest is unaffected by the upgrade. A policy using it reports a `ModeValueDeprecated` condition, warns in the operator log, and counts toward `pgroles.deprecated.mode_plan`. **Upgrade:** change `mode: plan` to `mode: observe` in your manifests at your convenience — a future release removes the `plan` value, and that removal will be the breaking change. - **BREAKING: policy content now has explicit size limits.** Identifiers (role, schema, owner, member names) are capped at 63 characters *and* 63 bytes — the point past which PostgreSQL silently truncates — and every list and map has a bound: 1024 roles, 4096 grants, 2048 memberships, and so on (full table in the [manifest reference](https://hardbyte.github.io/pgroles/docs/manifest-reference/)). The bounds apply to `PostgresPolicy`, to candidates, and to `pgroles validate` alike, and they are what makes candidate immutability enforceable by the API server. **Upgrade:** a policy exceeding a limit is rejected on its next apply with a field-level error. Each limit sits at least 20× above the corresponding count in the largest policy known to run in production; previously the same policy would eventually have hit an opaque `etcdserver: request is too large`. (#182, #173) -- **BREAKING: the approval digest encoding is now `pgroles.io/approval-effect/v2`**, which binds the target identity above. - **Upgrade:** on the first reconcile after upgrading, every open plan is superseded and replaced by an equivalent plan under v2, and recorded decisions do not carry over — open plans need one fresh approval. Nothing executes in the meantime. Deliberately, a `pg_upgrade` (fresh `system_identifier`) or a blue-green cutover also moves the identity and invalidates any approval open across it; re-approve the fresh plan afterwards. (#180) +- **BREAKING: the approval digest encoding is now `pgroles.io/approval-effect/v3`.** v2 binds the target identity above, and v3 additionally carries a default-privilege rule's scope as a tagged `scope` value instead of a bare `schema` string, which could not express an owner-wide rule. + **Upgrade:** on the first reconcile after upgrading, every open plan is superseded and replaced by an equivalent plan under v3, and recorded decisions do not carry over — open plans need one fresh approval. Nothing executes in the meantime. Deliberately, a `pg_upgrade` (fresh `system_identifier`) or a blue-green cutover also moves the identity and invalidates any approval open across it; re-approve the fresh plan afterwards. (#180) - **URL-mode connections bind the endpoint they resolve to**, not only the Secret name and key — editing the URL inside a referenced Secret is no longer invisible to an open approval. Credentials stay excluded, so password and token rotation still do not invalidate approvals. (#180, #185) diff --git a/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml b/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml index 28ca0fb..5e336df 100644 --- a/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml +++ b/charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml @@ -332,12 +332,20 @@ "default": [], "description": "One-off default privileges.", "items": { - "description": "Default privilege configuration.", + "description": "Default privilege configuration.\n\nThe scope rules are also expressed as CEL so the API server rejects a bad\nentry at apply time. `resolved_scope` enforces the same rules for the CLI,\nwhich has no admission step.", "properties": { "grant": { "items": { "description": "A single default privilege grant entry.", "properties": { + "ensure": { + "description": "Whether the listed privileges must exist or must not exist.\n\n`absent` asserts one ACL edge only. It plans a REVOKE when the privilege is\nfound live, and it says nothing about access the grantee may still have\nthrough role membership or ownership.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "on_type": { "description": "PostgreSQL object types that can have privileges granted on them.", "enum": [ @@ -376,7 +384,7 @@ "type": "array" }, "role": { - "description": "The role receiving the default privilege. Only used in top-level default_privileges\n(in profiles, the role is determined by expansion).", + "description": "The role receiving the default privilege. Only used in top-level default_privileges\n(in profiles, the role is determined by expansion). The exact-uppercase\nvalue `PUBLIC` means the PostgreSQL PUBLIC pseudo-role.", "maxLength": 63, "minLength": 1, "nullable": true, @@ -400,16 +408,54 @@ "type": "string" }, "schema": { + "description": "Schema shorthand, equivalent to `scope: {type: schema, schema: ...}`.\nExactly one of `schema` and `scope` must be set.", "maxLength": 63, "minLength": 1, + "nullable": true, "type": "string" + }, + "scope": { + "description": "Where the defaults apply: one schema, or owner-wide (global). Global\nscope renders `ALTER DEFAULT PRIVILEGES` without an `IN SCHEMA` clause.", + "nullable": true, + "properties": { + "schema": { + "description": "Schema name. Required for `type: schema`, forbidden for `type: global`.", + "maxLength": 63, + "minLength": 1, + "nullable": true, + "type": "string" + }, + "type": { + "description": "The kind of default-privilege scope.", + "enum": [ + "global", + "schema" + ], + "type": "string" + } + }, + "required": [ + "type" + ], + "type": "object", + "x-kubernetes-validations": [ + { + "message": "`schema` is required when type is `schema` and forbidden when type is `global`", + "rule": "has(self.schema) == (self.type == 'schema')" + } + ] } }, "required": [ - "grant", - "schema" + "grant" ], - "type": "object" + "type": "object", + "x-kubernetes-validations": [ + { + "message": "exactly one of `schema` and `scope` must be set", + "rule": "has(self.schema) != has(self.scope)" + } + ] }, "maxItems": 512, "type": "array" @@ -420,6 +466,14 @@ "items": { "description": "A concrete grant on a specific object or wildcard.", "properties": { + "ensure": { + "description": "Whether the listed privileges must exist or must not exist.\n\n`absent` asserts one ACL edge only. It plans a REVOKE when the privilege is\nfound live, and it says nothing about access the grantee may still have\nthrough role membership or ownership.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "object": { "description": "Target object for a grant.", "properties": { @@ -481,6 +535,7 @@ "type": "array" }, "role": { + "description": "The grantee. The exact-uppercase value `PUBLIC` means the PostgreSQL\nPUBLIC pseudo-role; any other value is an ordinary role name.", "maxLength": 63, "minLength": 1, "type": "string" @@ -579,6 +634,14 @@ "items": { "description": "Default privilege grant within a profile.", "properties": { + "ensure": { + "description": "Whether the privilege must be present or absent. Matches the\ntop-level `default_privileges` entries, which carry the same field.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "on_type": { "description": "PostgreSQL object types that can have privileges granted on them.", "enum": [ diff --git a/charts/pgroles-operator/crds/postgrespolicycandidates.pgroles.io.yaml b/charts/pgroles-operator/crds/postgrespolicycandidates.pgroles.io.yaml index c25a1e7..77d162e 100644 --- a/charts/pgroles-operator/crds/postgrespolicycandidates.pgroles.io.yaml +++ b/charts/pgroles-operator/crds/postgrespolicycandidates.pgroles.io.yaml @@ -69,12 +69,20 @@ "default_privileges": { "description": "One-off default privileges.", "items": { - "description": "Default privilege configuration.", + "description": "Default privilege configuration.\n\nThe scope rules are also expressed as CEL so the API server rejects a bad\nentry at apply time. `resolved_scope` enforces the same rules for the CLI,\nwhich has no admission step.", "properties": { "grant": { "items": { "description": "A single default privilege grant entry.", "properties": { + "ensure": { + "description": "Whether the listed privileges must exist or must not exist.\n\n`absent` asserts one ACL edge only. It plans a REVOKE when the privilege is\nfound live, and it says nothing about access the grantee may still have\nthrough role membership or ownership.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "on_type": { "description": "PostgreSQL object types that can have privileges granted on them.", "enum": [ @@ -113,7 +121,7 @@ "type": "array" }, "role": { - "description": "The role receiving the default privilege. Only used in top-level default_privileges\n(in profiles, the role is determined by expansion).", + "description": "The role receiving the default privilege. Only used in top-level default_privileges\n(in profiles, the role is determined by expansion). The exact-uppercase\nvalue `PUBLIC` means the PostgreSQL PUBLIC pseudo-role.", "maxLength": 63, "minLength": 1, "nullable": true, @@ -137,16 +145,54 @@ "type": "string" }, "schema": { + "description": "Schema shorthand, equivalent to `scope: {type: schema, schema: ...}`.\nExactly one of `schema` and `scope` must be set.", "maxLength": 63, "minLength": 1, + "nullable": true, "type": "string" + }, + "scope": { + "description": "Where the defaults apply: one schema, or owner-wide (global). Global\nscope renders `ALTER DEFAULT PRIVILEGES` without an `IN SCHEMA` clause.", + "nullable": true, + "properties": { + "schema": { + "description": "Schema name. Required for `type: schema`, forbidden for `type: global`.", + "maxLength": 63, + "minLength": 1, + "nullable": true, + "type": "string" + }, + "type": { + "description": "The kind of default-privilege scope.", + "enum": [ + "global", + "schema" + ], + "type": "string" + } + }, + "required": [ + "type" + ], + "type": "object", + "x-kubernetes-validations": [ + { + "message": "`schema` is required when type is `schema` and forbidden when type is `global`", + "rule": "has(self.schema) == (self.type == 'schema')" + } + ] } }, "required": [ - "grant", - "schema" + "grant" ], - "type": "object" + "type": "object", + "x-kubernetes-validations": [ + { + "message": "exactly one of `schema` and `scope` must be set", + "rule": "has(self.schema) != has(self.scope)" + } + ] }, "maxItems": 512, "type": "array" @@ -156,6 +202,14 @@ "items": { "description": "A concrete grant on a specific object or wildcard.", "properties": { + "ensure": { + "description": "Whether the listed privileges must exist or must not exist.\n\n`absent` asserts one ACL edge only. It plans a REVOKE when the privilege is\nfound live, and it says nothing about access the grantee may still have\nthrough role membership or ownership.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "object": { "description": "Target object for a grant.", "properties": { @@ -217,6 +271,7 @@ "type": "array" }, "role": { + "description": "The grantee. The exact-uppercase value `PUBLIC` means the PostgreSQL\nPUBLIC pseudo-role; any other value is an ordinary role name.", "maxLength": 63, "minLength": 1, "type": "string" @@ -298,6 +353,14 @@ "items": { "description": "Default privilege grant within a profile.", "properties": { + "ensure": { + "description": "Whether the privilege must be present or absent. Matches the\ntop-level `default_privileges` entries, which carry the same field.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "on_type": { "description": "PostgreSQL object types that can have privileges granted on them.", "enum": [ diff --git a/crates/pgroles-cli/src/lib.rs b/crates/pgroles-cli/src/lib.rs index 6771383..d94dd66 100644 --- a/crates/pgroles-cli/src/lib.rs +++ b/crates/pgroles-cli/src/lib.rs @@ -11,7 +11,7 @@ use anyhow::{Context, Result}; use pgroles_core::composition::{self, ComposedPolicy, PolicyBundle, PolicyDocument}; use pgroles_core::diff::{self, Change}; use pgroles_core::manifest::{self, ExpandedManifest, PolicyManifest, RoleRetirement}; -use pgroles_core::model::RoleGraph; +use pgroles_core::model::{DefaultPrivilegeScope, RoleGraph}; use pgroles_core::ownership::ManagedScope; use pgroles_core::report::{self, PlanOutputMode}; use pgroles_core::sql; @@ -212,6 +212,11 @@ pub struct PlanSummary { pub revokes: usize, pub default_privileges_set: usize, pub default_privileges_revoked: usize, + /// Global (owner-wide) default privilege changes, counted separately from + /// the schema-scoped totals above because they affect every schema in the + /// database. + pub global_default_privileges_set: usize, + pub global_default_privileges_revoked: usize, pub members_added: usize, pub members_removed: usize, pub passwords_set: usize, @@ -236,8 +241,20 @@ impl PlanSummary { summary.grants += 1 } Change::Revoke { .. } => summary.revokes += 1, - Change::SetDefaultPrivilege { .. } => summary.default_privileges_set += 1, - Change::RevokeDefaultPrivilege { .. } => summary.default_privileges_revoked += 1, + Change::SetDefaultPrivilege { scope, .. } => { + if matches!(scope, DefaultPrivilegeScope::Global) { + summary.global_default_privileges_set += 1; + } else { + summary.default_privileges_set += 1; + } + } + Change::RevokeDefaultPrivilege { scope, .. } => { + if matches!(scope, DefaultPrivilegeScope::Global) { + summary.global_default_privileges_revoked += 1; + } else { + summary.default_privileges_revoked += 1; + } + } Change::AddMember { .. } => summary.members_added += 1, Change::RemoveMember { .. } => summary.members_removed += 1, Change::SetPassword { .. } => summary.passwords_set += 1, @@ -261,6 +278,8 @@ impl PlanSummary { + self.revokes + self.default_privileges_set + self.default_privileges_revoked + + self.global_default_privileges_set + + self.global_default_privileges_revoked + self.members_added + self.members_removed + self.passwords_set @@ -313,6 +332,14 @@ impl PlanSummary { "default privilege(s) to revoke", self.default_privileges_revoked, ), + ( + "GLOBAL default privilege(s) to set (affects every schema)", + self.global_default_privileges_set, + ), + ( + "GLOBAL default privilege(s) to revoke (affects every schema)", + self.global_default_privileges_revoked, + ), ("membership(s) to add", self.members_added), ("membership(s) to remove", self.members_removed), ("password(s) to set", self.passwords_set), @@ -1078,7 +1105,7 @@ schemas: .insert("stale-role".to_string(), RoleState::default()); current.grants.insert( GrantKey { - role: "analytics".to_string(), + role: "analytics".into(), object_type: ObjectType::Table, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -1167,7 +1194,10 @@ roles: let parsed: serde_json::Value = serde_json::from_str(&json_output).unwrap(); assert!(parsed.is_object()); - assert_eq!(parsed["schema_version"], "pgroles.bundle_plan.v1"); + assert_eq!( + parsed["schema_version"], + pgroles_core::report::BUNDLE_PLAN_SCHEMA_VERSION + ); assert_eq!(parsed["managed_scope"]["roles"][0], "app"); assert_eq!(parsed["changes"][0]["owner"]["document"], "app"); assert_eq!(parsed["changes"][0]["owner"]["managed_key"]["kind"], "role"); diff --git a/crates/pgroles-cli/src/main.rs b/crates/pgroles-cli/src/main.rs index 37b0716..1fcc7f4 100644 --- a/crates/pgroles-cli/src/main.rs +++ b/crates/pgroles-cli/src/main.rs @@ -743,6 +743,7 @@ async fn cmd_diff( &changes, &validated.composed.managed_change_surface, )?; + preflight_authority(&pool, &changes, ¤t, false).await?; let drop_safety = inspect_drop_safety(&pool, &changes, &validated.composed.manifest.retirements).await?; let summary = PlanSummary::from_changes(&changes); @@ -802,6 +803,7 @@ async fn cmd_diff( let resolved_passwords = resolve_passwords(&validated.expanded).context("failed to resolve role passwords")?; let changes = inject_password_changes(changes, &resolved_passwords); + preflight_authority(&pool, &changes, ¤t, false).await?; let drop_safety = inspect_drop_safety(&pool, &changes, &validated.manifest.retirements).await?; let summary = PlanSummary::from_changes(&changes); @@ -877,7 +879,6 @@ async fn cmd_apply( &changes, &validated.composed.managed_change_surface, )?; - // Validate changes against privilege level. let priv_warnings = pgroles_inspect::cloud::validate_changes_for_privilege_level( &changes, @@ -904,12 +905,15 @@ async fn cmd_apply( println!("-- DRY RUN: the following SQL would be executed:\n"); print!("{sql_output}"); eprintln!("\n{}", summary.format_plan()); + preflight_authority(&pool, &changes, ¤t, false).await?; if !drop_safety.is_empty() { eprintln!("\n{drop_safety}"); } return Ok(()); } + preflight_authority(&pool, &changes, ¤t, true).await?; + if drop_safety.has_blockers() { anyhow::bail!("{}", drop_safety.blockers); } @@ -960,7 +964,6 @@ async fn cmd_apply( let resolved_passwords = resolve_passwords(&validated.expanded).context("failed to resolve role passwords")?; let changes = inject_password_changes(changes, &resolved_passwords); - // Validate changes against privilege level. let priv_warnings = pgroles_inspect::cloud::validate_changes_for_privilege_level(&changes, &privilege_level); @@ -984,12 +987,15 @@ async fn cmd_apply( println!("-- DRY RUN: the following SQL would be executed:\n"); print!("{sql_output}"); eprintln!("\n{}", summary.format_plan()); + preflight_authority(&pool, &changes, ¤t, false).await?; if !drop_safety.is_empty() { eprintln!("\n{drop_safety}"); } return Ok(()); } + preflight_authority(&pool, &changes, ¤t, true).await?; + if drop_safety.has_blockers() { anyhow::bail!("{}", drop_safety.blockers); } @@ -1579,6 +1585,40 @@ async fn inspect_current_for_plan_with_config( Ok(inspection.graph) } +/// Report planned default-privilege changes and PUBLIC revokes the executor +/// lacks the authority to perform. Runs on the final change list, so converged +/// state never reports anything. +/// +/// `blocking` is true only where SQL actually runs. `diff` and `apply +/// --dry-run` execute nothing, so they warn and still produce their plan — +/// a CI drift check running as a read-only role keeps working. This mirrors +/// how drop safety is handled, which also warns until the moment of execution. +async fn preflight_authority( + pool: &PgPool, + changes: &[pgroles_core::diff::Change], + current: &pgroles_core::model::RoleGraph, + blocking: bool, +) -> Result<()> { + let issues = pgroles_inspect::preflight_authority_issues(pool, changes, current) + .await + .context("failed to check executor authority")?; + if issues.is_empty() { + return Ok(()); + } + let message = issues + .iter() + .map(ToString::to_string) + .collect::>() + .join("\n"); + if blocking { + anyhow::bail!("{message}"); + } + for issue in &issues { + eprintln!("Warning: {issue}"); + } + Ok(()) +} + async fn inspect_drop_safety( pool: &PgPool, changes: &[pgroles_core::diff::Change], diff --git a/crates/pgroles-cli/tests/cli.rs b/crates/pgroles-cli/tests/cli.rs index 92bb101..389e315 100644 --- a/crates/pgroles-cli/tests/cli.rs +++ b/crates/pgroles-cli/tests/cli.rs @@ -3142,7 +3142,10 @@ roles: let parsed: serde_json::Value = serde_json::from_slice(&output).expect("diff json should parse"); - assert_eq!(parsed["schema_version"], "pgroles.bundle_plan.v1"); + assert_eq!( + parsed["schema_version"], + pgroles_core::report::BUNDLE_PLAN_SCHEMA_VERSION + ); assert_eq!(parsed["managed_scope"]["roles"][0], managed_role); assert_eq!(parsed["changes"][0]["owner"]["document"], "app"); assert_eq!(parsed["changes"][0]["owner"]["managed_key"]["kind"], "role"); @@ -4890,4 +4893,680 @@ grants: "# )); } + + /// The `pg_default_acl` ACL text for an owner's global (namespace 0) + /// defaults, or `None` when PostgreSQL has not materialized a row yet. + fn query_global_default_acl(owner: &str, object_type: &str) -> Option { + with_runtime(async { + let pool = PgPool::connect(&database_url()) + .await + .expect("failed to connect to live test database"); + let row = sqlx::query( + r#" + SELECT da.defaclacl::text AS acl + FROM pg_default_acl da + JOIN pg_roles owner_role ON owner_role.oid = da.defaclrole + WHERE owner_role.rolname = $1 + AND da.defaclnamespace = 0 + AND da.defaclobjtype::text = $2 + "#, + ) + .bind(owner) + .bind(object_type) + .fetch_optional(&pool) + .await + .expect("failed to query global default privileges"); + row.map(|row| row.get("acl")) + }) + } + + /// Set up the proposal's SECURITY DEFINER scenario: an owner role, an + /// allowed caller, an unrelated bystander, and one existing routine. + struct PublicFixture { + schema: String, + owner: String, + caller: String, + bystander: String, + } + + impl PublicFixture { + fn new(prefix: &str) -> (Self, TestDbCleanup) { + let fixture = PublicFixture { + schema: unique_name(&format!("{prefix}_schema")), + owner: unique_name(&format!("{prefix}_owner")), + caller: unique_name(&format!("{prefix}_caller")), + bystander: unique_name(&format!("{prefix}_bystander")), + }; + // DROP OWNED must run first: a role owning global default + // privileges is undroppable while pg_default_acl references it. + let cleanup = TestDbCleanup::new(format!( + r#"DROP SCHEMA IF EXISTS "{schema}" CASCADE; + DO $$ BEGIN + IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = '{owner}') THEN + EXECUTE 'DROP OWNED BY "{owner}" CASCADE'; + END IF; + IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = '{caller}') THEN + EXECUTE 'DROP OWNED BY "{caller}" CASCADE'; + END IF; + END $$; + DROP ROLE IF EXISTS "{owner}"; + DROP ROLE IF EXISTS "{caller}"; + DROP ROLE IF EXISTS "{bystander}";"#, + schema = fixture.schema, + owner = fixture.owner, + caller = fixture.caller, + bystander = fixture.bystander, + )); + execute_sql(&format!( + r#"CREATE ROLE "{owner}"; + CREATE ROLE "{caller}"; + CREATE ROLE "{bystander}"; + CREATE SCHEMA "{schema}" AUTHORIZATION "{owner}"; + SET ROLE "{owner}"; + CREATE FUNCTION "{schema}".secret(x integer) RETURNS integer + LANGUAGE sql SECURITY DEFINER AS 'SELECT x'; + RESET ROLE;"#, + schema = fixture.schema, + owner = fixture.owner, + caller = fixture.caller, + bystander = fixture.bystander, + )); + (fixture, cleanup) + } + + fn secret_signature(&self) -> String { + format!("\"{}\".secret(integer)", self.schema) + } + + /// A manifest asserting PUBLIC holds no EXECUTE on existing or future + /// routines, while the caller keeps access. + fn manifest(&self) -> String { + self.manifest_with_extra("", "") + } + + /// `extra_roles` and `extra_grants` are spliced into the matching + /// sections so callers never append after `default_privileges`. + fn manifest_with_extra(&self, extra_roles: &str, extra_grants: &str) -> String { + format!( + r#" +roles: + - name: {owner} + - name: {caller} +{extra_roles}grants: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: {{ type: function, schema: {schema}, name: "*" }} +{extra_grants} + - role: {caller} + privileges: [USAGE] + object: {{ type: schema, name: {schema} }} + - role: {caller} + privileges: [EXECUTE] + object: {{ type: function, schema: {schema}, name: "*" }} +default_privileges: + - owner: {owner} + scope: {{ type: global }} + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function + - owner: {owner} + schema: {schema} + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function + - role: {caller} + privileges: [EXECUTE] + on_type: function +"#, + owner = self.owner, + caller = self.caller, + schema = self.schema, + extra_roles = extra_roles, + extra_grants = extra_grants, + ) + } + } + + /// Apply until the plan is empty, returning the number of passes. + /// + /// pgroles needs a second pass whenever the first one materializes an + /// object ACL: PostgreSQL writes the owner's implicit self-grant into the + /// ACL at that moment, and authoritative mode then revokes it. That is + /// pre-existing behavior for any grant on an owner-managed object, not + /// something absence assertions introduce. + fn apply_until_converged(manifest_path: &std::path::Path, max_passes: usize) -> usize { + for pass in 1..=max_passes { + pgroles_cmd() + .args([ + "apply", + "--file", + manifest_path.to_str().unwrap(), + "--database-url", + &database_url(), + ]) + .assert() + .success(); + + let output = pgroles_cmd() + .args([ + "diff", + "--file", + manifest_path.to_str().unwrap(), + "--database-url", + &database_url(), + "--format", + "summary", + "--no-exit-code", + ]) + .assert() + .success() + .get_output() + .stdout + .clone(); + if String::from_utf8_lossy(&output).contains("No changes needed") { + return pass; + } + } + panic!("plan did not converge within {max_passes} passes"); + } + + #[test] + #[ignore] + fn public_absence_revokes_implicit_execute_and_converges() { + let (fixture, _cleanup) = PublicFixture::new("pubabs"); + let manifest = write_temp_manifest(&fixture.manifest()); + + // PostgreSQL grants EXECUTE to PUBLIC implicitly, with no ACL row. + assert!(query_has_function_privilege( + &fixture.bystander, + &fixture.secret_signature() + )); + + let plan = String::from_utf8_lossy( + &pgroles_cmd() + .args([ + "diff", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &database_url(), + "--format", + "sql", + "--no-exit-code", + ]) + .assert() + .success() + .get_output() + .stdout, + ) + .to_string(); + assert!( + plan.contains(&format!( + r#"REVOKE EXECUTE ON ALL ROUTINES IN SCHEMA "{}" FROM PUBLIC;"#, + fixture.schema + )), + "plan should revoke PUBLIC EXECUTE on existing routines:\n{plan}" + ); + + apply_until_converged(manifest.path(), 3); + + assert!( + !query_has_function_privilege(&fixture.bystander, &fixture.secret_signature()), + "PUBLIC EXECUTE should be gone" + ); + assert!( + query_has_function_privilege(&fixture.caller, &fixture.secret_signature()), + "the allowed caller should keep EXECUTE" + ); + } + + #[test] + #[ignore] + fn global_default_revoke_renders_without_in_schema_and_protects_new_routines() { + let (fixture, _cleanup) = PublicFixture::new("pubglobal"); + let manifest = write_temp_manifest(&fixture.manifest()); + + let plan = String::from_utf8_lossy( + &pgroles_cmd() + .args([ + "diff", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &database_url(), + "--format", + "sql", + "--no-exit-code", + ]) + .assert() + .success() + .get_output() + .stdout, + ) + .to_string(); + let global_revoke = format!( + r#"ALTER DEFAULT PRIVILEGES FOR ROLE "{}" REVOKE EXECUTE ON ROUTINES FROM PUBLIC;"#, + fixture.owner + ); + assert!( + plan.contains(&global_revoke), + "global default revoke must omit IN SCHEMA:\n{plan}" + ); + + apply_until_converged(manifest.path(), 3); + + // The built-in default is now an explicit row without PUBLIC. + let acl = query_global_default_acl(&fixture.owner, "f") + .expect("global default row should exist after the revoke"); + // A PUBLIC entry has an empty grantee, so it renders as `{=X/owner}` + // or `,=X/owner`. The owner's own `owner=X/owner` entry must remain. + assert!( + !acl.contains("{=") && !acl.contains(",="), + "no PUBLIC entry expected in global defaults, got {acl}" + ); + assert!( + acl.contains(&format!("{owner}=X/", owner = fixture.owner)), + "the owner keeps its own default EXECUTE, got {acl}" + ); + + // Routines created afterwards inherit the corrected default. + execute_sql(&format!( + r#"SET ROLE "{owner}"; + CREATE FUNCTION "{schema}".later(y integer) RETURNS integer + LANGUAGE sql AS 'SELECT y'; + CREATE PROCEDURE "{schema}".later_proc() LANGUAGE sql AS 'SELECT 1'; + RESET ROLE;"#, + owner = fixture.owner, + schema = fixture.schema, + )); + + for signature in [ + format!("\"{}\".later(integer)", fixture.schema), + format!("\"{}\".later_proc()", fixture.schema), + ] { + assert!( + !query_has_function_privilege(&fixture.bystander, &signature), + "new routine {signature} must not be executable by PUBLIC" + ); + assert!( + query_has_function_privilege(&fixture.caller, &signature), + "new routine {signature} should be executable by the allowed caller" + ); + } + + // Creating routines must not reopen PUBLIC drift. (pgroles may still + // plan a revoke of the owner's self-grant that PostgreSQL + // materialized into each new ACL — pre-existing behavior for any + // grant on an owner-managed object, unrelated to PUBLIC.) + let plan_after = String::from_utf8_lossy( + &pgroles_cmd() + .args([ + "diff", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &database_url(), + "--format", + "sql", + "--no-exit-code", + ]) + .assert() + .success() + .get_output() + .stdout, + ) + .to_string(); + assert!( + !plan_after.contains("FROM PUBLIC"), + "new routines must not reopen PUBLIC drift:\n{plan_after}" + ); + } + + #[test] + #[ignore] + fn drifted_public_grant_is_revoked_again() { + let (fixture, _cleanup) = PublicFixture::new("pubdrift"); + let manifest = write_temp_manifest(&fixture.manifest()); + apply_until_converged(manifest.path(), 3); + + // Someone re-grants by hand, both on the object and as a schema default. + execute_sql(&format!( + r#"GRANT EXECUTE ON FUNCTION "{schema}".secret(integer) TO PUBLIC; + ALTER DEFAULT PRIVILEGES FOR ROLE "{owner}" IN SCHEMA "{schema}" + GRANT EXECUTE ON FUNCTIONS TO PUBLIC;"#, + schema = fixture.schema, + owner = fixture.owner, + )); + assert!(query_has_function_privilege( + &fixture.bystander, + &fixture.secret_signature() + )); + + apply_until_converged(manifest.path(), 3); + + assert!( + !query_has_function_privilege(&fixture.bystander, &fixture.secret_signature()), + "the re-granted PUBLIC EXECUTE should be revoked again" + ); + } + + #[test] + #[ignore] + fn additive_mode_ignores_absence_while_adopt_applies_it() { + let (fixture, _cleanup) = PublicFixture::new("pubmode"); + let manifest = write_temp_manifest(&fixture.manifest()); + + // Additive never revokes, so the absence assertion is skipped rather + // than failing the run. + pgroles_cmd() + .args([ + "apply", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &database_url(), + "--mode", + "additive", + ]) + .assert() + .success(); + assert!( + query_has_function_privilege(&fixture.bystander, &fixture.secret_signature()), + "additive mode must not revoke PUBLIC EXECUTE" + ); + + // Adopt applies the same assertion. + pgroles_cmd() + .args([ + "apply", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &database_url(), + "--mode", + "adopt", + ]) + .assert() + .success(); + assert!( + !query_has_function_privilege(&fixture.bystander, &fixture.secret_signature()), + "adopt mode must apply the absence assertion" + ); + } + + #[test] + #[ignore] + fn overloaded_routines_converge_under_one_wildcard_absence() { + let (fixture, _cleanup) = PublicFixture::new("puboverload"); + execute_sql(&format!( + r#"SET ROLE "{owner}"; + CREATE FUNCTION "{schema}".secret(x text) RETURNS text + LANGUAGE sql SECURITY DEFINER AS 'SELECT x'; + CREATE PROCEDURE "{schema}".secret_proc() LANGUAGE sql AS 'SELECT 1'; + RESET ROLE;"#, + owner = fixture.owner, + schema = fixture.schema, + )); + + let manifest = write_temp_manifest(&fixture.manifest()); + apply_until_converged(manifest.path(), 3); + + for signature in [ + format!("\"{}\".secret(integer)", fixture.schema), + format!("\"{}\".secret(text)", fixture.schema), + format!("\"{}\".secret_proc()", fixture.schema), + ] { + assert!( + !query_has_function_privilege(&fixture.bystander, &signature), + "{signature} should not be executable by PUBLIC" + ); + } + } + + #[test] + #[ignore] + fn unmanaged_public_grants_in_other_schemas_are_left_alone() { + // Guards the #108 class of regression: declaring PUBLIC rules for one + // schema must not turn every other PUBLIC ACL into drift, and a role + // wildcard elsewhere must still converge. + let (fixture, _cleanup) = PublicFixture::new("pubscope"); + let other_schema = unique_name("pubscope_other"); + let _other_cleanup = TestDbCleanup::new(format!( + r#"DROP SCHEMA IF EXISTS "{other_schema}" CASCADE;"# + )); + execute_sql(&format!( + r#"CREATE SCHEMA "{other_schema}" AUTHORIZATION "{owner}"; + SET ROLE "{owner}"; + CREATE FUNCTION "{other_schema}".extension_like() RETURNS integer + LANGUAGE sql AS 'SELECT 1'; + GRANT EXECUTE ON FUNCTION "{other_schema}".extension_like() TO PUBLIC; + RESET ROLE;"#, + other_schema = other_schema, + owner = fixture.owner, + )); + + let manifest = write_temp_manifest(&fixture.manifest_with_extra( + "", + &format!( + r#" - role: {caller} + privileges: [EXECUTE] + object: {{ type: function, schema: {other_schema}, name: "*" }} +"#, + caller = fixture.caller, + other_schema = other_schema, + ), + )); + + apply_until_converged(manifest.path(), 3); + + assert!( + query_has_function_privilege( + &fixture.bystander, + &format!("\"{other_schema}\".extension_like()") + ), + "PUBLIC EXECUTE outside the declared scope must survive" + ); + assert!( + query_has_function_privilege( + &fixture.caller, + &format!("\"{other_schema}\".extension_like()") + ), + "the role wildcard in the other schema should converge" + ); + } + + #[test] + #[ignore] + fn a_role_named_public_is_not_the_public_pseudo_role() { + let (fixture, _cleanup) = PublicFixture::new("publower"); + // PostgreSQL reserves the bare name `public`, so use the closest + // legal spelling. It must survive while PUBLIC is revoked. + let role = unique_name("public_role"); + let _role_cleanup = TestDbCleanup::new(format!( + r#"DO $$ BEGIN + IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = '{role}') THEN + EXECUTE 'DROP OWNED BY "{role}" CASCADE'; + END IF; + END $$; + DROP ROLE IF EXISTS "{role}";"# + )); + execute_sql(&format!(r#"CREATE ROLE "{role}";"#)); + + let manifest = write_temp_manifest(&fixture.manifest_with_extra( + &format!(" - name: {role}\n"), + &format!( + r#" - role: {role} + privileges: [EXECUTE] + object: {{ type: function, schema: {schema}, name: "*" }} +"#, + role = role, + schema = fixture.schema, + ), + )); + + apply_until_converged(manifest.path(), 3); + + // The quoted role keeps EXECUTE; the pseudo-role does not. + assert!( + query_has_function_privilege(&role, &fixture.secret_signature()), + "a role whose name resembles the keyword should keep EXECUTE" + ); + assert!( + !query_has_function_privilege(&fixture.bystander, &fixture.secret_signature()), + "PUBLIC should still be revoked" + ); + } + + #[test] + #[ignore] + fn empty_schema_absence_is_vacuously_converged() { + let schema = unique_name("pubempty_schema"); + let owner = unique_name("pubempty_owner"); + let _cleanup = TestDbCleanup::new(format!( + r#"DROP SCHEMA IF EXISTS "{schema}" CASCADE; DROP ROLE IF EXISTS "{owner}";"# + )); + execute_sql(&format!( + r#"CREATE ROLE "{owner}"; CREATE SCHEMA "{schema}" AUTHORIZATION "{owner}";"# + )); + + let manifest = write_temp_manifest(&format!( + r#" +roles: + - name: {owner} +grants: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: {{ type: function, schema: {schema}, name: "*" }} +"# + )); + + pgroles_cmd() + .args([ + "diff", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &database_url(), + "--format", + "summary", + "--no-exit-code", + ]) + .assert() + .success() + .stdout(predicate::str::contains("No changes needed")); + } + + #[test] + #[ignore] + fn preflight_warns_on_diff_and_blocks_only_on_apply() { + let (fixture, _cleanup) = PublicFixture::new("pfgate"); + let reporter = unique_name("pfgate_reporter"); + let _reporter_cleanup = TestDbCleanup::new(format!( + r#"DO $$ BEGIN + IF EXISTS (SELECT 1 FROM pg_roles WHERE rolname = '{reporter}') THEN + EXECUTE 'DROP OWNED BY "{reporter}" CASCADE'; + END IF; + END $$; + DROP ROLE IF EXISTS "{reporter}";"# + )); + execute_sql(&format!( + r#"CREATE ROLE "{reporter}" LOGIN PASSWORD 'testpassword'; + GRANT USAGE ON SCHEMA "{schema}" TO "{reporter}";"#, + reporter = reporter, + schema = fixture.schema, + )); + + // Only the absence rules: a positive wildcard grant would trip the + // pre-existing UnsatisfiableWildcardGrant diagnostic first and mask + // what this test is about. + let manifest = write_temp_manifest(&format!( + r#" +roles: + - name: {owner} +grants: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: {{ type: function, schema: {schema}, name: "*" }} +default_privileges: + - owner: {owner} + scope: {{ type: global }} + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function +"#, + owner = fixture.owner, + schema = fixture.schema, + )); + let reporter_url = database_url_for_role(&reporter, "testpassword"); + + // A CI drift check running as a read-only role must still get its + // plan: diff executes no SQL, so authority problems are warnings. + let diff_output = pgroles_cmd() + .args([ + "diff", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &reporter_url, + "--format", + "summary", + "--no-exit-code", + ]) + .assert() + .success() + .get_output() + .clone(); + assert!( + String::from_utf8_lossy(&diff_output.stdout).contains("change(s)"), + "diff should still produce a plan for a low-privilege role" + ); + assert!( + String::from_utf8_lossy(&diff_output.stderr) + .contains("UnsatisfiableDefaultPrivilegeChange"), + "diff should warn about executor authority" + ); + + // A dry run executes nothing either, so it also only warns. + pgroles_cmd() + .args([ + "apply", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &reporter_url, + "--dry-run", + ]) + .assert() + .success(); + + // The real apply is where authority actually matters. + pgroles_cmd() + .args([ + "apply", + "--file", + manifest.path().to_str().unwrap(), + "--database-url", + &reporter_url, + ]) + .assert() + .failure() + .stderr(predicate::str::contains( + "UnsatisfiableDefaultPrivilegeChange", + )); + + // Nothing ran, so PUBLIC still holds EXECUTE. + assert!( + query_has_function_privilege(&reporter, &fixture.secret_signature()), + "the blocked apply must not have changed anything" + ); + } } diff --git a/crates/pgroles-core/src/approval.rs b/crates/pgroles-core/src/approval.rs index 36c7823..f491dbc 100644 --- a/crates/pgroles-core/src/approval.rs +++ b/crates/pgroles-core/src/approval.rs @@ -50,14 +50,18 @@ use crate::diff::{Change, ReconciliationMode}; /// superseded rather than silently accepted. pub const APPROVAL_EFFECT_ENCODING_V1: &str = "pgroles.io/approval-effect/v1"; -/// Current canonical effect encoding. -/// /// v2 adds the resolved target identity — physical (`system_identifier`) and -/// logical (host/port/database fingerprint) — to the bound inputs. Every -/// digest changes once when an operator upgrades to this encoding, so every -/// open plan supersedes exactly once and is re-reviewed. +/// logical (host/port/database fingerprint) — to the bound inputs. pub const APPROVAL_EFFECT_ENCODING_V2: &str = "pgroles.io/approval-effect/v2"; +/// Current canonical effect encoding. +/// +/// v3 carries a default-privilege rule's scope as a tagged `scope` value +/// instead of a bare `schema` string, which could not express an owner-wide +/// rule. Every digest changes once when an operator upgrades to this +/// encoding, so every open plan supersedes exactly once and is re-reviewed. +pub const APPROVAL_EFFECT_ENCODING_V3: &str = "pgroles.io/approval-effect/v3"; + /// Refusal to produce a digest that would not bind what it claims to bind. #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] pub enum ApprovalDigestError { @@ -303,7 +307,7 @@ pub fn canonical_change_set_bytes( let owned_schemas: BTreeSet<&str> = inputs.owned_schemas.iter().map(String::as_str).collect(); Ok(serde_json::to_vec(&CanonicalChangeSet { - effect_encoding: APPROVAL_EFFECT_ENCODING_V2, + effect_encoding: APPROVAL_EFFECT_ENCODING_V3, reconciliation_mode: inputs.reconciliation_mode, target: inputs.target, target_physical_identity: inputs.target_identity.physical.as_deref(), @@ -355,7 +359,7 @@ fn sha256_prefixed(bytes: &[u8]) -> String { mod tests { use super::*; use crate::manifest::{ObjectType, Privilege}; - use crate::model::RoleState; + use crate::model::{Grantee, RoleState}; fn versions(entries: &[(&str, &str)]) -> BTreeMap { entries @@ -440,7 +444,7 @@ mod tests { fn grant(role: &str) -> Change { Change::Grant { - role: role.to_string(), + role: Grantee::parse(role), privileges: [Privilege::Select].into_iter().collect(), object_type: ObjectType::Table, schema: Some("inventory".to_string()), @@ -807,7 +811,7 @@ mod tests { // first released v2 onward, any change here means a new constant. assert_eq!( encoded, - r#"{"effect_encoding":"pgroles.io/approval-effect/v2","reconciliation_mode":"Authoritative","target":"default/postgres-credentials:url","target_physical_identity":"7412330000000000001","target_logical_fingerprint":"sha256:fingerprint","owned_roles":[],"owned_schemas":[],"effects":[{"Grant":{"name":"orders","object_type":"table","privileges":["SELECT"],"role":"reporting","schema":"inventory"}},{"SetPassword":{"name":"app","password_source":"role-passwords:app:7"}}]}"#, + r#"{"effect_encoding":"pgroles.io/approval-effect/v3","reconciliation_mode":"Authoritative","target":"default/postgres-credentials:url","target_physical_identity":"7412330000000000001","target_logical_fingerprint":"sha256:fingerprint","owned_roles":[],"owned_schemas":[],"effects":[{"Grant":{"name":"orders","object_type":"table","privileges":["SELECT"],"role":"reporting","schema":"inventory"}},{"SetPassword":{"name":"app","password_source":"role-passwords:app:7"}}]}"#, "canonical encoding changed; bump the encoding constant rather than \ editing this fixture" ); diff --git a/crates/pgroles-core/src/composition.rs b/crates/pgroles-core/src/composition.rs index 4aceaa2..5fb4ef2 100644 --- a/crates/pgroles-core/src/composition.rs +++ b/crates/pgroles-core/src/composition.rs @@ -35,6 +35,11 @@ pub enum CompositionError { )] SchemaBindingsOutOfScope { document: String, schema: String }, + #[error( + "policy document \"{document}\" declares global default privileges for owner \"{owner}\" but does not own that role — global defaults affect every schema, so only the document with role \"{owner}\" in its scope may manage them" + )] + GlobalDefaultPrivilegeOutOfScope { document: String, owner: String }, + #[error("policy documents \"{first}\" and \"{second}\" both manage role \"{role}\"")] DuplicateManagedRole { role: String, @@ -347,15 +352,34 @@ fn validate_document_scope( } for default_privilege in &document.fragment.default_privileges { - if !has_schema_facet( - &schema_scope, - &default_privilege.schema, - SchemaBindingFacet::Bindings, - ) { - return Err(CompositionError::SchemaBindingsOutOfScope { + let scope = default_privilege.resolved_scope().map_err(|error| { + CompositionError::InvalidDocument { document: document_name.clone(), - schema: default_privilege.schema.clone(), - }); + error, + } + })?; + match scope { + crate::model::DefaultPrivilegeScope::Schema { schema } => { + if !has_schema_facet(&schema_scope, &schema, SchemaBindingFacet::Bindings) { + return Err(CompositionError::SchemaBindingsOutOfScope { + document: document_name.clone(), + schema, + }); + } + } + crate::model::DefaultPrivilegeScope::Global => { + let owner = default_privilege + .owner + .as_deref() + .or(bundle.shared.default_owner.as_deref()) + .unwrap_or("postgres"); + if !owned_roles.contains(owner) { + return Err(CompositionError::GlobalDefaultPrivilegeOutOfScope { + document: document_name.clone(), + owner: owner.to_string(), + }); + } + } } } @@ -417,11 +441,25 @@ fn register_document_ownership( } } - for grant in desired.grants.keys() { + // Absence keys claim ownership too: two fragments may not assert the same + // key even with opposite ensure, and the index collision is what catches + // a present-vs-absent split across fragments. One document may hold the + // same key in both maps (disjoint privileges), so deduplicate first. + let grant_keys: BTreeSet<&crate::model::GrantKey> = desired + .grants + .keys() + .chain(desired.grant_absences.keys()) + .collect(); + for grant in grant_keys { register_grant_owner(ownership, grant, &label)?; } - for default_privilege in desired.default_privileges.keys() { + let default_privilege_keys: BTreeSet<&crate::model::DefaultPrivKey> = desired + .default_privileges + .keys() + .chain(desired.default_privilege_absences.keys()) + .collect(); + for default_privilege in default_privilege_keys { register_default_privilege_owner(ownership, default_privilege, &label)?; } @@ -549,8 +587,8 @@ fn format_grant_key(key: &GrantKey) -> String { fn format_default_privilege_key(key: &DefaultPrivKey) -> String { format!( - "owner \"{}\" schema \"{}\" on {} to \"{}\"", - key.owner, key.schema, key.on_type, key.grantee + "owner \"{}\" {} on {} to \"{}\"", + key.owner, key.scope, key.on_type, key.grantee ) } @@ -829,9 +867,11 @@ grants: let mut ownership = OwnershipIndex::default(); let key = DefaultPrivKey { owner: "app_owner".to_string(), - schema: "inventory".to_string(), + scope: crate::model::DefaultPrivilegeScope::Schema { + schema: "inventory".to_string(), + }, on_type: crate::manifest::ObjectType::Table, - grantee: "app".to_string(), + grantee: "app".into(), }; register_default_privilege_owner(&mut ownership, &key, "first") @@ -906,7 +946,7 @@ memberships: let result = validate_changes_against_managed_surface( &[Change::Revoke { - role: "app".to_string(), + role: "app".into(), privileges: BTreeSet::from([Privilege::Connect]), object_type: ObjectType::Database, schema: None, @@ -943,4 +983,199 @@ memberships: assert_eq!(SchemaBindingFacet::Bindings.to_string(), "bindings"); assert_eq!(Privilege::Usage.to_string(), "USAGE"); } + + // ----------------------------------------------------------------------- + // Global default privileges and absence assertions across fragments + // ----------------------------------------------------------------------- + + fn single_source_bundle(file: &str) -> PolicyBundle { + PolicyBundle { + shared: SharedPolicy::default(), + sources: vec![BundleSource { + file: file.to_string(), + }], + } + } + + fn document(source: &str, yaml: &str) -> PolicyDocument { + PolicyDocument { + source: source.to_string(), + fragment: parse_policy_fragment(yaml).expect("fragment should parse"), + } + } + + #[test] + fn global_default_privileges_require_owning_the_owner_role() { + let owned = document( + "api.yaml", + r#" +scope: + roles: [api_owner] +roles: + - name: api_owner +default_privileges: + - owner: api_owner + scope: { type: global } + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function +"#, + ); + compose_bundle(&single_source_bundle("api.yaml"), &[owned]) + .expect("owning the role should allow managing its global defaults"); + + // A global default reaches every schema in the database, so a document + // that does not own the role must not declare one. + let unowned = document( + "api.yaml", + r#" +scope: + roles: [someone_else] +default_privileges: + - owner: api_owner + scope: { type: global } + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function +"#, + ); + let error = compose_bundle(&single_source_bundle("api.yaml"), &[unowned]) + .expect_err("scope validation should fail"); + assert!(matches!( + error, + CompositionError::GlobalDefaultPrivilegeOutOfScope { owner, .. } if owner == "api_owner" + )); + } + + #[test] + fn two_fragments_may_not_split_one_assertion_into_present_and_absent() { + let bundle = PolicyBundle { + shared: SharedPolicy::default(), + sources: vec![ + BundleSource { + file: "a.yaml".to_string(), + }, + BundleSource { + file: "b.yaml".to_string(), + }, + ], + }; + let present = document( + "a.yaml", + r#" +policy: + name: a +scope: + schemas: + - name: api + facets: [bindings] +grants: + - role: PUBLIC + privileges: [EXECUTE] + object: { type: function, schema: api, name: "f()" } +"#, + ); + let absent = document( + "b.yaml", + r#" +policy: + name: b +scope: + schemas: + - name: api + facets: [bindings] +grants: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: api, name: "f()" } +"#, + ); + + // Managing a schema's grants requires its `bindings` facet, and one + // facet has exactly one owner, so the split is caught before the + // grant keys are even compared. + let error = compose_bundle(&bundle, &[present, absent]) + .expect_err("the same key claimed twice must fail"); + assert!( + matches!( + error, + CompositionError::DuplicateManagedSchemaFacet { ref schema, .. } if schema == "api" + ), + "unexpected error: {error}" + ); + } + + #[test] + fn one_fragment_may_not_split_an_assertion_into_present_and_absent() { + let document = document( + "api.yaml", + r#" +scope: + schemas: + - name: api + facets: [bindings] +grants: + - role: PUBLIC + privileges: [EXECUTE] + object: { type: function, schema: api, name: "f()" } + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: api, name: "f()" } +"#, + ); + + let error = compose_bundle(&single_source_bundle("api.yaml"), &[document]) + .expect_err("conflicting ensure must fail"); + assert!( + matches!( + error, + CompositionError::InvalidDocument { + error: ManifestError::ConflictingGrantEnsure { .. }, + .. + } + ), + "unexpected error: {error}" + ); + } + + #[test] + fn managed_surface_allows_global_defaults_only_for_owned_owners() { + let mut surface = ManagedChangeSurface { + roles: BTreeSet::from(["api_owner".to_string()]), + ..ManagedChangeSurface::default() + }; + + let change_for = |owner: &str| Change::RevokeDefaultPrivilege { + owner: owner.to_string(), + scope: crate::model::DefaultPrivilegeScope::Global, + on_type: ObjectType::Function, + grantee: crate::model::Grantee::Public, + privileges: BTreeSet::from([Privilege::Execute]), + }; + + validate_changes_against_managed_surface(&[change_for("api_owner")], &surface) + .expect("owned owner is in scope"); + + let error = validate_changes_against_managed_surface(&[change_for("stranger")], &surface) + .expect_err("unowned owner is out of scope"); + assert!(matches!(error, ManagedChangeError::OutOfScope { .. })); + + // An explicitly claimed key is allowed regardless of role ownership. + surface + .explicit_default_privileges + .insert(crate::model::DefaultPrivKey { + owner: "stranger".to_string(), + scope: crate::model::DefaultPrivilegeScope::Global, + on_type: ObjectType::Function, + grantee: crate::model::Grantee::Public, + }); + validate_changes_against_managed_surface(&[change_for("stranger")], &surface) + .expect("explicit claim is in scope"); + } } diff --git a/crates/pgroles-core/src/diff.rs b/crates/pgroles-core/src/diff.rs index 6499a85..e187f89 100644 --- a/crates/pgroles-core/src/diff.rs +++ b/crates/pgroles-core/src/diff.rs @@ -12,8 +12,8 @@ use std::collections::{BTreeMap, BTreeSet}; use crate::manifest::{ObjectType, Privilege, RoleDefinition, RoleRetirement}; use crate::model::{ - DefaultPrivKey, GrantKey, MembershipEdge, RoleAttribute, RoleGraph, RoleState, - default_schema_owner_privileges, + DefaultPrivKey, DefaultPrivilegeScope, GrantKey, Grantee, MembershipEdge, RoleAttribute, + RoleGraph, RoleState, default_schema_owner_privileges, }; // --------------------------------------------------------------------------- @@ -62,18 +62,18 @@ pub enum Change { comment: Option, }, - /// Grant privileges on an object to a role. + /// Grant privileges on an object to a grantee. Grant { - role: String, + role: Grantee, privileges: BTreeSet, object_type: ObjectType, schema: Option, name: Option, }, - /// Revoke privileges on an object from a role. + /// Revoke privileges on an object from a grantee. Revoke { - role: String, + role: Grantee, privileges: BTreeSet, object_type: ObjectType, schema: Option, @@ -83,18 +83,18 @@ pub enum Change { /// Set default privileges (ALTER DEFAULT PRIVILEGES ... GRANT ...). SetDefaultPrivilege { owner: String, - schema: String, + scope: DefaultPrivilegeScope, on_type: ObjectType, - grantee: String, + grantee: Grantee, privileges: BTreeSet, }, /// Revoke default privileges (ALTER DEFAULT PRIVILEGES ... REVOKE ...). RevokeDefaultPrivilege { owner: String, - schema: String, + scope: DefaultPrivilegeScope, on_type: ObjectType, - grantee: String, + grantee: Grantee, privileges: BTreeSet, }, @@ -272,9 +272,11 @@ fn filter_additive_changes(changes: Vec) -> Vec { Change::EnsureSchemaOwnerPrivileges { name, owner, .. } => { !skipped_owner_transfers.contains(&(name.clone(), owner.clone())) } - Change::SetDefaultPrivilege { schema, owner, .. } => { - !skipped_owner_transfers.contains(&(schema.clone(), owner.clone())) - } + Change::SetDefaultPrivilege { + scope: DefaultPrivilegeScope::Schema { schema }, + owner, + .. + } => !skipped_owner_transfers.contains(&(schema.clone(), owner.clone())), Change::AlterRole { name, attributes } => { created_roles.contains(name) && attributes @@ -691,29 +693,47 @@ fn diff_grants( // privilege the wildcard still declares). // - the absent-key branch (current has a per-name entry that desired // covers only via wildcard). - let desired_wildcards: BTreeMap<(&str, &Option, ObjectType), &BTreeSet> = + let desired_wildcards: BTreeMap<(&Grantee, &Option, ObjectType), &BTreeSet> = desired .grants .iter() .filter(|(k, _)| k.name.as_deref() == Some("*") && k.schema.is_some()) - .map(|(k, v)| ((k.role.as_str(), &k.schema, k.object_type), &v.privileges)) + .map(|(k, v)| ((&k.role, &k.schema, k.object_type), &v.privileges)) + .collect(); + + // Absence wildcards suppress per-name revokes for the same reason: the + // single `ON ALL` revoke they emit already covers every object, so a + // per-name revoke of the same privilege would only duplicate it. + let absence_wildcards: BTreeMap<(&Grantee, &Option, ObjectType), &BTreeSet> = + desired + .grant_absences + .iter() + .filter(|(k, _)| k.name.as_deref() == Some("*") && k.schema.is_some()) + .map(|(k, v)| ((&k.role, &k.schema, k.object_type), v)) .collect(); // Returns the subset of `candidate` not shadowed by a desired wildcard - // for the same (role, schema, type). The wildcard itself is never - // shadowed (it has name="*", not a specific object name). + // (present or absent) for the same (role, schema, type). The wildcard + // itself is never shadowed (it has name="*", not a specific object name). let shadow_filter = |key: &GrantKey, candidate: BTreeSet| -> BTreeSet { if key.name.as_deref() == Some("*") { return candidate; } - match desired_wildcards.get(&(key.role.as_str(), &key.schema, key.object_type)) { - Some(wildcard_privileges) => { - candidate.difference(wildcard_privileges).copied().collect() - } - None => candidate, + let mut filtered = candidate; + let selector = (&key.role, &key.schema, key.object_type); + if let Some(wildcard_privileges) = desired_wildcards.get(&selector) { + filtered = filtered.difference(wildcard_privileges).copied().collect(); } + if let Some(absent_privileges) = absence_wildcards.get(&selector) { + filtered = filtered.difference(absent_privileges).copied().collect(); + } + filtered }; + // Revokes accumulate per key so a key hit by both a convergence branch + // and an absence assertion emits one merged REVOKE. + let mut revokes: BTreeMap> = BTreeMap::new(); + // Grants in desired but not in current → GRANT (full set) // Grants in both → diff the privilege sets for (key, desired_state) in &desired.grants { @@ -729,35 +749,82 @@ fn diff_grants( .difference(¤t_state.privileges) .copied() .collect(); + if !to_add.is_empty() { + grants_out.push(change_grant(key, &to_add)); + } + + // PUBLIC state is assertion-driven: only an `ensure: absent` + // rule may revoke from PUBLIC, never mere absence from the + // desired set. + if key.role.is_public() { + continue; + } + let to_remove: BTreeSet = current_state .privileges .difference(&desired_state.privileges) .copied() .collect(); let to_remove = shadow_filter(key, to_remove); - - if !to_add.is_empty() { - grants_out.push(change_grant(key, &to_add)); - } if !to_remove.is_empty() { - revokes_out.push(change_revoke(key, &to_remove)); + revokes.entry(key.clone()).or_default().extend(to_remove); } } } } // Grant targets in current but not in desired → REVOKE the privileges - // that aren't shadowed by a desired wildcard for the same scope. + // that aren't shadowed by a desired wildcard for the same scope. PUBLIC + // keys are exempt: unmentioned PUBLIC ACLs are unmanaged, not drift. for (key, current_state) in ¤t.grants { - if desired.grants.contains_key(key) { + if desired.grants.contains_key(key) || key.role.is_public() { continue; } let to_revoke = shadow_filter(key, current_state.privileges.clone()); if !to_revoke.is_empty() { - revokes_out.push(change_revoke(key, &to_revoke)); + revokes.entry(key.clone()).or_default().extend(to_revoke); } } + + // Absence assertions: revoke `absent ∩ current`. A wildcard assertion + // range-scans every current key under its (grantee, type, schema) prefix, + // so one `ON ALL` revoke covers however many objects still hold the + // privilege, and an empty range is vacuously converged. + for (key, absent_privileges) in &desired.grant_absences { + let held: BTreeSet = if key.name.as_deref() == Some("*") { + let range_start = GrantKey { + role: key.role.clone(), + object_type: key.object_type, + schema: key.schema.clone(), + name: None, + }; + current + .grants + .range(range_start..) + .take_while(|(k, _)| { + k.role == key.role && k.object_type == key.object_type && k.schema == key.schema + }) + .flat_map(|(_, state)| state.privileges.iter().copied()) + .collect() + } else { + current + .grants + .get(key) + .map(|state| state.privileges.clone()) + .unwrap_or_default() + }; + + let to_revoke: BTreeSet = + absent_privileges.intersection(&held).copied().collect(); + if !to_revoke.is_empty() { + revokes.entry(key.clone()).or_default().extend(to_revoke); + } + } + + for (key, privileges) in &revokes { + revokes_out.push(change_revoke(key, privileges)); + } } fn change_grant(key: &GrantKey, privileges: &BTreeSet) -> Change { @@ -790,6 +857,10 @@ fn diff_default_privileges( set_out: &mut Vec, revoke_out: &mut Vec, ) { + // Revokes accumulate per key so a key hit by both a convergence branch + // and an absence assertion emits one merged REVOKE. + let mut revokes: BTreeMap> = BTreeMap::new(); + for (key, desired_state) in &desired.default_privileges { match current.default_privileges.get(key) { None => { @@ -801,33 +872,61 @@ fn diff_default_privileges( .difference(¤t_state.privileges) .copied() .collect(); + if !to_add.is_empty() { + set_out.push(change_set_default(key, &to_add)); + } + + // PUBLIC defaults are assertion-driven: only `ensure: absent` + // may revoke them. + if key.grantee.is_public() { + continue; + } + let to_remove: BTreeSet = current_state .privileges .difference(&desired_state.privileges) .copied() .collect(); - - if !to_add.is_empty() { - set_out.push(change_set_default(key, &to_add)); - } if !to_remove.is_empty() { - revoke_out.push(change_revoke_default(key, &to_remove)); + revokes.entry(key.clone()).or_default().extend(to_remove); } } } } for (key, current_state) in ¤t.default_privileges { - if !desired.default_privileges.contains_key(key) { - revoke_out.push(change_revoke_default(key, ¤t_state.privileges)); + if desired.default_privileges.contains_key(key) || key.grantee.is_public() { + continue; } + revokes + .entry(key.clone()) + .or_default() + .extend(current_state.privileges.iter().copied()); + } + + // Absence assertions: revoke `absent ∩ current`. Keys here are exact — + // default privileges have no wildcard selector. + for (key, absent_privileges) in &desired.default_privilege_absences { + if let Some(current_state) = current.default_privileges.get(key) { + let to_revoke: BTreeSet = absent_privileges + .intersection(¤t_state.privileges) + .copied() + .collect(); + if !to_revoke.is_empty() { + revokes.entry(key.clone()).or_default().extend(to_revoke); + } + } + } + + for (key, privileges) in &revokes { + revoke_out.push(change_revoke_default(key, privileges)); } } fn change_set_default(key: &DefaultPrivKey, privileges: &BTreeSet) -> Change { Change::SetDefaultPrivilege { owner: key.owner.clone(), - schema: key.schema.clone(), + scope: key.scope.clone(), on_type: key.on_type, grantee: key.grantee.clone(), privileges: privileges.clone(), @@ -837,7 +936,7 @@ fn change_set_default(key: &DefaultPrivKey, privileges: &BTreeSet) -> fn change_revoke_default(key: &DefaultPrivKey, privileges: &BTreeSet) -> Change { Change::RevokeDefaultPrivilege { owner: key.owner.clone(), - schema: key.schema.clone(), + scope: key.scope.clone(), on_type: key.on_type, grantee: key.grantee.clone(), privileges: privileges.clone(), @@ -1033,7 +1132,7 @@ mod tests { for grantee in ["z", "bystander"] { current.grants.insert( GrantKey { - role: grantee.to_string(), + role: grantee.into(), object_type: ObjectType::Schema, schema: None, name: Some("s".to_string()), @@ -1071,7 +1170,7 @@ mod tests { !changes.iter().any(|c| matches!( c, Change::Revoke { role, object_type: ObjectType::Schema, name: Some(n), .. } - if role == "z" && n == "s" + if role.as_str() == "z" && n == "s" )), "revoke against incoming owner must be suppressed: {changes:?}" ); @@ -1080,7 +1179,7 @@ mod tests { changes.iter().any(|c| matches!( c, Change::Revoke { role, object_type: ObjectType::Schema, name: Some(n), .. } - if role == "bystander" && n == "s" + if role.as_str() == "bystander" && n == "s" )), "bystander's stale grant must still be revoked: {changes:?}" ); @@ -1330,7 +1429,7 @@ memberships: let current = empty_graph(); let mut desired = empty_graph(); let key = GrantKey { - role: "r1".to_string(), + role: "r1".into(), object_type: ObjectType::Table, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -1348,7 +1447,7 @@ memberships: Change::Grant { role, privileges, .. } => { - assert_eq!(role, "r1"); + assert_eq!(role.as_str(), "r1"); assert!(privileges.contains(&Privilege::Select)); assert!(privileges.contains(&Privilege::Insert)); } @@ -1360,7 +1459,7 @@ memberships: fn diff_revokes_removed_privileges() { let mut current = empty_graph(); let key = GrantKey { - role: "r1".to_string(), + role: "r1".into(), object_type: ObjectType::Table, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -1386,7 +1485,7 @@ memberships: Change::Revoke { role, privileges, .. } => { - assert_eq!(role, "r1"); + assert_eq!(role.as_str(), "r1"); assert!(privileges.contains(&Privilege::Insert)); assert!(!privileges.contains(&Privilege::Select)); } @@ -1398,7 +1497,7 @@ memberships: fn diff_revokes_entire_grant_target_when_absent_from_desired() { let mut current = empty_graph(); let key = GrantKey { - role: "r1".to_string(), + role: "r1".into(), object_type: ObjectType::Schema, schema: None, name: Some("myschema".to_string()), @@ -1413,7 +1512,7 @@ memberships: let changes = diff(¤t, &desired); assert_eq!(changes.len(), 1); - assert!(matches!(&changes[0], Change::Revoke { role, .. } if role == "r1")); + assert!(matches!(&changes[0], Change::Revoke { role, .. } if role.as_str() == "r1")); } #[test] @@ -1505,9 +1604,11 @@ memberships: let mut current = empty_graph(); let key = DefaultPrivKey { owner: "app_owner".to_string(), - schema: "inventory".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "inventory".to_string(), + }, on_type: ObjectType::Table, - grantee: "inventory-editor".to_string(), + grantee: "inventory-editor".into(), }; current.default_privileges.insert( key.clone(), @@ -1577,7 +1678,7 @@ memberships: .insert("role1".to_string(), RoleState::default()); graph.grants.insert( GrantKey { - role: "role1".to_string(), + role: "role1".into(), object_type: ObjectType::Table, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -1705,14 +1806,14 @@ memberships: comment: Some("hello".to_string()), }, Change::Grant { - role: "r1".to_string(), + role: "r1".into(), privileges: BTreeSet::from([Privilege::Select]), object_type: ObjectType::Table, schema: Some("public".to_string()), name: Some("*".to_string()), }, Change::Revoke { - role: "r1".to_string(), + role: "r1".into(), privileges: BTreeSet::from([Privilege::Insert]), object_type: ObjectType::Table, schema: Some("public".to_string()), @@ -1720,16 +1821,20 @@ memberships: }, Change::SetDefaultPrivilege { owner: "owner".to_string(), - schema: "public".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "public".to_string(), + }, on_type: ObjectType::Table, - grantee: "r1".to_string(), + grantee: "r1".into(), privileges: BTreeSet::from([Privilege::Select]), }, Change::RevokeDefaultPrivilege { owner: "owner".to_string(), - schema: "public".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "public".to_string(), + }, on_type: ObjectType::Table, - grantee: "r1".to_string(), + grantee: "r1".into(), privileges: BTreeSet::from([Privilege::Delete]), }, Change::AddMember { @@ -1902,13 +2007,15 @@ memberships: }, Change::SetDefaultPrivilege { owner: "new_owner".to_string(), - schema: "inventory".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "inventory".to_string(), + }, on_type: ObjectType::Table, - grantee: "inventory-editor".to_string(), + grantee: "inventory-editor".into(), privileges: BTreeSet::from([Privilege::Select]), }, Change::Grant { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), privileges: BTreeSet::from([Privilege::Usage]), object_type: ObjectType::Schema, schema: None, @@ -1918,7 +2025,9 @@ memberships: let filtered = filter_changes(changes, ReconciliationMode::Additive); assert_eq!(filtered.len(), 1); - assert!(matches!(&filtered[0], Change::Grant { role, .. } if role == "inventory-editor")); + assert!( + matches!(&filtered[0], Change::Grant { role, .. } if role.as_str() == "inventory-editor") + ); } #[test] @@ -1966,7 +2075,7 @@ memberships: fn filter_additive_only_destructive_changes_yields_empty() { let changes = vec![ Change::Revoke { - role: "r1".to_string(), + role: "r1".into(), privileges: BTreeSet::from([Privilege::Select]), object_type: ObjectType::Table, schema: Some("public".to_string()), @@ -1988,14 +2097,14 @@ memberships: state: RoleState::default(), }, Change::Grant { - role: "new-role".to_string(), + role: "new-role".into(), privileges: BTreeSet::from([Privilege::Select]), object_type: ObjectType::Table, schema: Some("public".to_string()), name: Some("*".to_string()), }, Change::Revoke { - role: "existing-role".to_string(), + role: "existing-role".into(), privileges: BTreeSet::from([Privilege::Insert]), object_type: ObjectType::Table, schema: Some("public".to_string()), @@ -2039,7 +2148,7 @@ memberships: fn apply_role_retirements_inserts_cleanup_before_drop() { let changes = vec![ Change::Grant { - role: "analytics".to_string(), + role: "analytics".into(), privileges: BTreeSet::from([Privilege::Select]), object_type: ObjectType::Table, schema: Some("public".to_string()), @@ -2104,7 +2213,7 @@ memberships: fn inject_password_for_existing_role() { // No CreateRole — role already exists. Only grants change. let changes = vec![Change::Grant { - role: "app-svc".to_string(), + role: "app-svc".into(), privileges: BTreeSet::from([crate::manifest::Privilege::Select]), object_type: crate::manifest::ObjectType::Table, schema: Some("public".to_string()), @@ -2305,7 +2414,7 @@ memberships: state: RoleState::default(), }, Change::Grant { - role: "role-c".to_string(), + role: "role-c".into(), privileges: BTreeSet::from([crate::manifest::Privilege::Select]), object_type: crate::manifest::ObjectType::Table, schema: Some("public".to_string()), @@ -2379,7 +2488,7 @@ memberships: /// oscillation between two stable states. #[test] fn diff_does_not_revoke_per_name_grants_covered_by_desired_wildcard() { - let role = "cdc-editor".to_string(); + let role: Grantee = "cdc-editor".into(); let schema = "cdc".to_string(); let object_type = ObjectType::Function; @@ -2466,7 +2575,7 @@ memberships: /// wildcard is unsatisfied, the next reconcile inverts again). #[test] fn diff_does_not_revoke_extra_privileges_covered_by_desired_wildcard() { - let role = "viewer".to_string(); + let role: Grantee = "viewer".into(); let schema = "myschema".to_string(); let object_type = ObjectType::Table; @@ -2576,4 +2685,269 @@ memberships: other => panic!("expected AlterRole, got: {other:?}"), } } + + // ----------------------------------------------------------------------- + // Absence assertions (ensure: absent) + // ----------------------------------------------------------------------- + + fn public_function_key(schema: &str, name: &str) -> GrantKey { + GrantKey { + role: Grantee::Public, + object_type: ObjectType::Function, + schema: Some(schema.to_string()), + name: Some(name.to_string()), + } + } + + fn graph_with_public_grant(schema: &str, name: &str, privileges: &[Privilege]) -> RoleGraph { + let mut graph = RoleGraph::default(); + graph.grants.insert( + public_function_key(schema, name), + GrantState { + privileges: privileges.iter().copied().collect(), + }, + ); + graph + } + + #[test] + fn absence_revokes_only_the_privileges_actually_held() { + let current = graph_with_public_grant("api", "f()", &[Privilege::Execute]); + let mut desired = RoleGraph::default(); + desired.grant_absences.insert( + public_function_key("api", "f()"), + [Privilege::Execute, Privilege::Usage].into_iter().collect(), + ); + + let changes = diff(¤t, &desired); + assert_eq!( + changes, + vec![Change::Revoke { + role: Grantee::Public, + privileges: [Privilege::Execute].into_iter().collect(), + object_type: ObjectType::Function, + schema: Some("api".to_string()), + name: Some("f()".to_string()), + }] + ); + } + + #[test] + fn absence_of_a_privilege_that_is_not_held_plans_nothing() { + let current = graph_with_public_grant("api", "f()", &[Privilege::Execute]); + let mut desired = RoleGraph::default(); + desired.grant_absences.insert( + public_function_key("api", "f()"), + [Privilege::Usage].into_iter().collect(), + ); + + assert!(diff(¤t, &desired).is_empty()); + } + + #[test] + fn wildcard_absence_emits_one_revoke_covering_every_matching_object() { + let mut current = RoleGraph::default(); + for name in ["f(integer)", "f(text)", "g()"] { + current.grants.insert( + public_function_key("api", name), + GrantState { + privileges: [Privilege::Execute].into_iter().collect(), + }, + ); + } + + let mut desired = RoleGraph::default(); + desired.grant_absences.insert( + GrantKey { + role: Grantee::Public, + object_type: ObjectType::Function, + schema: Some("api".to_string()), + name: Some("*".to_string()), + }, + [Privilege::Execute].into_iter().collect(), + ); + + let changes = diff(¤t, &desired); + assert_eq!( + changes, + vec![Change::Revoke { + role: Grantee::Public, + privileges: [Privilege::Execute].into_iter().collect(), + object_type: ObjectType::Function, + schema: Some("api".to_string()), + name: Some("*".to_string()), + }] + ); + } + + #[test] + fn wildcard_absence_over_an_empty_scope_is_vacuously_converged() { + let mut desired = RoleGraph::default(); + desired.grant_absences.insert( + GrantKey { + role: Grantee::Public, + object_type: ObjectType::Function, + schema: Some("api".to_string()), + name: Some("*".to_string()), + }, + [Privilege::Execute].into_iter().collect(), + ); + + assert!(diff(&RoleGraph::default(), &desired).is_empty()); + } + + #[test] + fn unmentioned_public_grants_are_never_revoked() { + // PUBLIC state is assertion-driven: without an absence rule naming it, + // a live PUBLIC grant is unmanaged, not drift. + let current = graph_with_public_grant("api", "f()", &[Privilege::Execute]); + assert!(diff(¤t, &RoleGraph::default()).is_empty()); + + // Same for a PUBLIC key the manifest declares with other privileges. + let mut desired = RoleGraph::default(); + desired.grants.insert( + public_function_key("api", "f()"), + GrantState { + privileges: [Privilege::Usage].into_iter().collect(), + }, + ); + let changes = diff(¤t, &desired); + assert!( + changes + .iter() + .all(|change| !matches!(change, Change::Revoke { .. })), + "unexpected revoke in {changes:?}" + ); + } + + #[test] + fn a_key_hit_by_convergence_and_absence_emits_one_merged_revoke() { + let key = GrantKey { + role: "reader".into(), + object_type: ObjectType::Table, + schema: Some("app".to_string()), + name: Some("t".to_string()), + }; + let mut current = RoleGraph::default(); + current.grants.insert( + key.clone(), + GrantState { + privileges: [Privilege::Select, Privilege::Insert, Privilege::Delete] + .into_iter() + .collect(), + }, + ); + + let mut desired = RoleGraph::default(); + desired.grants.insert( + key.clone(), + GrantState { + privileges: [Privilege::Select].into_iter().collect(), + }, + ); + desired + .grant_absences + .insert(key, [Privilege::Delete].into_iter().collect()); + + let changes = diff(¤t, &desired); + let revokes: Vec<&Change> = changes + .iter() + .filter(|change| matches!(change, Change::Revoke { .. })) + .collect(); + assert_eq!(revokes.len(), 1, "expected one merged revoke: {revokes:?}"); + let Change::Revoke { privileges, .. } = revokes[0] else { + unreachable!() + }; + assert_eq!( + *privileges, + [Privilege::Insert, Privilege::Delete] + .into_iter() + .collect::>() + ); + } + + #[test] + fn default_privilege_absence_revokes_in_both_scopes() { + let global_key = DefaultPrivKey { + owner: "owner".to_string(), + scope: DefaultPrivilegeScope::Global, + on_type: ObjectType::Function, + grantee: Grantee::Public, + }; + let schema_key = DefaultPrivKey { + owner: "owner".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "api".to_string(), + }, + on_type: ObjectType::Function, + grantee: Grantee::Public, + }; + + let mut current = RoleGraph::default(); + for key in [&global_key, &schema_key] { + current.default_privileges.insert( + key.clone(), + DefaultPrivState { + privileges: [Privilege::Execute].into_iter().collect(), + }, + ); + } + + let mut desired = RoleGraph::default(); + for key in [&global_key, &schema_key] { + desired + .default_privilege_absences + .insert(key.clone(), [Privilege::Execute].into_iter().collect()); + } + + let changes = diff(¤t, &desired); + assert_eq!(changes.len(), 2, "{changes:?}"); + assert!(changes.iter().all(|change| matches!( + change, + Change::RevokeDefaultPrivilege { + grantee: Grantee::Public, + .. + } + ))); + } + + #[test] + fn additive_mode_ignores_absence_while_adopt_and_authoritative_apply_it() { + let current = graph_with_public_grant("api", "f()", &[Privilege::Execute]); + let mut desired = RoleGraph::default(); + desired.grant_absences.insert( + public_function_key("api", "f()"), + [Privilege::Execute].into_iter().collect(), + ); + // A grant in the same plan proves additive keeps the rest of the plan. + desired + .roles + .insert("newcomer".to_string(), RoleState::default()); + + let changes = diff(¤t, &desired); + + let additive = filter_changes(changes.clone(), ReconciliationMode::Additive); + assert!( + !additive + .iter() + .any(|change| matches!(change, Change::Revoke { .. })), + "additive must ignore absence: {additive:?}" + ); + assert!( + additive + .iter() + .any(|change| matches!(change, Change::CreateRole { .. })), + "additive must keep the rest of the plan" + ); + + for mode in [ReconciliationMode::Adopt, ReconciliationMode::Authoritative] { + let filtered = filter_changes(changes.clone(), mode); + assert!( + filtered + .iter() + .any(|change| matches!(change, Change::Revoke { .. })), + "{mode:?} must apply absence" + ); + } + } } diff --git a/crates/pgroles-core/src/export.rs b/crates/pgroles-core/src/export.rs index debdc75..8117524 100644 --- a/crates/pgroles-core/src/export.rs +++ b/crates/pgroles-core/src/export.rs @@ -7,17 +7,30 @@ use std::collections::BTreeMap; use crate::manifest::{ - DefaultPrivilege, DefaultPrivilegeGrant, Grant, MemberSpec, Membership, ObjectTarget, - PolicyManifest, RoleDefinition, SchemaBinding, + DefaultPrivilege, DefaultPrivilegeGrant, DefaultPrivilegeScopeSpec, DefaultPrivilegeScopeType, + Ensure, Grant, MemberSpec, Membership, ObjectTarget, PolicyManifest, RoleDefinition, + SchemaBinding, }; -use crate::model::RoleGraph; +use crate::model::{DefaultPrivilegeScope, RoleGraph}; /// Convert a [`RoleGraph`] into a flat [`PolicyManifest`]. /// /// The resulting manifest uses no profiles — all roles, grants, default /// privileges, and memberships are emitted as top-level entries. This makes /// the output straightforward and correct for round-tripping. +/// +/// Absence assertions are intentionally not exported. `generate` builds +/// this from an inspected graph, where the maps are always empty, and inventing +/// `ensure: absent` from a privilege that merely happens to be missing would +/// assert policy the user never stated. A caller passing a *desired* graph +/// would lose those assertions — export the manifest it came from instead. pub fn role_graph_to_manifest(graph: &RoleGraph) -> PolicyManifest { + debug_assert!( + graph.grant_absences.is_empty() && graph.default_privilege_absences.is_empty(), + "role_graph_to_manifest cannot represent absence assertions; \ + export the source manifest rather than a desired graph" + ); + // --- Roles --- let roles: Vec = graph .roles @@ -89,13 +102,14 @@ pub fn role_graph_to_manifest(graph: &RoleGraph) -> PolicyManifest { .grants .iter() .map(|(key, state)| Grant { - role: key.role.clone(), + role: key.role.as_str().to_string(), privileges: state.privileges.iter().copied().collect(), object: ObjectTarget { object_type: key.object_type, schema: key.schema.clone(), name: key.name.clone(), }, + ensure: Ensure::Present, }) .collect(); @@ -112,24 +126,42 @@ pub fn role_graph_to_manifest(graph: &RoleGraph) -> PolicyManifest { .collect(); // --- Default privileges --- - // Group by (owner, schema) to produce compact default_privileges entries. - let mut dp_groups: BTreeMap<(String, String), Vec> = BTreeMap::new(); + // Group by (owner, scope) to produce compact default_privileges entries. + let mut dp_groups: BTreeMap<(String, DefaultPrivilegeScope), Vec> = + BTreeMap::new(); for (key, state) in &graph.default_privileges { dp_groups - .entry((key.owner.clone(), key.schema.clone())) + .entry((key.owner.clone(), key.scope.clone())) .or_default() .push(DefaultPrivilegeGrant { - role: Some(key.grantee.clone()), + role: Some(key.grantee.as_str().to_string()), privileges: state.privileges.iter().copied().collect(), on_type: key.on_type, + ensure: Ensure::Present, }); } let default_privileges: Vec = dp_groups .into_iter() - .map(|((owner, schema), grant)| DefaultPrivilege { - owner: Some(owner), - schema, - grant, + .map(|((owner, scope), grant)| { + // Schema scope keeps the `schema:` shorthand so exported YAML for + // existing databases is unchanged; only global scope needs the + // explicit `scope` field. + let (schema, scope) = match scope { + DefaultPrivilegeScope::Schema { schema } => (Some(schema), None), + DefaultPrivilegeScope::Global => ( + None, + Some(DefaultPrivilegeScopeSpec { + scope_type: DefaultPrivilegeScopeType::Global, + schema: None, + }), + ), + }; + DefaultPrivilege { + owner: Some(owner), + schema, + scope, + grant, + } }) .collect(); diff --git a/crates/pgroles-core/src/manifest.rs b/crates/pgroles-core/src/manifest.rs index 8b7c77f..8cc0c78 100644 --- a/crates/pgroles-core/src/manifest.rs +++ b/crates/pgroles-core/src/manifest.rs @@ -26,8 +26,8 @@ pub enum ManifestError { #[error("role_pattern must contain {{profile}} placeholder, got: \"{0}\"")] InvalidRolePattern(String), - #[error("top-level default privilege for schema \"{schema}\" must specify grant.role")] - MissingDefaultPrivilegeRole { schema: String }, + #[error("top-level default privilege for {scope} must specify grant.role")] + MissingDefaultPrivilegeRole { scope: String }, #[error("duplicate retirement entry for role: \"{0}\"")] DuplicateRetirement(String), @@ -72,6 +72,48 @@ pub enum ManifestError { actual: usize, limit: u32, }, + + #[error( + "\"PUBLIC\" is reserved for the PostgreSQL PUBLIC pseudo-role and cannot be used as {context}" + )] + ReservedPublicName { context: String }, + + // The conflict variants describe their assertion in one preformatted + // `target` string. Keeping each field count low matters: `ManifestError` + // travels inside `CompositionError`, whose size clippy polices. + #[error("grant {target} declares privilege {privilege} as both present and absent")] + ConflictingGrantEnsure { target: String, privilege: String }, + + #[error( + "grants {target} declare privilege {privilege} as present for one object selector and absent for another — grants apply before revokes, so the plan could never converge" + )] + ConflictingWildcardEnsure { target: String, privilege: String }, + + #[error("default privileges {target} declare privilege {privilege} as both present and absent")] + ConflictingDefaultPrivilegeEnsure { target: String, privilege: String }, + + #[error( + "default privilege entry for owner \"{owner}\" sets both `schema` and `scope` — use exactly one" + )] + DefaultPrivilegeScopeConflict { owner: String }, + + #[error("default privilege entry for owner \"{owner}\" needs either `schema` or `scope`")] + DefaultPrivilegeScopeMissing { owner: String }, + + #[error("default privilege scope of type `schema` needs a `schema` name")] + DefaultPrivilegeScopeSchemaMissing, + + #[error("default privilege scope of type `global` must not name a schema (got \"{schema}\")")] + DefaultPrivilegeScopeSchemaForbidden { schema: String }, + + // `scope` renders its own noun, so the template must not add one. + #[error("default privileges cannot target on_type `{on_type}` in {scope}")] + InvalidDefaultPrivilegeOnType { on_type: String, scope: String }, + + #[error( + "profile \"{profile}\" declares an `ensure: absent` default privilege — profiles are additive templates and cannot assert absence" + )] + ProfileAbsentDefaultPrivilege { profile: String }, } // --------------------------------------------------------------------------- @@ -145,6 +187,25 @@ impl std::fmt::Display for Privilege { } } +/// Whether the listed privileges must exist or must not exist. +/// +/// `absent` asserts one ACL edge only. It plans a REVOKE when the privilege is +/// found live, and it says nothing about access the grantee may still have +/// through role membership or ownership. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "lowercase")] +pub enum Ensure { + #[default] + Present, + Absent, +} + +impl Ensure { + pub fn is_present(&self) -> bool { + matches!(self, Ensure::Present) + } +} + // --------------------------------------------------------------------------- // YAML manifest types // --------------------------------------------------------------------------- @@ -495,12 +556,16 @@ pub struct PasswordSource { /// A concrete grant on a specific object or wildcard. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct Grant { + /// The grantee. The exact-uppercase value `PUBLIC` means the PostgreSQL + /// PUBLIC pseudo-role; any other value is an ordinary role name. #[schemars(length(min = 1, max = MAX_IDENTIFIER))] pub role: String, #[schemars(length(min = 1, max = MAX_PRIVILEGES))] pub privileges: Vec, #[serde(alias = "on")] pub object: ObjectTarget, + #[serde(default, skip_serializing_if = "Ensure::is_present")] + pub ensure: Ensure, } /// Target object for a grant. @@ -521,25 +586,107 @@ pub struct ObjectTarget { } /// Default privilege configuration. +/// +/// The scope rules are also expressed as CEL so the API server rejects a bad +/// entry at apply time. `resolved_scope` enforces the same rules for the CLI, +/// which has no admission step. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[schemars(extend("x-kubernetes-validations" = [serde_json::json!({ + "rule": "has(self.schema) != has(self.scope)", + "message": "exactly one of `schema` and `scope` must be set" +})]))] pub struct DefaultPrivilege { /// The role that owns newly created objects. If omitted, uses manifest's default_owner. #[serde(default, skip_serializing_if = "Option::is_none")] #[schemars(length(min = 1, max = MAX_IDENTIFIER))] pub owner: Option, + /// Schema shorthand, equivalent to `scope: {type: schema, schema: ...}`. + /// Exactly one of `schema` and `scope` must be set. + #[serde(default, skip_serializing_if = "Option::is_none")] #[schemars(length(min = 1, max = MAX_IDENTIFIER))] - pub schema: String, + pub schema: Option, + + /// Where the defaults apply: one schema, or owner-wide (global). Global + /// scope renders `ALTER DEFAULT PRIVILEGES` without an `IN SCHEMA` clause. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub scope: Option, #[schemars(length(max = MAX_DEFAULT_PRIVILEGE_GRANTS))] pub grant: Vec, } +/// Scope selector for a default-privileges entry. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[schemars(extend("x-kubernetes-validations" = [serde_json::json!({ + "rule": "has(self.schema) == (self.type == 'schema')", + "message": "`schema` is required when type is `schema` and forbidden when type is `global`" +})]))] +pub struct DefaultPrivilegeScopeSpec { + #[serde(rename = "type")] + pub scope_type: DefaultPrivilegeScopeType, + + /// Schema name. Required for `type: schema`, forbidden for `type: global`. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[schemars(length(min = 1, max = MAX_IDENTIFIER))] + pub schema: Option, +} + +/// The kind of default-privilege scope. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "lowercase")] +pub enum DefaultPrivilegeScopeType { + Global, + Schema, +} + +impl DefaultPrivilege { + /// Resolve the `schema` shorthand and the `scope` field into one scope. + pub fn resolved_scope(&self) -> Result { + use crate::model::DefaultPrivilegeScope; + + let owner_context = || { + self.owner + .clone() + .unwrap_or_else(|| "(default owner)".to_string()) + }; + + match (&self.schema, &self.scope) { + (Some(_), Some(_)) => Err(ManifestError::DefaultPrivilegeScopeConflict { + owner: owner_context(), + }), + (None, None) => Err(ManifestError::DefaultPrivilegeScopeMissing { + owner: owner_context(), + }), + (Some(schema), None) => Ok(DefaultPrivilegeScope::Schema { + schema: schema.clone(), + }), + (None, Some(spec)) => match (spec.scope_type, &spec.schema) { + (DefaultPrivilegeScopeType::Global, None) => Ok(DefaultPrivilegeScope::Global), + (DefaultPrivilegeScopeType::Global, Some(schema)) => { + Err(ManifestError::DefaultPrivilegeScopeSchemaForbidden { + schema: schema.clone(), + }) + } + (DefaultPrivilegeScopeType::Schema, Some(schema)) => { + Ok(DefaultPrivilegeScope::Schema { + schema: schema.clone(), + }) + } + (DefaultPrivilegeScopeType::Schema, None) => { + Err(ManifestError::DefaultPrivilegeScopeSchemaMissing) + } + }, + } + } +} + /// A single default privilege grant entry. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct DefaultPrivilegeGrant { /// The role receiving the default privilege. Only used in top-level default_privileges - /// (in profiles, the role is determined by expansion). + /// (in profiles, the role is determined by expansion). The exact-uppercase + /// value `PUBLIC` means the PostgreSQL PUBLIC pseudo-role. #[serde(default, skip_serializing_if = "Option::is_none")] #[schemars(length(min = 1, max = MAX_IDENTIFIER))] pub role: Option, @@ -547,6 +694,9 @@ pub struct DefaultPrivilegeGrant { #[schemars(length(min = 1, max = MAX_PRIVILEGES))] pub privileges: Vec, pub on_type: ObjectType, + + #[serde(default, skip_serializing_if = "Ensure::is_present")] + pub ensure: Ensure, } /// A membership declaration — which members belong to a role. @@ -789,11 +939,16 @@ pub fn validate_bounds(manifest: &PolicyManifest) -> Result<(), ManifestError> { MAX_DEFAULT_PRIVILEGES, )?; for default_privilege in &manifest.default_privileges { - text( - "default privilege schema", - &default_privilege.schema, - MAX_IDENTIFIER, - )?; + if let Some(schema) = &default_privilege.schema { + text("default privilege schema", schema, MAX_IDENTIFIER)?; + } + if let Some(schema) = default_privilege + .scope + .as_ref() + .and_then(|s| s.schema.as_ref()) + { + text("default privilege scope schema", schema, MAX_IDENTIFIER)?; + } if let Some(owner) = &default_privilege.owner { text("default privilege owner", owner, MAX_IDENTIFIER)?; } @@ -1006,6 +1161,7 @@ pub fn expand_manifest(manifest: &PolicyManifest) -> Result Result = profile .default_privileges .iter() - .map(|dp| DefaultPrivilegeGrant { - role: Some(role_name.clone()), - privileges: dp.privileges.clone(), - on_type: dp.on_type, + .map(|dp| { + if dp.ensure == Ensure::Absent { + return Err(ManifestError::ProfileAbsentDefaultPrivilege { + profile: profile_name.clone(), + }); + } + Ok(DefaultPrivilegeGrant { + role: Some(role_name.clone()), + privileges: dp.privileges.clone(), + on_type: dp.on_type, + ensure: Ensure::Present, + }) }) - .collect(); + .collect::>()?; default_privileges.push(DefaultPrivilege { owner, - schema: schema_binding.name.clone(), + schema: Some(schema_binding.name.clone()), + scope: None, grant: expanded_grants, }); } @@ -1040,7 +1205,7 @@ pub fn expand_manifest(manifest: &PolicyManifest) -> Result Result = HashSet::new(); for role in &roles { @@ -1149,6 +1337,216 @@ pub fn expand_manifest(manifest: &PolicyManifest) -> Result Result<(), ManifestError> { + let reserved = |value: Option<&str>, context: &str| -> Result<(), ManifestError> { + if value == Some("PUBLIC") { + return Err(ManifestError::ReservedPublicName { + context: context.to_string(), + }); + } + Ok(()) + }; + + reserved(manifest.default_owner.as_deref(), "default_owner")?; + for role in roles { + reserved(Some(&role.name), "a role name")?; + } + for schema in schemas { + reserved(schema.owner.as_deref(), "a schema owner")?; + } + for default_priv in default_privileges { + reserved(default_priv.owner.as_deref(), "a default privilege owner")?; + } + for membership in memberships { + reserved(Some(&membership.role), "a membership role")?; + for member in &membership.members { + reserved(Some(&member.name), "a membership member")?; + } + } + for retirement in &manifest.retirements { + reserved(Some(&retirement.role), "a retirement role")?; + reserved( + retirement.reassign_owned_to.as_deref(), + "a retirement ownership target", + )?; + } + Ok(()) +} + +/// Resolve every default-privilege scope and check the on_type matrix. +/// +/// Database defaults do not exist in PostgreSQL. Schema defaults exist only in +/// global scope because schemas are not contained in another schema. Views and +/// materialized views are rejected because `pg_default_acl` stores them as +/// plain relations ('r'), so a view-typed key could never converge — declare +/// `on_type: table` instead, which is what PostgreSQL actually applies. +fn validate_default_privilege_scopes( + default_privileges: &[DefaultPrivilege], +) -> Result<(), ManifestError> { + use crate::model::DefaultPrivilegeScope; + + for default_priv in default_privileges { + let scope = default_priv.resolved_scope()?; + for grant in &default_priv.grant { + let allowed = !matches!( + (&scope, grant.on_type), + (_, ObjectType::Database) + | (_, ObjectType::View | ObjectType::MaterializedView) + | (DefaultPrivilegeScope::Schema { .. }, ObjectType::Schema) + ); + if !allowed { + return Err(ManifestError::InvalidDefaultPrivilegeOnType { + on_type: grant.on_type.to_string(), + scope: scope.to_string(), + }); + } + } + } + Ok(()) +} + +fn describe_object_target(target: &ObjectTarget) -> String { + match (&target.schema, &target.name) { + (Some(schema), Some(name)) => { + format!("{} \"{}\".\"{}\"", target.object_type, schema, name) + } + (Some(schema), None) => format!("{} in schema \"{}\"", target.object_type, schema), + (None, Some(name)) => format!("{} \"{}\"", target.object_type, name), + (None, None) => target.object_type.to_string(), + } +} + +/// Reject the same assertion key declared both present and absent. +/// +/// Grants are keyed per privilege on (grantee, object type, schema, name); +/// defaults on (resolved owner, scope, grantee, on_type). A wildcard and a +/// named selector with opposite ensure for the same privilege are also +/// rejected: grants apply before revokes, so such a pair re-grants or +/// re-revokes itself every reconcile and never converges. +fn validate_ensure_conflicts( + default_owner: Option<&str>, + grants: &[Grant], + default_privileges: &[DefaultPrivilege], +) -> Result<(), ManifestError> { + type GrantAssertionKey = ( + String, + ObjectType, + Option, + Option, + Privilege, + ); + let mut grant_assertions: BTreeMap = BTreeMap::new(); + // (grantee, object_type, schema, privilege) -> ensures seen on wildcard + // and on named selectors, for the cross-selector rule. + let mut selector_ensures: BTreeMap<(String, ObjectType, String, Privilege), [bool; 4]> = + BTreeMap::new(); + + for grant in grants { + for privilege in &grant.privileges { + let key = ( + grant.role.clone(), + grant.object.object_type, + grant.object.schema.clone(), + grant.object.name.clone(), + *privilege, + ); + if let Some(existing) = grant_assertions.insert(key, grant.ensure) + && existing != grant.ensure + { + return Err(ManifestError::ConflictingGrantEnsure { + target: format!( + "for \"{}\" on {}", + grant.role, + describe_object_target(&grant.object) + ), + privilege: privilege.to_string(), + }); + } + + if let (Some(schema), Some(name)) = (&grant.object.schema, &grant.object.name) { + let flags = selector_ensures + .entry(( + grant.role.clone(), + grant.object.object_type, + schema.clone(), + *privilege, + )) + .or_default(); + let index = match (name == "*", grant.ensure) { + (true, Ensure::Present) => 0, + (true, Ensure::Absent) => 1, + (false, Ensure::Present) => 2, + (false, Ensure::Absent) => 3, + }; + flags[index] = true; + let (wild_present, wild_absent, named_present, named_absent) = + (flags[0], flags[1], flags[2], flags[3]); + if (wild_present && named_absent) || (wild_absent && named_present) { + return Err(ManifestError::ConflictingWildcardEnsure { + target: format!( + "for \"{}\" on {} in schema \"{schema}\"", + grant.role, grant.object.object_type + ), + privilege: privilege.to_string(), + }); + } + } + } + } + + type DefaultAssertionKey = ( + String, + crate::model::DefaultPrivilegeScope, + String, + ObjectType, + Privilege, + ); + let mut default_assertions: BTreeMap = BTreeMap::new(); + + for default_priv in default_privileges { + let owner = default_priv + .owner + .as_deref() + .or(default_owner) + .unwrap_or("postgres") + .to_string(); + let scope = default_priv.resolved_scope()?; + for grant in &default_priv.grant { + let Some(grantee) = &grant.role else { continue }; + for privilege in &grant.privileges { + let key = ( + owner.clone(), + scope.clone(), + grantee.clone(), + grant.on_type, + *privilege, + ); + if let Some(existing) = default_assertions.insert(key, grant.ensure) + && existing != grant.ensure + { + return Err(ManifestError::ConflictingDefaultPrivilegeEnsure { + target: format!( + "for owner \"{owner}\" ({scope}, {} to \"{grantee}\")", + grant.on_type + ), + privilege: privilege.to_string(), + }); + } + } + } + } + + Ok(()) +} + /// Validate that a string is a plausible ISO 8601 timestamp. /// /// Accepts formats like: @@ -1818,14 +2216,20 @@ schemas: expanded.default_privileges[0].owner, Some("app_owner".to_string()) ); - assert_eq!(expanded.default_privileges[0].schema, "inventory"); + assert_eq!( + expanded.default_privileges[0].schema.as_deref(), + Some("inventory") + ); // legacy uses override assert_eq!( expanded.default_privileges[1].owner, Some("legacy_admin".to_string()) ); - assert_eq!(expanded.default_privileges[1].schema, "legacy"); + assert_eq!( + expanded.default_privileges[1].schema.as_deref(), + Some("legacy") + ); } #[test] @@ -2381,4 +2785,335 @@ spec: assert_eq!(from_bare.schemas.len(), from_cr.schemas.len()); assert_eq!(from_bare.profiles.len(), from_cr.profiles.len()); } + + // ----------------------------------------------------------------------- + // ensure / PUBLIC / default-privilege scopes + // ----------------------------------------------------------------------- + + fn expand(yaml: &str) -> Result { + expand_manifest(&parse_manifest(yaml).unwrap()) + } + + #[test] + fn legacy_manifest_defaults_to_present_and_schema_scope() { + let expanded = expand( + r#" +grants: + - role: reader + privileges: [SELECT] + object: { type: table, schema: app, name: "*" } +default_privileges: + - owner: app_owner + schema: app + grant: + - role: reader + privileges: [SELECT] + on_type: table +"#, + ) + .unwrap(); + + assert_eq!(expanded.grants[0].ensure, Ensure::Present); + assert_eq!( + expanded.default_privileges[0].grant[0].ensure, + Ensure::Present + ); + assert_eq!( + expanded.default_privileges[0].resolved_scope().unwrap(), + crate::model::DefaultPrivilegeScope::Schema { + schema: "app".to_string() + } + ); + } + + #[test] + fn ensure_and_scope_round_trip_through_yaml() { + let yaml = r#" +grants: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: api, name: "*" } +default_privileges: + - owner: api_owner + scope: { type: global } + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function +"#; + let manifest = parse_manifest(yaml).unwrap(); + assert_eq!(manifest.grants[0].ensure, Ensure::Absent); + assert_eq!( + manifest.default_privileges[0].resolved_scope().unwrap(), + crate::model::DefaultPrivilegeScope::Global + ); + + // Present/schema entries stay out of the serialized form so existing + // manifests keep their shape. + let reserialized = serde_yaml::to_string(&manifest).unwrap(); + assert!(reserialized.contains("ensure: absent")); + assert!(!reserialized.contains("ensure: present")); + } + + #[test] + fn public_is_reserved_everywhere_a_real_role_is_named() { + let cases = [ + "roles:\n - name: PUBLIC\n", + "memberships:\n - role: PUBLIC\n members:\n - name: app\n", + "memberships:\n - role: app\n members:\n - name: PUBLIC\n", + "retirements:\n - role: PUBLIC\n", + "default_owner: PUBLIC\n", + "schemas:\n - name: app\n owner: PUBLIC\n profiles: []\n", + "default_privileges:\n - owner: PUBLIC\n schema: app\n grant:\n - role: r\n privileges: [SELECT]\n on_type: table\n", + ]; + for yaml in cases { + assert!( + matches!(expand(yaml), Err(ManifestError::ReservedPublicName { .. })), + "expected PUBLIC to be rejected in: {yaml}" + ); + } + } + + #[test] + fn public_is_allowed_as_a_grantee_and_lowercase_public_is_an_ordinary_role() { + expand( + r#" +roles: + - name: public +grants: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: api, name: "*" } + - role: public + privileges: [USAGE] + object: { type: schema, name: api } +"#, + ) + .unwrap(); + } + + #[test] + fn default_privilege_scope_must_be_specified_exactly_once() { + let both = expand( + r#" +default_privileges: + - owner: o + schema: app + scope: { type: global } + grant: + - role: r + privileges: [SELECT] + on_type: table +"#, + ); + assert!(matches!( + both, + Err(ManifestError::DefaultPrivilegeScopeConflict { .. }) + )); + + let neither = expand( + r#" +default_privileges: + - owner: o + grant: + - role: r + privileges: [SELECT] + on_type: table +"#, + ); + assert!(matches!( + neither, + Err(ManifestError::DefaultPrivilegeScopeMissing { .. }) + )); + + let schema_scope_without_name = expand( + r#" +default_privileges: + - owner: o + scope: { type: schema } + grant: + - role: r + privileges: [SELECT] + on_type: table +"#, + ); + assert!(matches!( + schema_scope_without_name, + Err(ManifestError::DefaultPrivilegeScopeSchemaMissing) + )); + + let global_with_name = expand( + r#" +default_privileges: + - owner: o + scope: { type: global, schema: app } + grant: + - role: r + privileges: [SELECT] + on_type: table +"#, + ); + assert!(matches!( + global_with_name, + Err(ManifestError::DefaultPrivilegeScopeSchemaForbidden { .. }) + )); + } + + #[test] + fn default_privilege_on_type_matrix_is_enforced_per_scope() { + let dp = |scope: &str, on_type: &str| { + expand(&format!( + r#" +default_privileges: + - owner: o + {scope} + grant: + - role: r + privileges: [USAGE] + on_type: {on_type} +"# + )) + }; + + // Schemas are not contained in a schema, so they are global-only. + assert!(dp("scope: { type: global }", "schema").is_ok()); + assert!(matches!( + dp("schema: app", "schema"), + Err(ManifestError::InvalidDefaultPrivilegeOnType { .. }) + )); + + // PostgreSQL has no database-level default privileges. + for scope in ["scope: { type: global }", "schema: app"] { + assert!(matches!( + dp(scope, "database"), + Err(ManifestError::InvalidDefaultPrivilegeOnType { .. }) + )); + // pg_default_acl stores views as plain relations, so a view-typed + // key could never converge. + assert!(matches!( + dp(scope, "view"), + Err(ManifestError::InvalidDefaultPrivilegeOnType { .. }) + )); + for on_type in ["table", "sequence", "function", "type"] { + assert!(dp(scope, on_type).is_ok(), "{scope} / {on_type}"); + } + } + } + + #[test] + fn same_assertion_declared_present_and_absent_is_rejected() { + let grants = expand( + r#" +grants: + - role: PUBLIC + privileges: [EXECUTE] + object: { type: function, schema: api, name: f() } + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: api, name: f() } +"#, + ); + assert!(matches!( + grants, + Err(ManifestError::ConflictingGrantEnsure { .. }) + )); + + let defaults = expand( + r#" +default_privileges: + - owner: o + schema: app + grant: + - role: r + privileges: [SELECT] + on_type: table + - role: r + ensure: absent + privileges: [SELECT] + on_type: table +"#, + ); + assert!(matches!( + defaults, + Err(ManifestError::ConflictingDefaultPrivilegeEnsure { .. }) + )); + } + + #[test] + fn disjoint_privileges_on_one_target_may_mix_present_and_absent() { + expand( + r#" +grants: + - role: reader + privileges: [SELECT] + object: { type: table, schema: app, name: t } + - role: reader + ensure: absent + privileges: [DELETE] + object: { type: table, schema: app, name: t } +"#, + ) + .unwrap(); + } + + #[test] + fn wildcard_and_named_selectors_may_not_disagree_on_ensure() { + // Grants run before revokes, so this pair would never converge. + let conflicting = expand( + r#" +grants: + - role: PUBLIC + privileges: [EXECUTE] + object: { type: function, schema: api, name: "*" } + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: api, name: f() } +"#, + ); + assert!(matches!( + conflicting, + Err(ManifestError::ConflictingWildcardEnsure { .. }) + )); + + // A different privilege on each selector is fine. + expand( + r#" +grants: + - role: reader + privileges: [SELECT] + object: { type: table, schema: app, name: "*" } + - role: reader + ensure: absent + privileges: [DELETE] + object: { type: table, schema: app, name: t } +"#, + ) + .unwrap(); + } + + #[test] + fn profiles_may_not_assert_absence() { + let result = expand( + r#" +profiles: + editor: + default_privileges: + - ensure: absent + privileges: [SELECT] + on_type: table +schemas: + - name: app + profiles: [editor] +"#, + ); + assert!(matches!( + result, + Err(ManifestError::ProfileAbsentDefaultPrivilege { .. }) + )); + } } diff --git a/crates/pgroles-core/src/model.rs b/crates/pgroles-core/src/model.rs index 8b88c40..21f0d5d 100644 --- a/crates/pgroles-core/src/model.rs +++ b/crates/pgroles-core/src/model.rs @@ -7,7 +7,7 @@ use std::collections::{BTreeMap, BTreeSet}; -use crate::manifest::{ExpandedManifest, Grant, ObjectType, Privilege, RoleDefinition}; +use crate::manifest::{Ensure, ExpandedManifest, Grant, ObjectType, Privilege, RoleDefinition}; // --------------------------------------------------------------------------- // Role attributes @@ -176,6 +176,102 @@ pub struct SchemaState { pub owner_privileges: BTreeSet, } +// --------------------------------------------------------------------------- +// Grantees and scopes +// --------------------------------------------------------------------------- + +/// A privilege grantee: either an ordinary role or the PostgreSQL PUBLIC +/// pseudo-role (ACL grantee OID 0). +/// +/// PUBLIC is typed rather than spelled as a magic string so that SQL rendering +/// can never quote it (`"PUBLIC"` would name a real role) and so a role +/// literally named `PUBLIC` can never be confused with the pseudo-role. +/// `Public` sorts before every role name, which keeps BTreeMap output +/// deterministic. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum Grantee { + Public, + Role(String), +} + +/// How the PUBLIC pseudo-role spells itself wherever a grantee is carried as a +/// bare string. +pub const PUBLIC_ROLE: &str = "PUBLIC"; + +impl Grantee { + /// Parse a manifest grantee string. The exact-uppercase `PUBLIC` is the + /// pseudo-role; everything else is a role name. + pub fn parse(s: &str) -> Self { + if s == PUBLIC_ROLE { + Grantee::Public + } else { + Grantee::Role(s.to_string()) + } + } + + pub fn is_public(&self) -> bool { + matches!(self, Grantee::Public) + } + + pub fn as_str(&self) -> &str { + match self { + Grantee::Public => PUBLIC_ROLE, + Grantee::Role(name) => name, + } + } +} + +impl std::fmt::Display for Grantee { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +impl From<&str> for Grantee { + fn from(s: &str) -> Self { + Grantee::parse(s) + } +} + +// Serialized as a plain string so plan JSON keeps the shape it had when the +// grantee was a `String`. +impl serde::Serialize for Grantee { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(self.as_str()) + } +} + +/// Where a default-privilege rule applies. +/// +/// `Global` is the owner-wide layer (`pg_default_acl.defaclnamespace = 0`), +/// which affects every schema in the database and renders without an +/// `IN SCHEMA` clause. `Global` sorts before `Schema` so the global layer +/// appears first in deterministic output. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, serde::Serialize)] +#[serde(tag = "type", rename_all = "lowercase")] +pub enum DefaultPrivilegeScope { + Global, + Schema { schema: String }, +} + +impl DefaultPrivilegeScope { + pub fn schema(&self) -> Option<&str> { + match self { + DefaultPrivilegeScope::Global => None, + DefaultPrivilegeScope::Schema { schema } => Some(schema), + } + } +} + +impl std::fmt::Display for DefaultPrivilegeScope { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + DefaultPrivilegeScope::Global => write!(f, "global scope"), + DefaultPrivilegeScope::Schema { schema } => write!(f, "schema \"{schema}\""), + } + } +} + // --------------------------------------------------------------------------- // Grants // --------------------------------------------------------------------------- @@ -183,10 +279,13 @@ pub struct SchemaState { /// Unique key identifying a grant target — (grantee, object_type, schema, name). /// /// We use `Ord` so these can live in a `BTreeMap` for deterministic output. +/// The field order also matters for the diff engine: absence assertions with a +/// wildcard name range-scan all keys sharing the (role, object_type, schema) +/// prefix, which needs `name` to be the last field. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, serde::Serialize)] pub struct GrantKey { - /// The role receiving the privilege. - pub role: String, + /// The grantee receiving the privilege. + pub role: Grantee, /// The kind of object. pub object_type: ObjectType, /// Schema name. `None` for schema-level and database-level grants. @@ -210,12 +309,12 @@ pub struct GrantState { pub struct DefaultPrivKey { /// The owner role context (whose newly-created objects get these defaults). pub owner: String, - /// The schema where the default applies. - pub schema: String, + /// Where the default applies: one schema, or owner-wide. + pub scope: DefaultPrivilegeScope, /// The type of object affected. pub on_type: ObjectType, - /// The grantee role. - pub grantee: String, + /// The grantee. + pub grantee: Grantee, } /// The privilege set for a default privilege rule. @@ -257,10 +356,16 @@ pub struct RoleGraph { pub schemas: BTreeMap, /// Object privilege grants, keyed by grant target. pub grants: BTreeMap, - /// Default privilege rules, keyed by (owner, schema, type, grantee). + /// Default privilege rules, keyed by (owner, scope, type, grantee). pub default_privileges: BTreeMap, /// Membership edges. pub memberships: BTreeSet, + /// Privileges asserted absent per grant target (`ensure: absent`). + /// Only the desired graph populates this; inspection leaves it empty. + pub grant_absences: BTreeMap>, + /// Privileges asserted absent per default-privilege rule. + /// Only the desired graph populates this; inspection leaves it empty. + pub default_privilege_absences: BTreeMap>, } impl RoleGraph { @@ -298,11 +403,20 @@ impl RoleGraph { // --- Grants --- for grant in &expanded.grants { let key = grant_key_from_manifest(grant); - let entry = graph.grants.entry(key).or_insert_with(|| GrantState { - privileges: BTreeSet::new(), - }); + let privileges = match grant.ensure { + Ensure::Present => { + &mut graph + .grants + .entry(key) + .or_insert_with(|| GrantState { + privileges: BTreeSet::new(), + }) + .privileges + } + Ensure::Absent => graph.grant_absences.entry(key).or_default(), + }; for privilege in &grant.privileges { - entry.privileges.insert(*privilege); + privileges.insert(*privilege); } } @@ -314,30 +428,36 @@ impl RoleGraph { .or(default_owner) .unwrap_or("postgres") .to_string(); + let scope = default_priv.resolved_scope()?; for grant in &default_priv.grant { - let grantee = grant.role.clone().ok_or_else(|| { + let grantee = grant.role.as_deref().map(Grantee::parse).ok_or_else(|| { crate::manifest::ManifestError::MissingDefaultPrivilegeRole { - schema: default_priv.schema.clone(), + scope: scope.to_string(), } })?; let key = DefaultPrivKey { owner: owner.clone(), - schema: default_priv.schema.clone(), + scope: scope.clone(), on_type: grant.on_type, grantee, }; - let entry = - graph - .default_privileges - .entry(key) - .or_insert_with(|| DefaultPrivState { - privileges: BTreeSet::new(), - }); + let privileges = match grant.ensure { + Ensure::Present => { + &mut graph + .default_privileges + .entry(key) + .or_insert_with(|| DefaultPrivState { + privileges: BTreeSet::new(), + }) + .privileges + } + Ensure::Absent => graph.default_privilege_absences.entry(key).or_default(), + }; for privilege in &grant.privileges { - entry.privileges.insert(*privilege); + privileges.insert(*privilege); } } } @@ -364,7 +484,7 @@ impl RoleGraph { fn grant_key_from_manifest(grant: &Grant) -> GrantKey { GrantKey { - role: grant.role.clone(), + role: Grantee::parse(&grant.role), object_type: grant.object.object_type, schema: grant.object.schema.clone(), name: grant.object.name.clone(), @@ -626,9 +746,14 @@ memberships: assert_eq!(graph.default_privileges.len(), 1); let dp_key = graph.default_privileges.keys().next().unwrap(); assert_eq!(dp_key.owner, "app_owner"); - assert_eq!(dp_key.schema, "inventory"); + assert_eq!( + dp_key.scope, + DefaultPrivilegeScope::Schema { + schema: "inventory".to_string() + } + ); assert_eq!(dp_key.on_type, ObjectType::Table); - assert_eq!(dp_key.grantee, "inventory-editor"); + assert_eq!(dp_key.grantee.as_str(), "inventory-editor"); let dp_privs = &graph.default_privileges.values().next().unwrap().privileges; assert!(dp_privs.contains(&Privilege::Select)); assert!(dp_privs.contains(&Privilege::Insert)); diff --git a/crates/pgroles-core/src/overlap.rs b/crates/pgroles-core/src/overlap.rs index 8e03bcc..d7a5da2 100644 --- a/crates/pgroles-core/src/overlap.rs +++ b/crates/pgroles-core/src/overlap.rs @@ -28,7 +28,7 @@ //! //! # Intersection //! -//! Two pairs intersect only if their roles are equal, and then: +//! Two pairs intersect only if their roles match, and then: //! //! * a schema-level object intersects any object *within* that schema, because //! a grant on the schema and a grant on a table in it can be the same access; @@ -37,12 +37,15 @@ //! the conservative direction: the cost of a false intersection is one extra //! review round, the cost of a false miss is an unreviewed effect. //! * everything else intersects only itself. +//! +//! Roles match when they are equal, and `PUBLIC` matches every role, because a +//! privilege held by PUBLIC is held by every role. use std::collections::BTreeSet; use crate::diff::Change; use crate::manifest::ObjectType; -use crate::model::MembershipEdge; +use crate::model::{DefaultPrivilegeScope, MembershipEdge, PUBLIC_ROLE}; /// The object half of an effect pair. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] @@ -82,10 +85,17 @@ impl EffectPair { /// Does this pair intersect `other` under the ADR-001 Decision 6 rule? pub fn intersects(&self, other: &EffectPair) -> bool { - self.role == other.role && objects_intersect(&self.object, &other.object) + roles_intersect(&self.role, &other.role) && objects_intersect(&self.object, &other.object) } } +/// A privilege held by PUBLIC is held by every role, so an effect on PUBLIC +/// reaches the same objects as an effect on any named role. `PUBLIC` is +/// reserved and can never name a real role, so the comparison is unambiguous. +fn roles_intersect(left: &str, right: &str) -> bool { + left == right || left == PUBLIC_ROLE || right == PUBLIC_ROLE +} + fn objects_intersect(left: &EffectObject, right: &EffectObject) -> bool { use EffectObject::*; match (left, right) { @@ -113,6 +123,17 @@ fn objects_intersect(left: &EffectObject, right: &EffectObject) -> bool { } } +/// Project a default-privilege scope into the pair space. +fn default_privilege_object(scope: &DefaultPrivilegeScope) -> EffectObject { + match scope { + DefaultPrivilegeScope::Schema { schema } => EffectObject::Schema(schema.clone()), + // A global rule applies in every schema the owner creates objects in, + // including schemas no policy names, so nothing bounds it to a schema + // and it must be treated as overlapping anything on the same role. + DefaultPrivilegeScope::Global => EffectObject::RoleAttributes, + } +} + /// Project a grant/revoke target into the pair space. fn grant_object(object_type: ObjectType, schema: Option<&str>, name: Option<&str>) -> EffectObject { match object_type { @@ -187,27 +208,30 @@ pub fn change_pairs(change: &Change) -> Vec { name, .. } => vec![EffectPair::new( - role, + role.as_str(), grant_object(*object_type, schema.as_deref(), name.as_deref()), )], - // Default privileges are scoped to a schema and change what the - // grantee will hold on objects the owner creates there. Both roles are - // touched: the grantee gains access, the owner's future objects change. + // Default privileges change what the grantee will hold on objects the + // owner creates. Both roles are touched: the grantee gains access, the + // owner's future objects change. Change::SetDefaultPrivilege { owner, - schema, + scope, grantee, .. } | Change::RevokeDefaultPrivilege { owner, - schema, + scope, grantee, .. - } => vec![ - EffectPair::new(grantee, EffectObject::Schema(schema.clone())), - EffectPair::new(owner, EffectObject::Schema(schema.clone())), - ], + } => { + let object = default_privilege_object(scope); + vec![ + EffectPair::new(grantee.as_str(), object.clone()), + EffectPair::new(owner, object), + ] + } Change::AddMember { role, member, .. } | Change::RemoveMember { role, member } => { vec![EffectPair::new(member, EffectObject::Role(role.clone()))] } @@ -272,6 +296,7 @@ pub fn describe_pair(pair: &EffectPair) -> String { mod tests { use super::*; use crate::manifest::Privilege; + use crate::model::Grantee; fn grant( role: &str, @@ -280,7 +305,7 @@ mod tests { name: Option<&str>, ) -> Change { Change::Grant { - role: role.to_string(), + role: Grantee::parse(role), privileges: BTreeSet::from([Privilege::Select]), object_type, schema: schema.map(str::to_string), @@ -412,7 +437,7 @@ mod tests { Some("orders"), )]); let revoked = effect_pairs(&[Change::Revoke { - role: "app_rw".to_string(), + role: Grantee::parse("app_rw"), privileges: BTreeSet::from([Privilege::Select]), object_type: ObjectType::Table, schema: Some("app".to_string()), @@ -425,9 +450,11 @@ mod tests { fn default_privileges_touch_both_grantee_and_owner_at_schema_level() { let pairs = effect_pairs(&[Change::SetDefaultPrivilege { owner: "app_owner".to_string(), - schema: "app".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "app".to_string(), + }, on_type: ObjectType::Table, - grantee: "app_ro".to_string(), + grantee: Grantee::parse("app_ro"), privileges: BTreeSet::from([Privilege::Select]), }]); assert_eq!( @@ -439,6 +466,52 @@ mod tests { ); } + #[test] + fn a_public_effect_overlaps_every_named_role_on_the_same_object() { + let object = EffectObject::Relation { + schema: "app".to_string(), + name: "orders".to_string(), + object_type: ObjectType::Table, + }; + let public = EffectPair::new(PUBLIC_ROLE, object.clone()); + let named = EffectPair::new("alice", object); + assert!(public.intersects(&named)); + assert!(named.intersects(&public)); + } + + #[test] + fn a_public_effect_still_respects_object_scope() { + let public = EffectPair::new(PUBLIC_ROLE, EffectObject::Schema("app".to_string())); + let elsewhere = EffectPair::new("alice", EffectObject::Schema("other".to_string())); + assert!(!public.intersects(&elsewhere)); + } + + #[test] + fn a_global_default_privilege_is_not_bounded_to_any_schema() { + let pairs = effect_pairs(&[Change::SetDefaultPrivilege { + owner: "app_owner".to_string(), + scope: DefaultPrivilegeScope::Global, + on_type: ObjectType::Table, + grantee: Grantee::parse("app_ro"), + privileges: BTreeSet::from([Privilege::Select]), + }]); + assert_eq!( + pairs, + BTreeSet::from([ + EffectPair::new("app_owner", EffectObject::RoleAttributes), + EffectPair::new("app_ro", EffectObject::RoleAttributes), + ]) + ); + // It must overlap a schema-scoped effect on the same role, since the + // global rule reaches every schema. + assert!( + EffectPair::new("app_ro", EffectObject::RoleAttributes).intersects(&EffectPair::new( + "app_ro", + EffectObject::Schema("anything".to_string()) + )) + ); + } + #[test] fn a_non_overlapping_overlay_leaves_the_candidate_alone() { // The case the ADR exists to protect: continuous ephemeral traffic on diff --git a/crates/pgroles-core/src/ownership.rs b/crates/pgroles-core/src/ownership.rs index b739bc1..bf10a48 100644 --- a/crates/pgroles-core/src/ownership.rs +++ b/crates/pgroles-core/src/ownership.rs @@ -4,7 +4,7 @@ use thiserror::Error; use crate::diff::Change; use crate::manifest::{ObjectType, SchemaBindingFacet}; -use crate::model::{DefaultPrivKey, GrantKey}; +use crate::model::{DefaultPrivKey, DefaultPrivilegeScope, GrantKey, Grantee}; #[derive(Debug, Clone, Default)] pub struct OwnershipIndex { @@ -160,20 +160,20 @@ impl ManagedChangeSurface { }), Change::SetDefaultPrivilege { owner, - schema, + scope, on_type, grantee, .. } | Change::RevokeDefaultPrivilege { owner, - schema, + scope, on_type, grantee, .. } => self.allows_default_privilege_change(&DefaultPrivKey { owner: owner.clone(), - schema: schema.clone(), + scope: scope.clone(), on_type: *on_type, grantee: grantee.clone(), }), @@ -200,11 +200,20 @@ impl ManagedChangeSurface { return true; } - key.object_type == ObjectType::Database && self.roles.contains(&key.role) + key.object_type == ObjectType::Database + && matches!(&key.role, Grantee::Role(role) if self.roles.contains(role)) } fn allows_default_privilege_change(&self, key: &DefaultPrivKey) -> bool { - self.explicit_default_privileges.contains(key) || self.binding_schemas.contains(&key.schema) + if self.explicit_default_privileges.contains(key) { + return true; + } + match &key.scope { + DefaultPrivilegeScope::Schema { schema } => self.binding_schemas.contains(schema), + // A global default is a property of the owner role, so the + // authority over the owner is the authority over the default. + DefaultPrivilegeScope::Global => self.roles.contains(&key.owner), + } } } @@ -262,21 +271,23 @@ pub(crate) fn describe_change(change: &Change) -> String { ), Change::SetDefaultPrivilege { owner, - schema, + scope, on_type, grantee, .. } => format!( - "set default privilege for owner \"{owner}\" schema \"{schema}\" on {on_type} to \"{grantee}\"" + "set default privilege for owner \"{owner}\" {scope} on {on_type} to {}", + describe_grantee(grantee) ), Change::RevokeDefaultPrivilege { owner, - schema, + scope, on_type, grantee, .. } => format!( - "revoke default privilege for owner \"{owner}\" schema \"{schema}\" on {on_type} from \"{grantee}\"" + "revoke default privilege for owner \"{owner}\" {scope} on {on_type} from {}", + describe_grantee(grantee) ), Change::AddMember { role, member, .. } => { format!("add membership \"{role}\" -> \"{member}\"") @@ -292,9 +303,20 @@ pub(crate) fn describe_change(change: &Change) -> String { } } +/// Name a grantee in prose. +/// +/// PUBLIC is a pseudo-role, so it is neither labelled `role` nor quoted — +/// `"PUBLIC"` would name a real role of that name, which cannot exist. +fn describe_grantee(grantee: &Grantee) -> String { + match grantee { + Grantee::Public => "PUBLIC".to_string(), + Grantee::Role(name) => format!("role \"{name}\""), + } +} + fn format_grant_action( action: &str, - role: &str, + role: &Grantee, object_type: ObjectType, schema: Option<&str>, name: Option<&str>, @@ -305,5 +327,54 @@ fn format_grant_action( (None, Some(name)) => name.to_string(), (None, None) => "".to_string(), }; - format!("{action} for role \"{role}\" on {object_type} \"{target}\"") + format!( + "{action} for {} on {object_type} \"{target}\"", + describe_grantee(role) + ) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::manifest::Privilege; + use std::collections::BTreeSet; + + #[test] + fn public_is_named_as_the_pseudo_role_not_as_a_quoted_role() { + let described = describe_change(&Change::Revoke { + role: Grantee::Public, + privileges: BTreeSet::from([Privilege::Execute]), + object_type: ObjectType::Function, + schema: Some("app".to_string()), + name: Some("f()".to_string()), + }); + assert!(described.contains("for PUBLIC"), "got {described}"); + assert!(!described.contains("\"PUBLIC\""), "got {described}"); + } + + #[test] + fn a_named_grantee_keeps_the_role_label() { + let described = describe_change(&Change::Revoke { + role: Grantee::Role("reader".to_string()), + privileges: BTreeSet::from([Privilege::Select]), + object_type: ObjectType::Table, + schema: Some("app".to_string()), + name: Some("orders".to_string()), + }); + assert!(described.contains("for role \"reader\""), "got {described}"); + } + + #[test] + fn a_global_default_privilege_scope_reads_without_a_doubled_noun() { + let described = describe_change(&Change::SetDefaultPrivilege { + owner: "app_owner".to_string(), + scope: DefaultPrivilegeScope::Global, + on_type: ObjectType::Table, + grantee: Grantee::Public, + privileges: BTreeSet::from([Privilege::Select]), + }); + assert!(described.contains("global scope"), "got {described}"); + assert!(!described.contains("scope scope"), "got {described}"); + assert!(described.contains("to PUBLIC"), "got {described}"); + } } diff --git a/crates/pgroles-core/src/report.rs b/crates/pgroles-core/src/report.rs index 58960f0..e918c19 100644 --- a/crates/pgroles-core/src/report.rs +++ b/crates/pgroles-core/src/report.rs @@ -3,13 +3,15 @@ use thiserror::Error; use crate::diff::Change; use crate::manifest::{ObjectType, SchemaBindingFacet}; -use crate::model::{DefaultPrivKey, GrantKey}; +use crate::model::{DefaultPrivKey, DefaultPrivilegeScope, GrantKey, Grantee}; use crate::ownership::{ ManagedScope, MembershipKey, OwnershipIndex, SchemaFacetKey, describe_change, grant_schema_name, }; use crate::visual::VisualManagedScope; -pub const BUNDLE_PLAN_SCHEMA_VERSION: &str = "pgroles.bundle_plan.v1"; +// v2: default-privilege changes and ownership keys carry a tagged `scope` +// instead of a bare `schema` string, to represent global (owner-wide) scope. +pub const BUNDLE_PLAN_SCHEMA_VERSION: &str = "pgroles.bundle_plan.v2"; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum PlanOutputMode { @@ -83,16 +85,16 @@ pub enum ManagedOwnershipKey { facet: SchemaBindingFacet, }, Grant { - role: String, + role: Grantee, object_type: ObjectType, schema: Option, name: Option, }, DefaultPrivilege { owner: String, - schema: String, + scope: DefaultPrivilegeScope, on_type: ObjectType, - grantee: String, + grantee: Grantee, }, Membership { role: String, @@ -243,11 +245,13 @@ fn lookup_bundle_change_owner( if *object_type == ObjectType::Database { return ownership .roles - .get(role) + .get(role.as_str()) .cloned() .map(|document| BundleChangeOwner { document, - managed_key: ManagedOwnershipKey::Role { name: role.clone() }, + managed_key: ManagedOwnershipKey::Role { + name: role.as_str().to_string(), + }, }) .ok_or_else(|| BundlePlanError::MissingOwner { change: describe_change(change), @@ -267,21 +271,21 @@ fn lookup_bundle_change_owner( } Change::SetDefaultPrivilege { owner, - schema, + scope, on_type, grantee, .. } | Change::RevokeDefaultPrivilege { owner, - schema, + scope, on_type, grantee, .. } => { let key = DefaultPrivKey { owner: owner.clone(), - schema: schema.clone(), + scope: scope.clone(), on_type: *on_type, grantee: grantee.clone(), }; @@ -291,14 +295,35 @@ fn lookup_bundle_change_owner( document: document.clone(), managed_key: ManagedOwnershipKey::DefaultPrivilege { owner: key.owner.clone(), - schema: key.schema.clone(), + scope: key.scope.clone(), on_type: key.on_type, grantee: key.grantee.clone(), }, }); } - lookup_bundle_schema_facet(schema, SchemaBindingFacet::Bindings, ownership, change) + match scope { + DefaultPrivilegeScope::Schema { schema } => lookup_bundle_schema_facet( + schema, + SchemaBindingFacet::Bindings, + ownership, + change, + ), + // Global defaults belong to whichever document owns the role. + DefaultPrivilegeScope::Global => ownership + .roles + .get(owner) + .cloned() + .map(|document| BundleChangeOwner { + document, + managed_key: ManagedOwnershipKey::Role { + name: owner.clone(), + }, + }) + .ok_or_else(|| BundlePlanError::MissingOwner { + change: describe_change(change), + }), + } } Change::AddMember { role, member, .. } | Change::RemoveMember { role, member } => { let key = MembershipKey { @@ -454,6 +479,10 @@ roles: .expect("bundle plan should annotate"); let json = serde_json::to_value(&plan).expect("bundle plan should serialize"); + // Pinned to the literal on purpose: comparing against the constant + // the code writes would let a version bump through without anyone + // documenting the migration. + assert_eq!(json["schema_version"], "pgroles.bundle_plan.v2"); assert_eq!(json["schema_version"], BUNDLE_PLAN_SCHEMA_VERSION); assert_eq!(json["managed_scope"]["roles"][0], "app"); assert_eq!(json["changes"][0]["category"], "role"); diff --git a/crates/pgroles-core/src/sql.rs b/crates/pgroles-core/src/sql.rs index 286e0a8..283071f 100644 --- a/crates/pgroles-core/src/sql.rs +++ b/crates/pgroles-core/src/sql.rs @@ -9,7 +9,7 @@ use std::fmt::Write; use crate::diff::Change; use crate::manifest::{ObjectType, Privilege}; -use crate::model::{RoleAttribute, RoleState}; +use crate::model::{DefaultPrivilegeScope, Grantee, RoleAttribute, RoleState}; // --------------------------------------------------------------------------- // Identifier quoting @@ -103,7 +103,7 @@ pub fn render_statements_with_context(change: &Change, ctx: &SqlContext) -> Vec< owner, privileges, } => render_grant( - owner, + &Grantee::Role(owner.clone()), privileges, ObjectType::Schema, None, @@ -142,18 +142,18 @@ pub fn render_statements_with_context(change: &Change, ctx: &SqlContext) -> Vec< ), Change::SetDefaultPrivilege { owner, - schema, + scope, on_type, grantee, privileges, - } => render_set_default_privilege(owner, schema, *on_type, grantee, privileges), + } => render_set_default_privilege(owner, scope, *on_type, grantee, privileges), Change::RevokeDefaultPrivilege { owner, - schema, + scope, on_type, grantee, privileges, - } => render_revoke_default_privilege(owner, schema, *on_type, grantee, privileges), + } => render_revoke_default_privilege(owner, scope, *on_type, grantee, privileges), Change::AddMember { role, member, @@ -381,8 +381,19 @@ fn render_set_comment(name: &str, comment: &Option) -> Vec { // GRANT / REVOKE // --------------------------------------------------------------------------- +/// Render a grantee for a GRANT/REVOKE subject position. +/// +/// The PUBLIC pseudo-role must stay unquoted: `"PUBLIC"` would name an +/// ordinary role called PUBLIC instead. +pub fn render_grantee(grantee: &Grantee) -> String { + match grantee { + Grantee::Public => "PUBLIC".to_string(), + Grantee::Role(name) => quote_ident(name), + } +} + fn render_grant( - role: &str, + role: &Grantee, privileges: &BTreeSet, object_type: ObjectType, schema: Option<&str>, @@ -402,7 +413,7 @@ fn render_grant( } fn render_revoke( - role: &str, + role: &Grantee, privileges: &BTreeSet, object_type: ObjectType, schema: Option<&str>, @@ -423,7 +434,7 @@ fn render_revoke( fn render_privilege_statements( action: &str, - role: &str, + role: &Grantee, privilege_list: &str, object_type: ObjectType, schema: Option<&str>, @@ -450,14 +461,14 @@ fn render_privilege_statements( let target = format_object_target(object_type, schema, name); vec![format!( "{action} {privilege_list} ON {target} {subject_preposition} {};", - quote_ident(role) + render_grantee(role) )] } fn render_relation_wildcard( action: &str, subject_preposition: &str, - role: &str, + role: &Grantee, privilege_list: &str, object_type: ObjectType, schema: Option<&str>, @@ -476,20 +487,29 @@ fn render_relation_wildcard( "{action} {privilege_list} ON TABLE {}.{} {subject_preposition} {};", quote_ident(schema_name), quote_ident(object_name), - quote_ident(role), + render_grantee(role), ) }) .collect(); } + // Inside the DO-block the grantee is normally a `%I` format argument. + // PUBLIC must instead be embedded literally: `%I` would render it as the + // quoted identifier "PUBLIC", which names an ordinary role. + let (grantee_placeholder, grantee_argument) = match role { + Grantee::Public => ("PUBLIC".to_string(), String::new()), + Grantee::Role(name) => ("%I".to_string(), format!(", {}", quote_literal(name))), + }; + vec![format!( - "DO $pgroles$\nDECLARE obj record;\nBEGIN\n FOR obj IN\n SELECT n.nspname AS schema_name, c.relname AS object_name\n FROM pg_class c\n JOIN pg_namespace n ON n.oid = c.relnamespace\n WHERE c.relkind IN ({})\n AND n.nspname = {}\n ORDER BY c.relname\n LOOP\n EXECUTE format('{} {} ON TABLE %I.%I {} %I;', obj.schema_name, obj.object_name, {});\n END LOOP;\nEND\n$pgroles$;", + "DO $pgroles$\nDECLARE obj record;\nBEGIN\n FOR obj IN\n SELECT n.nspname AS schema_name, c.relname AS object_name\n FROM pg_class c\n JOIN pg_namespace n ON n.oid = c.relnamespace\n WHERE c.relkind IN ({})\n AND n.nspname = {}\n ORDER BY c.relname\n LOOP\n EXECUTE format('{} {} ON TABLE %I.%I {} {};', obj.schema_name, obj.object_name{});\n END LOOP;\nEND\n$pgroles$;", relation_relkinds_sql(object_type), quote_literal(schema_name), action, privilege_list, subject_preposition, - quote_literal(role), + grantee_placeholder, + grantee_argument, )] } @@ -627,41 +647,72 @@ fn format_privileges(privileges: &BTreeSet) -> String { // ALTER DEFAULT PRIVILEGES // --------------------------------------------------------------------------- +/// Map ObjectType to the keyword used in `ALTER DEFAULT PRIVILEGES ... ON`. +/// +/// This differs from [`sql_object_type_plural`]: default privileges support +/// TYPES and SCHEMAS, and there is no `ALL ... IN SCHEMA` restriction here. +/// Manifest validation guarantees SCHEMAS only appears in global scope and +/// DATABASE never appears. +fn default_privilege_object_keyword(on_type: ObjectType) -> &'static str { + match on_type { + ObjectType::Table | ObjectType::View | ObjectType::MaterializedView => "TABLES", + ObjectType::Sequence => "SEQUENCES", + ObjectType::Function => "ROUTINES", + ObjectType::Type => "TYPES", + ObjectType::Schema => "SCHEMAS", + // Manifest validation rejects database default privileges, so this is + // unreachable. Rendering `TABLES` here would silently target the wrong + // objects if that ever changed. + ObjectType::Database => { + unreachable!("default privileges on a database are rejected during manifest validation") + } + } +} + +fn render_default_privilege_scope_clause(scope: &DefaultPrivilegeScope) -> String { + match scope { + DefaultPrivilegeScope::Global => String::new(), + DefaultPrivilegeScope::Schema { schema } => { + format!(" IN SCHEMA {}", quote_ident(schema)) + } + } +} + fn render_set_default_privilege( owner: &str, - schema: &str, + scope: &DefaultPrivilegeScope, on_type: ObjectType, - grantee: &str, + grantee: &Grantee, privileges: &BTreeSet, ) -> Vec { let privilege_list = format_privileges(privileges); - let type_plural = sql_object_type_plural(on_type); + let type_keyword = default_privilege_object_keyword(on_type); vec![format!( - "ALTER DEFAULT PRIVILEGES FOR ROLE {} IN SCHEMA {} GRANT {} ON {} TO {};", + "ALTER DEFAULT PRIVILEGES FOR ROLE {}{} GRANT {} ON {} TO {};", quote_ident(owner), - quote_ident(schema), + render_default_privilege_scope_clause(scope), privilege_list, - type_plural, - quote_ident(grantee) + type_keyword, + render_grantee(grantee) )] } fn render_revoke_default_privilege( owner: &str, - schema: &str, + scope: &DefaultPrivilegeScope, on_type: ObjectType, - grantee: &str, + grantee: &Grantee, privileges: &BTreeSet, ) -> Vec { let privilege_list = format_privileges(privileges); - let type_plural = sql_object_type_plural(on_type); + let type_keyword = default_privilege_object_keyword(on_type); vec![format!( - "ALTER DEFAULT PRIVILEGES FOR ROLE {} IN SCHEMA {} REVOKE {} ON {} FROM {};", + "ALTER DEFAULT PRIVILEGES FOR ROLE {}{} REVOKE {} ON {} FROM {};", quote_ident(owner), - quote_ident(schema), + render_default_privilege_scope_clause(scope), privilege_list, - type_plural, - quote_ident(grantee) + type_keyword, + render_grantee(grantee) )] } @@ -994,7 +1045,7 @@ mod tests { #[test] fn render_grant_schema_usage() { let change = Change::Grant { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), privileges: BTreeSet::from([Privilege::Usage]), object_type: ObjectType::Schema, schema: None, @@ -1010,7 +1061,7 @@ mod tests { #[test] fn render_grant_all_tables() { let change = Change::Grant { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), privileges: BTreeSet::from([Privilege::Select, Privilege::Insert]), object_type: ObjectType::Table, schema: Some("inventory".to_string()), @@ -1033,7 +1084,7 @@ mod tests { #[test] fn render_grant_specific_table() { let change = Change::Grant { - role: "r1".to_string(), + role: "r1".into(), privileges: BTreeSet::from([Privilege::Select]), object_type: ObjectType::Table, schema: Some("public".to_string()), @@ -1046,7 +1097,7 @@ mod tests { #[test] fn render_grant_specific_function() { let change = Change::Grant { - role: "r1".to_string(), + role: "r1".into(), privileges: BTreeSet::from([Privilege::Execute]), object_type: ObjectType::Function, schema: Some("public".to_string()), @@ -1062,7 +1113,7 @@ mod tests { #[test] fn render_revoke_specific_routine_for_function_object() { let change = Change::Revoke { - role: "r1".to_string(), + role: "r1".into(), privileges: BTreeSet::from([Privilege::Execute]), object_type: ObjectType::Function, schema: Some("public".to_string()), @@ -1078,7 +1129,7 @@ mod tests { #[test] fn render_grant_all_routines_for_function_wildcard() { let change = Change::Grant { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), privileges: BTreeSet::from([Privilege::Execute]), object_type: ObjectType::Function, schema: Some("inventory".to_string()), @@ -1094,7 +1145,7 @@ mod tests { #[test] fn render_revoke_all_sequences() { let change = Change::Revoke { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), privileges: BTreeSet::from([Privilege::Usage, Privilege::Select]), object_type: ObjectType::Sequence, schema: Some("inventory".to_string()), @@ -1111,9 +1162,11 @@ mod tests { fn render_set_default_privilege() { let change = Change::SetDefaultPrivilege { owner: "app_owner".to_string(), - schema: "inventory".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "inventory".to_string(), + }, on_type: ObjectType::Table, - grantee: "inventory-editor".to_string(), + grantee: "inventory-editor".into(), privileges: BTreeSet::from([Privilege::Select, Privilege::Insert]), }; let sql = render(&change); @@ -1127,9 +1180,11 @@ mod tests { fn render_revoke_default_privilege() { let change = Change::RevokeDefaultPrivilege { owner: "app_owner".to_string(), - schema: "inventory".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "inventory".to_string(), + }, on_type: ObjectType::Function, - grantee: "inventory-editor".to_string(), + grantee: "inventory-editor".into(), privileges: BTreeSet::from([Privilege::Execute]), }; let sql = render(&change); @@ -1311,7 +1366,7 @@ mod tests { vec!["daily_sales".to_string(), "weekly_sales".to_string()], )])); let change = Change::Revoke { - role: "analytics".to_string(), + role: "analytics".into(), privileges: [Privilege::Select].into_iter().collect(), object_type: ObjectType::MaterializedView, schema: Some("reporting".to_string()), @@ -1333,7 +1388,7 @@ mod tests { #[test] fn render_materialized_view_wildcard_without_inventory_uses_catalog_loop() { let change = Change::Revoke { - role: "analytics".to_string(), + role: "analytics".into(), privileges: [Privilege::Select].into_iter().collect(), object_type: ObjectType::MaterializedView, schema: Some("reporting".to_string()), @@ -1525,4 +1580,127 @@ memberships: let sql = render(&change); assert_eq!(sql, "ALTER ROLE \"r1\" VALID UNTIL 'infinity';"); } + + // ----------------------------------------------------------------------- + // PUBLIC grantee and global default privileges + // ----------------------------------------------------------------------- + + #[test] + fn public_renders_unquoted_but_a_role_named_public_does_not() { + let revoke_public = render(&Change::Revoke { + role: Grantee::Public, + privileges: [Privilege::Execute].into_iter().collect(), + object_type: ObjectType::Function, + schema: Some("api".to_string()), + name: Some("f()".to_string()), + }); + assert_eq!( + revoke_public, + r#"REVOKE EXECUTE ON ROUTINE "api"."f"() FROM PUBLIC;"# + ); + + let grant_public = render(&Change::Grant { + role: Grantee::Public, + privileges: [Privilege::Usage].into_iter().collect(), + object_type: ObjectType::Schema, + schema: None, + name: Some("api".to_string()), + }); + assert_eq!(grant_public, r#"GRANT USAGE ON SCHEMA "api" TO PUBLIC;"#); + + // A real role that merely looks like the keyword stays quoted. + let lowercase = render(&Change::Grant { + role: "public".into(), + privileges: [Privilege::Usage].into_iter().collect(), + object_type: ObjectType::Schema, + schema: None, + name: Some("api".to_string()), + }); + assert_eq!(lowercase, r#"GRANT USAGE ON SCHEMA "api" TO "public";"#); + } + + #[test] + fn public_wildcard_revoke_uses_all_routines_in_schema() { + assert_eq!( + render(&Change::Revoke { + role: Grantee::Public, + privileges: [Privilege::Execute].into_iter().collect(), + object_type: ObjectType::Function, + schema: Some("api".to_string()), + name: Some("*".to_string()), + }), + r#"REVOKE EXECUTE ON ALL ROUTINES IN SCHEMA "api" FROM PUBLIC;"# + ); + } + + #[test] + fn relation_wildcard_do_block_embeds_public_literally() { + // `%I` would render the keyword as the quoted identifier "PUBLIC", + // which names an ordinary role. + let sql = render(&Change::Revoke { + role: Grantee::Public, + privileges: [Privilege::Select].into_iter().collect(), + object_type: ObjectType::Table, + schema: Some("app".to_string()), + name: Some("*".to_string()), + }); + assert!(sql.contains("FROM PUBLIC;'"), "{sql}"); + assert!(!sql.contains("FROM %I"), "{sql}"); + + // Ordinary roles still go through the %I parameter. + let role_sql = render(&Change::Revoke { + role: "reader".into(), + privileges: [Privilege::Select].into_iter().collect(), + object_type: ObjectType::Table, + schema: Some("app".to_string()), + name: Some("*".to_string()), + }); + assert!(role_sql.contains("FROM %I"), "{role_sql}"); + assert!(role_sql.contains("'reader'"), "{role_sql}"); + } + + #[test] + fn global_default_privileges_render_without_in_schema() { + assert_eq!( + render(&Change::RevokeDefaultPrivilege { + owner: "function_owner".to_string(), + scope: DefaultPrivilegeScope::Global, + on_type: ObjectType::Function, + grantee: Grantee::Public, + privileges: [Privilege::Execute].into_iter().collect(), + }), + r#"ALTER DEFAULT PRIVILEGES FOR ROLE "function_owner" REVOKE EXECUTE ON ROUTINES FROM PUBLIC;"# + ); + + assert_eq!( + render(&Change::SetDefaultPrivilege { + owner: "app_owner".to_string(), + scope: DefaultPrivilegeScope::Schema { + schema: "app".to_string() + }, + on_type: ObjectType::Function, + grantee: "reader".into(), + privileges: [Privilege::Execute].into_iter().collect(), + }), + r#"ALTER DEFAULT PRIVILEGES FOR ROLE "app_owner" IN SCHEMA "app" GRANT EXECUTE ON ROUTINES TO "reader";"# + ); + } + + #[test] + fn default_privilege_object_keywords_cover_types_and_schemas() { + let keyword_for = |on_type| { + render(&Change::SetDefaultPrivilege { + owner: "o".to_string(), + scope: DefaultPrivilegeScope::Global, + on_type, + grantee: "r".into(), + privileges: [Privilege::Usage].into_iter().collect(), + }) + }; + assert!(keyword_for(ObjectType::Type).contains("ON TYPES")); + assert!(keyword_for(ObjectType::Schema).contains("ON SCHEMAS")); + assert!(keyword_for(ObjectType::Table).contains("ON TABLES")); + assert!(keyword_for(ObjectType::Sequence).contains("ON SEQUENCES")); + assert!(keyword_for(ObjectType::Function).contains("ON ROUTINES")); + } } diff --git a/crates/pgroles-core/src/suggest.rs b/crates/pgroles-core/src/suggest.rs index c0fec6d..1f678a0 100644 --- a/crates/pgroles-core/src/suggest.rs +++ b/crates/pgroles-core/src/suggest.rs @@ -201,6 +201,15 @@ pub fn suggest_profiles(input: &PolicyManifest, opts: &SuggestOptions) -> Sugges let mut role_dps: BTreeMap> = BTreeMap::new(); for dp in &input.default_privileges { + // Profiles are schema-scoped, so only schema-scoped entries can be + // folded into one; global entries stay as-is. + let Some(schema) = dp + .resolved_scope() + .ok() + .and_then(|scope| scope.schema().map(str::to_string)) + else { + continue; + }; let owner = dp .owner .clone() @@ -210,7 +219,7 @@ pub fn suggest_profiles(input: &PolicyManifest, opts: &SuggestOptions) -> Sugges if let Some(role) = &grant.role { role_dps.entry(role.clone()).or_default().push(( owner.clone(), - dp.schema.clone(), + schema.clone(), grant.clone(), )); } @@ -293,6 +302,14 @@ pub fn suggest_profiles(input: &PolicyManifest, opts: &SuggestOptions) -> Sugges let mut has_unrepresentable_grant = false; let role_grants_vec = role_grants.get(role_name).cloned().unwrap_or_default(); for g in &role_grants_vec { + // Profiles are additive templates with no way to say `ensure: + // absent`, and a global default privilege has no schema to hang a + // profile from. Either one makes the role unclusterable; folding + // it anyway would drop the assertion and fail the round-trip + // check, abandoning the whole suggestion run. + if g.ensure == crate::manifest::Ensure::Absent { + has_unrepresentable_grant = true; + } match g.object.object_type { ObjectType::Schema => match &g.object.name { Some(name) => { @@ -315,6 +332,13 @@ pub fn suggest_profiles(input: &PolicyManifest, opts: &SuggestOptions) -> Sugges }); continue; } + if role_has_global_default_privilege(input, role_name) { + skipped.push(SkipReason::UnrepresentableGrant { + role: role_name.clone(), + }); + continue; + } + let role_dp_vec = role_dps.get(role_name).cloned().unwrap_or_default(); for (_, schema, _) in &role_dp_vec { schemas_seen.insert(schema.clone()); @@ -607,6 +631,7 @@ pub fn suggest_profiles(input: &PolicyManifest, opts: &SuggestOptions) -> Sugges Some(DefaultPrivilege { owner: dp.owner.clone(), schema: dp.schema.clone(), + scope: dp.scope.clone(), grant: kept, }) } @@ -814,6 +839,7 @@ fn collapse_full_coverage_grants(grants: &mut Vec, inventory: &Inventory) to_add.push(Grant { role, privileges: first_privs.into_iter().collect(), + ensure: crate::manifest::Ensure::Present, object: ObjectTarget { object_type, schema: Some(schema), @@ -981,6 +1007,21 @@ fn is_valid_identifier(s: &str) -> bool { && !s.starts_with('_') } +/// Whether any global-scope default privilege names this role as grantee. +/// +/// Global scope has no schema, so a profile cannot express it. `role_dps` +/// deliberately skips these entries, which would otherwise be silently lost. +fn role_has_global_default_privilege(input: &PolicyManifest, role: &str) -> bool { + input.default_privileges.iter().any(|dp| { + dp.resolved_scope() + .is_ok_and(|scope| scope.schema().is_none()) + && dp + .grant + .iter() + .any(|grant| grant.role.as_deref() == Some(role)) + }) +} + fn build_profile( login: Option, inherit: Option, @@ -1036,6 +1077,7 @@ fn build_profile( role: None, // expansion fills this in privileges: privs, on_type: dpg.on_type, + ensure: crate::manifest::Ensure::Present, } }) .collect(); @@ -1115,6 +1157,7 @@ fn expand_wildcards_in_place(grants: &mut Vec, inventory: &Inventory) { out.push(Grant { role: g.role.clone(), privileges: g.privileges.clone(), + ensure: g.ensure, object: ObjectTarget { object_type: g.object.object_type, schema: g.object.schema.clone(), diff --git a/crates/pgroles-core/src/visual.rs b/crates/pgroles-core/src/visual.rs index f316558..5a8967a 100644 --- a/crates/pgroles-core/src/visual.rs +++ b/crates/pgroles-core/src/visual.rs @@ -9,7 +9,7 @@ use std::fmt::Write as _; use serde::{Deserialize, Serialize}; use crate::manifest::{ObjectType, Privilege}; -use crate::model::{DefaultPrivKey, GrantKey, RoleGraph}; +use crate::model::{DefaultPrivKey, GrantKey, PUBLIC_ROLE, RoleGraph}; use crate::ownership::ManagedScope; pub const VISUAL_GRAPH_SCHEMA_VERSION: &str = "pgroles.visual_graph.v1"; @@ -173,6 +173,29 @@ pub fn build_visual_graph(graph: &RoleGraph, source: VisualSource) -> VisualGrap } } + // --- PUBLIC pseudo-role node --- + // Only emitted when something actually addresses PUBLIC, so graphs that + // never mention it are unchanged. + let grants_public = graph.grants.keys().any(|key| key.role.is_public()); + let defaults_public = graph + .default_privileges + .keys() + .any(|key| key.grantee.is_public()); + if grants_public || defaults_public { + let node_id = grantee_node_id(PUBLIC_ROLE); + if node_ids.insert(node_id.clone()) { + nodes.push(VisualNode { + id: node_id, + label: PUBLIC_ROLE.to_string(), + kind: NodeKind::ExternalPrincipal, + managed: Some(false), + login: None, + privileges: Vec::new(), + comment: None, + }); + } + } + // --- Membership edges --- for edge in &graph.memberships { let source_id = if managed_role_names.contains(edge.member.as_str()) { @@ -224,7 +247,7 @@ pub fn build_visual_graph(graph: &RoleGraph, source: VisualSource) -> VisualGrap .collect::>() .join(","); edges.push(VisualEdge { - source: format!("role:{}", collapsed_key.role), + source: grantee_node_id(&collapsed_key.role), target: node_id, kind: EdgeKind::Grant, label: privilege_label, @@ -234,7 +257,11 @@ pub fn build_visual_graph(graph: &RoleGraph, source: VisualSource) -> VisualGrap // --- Default privilege nodes and edges --- for (key, state) in &graph.default_privileges { let node_id = default_priv_node_id(key); - let node_label = format!("defaults: {} -> {}.{}s", key.owner, key.schema, key.on_type); + let scope_label = key.scope.schema().unwrap_or("(global)"); + let node_label = format!( + "defaults: {} -> {}.{}s", + key.owner, scope_label, key.on_type + ); if node_ids.insert(node_id.clone()) { nodes.push(VisualNode { @@ -256,7 +283,7 @@ pub fn build_visual_graph(graph: &RoleGraph, source: VisualSource) -> VisualGrap .join(","); edges.push(VisualEdge { source: node_id, - target: format!("role:{}", key.grantee), + target: grantee_node_id(key.grantee.as_str()), kind: EdgeKind::DefaultPrivilege, label: privilege_label, }); @@ -314,6 +341,19 @@ impl CollapsedGrantKey { } } +/// Node id for the grantee of a grant or default-privilege edge. +/// +/// PUBLIC is a pseudo-role, so it never appears in `graph.roles` and a +/// `role:` id would dangle. It gets an unmanaged node instead, the same way +/// membership members that name no managed role do. +fn grantee_node_id(grantee: &str) -> String { + if grantee == PUBLIC_ROLE { + format!("external:{grantee}") + } else { + format!("role:{grantee}") + } +} + fn collapse_grants( grants: &BTreeMap, ) -> BTreeMap> { @@ -335,7 +375,7 @@ fn collapse_grants( }; let collapsed_key = CollapsedGrantKey { - role: key.role.clone(), + role: key.role.as_str().to_string(), object_type: key.object_type, scope, }; @@ -352,7 +392,10 @@ fn collapse_grants( fn default_priv_node_id(key: &DefaultPrivKey) -> String { format!( "default:{}:{}:{}:{}", - key.owner, key.schema, key.on_type, key.grantee + key.owner, + key.scope.schema().unwrap_or("(global)"), + key.on_type, + key.grantee ) } @@ -862,6 +905,74 @@ memberships: assert!(dp_nodes[0].label.contains("orders")); } + fn build_public_graph() -> RoleGraph { + let yaml = r#" +default_owner: app_owner + +schemas: + - name: orders + +roles: + - name: analytics + login: true + +grants: + - role: PUBLIC + privileges: [USAGE] + object: { type: schema, name: orders } + +default_privileges: + - owner: app_owner + scope: { type: global } + grant: + - role: PUBLIC + privileges: [EXECUTE] + on_type: function +"#; + let manifest = parse_manifest(yaml).unwrap(); + let expanded = expand_manifest(&manifest).unwrap(); + RoleGraph::from_expanded(&expanded, manifest.default_owner.as_deref()).unwrap() + } + + #[test] + fn every_edge_endpoint_resolves_to_a_node() { + let visual = build_visual_graph(&build_public_graph(), VisualSource::Desired); + let ids: BTreeSet<&str> = visual.nodes.iter().map(|n| n.id.as_str()).collect(); + for edge in &visual.edges { + assert!( + ids.contains(edge.source.as_str()), + "edge source {} has no node (ids: {ids:?})", + edge.source + ); + assert!( + ids.contains(edge.target.as_str()), + "edge target {} has no node (ids: {ids:?})", + edge.target + ); + } + } + + #[test] + fn public_gets_one_unmanaged_node() { + let visual = build_visual_graph(&build_public_graph(), VisualSource::Desired); + let public: Vec<_> = visual + .nodes + .iter() + .filter(|n| n.label == "PUBLIC") + .collect(); + assert_eq!(public.len(), 1, "expected exactly one PUBLIC node"); + assert_eq!(public[0].managed, Some(false)); + assert_eq!(public[0].kind, NodeKind::ExternalPrincipal); + // PUBLIC is not a managed role, so it must not claim a `role:` id. + assert!(!visual.nodes.iter().any(|n| n.id == "role:PUBLIC")); + } + + #[test] + fn a_graph_without_public_gains_no_public_node() { + let visual = build_visual_graph(&build_test_graph(), VisualSource::Desired); + assert!(!visual.nodes.iter().any(|n| n.label == "PUBLIC")); + } + #[test] fn visual_graph_nodes_are_sorted() { let graph = build_test_graph(); diff --git a/crates/pgroles-core/tests/approval_property.rs b/crates/pgroles-core/tests/approval_property.rs index 50ebd7f..2854827 100644 --- a/crates/pgroles-core/tests/approval_property.rs +++ b/crates/pgroles-core/tests/approval_property.rs @@ -26,7 +26,7 @@ use pgroles_core::approval::{ }; use pgroles_core::diff::{Change, ReconciliationMode}; use pgroles_core::manifest::{ObjectType, Privilege}; -use pgroles_core::model::{RoleAttribute, RoleState}; +use pgroles_core::model::{DefaultPrivilegeScope, Grantee, RoleAttribute, RoleState}; const CASES: usize = 400; @@ -90,6 +90,26 @@ fn privileges(rng: &mut Rng) -> BTreeSet { (0..count).map(|_| ALL[rng.usize(ALL.len())]).collect() } +fn grantee(rng: &mut Rng) -> Grantee { + // PUBLIC is a legal grantee everywhere a role name is, so the digest + // properties have to hold for it too. + if rng.usize(8) == 0 { + Grantee::Public + } else { + Grantee::Role(role(rng)) + } +} + +fn default_privilege_scope(rng: &mut Rng) -> DefaultPrivilegeScope { + if rng.bool() { + DefaultPrivilegeScope::Global + } else { + DefaultPrivilegeScope::Schema { + schema: SCHEMAS[rng.usize(SCHEMAS.len())].to_string(), + } + } +} + fn object_type(rng: &mut Rng) -> ObjectType { const ALL: &[ObjectType] = &[ ObjectType::Table, @@ -145,14 +165,14 @@ fn change(rng: &mut Rng) -> Change { }, }, 5 => Change::Grant { - role: role(rng), + role: grantee(rng), privileges: privileges(rng), object_type: object_type(rng), schema: Some(SCHEMAS[rng.usize(SCHEMAS.len())].to_string()), name: Some(OBJECTS[rng.usize(OBJECTS.len())].to_string()), }, 6 => Change::Revoke { - role: role(rng), + role: grantee(rng), privileges: privileges(rng), object_type: object_type(rng), schema: Some(SCHEMAS[rng.usize(SCHEMAS.len())].to_string()), @@ -160,16 +180,16 @@ fn change(rng: &mut Rng) -> Change { }, 7 => Change::SetDefaultPrivilege { owner: role(rng), - schema: SCHEMAS[rng.usize(SCHEMAS.len())].to_string(), + scope: default_privilege_scope(rng), on_type: object_type(rng), - grantee: role(rng), + grantee: grantee(rng), privileges: privileges(rng), }, 8 => Change::RevokeDefaultPrivilege { owner: role(rng), - schema: SCHEMAS[rng.usize(SCHEMAS.len())].to_string(), + scope: default_privilege_scope(rng), on_type: object_type(rng), - grantee: role(rng), + grantee: grantee(rng), privileges: privileges(rng), }, 9 => Change::AddMember { diff --git a/crates/pgroles-core/tests/diff_property.rs b/crates/pgroles-core/tests/diff_property.rs index ec26d6a..9b575de 100644 --- a/crates/pgroles-core/tests/diff_property.rs +++ b/crates/pgroles-core/tests/diff_property.rs @@ -55,8 +55,9 @@ use std::collections::{BTreeMap, BTreeSet}; use pgroles_core::diff::{Change, ReconciliationMode, diff, filter_changes}; use pgroles_core::manifest::{ObjectType, Privilege}; use pgroles_core::model::{ - DefaultPrivKey, DefaultPrivState, GrantKey, GrantState, MembershipEdge, RoleAttribute, - RoleGraph, RoleState, SchemaState, default_schema_owner_privileges, + DefaultPrivKey, DefaultPrivState, DefaultPrivilegeScope, GrantKey, GrantState, Grantee, + MembershipEdge, RoleAttribute, RoleGraph, RoleState, SchemaState, + default_schema_owner_privileges, }; // --------------------------------------------------------------------------- @@ -220,7 +221,7 @@ fn gen_grants(rng: &mut Rng, roles: &[String]) -> BTreeMap }; out.insert( GrantKey { - role, + role: Grantee::Role(role), object_type, schema, name, @@ -251,9 +252,11 @@ fn gen_default_privs( out.insert( DefaultPrivKey { owner: format!("own{}", rng.usize(3)), - schema: format!("s{}", rng.usize(3)), + scope: DefaultPrivilegeScope::Schema { + schema: format!("s{}", rng.usize(3)), + }, on_type, - grantee: roles[rng.usize(roles.len())].clone(), + grantee: Grantee::Role(roles[rng.usize(roles.len())].clone()), }, DefaultPrivState { privileges: gen_priv_set(rng), @@ -306,7 +309,9 @@ fn gen_graph(rng: &mut Rng, allow_none: bool, messy: bool) -> RoleGraph { roles, schemas: gen_schemas(rng, allow_none), grants: gen_grants(rng, &role_names), + grant_absences: BTreeMap::new(), default_privileges: gen_default_privs(rng, &role_names), + default_privilege_absences: BTreeMap::new(), memberships: gen_memberships(rng, &role_names), } } @@ -399,7 +404,7 @@ fn derive_current(rng: &mut Rng, desired: &RoleGraph) -> RoleGraph { for _ in 0..rng.usize(3) { c.grants.insert( GrantKey { - role: format!("r{}", rng.usize(6)), + role: Grantee::Role(format!("r{}", rng.usize(6))), object_type: ObjectType::Table, schema: Some(format!("s{}", rng.usize(3))), name: Some(format!("stray{}", rng.usize(3))), @@ -436,9 +441,11 @@ fn derive_current(rng: &mut Rng, desired: &RoleGraph) -> RoleGraph { c.default_privileges.insert( DefaultPrivKey { owner: format!("own{}", rng.usize(3)), - schema: format!("s{}", rng.usize(3)), + scope: DefaultPrivilegeScope::Schema { + schema: format!("s{}", rng.usize(3)), + }, on_type: ObjectType::Table, - grantee: format!("r{}", rng.usize(6)), + grantee: Grantee::Role(format!("r{}", rng.usize(6))), }, DefaultPrivState { privileges: [Privilege::Select].into_iter().collect(), @@ -583,7 +590,7 @@ fn apply_changes(graph: &RoleGraph, changes: &[Change]) -> RoleGraph { state.owner_privileges = [Privilege::Create, Privilege::Usage].into_iter().collect(); g.grants.remove(&GrantKey { - role: owner.clone(), + role: Grantee::Role(owner.clone()), object_type: ObjectType::Schema, schema: None, name: Some(name.clone()), @@ -626,36 +633,53 @@ fn apply_changes(graph: &RoleGraph, changes: &[Change]) -> RoleGraph { schema, name, } => { - let key = GrantKey { - role: role.clone(), - object_type: *object_type, - schema: schema.clone(), - name: name.clone(), - }; - let now_empty = if let Some(entry) = g.grants.get_mut(&key) { - for p in privileges { - entry.privileges.remove(p); - } - entry.privileges.is_empty() + // `REVOKE ... ON ALL ... IN SCHEMA` reaches every object of + // that type in the schema, not one key. + let affected: Vec = if name.as_deref() == Some("*") { + g.grants + .keys() + .filter(|key| { + key.role == *role + && key.object_type == *object_type + && key.schema == *schema + }) + .cloned() + .collect() } else { - false + vec![GrantKey { + role: role.clone(), + object_type: *object_type, + schema: schema.clone(), + name: name.clone(), + }] }; - // An emptied grant is indistinguishable from "no grant" in the - // model (from_expanded only ever inserts non-empty entries). - if now_empty { - g.grants.remove(&key); + + for key in affected { + let now_empty = if let Some(entry) = g.grants.get_mut(&key) { + for p in privileges { + entry.privileges.remove(p); + } + entry.privileges.is_empty() + } else { + false + }; + // An emptied grant is indistinguishable from "no grant" in + // the model (from_expanded only inserts non-empty entries). + if now_empty { + g.grants.remove(&key); + } } } Change::SetDefaultPrivilege { owner, - schema, + scope, on_type, grantee, privileges, } => { let key = DefaultPrivKey { owner: owner.clone(), - schema: schema.clone(), + scope: scope.clone(), on_type: *on_type, grantee: grantee.clone(), }; @@ -671,14 +695,14 @@ fn apply_changes(graph: &RoleGraph, changes: &[Change]) -> RoleGraph { } Change::RevokeDefaultPrivilege { owner, - schema, + scope, on_type, grantee, privileges, } => { let key = DefaultPrivKey { owner: owner.clone(), - schema: schema.clone(), + scope: scope.clone(), on_type: *on_type, grantee: grantee.clone(), }; @@ -919,3 +943,313 @@ fn additive_mode_soundness() { } } } + +// --------------------------------------------------------------------------- +// Property 6: absence assertions and the PUBLIC exemption +// --------------------------------------------------------------------------- +// +// The equality-based properties above cannot express these semantics: a +// converged graph legitimately keeps PUBLIC privileges no rule mentions, so it +// is not equal to `desired`. The invariants that do hold are checked directly. +// +// Objects are generated across several schemas and object types on purpose. A +// wildcard absence range-scans current grants by `(grantee, object_type, +// schema)` prefix, so a prefix mistake would revoke across a neighbouring +// schema or type and fail `unmanaged_public_survives` or `present_preserved`. + +/// A deliberately tiny privilege alphabet. Absence assertions only bite where +/// the same privilege appears in several scopes, so a wide pool would make the +/// interesting collisions vanishingly rare and the range-scan prefix would go +/// untested. +fn gen_narrow_priv_set(rng: &mut Rng) -> BTreeSet { + let pool = [Privilege::Select, Privilege::Execute]; + let mut out = BTreeSet::new(); + out.insert(pool[rng.usize(pool.len())]); + if rng.usize(3) == 0 { + out.insert(pool[rng.usize(pool.len())]); + } + out +} + +/// Every object coordinate the absence generators draw from. +fn absence_universe() -> Vec<(ObjectType, Option, String)> { + let mut out = Vec::new(); + for schema in ["s0", "s1"] { + for name in ["a", "b"] { + out.push(( + ObjectType::Function, + Some(schema.to_string()), + format!("fn_{name}"), + )); + out.push(( + ObjectType::Table, + Some(schema.to_string()), + format!("t_{name}"), + )); + } + } + out +} + +fn gen_absence_pair(rng: &mut Rng) -> (RoleGraph, RoleGraph) { + let universe = absence_universe(); + let grantees = [ + Grantee::Public, + Grantee::Role("r0".to_string()), + Grantee::Role("r1".to_string()), + ]; + + // --- current: live ACLs, PUBLIC and roles alike --- + let mut current = RoleGraph::default(); + for (object_type, schema, name) in &universe { + for grantee in &grantees { + if rng.usize(3) == 0 { + continue; + } + current.grants.insert( + GrantKey { + role: grantee.clone(), + object_type: *object_type, + schema: schema.clone(), + name: Some(name.clone()), + }, + GrantState { + privileges: gen_narrow_priv_set(rng), + }, + ); + } + } + + // --- desired: present grants plus absence assertions --- + let mut desired = RoleGraph::default(); + for (object_type, schema, name) in &universe { + for grantee in &grantees { + if rng.usize(4) != 0 { + continue; + } + desired.grants.insert( + GrantKey { + role: grantee.clone(), + object_type: *object_type, + schema: schema.clone(), + name: Some(name.clone()), + }, + GrantState { + privileges: gen_narrow_priv_set(rng), + }, + ); + } + } + + // Absences, exact and wildcard. A real manifest can never assert the same + // key+privilege both ways (validation rejects it), and a wildcard may not + // disagree with a named selector in the same scope — mirror both so the + // generated pair stays reachable from valid YAML. + let absence_count = rng.usize(5); + for _ in 0..absence_count { + let grantee = grantees[rng.usize(grantees.len())].clone(); + let (object_type, schema, name) = &universe[rng.usize(universe.len())]; + let wildcard = rng.usize(2) == 0; + let key = GrantKey { + role: grantee.clone(), + object_type: *object_type, + schema: schema.clone(), + name: Some(if wildcard { + "*".to_string() + } else { + name.clone() + }), + }; + + // Privileges already claimed present anywhere in this scope are off + // limits for an absence assertion. + let claimed: BTreeSet = desired + .grants + .iter() + .filter(|(k, _)| { + k.role == grantee && k.object_type == *object_type && k.schema == *schema + }) + .flat_map(|(_, state)| state.privileges.iter().copied()) + .collect(); + let absent: BTreeSet = gen_narrow_priv_set(rng) + .into_iter() + .filter(|p| !claimed.contains(p)) + .collect(); + if absent.is_empty() { + continue; + } + desired + .grant_absences + .entry(key) + .or_default() + .extend(absent); + } + + // Default-privilege absences, in both scopes. + let dp_count = rng.usize(3); + for _ in 0..dp_count { + let grantee = grantees[rng.usize(grantees.len())].clone(); + let scope = if rng.usize(2) == 0 { + DefaultPrivilegeScope::Global + } else { + DefaultPrivilegeScope::Schema { + schema: format!("s{}", rng.usize(2)), + } + }; + let key = DefaultPrivKey { + owner: format!("r{}", rng.usize(2)), + scope, + on_type: ObjectType::Function, + grantee, + }; + if desired.default_privileges.contains_key(&key) { + continue; + } + let privileges = gen_narrow_priv_set(rng); + if privileges.is_empty() { + continue; + } + if rng.usize(2) == 0 { + current.default_privileges.insert( + key.clone(), + DefaultPrivState { + privileges: gen_narrow_priv_set(rng), + }, + ); + } + desired.default_privilege_absences.insert(key, privileges); + } + + (current, desired) +} + +#[test] +fn absence_assertions_converge_and_leave_unmanaged_public_alone() { + let mut outer = Rng::new(0xAB5E_17CE); + for _ in 0..ITERATIONS { + let seed = outer.next_u64(); + let mut rng = Rng::new(seed); + let (current, desired) = gen_absence_pair(&mut rng); + + let changes = diff(¤t, &desired); + let converged = apply_changes(¤t, &changes); + + // 1. Every asserted absence holds afterwards. + for (key, absent) in &desired.grant_absences { + let held: BTreeSet = if key.name.as_deref() == Some("*") { + converged + .grants + .iter() + .filter(|(k, _)| { + k.role == key.role + && k.object_type == key.object_type + && k.schema == key.schema + }) + .flat_map(|(_, state)| state.privileges.iter().copied()) + .collect() + } else { + converged + .grants + .get(key) + .map(|state| state.privileges.clone()) + .unwrap_or_default() + }; + let still_held: Vec<&Privilege> = absent.intersection(&held).collect(); + assert!( + still_held.is_empty(), + "seed {seed}: absence not satisfied for {key:?}: {still_held:?}\n\ + --- CURRENT ---\n{current:#?}\n--- CHANGES ---\n{changes:#?}" + ); + } + for (key, absent) in &desired.default_privilege_absences { + if let Some(state) = converged.default_privileges.get(key) { + let still_held: Vec<&Privilege> = absent.intersection(&state.privileges).collect(); + assert!( + still_held.is_empty(), + "seed {seed}: default-privilege absence not satisfied for {key:?}: {still_held:?}" + ); + } + } + + // 2. Everything asserted present is present. + for (key, want) in &desired.grants { + let got = converged + .grants + .get(key) + .map(|state| state.privileges.clone()) + .unwrap_or_default(); + let missing: Vec<&Privilege> = want.privileges.difference(&got).collect(); + assert!( + missing.is_empty(), + "seed {seed}: present grant not satisfied for {key:?}: missing {missing:?}" + ); + } + + // 3. PUBLIC privileges no rule names survive. This is the property + // that keeps authoritative mode from stripping ACLs pgroles was + // never told about, and the one a bad range-scan prefix breaks. + for (key, state) in ¤t.grants { + if !key.role.is_public() { + continue; + } + for privilege in &state.privileges { + let named_absent = desired.grant_absences.iter().any(|(k, absent)| { + k.role == key.role + && k.object_type == key.object_type + && k.schema == key.schema + && (k.name == key.name || k.name.as_deref() == Some("*")) + && absent.contains(privilege) + }); + if named_absent { + continue; + } + assert!( + converged + .grants + .get(key) + .is_some_and(|state| state.privileges.contains(privilege)), + "seed {seed}: unmanaged PUBLIC {privilege} on {key:?} was revoked\n\ + --- CHANGES ---\n{changes:#?}" + ); + } + } + + // 4. Idempotence. + let residual = diff(&converged, &desired); + assert!( + residual.is_empty(), + "seed {seed}: not idempotent, residual: {residual:#?}\n\ + --- CURRENT ---\n{current:#?}\n--- CHANGES ---\n{changes:#?}" + ); + } +} + +#[test] +fn absence_assertions_are_ignored_in_additive_mode() { + let mut outer = Rng::new(0xADD1_71FE); + for _ in 0..ITERATIONS { + let seed = outer.next_u64(); + let mut rng = Rng::new(seed); + let (current, desired) = gen_absence_pair(&mut rng); + + let additive = pgroles_core::diff::filter_changes( + diff(¤t, &desired), + pgroles_core::diff::ReconciliationMode::Additive, + ); + let converged = apply_changes(¤t, &additive); + + // Additive never revokes, so nothing loses a privilege it held. + for (key, state) in ¤t.grants { + let after = converged + .grants + .get(key) + .map(|s| s.privileges.clone()) + .unwrap_or_default(); + let lost: Vec<&Privilege> = state.privileges.difference(&after).collect(); + assert!( + lost.is_empty(), + "seed {seed}: additive mode revoked {lost:?} from {key:?}" + ); + } + } +} diff --git a/crates/pgroles-core/tests/suggest_property.rs b/crates/pgroles-core/tests/suggest_property.rs index fdd308a..85153ed 100644 --- a/crates/pgroles-core/tests/suggest_property.rs +++ b/crates/pgroles-core/tests/suggest_property.rs @@ -11,8 +11,8 @@ use pgroles_core::diff::{Change, diff}; use pgroles_core::manifest::{ - DefaultPrivilege, DefaultPrivilegeGrant, Grant, ObjectTarget, ObjectType, PolicyManifest, - Privilege, RoleDefinition, SchemaBinding, expand_manifest, + DefaultPrivilege, DefaultPrivilegeGrant, Ensure, Grant, ObjectTarget, ObjectType, + PolicyManifest, Privilege, RoleDefinition, SchemaBinding, expand_manifest, }; use pgroles_core::model::RoleGraph; use pgroles_core::suggest::{ @@ -211,6 +211,7 @@ fn random_manifest(rng: &mut Rng) -> PolicyManifest { role: role_name.clone(), privileges: privs.clone(), object, + ensure: Ensure::Present, }); } for (ot, privs) in &kind.dp_templates { @@ -221,6 +222,7 @@ fn random_manifest(rng: &mut Rng) -> PolicyManifest { role: Some(role_name.clone()), privileges: privs.clone(), on_type: *ot, + ensure: Ensure::Present, }); } } @@ -230,7 +232,8 @@ fn random_manifest(rng: &mut Rng) -> PolicyManifest { .into_iter() .map(|((owner, schema), grant)| DefaultPrivilege { owner: Some(owner), - schema, + schema: Some(schema), + scope: None, grant, }) .collect(); diff --git a/crates/pgroles-inspect/src/defaults.rs b/crates/pgroles-inspect/src/defaults.rs index 97dbb68..8233655 100644 --- a/crates/pgroles-inspect/src/defaults.rs +++ b/crates/pgroles-inspect/src/defaults.rs @@ -2,33 +2,50 @@ //! //! Default privileges control the ACLs automatically applied to newly created //! objects. They are set via `ALTER DEFAULT PRIVILEGES FOR ROLE -//! IN SCHEMA GRANT ... ON TO `. +//! [IN SCHEMA ] GRANT/REVOKE ... ON TO/FROM `. //! //! The `pg_default_acl` table stores: //! - `defaclrole`: OID of the owner role -//! - `defaclnamespace`: OID of the schema (0 = global, i.e. all schemas) +//! - `defaclnamespace`: OID of the schema (0 = the owner-wide global layer) //! - `defaclobjtype`: char indicating the object type //! 'r' = relation (table), 'S' = sequence, 'f' = function, 'T' = type, 'n' = schema //! - `defaclacl`: the ACL array //! //! We use `aclexplode(defaclacl)` to decompose the ACL into individual grants. +//! +//! Two layers are inspected. The schema layer covers explicit rows in managed +//! schemas. The global layer is fetched only for `(owner, type)` pairs the +//! manifest declares with `scope: {type: global}`, and it reports the +//! *effective* default: when no explicit row exists, `acldefault(type, owner)` +//! stands in, so PostgreSQL's built-in `PUBLIC EXECUTE` on routines (and +//! `PUBLIC USAGE` on types) is visible to `ensure: absent` rules. Owner +//! self-entries are excluded in SQL: every `ALTER DEFAULT PRIVILEGES` +//! materializes the owner's implicit self-grant into the explicit row, and +//! reporting it would make authoritative mode revoke the owner's own default +//! on the next reconcile. The accepted blind spot is that an intentional +//! owner-self default change is invisible to pgroles. use std::collections::BTreeMap; use sqlx::PgPool; use pgroles_core::manifest::{ObjectType, Privilege}; -use pgroles_core::model::{DefaultPrivKey, DefaultPrivState}; +use pgroles_core::model::{DefaultPrivKey, DefaultPrivState, DefaultPrivilegeScope, Grantee}; + +use crate::DefaultPrivScopePattern; -/// A raw row from the `pg_default_acl` + `aclexplode()` query. +/// A raw row from the `pg_default_acl` + `aclexplode()` queries. #[derive(Debug, sqlx::FromRow)] struct DefaultAclRow { /// The owner role name (whose newly-created objects get these defaults). owner_name: String, - /// The schema name (NULL for global defaults — we filter these out). + /// The schema name (NULL for global-layer rows). schema_name: Option, - /// The grantee role name (NULL means PUBLIC — we skip those). + /// The grantee role name. NULL for PUBLIC (see `is_public`) and for + /// dangling grantee OIDs. grantee: Option, + /// True when the ACL grantee is OID 0, i.e. PUBLIC. + is_public: bool, /// The privilege character (same mapping as regular ACLs). privilege_type: String, /// The object type character from `defaclobjtype`. @@ -69,25 +86,57 @@ fn acl_char_to_privilege(character: &str) -> Option { } } -/// Fetch all default privileges from `pg_default_acl` for the given schemas and roles. +/// Map `ObjectType` to the `defaclobjtype` character. +fn object_type_to_defacl_char(object_type: ObjectType) -> Option<&'static str> { + match object_type { + ObjectType::Table => Some("r"), + ObjectType::Sequence => Some("S"), + ObjectType::Function => Some("f"), + ObjectType::Type => Some("T"), + ObjectType::Schema => Some("n"), + _ => None, + } +} + +/// Map `ObjectType` to the character `acldefault()` expects. /// -/// Returns a map of `DefaultPrivKey → DefaultPrivState` ready for insertion into a `RoleGraph`. +/// This is not the same alphabet as `defaclobjtype`. `acldefault` spells a +/// sequence `s`, and reads `S` as a foreign server, so passing the +/// `pg_default_acl` character straight through returns a foreign server's +/// defaults for sequences instead of erroring. +fn object_type_to_acldefault_char(object_type: ObjectType) -> Option<&'static str> { + match object_type { + ObjectType::Sequence => Some("s"), + other => object_type_to_defacl_char(other), + } +} + +/// Fetch default privileges for the managed schemas, roles, and declared +/// entry scopes. /// -/// Only returns defaults where the schema is in `managed_schemas` and the grantee -/// is in `managed_roles`. Owner filtering is intentionally NOT done here — we want -/// to capture defaults set by any owner (the manifest's `default_owner` or -/// per-schema `owner`) as long as the grantee is managed. -pub async fn fetch_default_privileges( +/// Returns a map of `DefaultPrivKey → DefaultPrivState` ready for insertion +/// into a `RoleGraph`. +/// +/// Role-grantee rows in the schema layer keep the historical rule: any owner, +/// as long as the schema is managed and the grantee is managed. PUBLIC rows +/// and everything in the global layer are assertion-scoped instead — they are +/// reported only for the exact `(owner, scope, on_type)` patterns the +/// manifest declares, and PUBLIC rows only for the privileges its rules +/// mention. That keeps pgroles from ever revoking a default it wasn't told +/// about. +pub(crate) async fn fetch_default_privileges( pool: &PgPool, managed_schemas: &[&str], managed_roles: &[&str], + scopes: &[DefaultPrivScopePattern], ) -> Result, sqlx::Error> { - let rows = sqlx::query_as::<_, DefaultAclRow>( + let mut rows = sqlx::query_as::<_, DefaultAclRow>( r#" SELECT owner_role.rolname AS owner_name, n.nspname AS schema_name, grantee_role.rolname AS grantee, + (acl.grantee = 0) AS is_public, acl.privilege_type, da.defaclobjtype::text AS obj_type_char FROM pg_default_acl da @@ -104,25 +153,74 @@ pub async fn fetch_default_privileges( .fetch_all(pool) .await?; + // Global layer: effective defaults for the declared (owner, type) pairs. + // The LEFT JOIN plus COALESCE(acldefault) is what synthesizes PostgreSQL's + // built-ins when no explicit row exists yet; `acl.grantee <> r.oid` drops + // owner self-entries (module doc explains why that is load-bearing). + let global_pairs: std::collections::BTreeSet<(String, String, String)> = scopes + .iter() + .filter(|pattern| pattern.schema.is_none()) + .filter_map(|pattern| { + let stored = object_type_to_defacl_char(pattern.on_type)?; + let builtin = object_type_to_acldefault_char(pattern.on_type)?; + Some(( + pattern.owner.clone(), + stored.to_string(), + builtin.to_string(), + )) + }) + .collect(); + if !global_pairs.is_empty() { + let owners: Vec = global_pairs + .iter() + .map(|(owner, _, _)| owner.clone()) + .collect(); + let chars: Vec = global_pairs + .iter() + .map(|(_, stored, _)| stored.clone()) + .collect(); + let builtin_chars: Vec = global_pairs + .iter() + .map(|(_, _, builtin)| builtin.clone()) + .collect(); + rows.extend( + sqlx::query_as::<_, DefaultAclRow>( + r#" + WITH global_scope(owner_name, obj_char, builtin_char) AS ( + SELECT * FROM unnest($1::text[], $2::text[], $3::text[]) + ) + SELECT + r.rolname::text AS owner_name, + NULL::text AS schema_name, + grantee_role.rolname::text AS grantee, + (acl.grantee = 0) AS is_public, + acl.privilege_type, + s.obj_char AS obj_type_char + FROM global_scope s + JOIN pg_roles r ON r.rolname = s.owner_name + LEFT JOIN pg_default_acl da + ON da.defaclrole = r.oid + AND da.defaclnamespace = 0 + AND da.defaclobjtype = s.obj_char::"char" + CROSS JOIN LATERAL aclexplode( + COALESCE(da.defaclacl, acldefault(s.builtin_char::"char", r.oid)) + ) AS acl + LEFT JOIN pg_roles grantee_role ON grantee_role.oid = acl.grantee + WHERE acl.grantee <> r.oid + ORDER BY r.rolname, s.obj_char + "#, + ) + .bind(&owners) + .bind(&chars) + .bind(&builtin_chars) + .fetch_all(pool) + .await?, + ); + } + let mut defaults: BTreeMap = BTreeMap::new(); for row in rows { - // Skip PUBLIC grantee (NULL) - let grantee = match row.grantee { - Some(ref name) => name, - None => continue, - }; - - // Skip if grantee isn't in the managed set - if !managed_roles.contains(&grantee.as_str()) { - continue; - } - - let schema_name = match row.schema_name { - Some(ref name) => name, - None => continue, // global defaults (namespace=0) — we don't manage these - }; - let privilege = match acl_char_to_privilege(&row.privilege_type) { Some(privilege) => privilege, None => continue, @@ -133,11 +231,43 @@ pub async fn fetch_default_privileges( None => continue, }; + let scope = match &row.schema_name { + Some(schema) => DefaultPrivilegeScope::Schema { + schema: schema.clone(), + }, + None => DefaultPrivilegeScope::Global, + }; + + let grantee = if row.is_public { + // PUBLIC rows are kept only when the manifest names this exact + // (owner, scope, type) pattern with this privilege. + let covered = scopes.iter().any(|pattern| { + pattern.owner == row.owner_name + && pattern.schema.as_deref() == row.schema_name.as_deref() + && pattern.on_type == on_type + && pattern.public_privileges.contains(&privilege) + }); + if !covered { + continue; + } + Grantee::Public + } else { + // Skip dangling grantee OIDs (NULL name but not PUBLIC). The + // global-layer query is already scoped to declared (owner, type) + // pairs, so the managed-role filter is the only check left for + // both layers. + let Some(name) = &row.grantee else { continue }; + if !managed_roles.contains(&name.as_str()) { + continue; + } + Grantee::Role(name.clone()) + }; + let key = DefaultPrivKey { owner: row.owner_name.clone(), - schema: schema_name.clone(), + scope, on_type, - grantee: grantee.clone(), + grantee, }; let entry = defaults.entry(key).or_insert_with(|| DefaultPrivState { diff --git a/crates/pgroles-inspect/src/lib.rs b/crates/pgroles-inspect/src/lib.rs index 40ec3c1..eda6293 100644 --- a/crates/pgroles-inspect/src/lib.rs +++ b/crates/pgroles-inspect/src/lib.rs @@ -8,6 +8,7 @@ pub mod cloud; mod defaults; mod identity; mod memberships; +mod preflight; mod privileges; mod public_grants; mod roles; @@ -27,9 +28,9 @@ use pgroles_core::ownership::ManagedScope; // Re-export the sub-modules' public items for testing / advanced use. pub use cloud::{CloudProvider, PrivilegeLevel, detect_privilege_level}; -pub use defaults::fetch_default_privileges; pub use identity::detect_system_identifier; pub use memberships::fetch_memberships; +pub use preflight::{AuthorityIssue, preflight_authority_issues}; pub use privileges::{ fetch_column_level_grants, fetch_database_privileges, fetch_object_inventory, fetch_privileges, fetch_relation_inventory, @@ -265,6 +266,8 @@ pub struct WildcardInspectionStats { #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub(crate) struct WildcardGrantPattern { + /// Grantee name. The reserved value `PUBLIC` means the pseudo-role; + /// key construction parses it via `Grantee::parse`. pub role: String, pub object_type: pgroles_core::manifest::ObjectType, pub schema: String, @@ -274,6 +277,39 @@ pub(crate) struct WildcardGrantPattern { pub privileges: std::collections::BTreeSet, } +/// An object scope for which the manifest declares PUBLIC rules (present or +/// absent). PUBLIC ACL rows enter the current graph only inside these scopes, +/// and only for the privileges the rules mention — pgroles never manages a +/// PUBLIC edge the manifest doesn't name. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub(crate) struct PublicObjectScope { + pub object_type: pgroles_core::manifest::ObjectType, + /// Schema containing the objects. `None` for schema- and database-typed + /// targets (which put the schema/database name in `name`). + pub schema: Option, + /// Object name, `"*"` for every object of the type in the schema, `None` + /// for database targets. + pub name: Option, + /// Union of the privileges named by rules for this scope; rows are + /// filtered to this set. + pub privileges: std::collections::BTreeSet, +} + +/// A default-privileges entry scope from the manifest, used to decide which +/// `pg_default_acl` layers to fetch and which PUBLIC rows to keep. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub(crate) struct DefaultPrivScopePattern { + /// The resolved owner (entry owner, or the manifest default_owner, or + /// "postgres"). + pub owner: String, + /// `None` means global scope (`pg_default_acl.defaclnamespace = 0`). + pub schema: Option, + pub on_type: pgroles_core::manifest::ObjectType, + /// Union of privileges from rules whose grantee is PUBLIC; empty when the + /// entry has no PUBLIC rules, in which case PUBLIC rows are skipped. + pub public_privileges: std::collections::BTreeSet, +} + // --------------------------------------------------------------------------- // Configuration // --------------------------------------------------------------------------- @@ -298,8 +334,15 @@ pub struct InspectConfig { /// Usually only needed if the manifest includes database-level grants. pub include_database_privileges: bool, - /// Wildcard grant selectors from the desired manifest. + /// Wildcard grant selectors from the desired manifest (present-ensure + /// only — absence assertions must stay per-object). pub(crate) wildcard_grants: Vec, + + /// Object scopes with declared PUBLIC rules. + pub(crate) public_object_scopes: Vec, + + /// Default-privilege entry scopes from the manifest. + pub(crate) default_priv_scopes: Vec, } impl InspectConfig { @@ -309,12 +352,26 @@ impl InspectConfig { expanded: &pgroles_core::manifest::ExpandedManifest, include_database_privileges: bool, ) -> Self { + use pgroles_core::manifest::{Ensure, ObjectType}; + let mut managed_roles: BTreeSet = BTreeSet::new(); let mut managed_schemas: BTreeSet = BTreeSet::new(); // Key for deduplicating wildcard grants: (role, object_type, schema). - type WildcardKey = (String, pgroles_core::manifest::ObjectType, String); + type WildcardKey = (String, ObjectType, String); let mut wildcard_map: BTreeMap> = BTreeMap::new(); + // Keyed by (object_type, schema, name) with privileges unioned across + // present and absent rules. + type PublicScopeKey = (ObjectType, Option, Option); + let mut public_scope_map: BTreeMap< + PublicScopeKey, + BTreeSet, + > = BTreeMap::new(); + type DefaultScopeKey = (String, Option, ObjectType); + let mut default_scope_map: BTreeMap< + DefaultScopeKey, + BTreeSet, + > = BTreeMap::new(); // Collect role names for role_def in &expanded.roles { @@ -327,16 +384,18 @@ impl InspectConfig { managed_schemas.insert(schema.clone()); } // Schema-level grants use the name field as the schema name - if grant.object.object_type == pgroles_core::manifest::ObjectType::Schema + if grant.object.object_type == ObjectType::Schema && let Some(ref name) = grant.object.name { managed_schemas.insert(name.clone()); } + // Absence assertions must stay per-object for the diff's + // range-scan, so only present wildcards become patterns. if grant.object.name.as_deref() == Some("*") + && grant.ensure == Ensure::Present && !matches!( grant.object.object_type, - pgroles_core::manifest::ObjectType::Schema - | pgroles_core::manifest::ObjectType::Database + ObjectType::Schema | ObjectType::Database ) && let Some(schema) = &grant.object.schema { @@ -346,11 +405,42 @@ impl InspectConfig { .or_default() .extend(grant.privileges.iter().copied()); } + if grant.role == "PUBLIC" { + let key = ( + grant.object.object_type, + grant.object.schema.clone(), + grant.object.name.clone(), + ); + public_scope_map + .entry(key) + .or_default() + .extend(grant.privileges.iter().copied()); + } } - // Collect schema names from default privileges + // Collect schema names and scope patterns from default privileges for dp in &expanded.default_privileges { - managed_schemas.insert(dp.schema.clone()); + // expand_manifest validated the scope already; ignore entries it + // would have rejected. + let Ok(scope) = dp.resolved_scope() else { + continue; + }; + let schema = scope.schema().map(str::to_string); + if let Some(schema) = &schema { + managed_schemas.insert(schema.clone()); + } + // Expansion has already resolved `default_owner` into every entry, + // so a still-missing owner means the manifest set neither. That is + // the same fallback the desired-state build applies. + let owner = dp.owner.clone().unwrap_or_else(|| "postgres".to_string()); + for grant in &dp.grant { + let entry = default_scope_map + .entry((owner.clone(), schema.clone(), grant.on_type)) + .or_default(); + if grant.role.as_deref() == Some("PUBLIC") { + entry.extend(grant.privileges.iter().copied()); + } + } } for schema in &expanded.schemas { @@ -373,6 +463,28 @@ impl InspectConfig { }, ) .collect(), + public_object_scopes: public_scope_map + .into_iter() + .map( + |((object_type, schema, name), privileges)| PublicObjectScope { + object_type, + schema, + name, + privileges, + }, + ) + .collect(), + default_priv_scopes: default_scope_map + .into_iter() + .map( + |((owner, schema, on_type), public_privileges)| DefaultPrivScopePattern { + owner, + schema, + on_type, + public_privileges, + }, + ) + .collect(), } } @@ -386,6 +498,13 @@ impl InspectConfig { ) -> Self { let base = Self::from_expanded(expanded, include_database_privileges); + let has_bindings = |schema: &str| { + scope + .schemas + .get(schema) + .is_some_and(|managed| managed.bindings) + }; + Self { managed_roles: scope.roles.iter().cloned().collect(), managed_schemas: scope.schemas.keys().cloned().collect(), @@ -398,11 +517,46 @@ impl InspectConfig { wildcard_grants: base .wildcard_grants .into_iter() - .filter(|pattern| { - scope - .schemas - .get(&pattern.schema) - .is_some_and(|managed| managed.bindings) + .filter(|pattern| has_bindings(&pattern.schema)) + .collect(), + public_object_scopes: base + .public_object_scopes + .into_iter() + .filter(|public_scope| match &public_scope.schema { + Some(schema) => has_bindings(schema), + // Schema-typed targets carry the schema in `name`; + // database targets pass through. + None => match public_scope.object_type { + pgroles_core::manifest::ObjectType::Schema => { + public_scope.name.as_deref().is_some_and(has_bindings) + } + _ => true, + }, + }) + .collect(), + default_priv_scopes: base + .default_priv_scopes + .into_iter() + .filter(|pattern| match &pattern.schema { + Some(schema) => has_bindings(schema), + // Global defaults belong to whoever owns the owner role. + // Composition already rejects a fragment declaring one for + // a role outside its scope, so reaching this branch means + // the owner was named in `scope.roles` without being + // defined. Dropping it silently would leave the rule + // planning nothing forever, so say so. + None => { + let owned = scope.roles.contains(&pattern.owner); + if !owned { + tracing::warn!( + owner = %pattern.owner, + on_type = %pattern.on_type, + "global default privileges declared for a role this policy does \ + not manage; the rule will not be inspected or reconciled" + ); + } + owned + } }) .collect(), } @@ -493,14 +647,13 @@ pub async fn inspect_all( // Object privileges (no wildcard patterns for unscoped inspection) if !schema_refs.is_empty() { - let privilege_grants = privileges::fetch_privileges_with_wildcards( - pool, - &schema_refs, - &role_refs, - &[], // no wildcard patterns - ) - .await? - .grants; + // No wildcard patterns and no PUBLIC scopes: `generate` reads only + // explicit managed-role state and never invents PUBLIC or absence + // policy from what it finds. + let privilege_grants = + privileges::fetch_privileges_with_wildcards(pool, &schema_refs, &role_refs, &[], &[]) + .await? + .grants; for (key, state) in privilege_grants { graph.grants.insert(key, state); } @@ -513,9 +666,11 @@ pub async fn inspect_all( graph.grants.insert(key, state); } - // Default privileges + // Default privileges (schema layer only — no declared scopes, so no + // global rows and no PUBLIC rows) if !schema_refs.is_empty() { - let default_privs = fetch_default_privileges(pool, &schema_refs, &role_refs).await?; + let default_privs = + defaults::fetch_default_privileges(pool, &schema_refs, &role_refs, &[]).await?; for (key, state) in default_privs { graph.default_privileges.insert(key, state); } @@ -611,6 +766,7 @@ pub async fn inspect_with_diagnostics( &privilege_schema_refs, &role_refs, &config.wildcard_grants, + &config.public_object_scopes, ) .await?; stats.record_phase("object_privileges", phase_started_at.elapsed()); @@ -659,11 +815,16 @@ pub async fn inspect_with_diagnostics( } // --- Default privileges --- - if !privilege_schema_refs.is_empty() { + if !privilege_schema_refs.is_empty() || !config.default_priv_scopes.is_empty() { debug!("inspecting default privileges from pg_default_acl"); let phase_started_at = Instant::now(); - let default_privs = - fetch_default_privileges(pool, &privilege_schema_refs, &role_refs).await?; + let default_privs = defaults::fetch_default_privileges( + pool, + &privilege_schema_refs, + &role_refs, + &config.default_priv_scopes, + ) + .await?; stats.record_phase("default_privileges", phase_started_at.elapsed()); for (key, state) in default_privs { graph.default_privileges.insert(key, state); @@ -905,7 +1066,7 @@ roles: ); graph.grants.insert( pgroles_core::model::GrantKey { - role: "inventory_owner".to_string(), + role: "inventory_owner".into(), object_type: pgroles_core::manifest::ObjectType::Schema, schema: None, name: Some("inventory".to_string()), @@ -918,7 +1079,7 @@ roles: ); graph.grants.insert( pgroles_core::model::GrantKey { - role: "inventory_reader".to_string(), + role: "inventory_reader".into(), object_type: pgroles_core::manifest::ObjectType::Schema, schema: None, name: Some("inventory".to_string()), @@ -937,7 +1098,7 @@ roles: graph .grants .keys() - .all(|key| key.role == "inventory_reader") + .all(|key| key.role.as_str() == "inventory_reader") ); } diff --git a/crates/pgroles-inspect/src/preflight.rs b/crates/pgroles-inspect/src/preflight.rs new file mode 100644 index 0000000..09b1acb --- /dev/null +++ b/crates/pgroles-inspect/src/preflight.rs @@ -0,0 +1,389 @@ +//! Executor-authority preflight for planned changes. +//! +//! Two failure modes are caught before apply: +//! +//! `ALTER DEFAULT PRIVILEGES FOR ROLE owner` requires the executor to be a +//! member of the owner role (or a superuser). Without that, apply fails +//! mid-transaction with a permission error, so the check only moves the +//! failure earlier and names the owner. +//! +//! `REVOKE ... FROM PUBLIC` is worse than a loud failure: issued by a role +//! without the owner's authority it silently removes nothing, the next +//! inspection still sees the privilege, and the controller re-plans the same +//! revoke forever. Implicit PUBLIC grants are always grantor-owned, so +//! membership in the object owner (which `pg_has_role` reports true for +//! superusers too) is exactly the authority the revoke needs. Role-grantee +//! revokes are deliberately not checked: an executor can hold revoke +//! authority for those without owner membership, and blocking such plans +//! would regress setups that work today. + +use std::collections::{BTreeMap, BTreeSet}; + +use sqlx::PgPool; + +use pgroles_core::diff::Change; +use pgroles_core::manifest::ObjectType; +use pgroles_core::model::{GrantKey, Grantee, RoleGraph}; + +/// Maximum number of objects named by an [`AuthorityIssue::PublicRevoke`]. +const REVOKE_EXAMPLE_LIMIT: usize = 5; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AuthorityIssue { + /// The executor cannot act as the owner of a planned + /// `ALTER DEFAULT PRIVILEGES` change. + DefaultPrivilegeOwner { owner: String, executor: String }, + + /// A planned `ALTER DEFAULT PRIVILEGES` names an owner that does not + /// exist and that this plan does not create. + MissingDefaultPrivilegeOwner { owner: String }, + + /// A planned `REVOKE ... FROM PUBLIC` covers objects whose owners the + /// executor cannot act as, so the revoke would silently do nothing there. + PublicRevoke { + object_type: ObjectType, + schema: Option, + executor: String, + skipped_count: usize, + /// Up to [`REVOKE_EXAMPLE_LIMIT`] affected objects as + /// `(name, owner)`. + examples: Vec<(String, String)>, + }, +} + +impl std::fmt::Display for AuthorityIssue { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + AuthorityIssue::DefaultPrivilegeOwner { owner, executor } => write!( + f, + "UnsatisfiableDefaultPrivilegeChange: cannot ALTER DEFAULT PRIVILEGES \ + FOR ROLE \"{owner}\" as executor \"{executor}\"; requires membership \ + in the owner role or superuser" + ), + AuthorityIssue::MissingDefaultPrivilegeOwner { owner } => write!( + f, + "UnsatisfiableDefaultPrivilegeChange: default privileges name owner \"{owner}\", \ + which does not exist and is not created by this plan" + ), + AuthorityIssue::PublicRevoke { + object_type, + schema, + executor, + skipped_count, + examples, + } => { + let examples = examples + .iter() + .map(|(name, owner)| format!("\"{name}\" (owner \"{owner}\")")) + .collect::>() + .join("; "); + write!( + f, + "UnsatisfiableRevoke: cannot revoke PUBLIC privileges on {object_type} \ + objects{} as executor \"{executor}\"; {skipped_count} object(s) have \ + owners the executor cannot act as, so the REVOKE would silently \ + change nothing", + match schema { + Some(schema) => format!(" in schema \"{schema}\""), + None => String::new(), + }, + )?; + if !examples.is_empty() { + write!(f, " (examples: {examples})")?; + } + Ok(()) + } + } + } +} + +#[derive(Debug, sqlx::FromRow)] +struct ObjectAuthorityRow { + schema_name: Option, + object_name: String, + owner_name: String, + can_act: bool, +} + +/// Check executor authority for the planned changes. +/// +/// `current` supplies the per-object PUBLIC state so the ownership check +/// covers exactly the objects an `ON ALL` revoke would have to touch, not +/// every object in the schema. +pub async fn preflight_authority_issues( + pool: &PgPool, + changes: &[Change], + current: &RoleGraph, +) -> Result, sqlx::Error> { + let mut issues = Vec::new(); + + let executor = { + let (user,): (String,) = sqlx::query_as("SELECT current_user::text") + .fetch_one(pool) + .await?; + user + }; + + // --- Default-privilege owner authority --- + let owners: BTreeSet = changes + .iter() + .filter_map(|change| match change { + Change::SetDefaultPrivilege { owner, .. } + | Change::RevokeDefaultPrivilege { owner, .. } => Some(owner.clone()), + _ => None, + }) + .collect(); + if !owners.is_empty() { + let owner_list: Vec = owners.iter().cloned().collect(); + let rows: Vec<(String, bool)> = sqlx::query_as( + r#" + SELECT r.rolname::text, pg_has_role(current_user, r.oid, 'USAGE') + FROM pg_roles r + WHERE r.rolname = ANY($1) + "#, + ) + .bind(&owner_list) + .fetch_all(pool) + .await?; + let known: BTreeSet = rows.iter().map(|(owner, _)| owner.clone()).collect(); + for (owner, can_act) in rows { + if !can_act { + issues.push(AuthorityIssue::DefaultPrivilegeOwner { + owner, + executor: executor.clone(), + }); + } + } + // An owner absent from pg_roles has no authority to check yet. That is + // expected when the same plan creates it, so only roles the plan does + // not create are reported. + let created: BTreeSet<&str> = changes + .iter() + .filter_map(|change| match change { + Change::CreateRole { name, .. } => Some(name.as_str()), + _ => None, + }) + .collect(); + for owner in &owners { + if !known.contains(owner) && !created.contains(owner.as_str()) { + issues.push(AuthorityIssue::MissingDefaultPrivilegeOwner { + owner: owner.clone(), + }); + } + } + } + + // --- PUBLIC revoke ownership --- + // Collect the objects each planned PUBLIC revoke touches, grouped per + // (object_type, schema). `AllInSchema` means every object of that type, + // which is what an `ON ALL` statement actually reaches. + #[derive(Default)] + struct RevokeTargets { + names: BTreeSet, + all_in_schema: bool, + } + + let mut targets: BTreeMap<(ObjectType, Option), RevokeTargets> = BTreeMap::new(); + for change in changes { + let Change::Revoke { + role: Grantee::Public, + object_type, + schema, + name, + .. + } = change + else { + continue; + }; + let entry = targets.entry((*object_type, schema.clone())).or_default(); + match name.as_deref() { + Some("*") => { + let range_start = GrantKey { + role: Grantee::Public, + object_type: *object_type, + schema: schema.clone(), + name: None, + }; + for (key, _) in current.grants.range(range_start..).take_while(|(key, _)| { + key.role == Grantee::Public + && key.object_type == *object_type + && key.schema == *schema + }) { + match key.name.as_deref() { + // Wildcard normalization collapses per-object PUBLIC + // rows into one `"*"` key when the same scope also has + // a present wildcard. Narrowing to the remaining + // per-object names would then check nothing, so treat + // the collapsed key as covering the whole scope. + Some("*") => entry.all_in_schema = true, + Some(object_name) => { + entry.names.insert(object_name.to_string()); + } + None => {} + } + } + } + Some(object_name) => { + entry.names.insert(object_name.to_string()); + } + None => {} + } + } + + for ((object_type, schema), target) in targets { + if target.names.is_empty() && !target.all_in_schema { + continue; + } + let rows = + fetch_object_authority(pool, object_type, schema.as_deref(), &target.names).await?; + let mut blocked: Vec<(String, String)> = rows + .into_iter() + .filter(|row| { + !row.can_act + && row.schema_name.as_deref() == schema.as_deref() + && (target.all_in_schema || target.names.contains(&row.object_name)) + }) + .map(|row| (row.object_name, row.owner_name)) + .collect(); + if blocked.is_empty() { + continue; + } + blocked.sort(); + issues.push(AuthorityIssue::PublicRevoke { + object_type, + schema, + executor: executor.clone(), + skipped_count: blocked.len(), + examples: blocked.into_iter().take(REVOKE_EXAMPLE_LIMIT).collect(), + }); + } + + Ok(issues) +} + +async fn fetch_object_authority( + pool: &PgPool, + object_type: ObjectType, + schema: Option<&str>, + names: &BTreeSet, +) -> Result, sqlx::Error> { + let schemas: Vec = schema.map(|s| vec![s.to_string()]).unwrap_or_default(); + let names: Vec = names.iter().cloned().collect(); + match object_type { + ObjectType::Table + | ObjectType::View + | ObjectType::MaterializedView + | ObjectType::Sequence => { + // Only the relkinds this object type actually names. Selecting + // every relation would report objects the planned statement cannot + // reach, and a wildcard target accepts them all. + let relkinds: Vec = match object_type { + ObjectType::Table => vec!["r".to_string(), "p".to_string()], + ObjectType::View => vec!["v".to_string()], + ObjectType::MaterializedView => vec!["m".to_string()], + _ => vec!["S".to_string()], + }; + sqlx::query_as::<_, ObjectAuthorityRow>( + r#" + SELECT + n.nspname::text AS schema_name, + c.relname::text AS object_name, + o.rolname::text AS owner_name, + pg_has_role(current_user, c.relowner, 'USAGE') AS can_act + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + JOIN pg_roles o ON o.oid = c.relowner + WHERE n.nspname = ANY($1) + AND c.relkind::text = ANY($2) + "#, + ) + .bind(&schemas) + .bind(&relkinds) + .fetch_all(pool) + .await + } + ObjectType::Function => { + sqlx::query_as::<_, ObjectAuthorityRow>( + r#" + SELECT + n.nspname::text AS schema_name, + (p.proname || '(' || pg_catalog.pg_get_function_identity_arguments(p.oid) || ')')::text + AS object_name, + o.rolname::text AS owner_name, + pg_has_role(current_user, p.proowner, 'USAGE') AS can_act + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace + JOIN pg_roles o ON o.oid = p.proowner + WHERE n.nspname = ANY($1) + "#, + ) + .bind(&schemas) + .fetch_all(pool) + .await + } + ObjectType::Type => { + sqlx::query_as::<_, ObjectAuthorityRow>( + r#" + SELECT + n.nspname::text AS schema_name, + t.typname::text AS object_name, + o.rolname::text AS owner_name, + pg_has_role(current_user, t.typowner, 'USAGE') AS can_act + FROM pg_type t + JOIN pg_namespace n ON n.oid = t.typnamespace + JOIN pg_roles o ON o.oid = t.typowner + WHERE n.nspname = ANY($1) + -- Only user-facing types. Every table carries a composite + -- type, and every type carries an array type; neither is + -- something a revoke can name. + AND ( + t.typrelid = 0 + OR (SELECT c.relkind FROM pg_class c WHERE c.oid = t.typrelid) = 'c' + ) + AND NOT EXISTS ( + SELECT 1 FROM pg_type el + WHERE el.oid = t.typelem AND el.typarray = t.oid + ) + "#, + ) + .bind(&schemas) + .fetch_all(pool) + .await + } + ObjectType::Schema => { + // Schema targets carry the schema in `name`, so the caller passes + // the names through `schemas`. + sqlx::query_as::<_, ObjectAuthorityRow>( + r#" + SELECT + NULL::text AS schema_name, + n.nspname::text AS object_name, + o.rolname::text AS owner_name, + pg_has_role(current_user, n.nspowner, 'USAGE') AS can_act + FROM pg_namespace n + JOIN pg_roles o ON o.oid = n.nspowner + WHERE n.nspname = ANY($1) + "#, + ) + .bind(&names) + .fetch_all(pool) + .await + } + ObjectType::Database => { + sqlx::query_as::<_, ObjectAuthorityRow>( + r#" + SELECT + NULL::text AS schema_name, + db.datname::text AS object_name, + o.rolname::text AS owner_name, + pg_has_role(current_user, db.datdba, 'USAGE') AS can_act + FROM pg_database db + JOIN pg_roles o ON o.oid = db.datdba + WHERE db.datname = current_database() + "#, + ) + .fetch_all(pool) + .await + } + } +} diff --git a/crates/pgroles-inspect/src/privileges.rs b/crates/pgroles-inspect/src/privileges.rs index ea260fd..b6deafe 100644 --- a/crates/pgroles-inspect/src/privileges.rs +++ b/crates/pgroles-inspect/src/privileges.rs @@ -3,11 +3,16 @@ //! Uses `aclexplode()` to decompose explicit ACL arrays from `pg_class`, //! `pg_namespace`, `pg_proc`, `pg_type`, and `pg_database`. //! -//! Managed-state inspection intentionally does not synthesize owner/default ACLs +//! Managed-role inspection intentionally does not synthesize owner/default ACLs //! from `acldefault(...)`. Doing so would make implicit owner privileges appear //! as explicit managed grants, causing drift where the manifest never declared -//! those self-grants. PUBLIC/default visibility is handled separately by the -//! `public_grants` module for informational output. +//! those self-grants. +//! +//! PUBLIC (ACL grantee OID 0) is different: when the manifest declares PUBLIC +//! rules, [`fetch_public_object_privileges`] reports PUBLIC's *effective* +//! privileges for exactly those scopes, using `acldefault` for NULL ACLs and +//! keeping only grantee-0 entries. Informational PUBLIC display for `inspect` +//! output still lives in the `public_grants` module. //! //! The privilege character mapping: //! r = SELECT, a = INSERT, w = UPDATE, d = DELETE, D = TRUNCATE, @@ -23,7 +28,9 @@ use crate::{ WildcardGrantPattern, WildcardInspectionStats, }; use pgroles_core::manifest::{ObjectType, Privilege}; -use pgroles_core::model::{GrantKey, GrantState}; +use pgroles_core::model::{GrantKey, GrantState, Grantee}; + +use crate::PublicObjectScope; /// A raw ACL row returned by our `aclexplode()` queries. #[derive(Debug, sqlx::FromRow)] @@ -234,7 +241,7 @@ pub async fn fetch_privileges( managed_roles: &[&str], ) -> Result, sqlx::Error> { Ok( - fetch_privileges_with_wildcards(pool, managed_schemas, managed_roles, &[]) + fetch_privileges_with_wildcards(pool, managed_schemas, managed_roles, &[], &[]) .await? .grants, ) @@ -495,6 +502,7 @@ pub(crate) async fn fetch_privileges_with_wildcards( managed_schemas: &[&str], managed_roles: &[&str], wildcard_grants: &[WildcardGrantPattern], + public_scopes: &[PublicObjectScope], ) -> Result { let mut grants: BTreeMap = BTreeMap::new(); let has_wildcards = !wildcard_grants.is_empty(); @@ -573,7 +581,9 @@ pub(crate) async fn fetch_privileges_with_wildcards( }; let key = GrantKey { - role: grantee.clone(), + // Joined through pg_roles, so this is always a real role name — + // even one spelled "PUBLIC" — never the pseudo-role. + role: Grantee::Role(grantee.clone()), object_type, schema, name, @@ -585,6 +595,17 @@ pub(crate) async fn fetch_privileges_with_wildcards( entry.privileges.insert(privilege); } + // PUBLIC state, fetched only for declared scopes. Merged before wildcard + // normalization so a present-ensure PUBLIC wildcard collapses its + // per-object rows like any role wildcard; scopes declared only absent are + // not wildcard patterns, so their rows stay per-object for the diff's + // range-scan. + if !public_scopes.is_empty() { + for (key, state) in fetch_public_object_privileges(pool, public_scopes).await? { + grants.insert(key, state); + } + } + let unsatisfied_wildcards = if has_wildcards { unsatisfied_wildcard_grants(&grants, &inventory, wildcard_grants) } else { @@ -624,6 +645,269 @@ async fn fetch_current_user(pool: &PgPool) -> Result { Ok(user) } +/// Fetch effective PUBLIC (ACL grantee OID 0) privileges for declared scopes. +/// +/// Unlike the managed-role fetches above, NULL ACLs are exploded through +/// `acldefault(...)` so PostgreSQL's implicit built-ins — EXECUTE on +/// routines, USAGE on types, CONNECT/TEMPORARY on the database — are visible. +/// An `ensure: absent` rule must see them or no revoke would ever be planned +/// on a fresh object. Only grantee-0 entries are returned, so the synthesis +/// can never leak implicit owner privileges into managed-role state. +/// +/// Rows are filtered to the privileges the manifest's PUBLIC rules mention +/// for the matching scope. A PUBLIC edge the manifest never names stays out +/// of the graph entirely, which is what keeps convergence from revoking +/// unmanaged PUBLIC grants (see the #108 wildcard regression). +/// +/// `acldefault` is applied to every object family for uniformity; it only +/// changes the result for functions, types, and the database, because the +/// other families' defaults contain no grantee-0 entries. Its contents are +/// identical on PG 16, 17, and 18. +pub(crate) async fn fetch_public_object_privileges( + pool: &PgPool, + scopes: &[PublicObjectScope], +) -> Result, sqlx::Error> { + let unique_schemas = |predicate: &dyn Fn(ObjectType) -> bool| -> Vec { + scopes + .iter() + .filter(|scope| predicate(scope.object_type)) + .filter_map(|scope| scope.schema.clone()) + .collect::>() + .into_iter() + .collect() + }; + + let mut rows: Vec = Vec::new(); + + let relation_schemas = unique_schemas(&|object_type| { + matches!( + object_type, + ObjectType::Table + | ObjectType::View + | ObjectType::MaterializedView + | ObjectType::Sequence + ) + }); + if !relation_schemas.is_empty() { + rows.extend( + sqlx::query_as::<_, AclRow>( + r#" + SELECT + NULL::text AS grantee, + acl.privilege_type, + n.nspname::text AS schema_name, + c.relname::text AS object_name, + CASE c.relkind + WHEN 'r' THEN 'table' + WHEN 'p' THEN 'table' + WHEN 'v' THEN 'view' + WHEN 'm' THEN 'materialized_view' + WHEN 'S' THEN 'sequence' + END AS obj_type + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + CROSS JOIN LATERAL aclexplode( + COALESCE( + c.relacl, + acldefault( + CASE WHEN c.relkind = 'S' THEN 'S' ELSE 'r' END::"char", + c.relowner + ) + ) + ) AS acl + WHERE n.nspname = ANY($1) + AND c.relkind IN ('r', 'p', 'v', 'm', 'S') + AND acl.grantee = 0 + ORDER BY n.nspname, c.relname + "#, + ) + .bind(&relation_schemas) + .fetch_all(pool) + .await?, + ); + } + + let function_schemas = unique_schemas(&|object_type| object_type == ObjectType::Function); + if !function_schemas.is_empty() { + rows.extend( + sqlx::query_as::<_, AclRow>( + r#" + SELECT + NULL::text AS grantee, + acl.privilege_type, + n.nspname::text AS schema_name, + (p.proname || '(' || pg_catalog.pg_get_function_identity_arguments(p.oid) || ')')::text AS object_name, + 'function' AS obj_type + FROM pg_proc p + JOIN pg_namespace n ON n.oid = p.pronamespace + CROSS JOIN LATERAL aclexplode( + COALESCE(p.proacl, acldefault('f'::"char", p.proowner)) + ) AS acl + WHERE n.nspname = ANY($1) + AND acl.grantee = 0 + ORDER BY n.nspname, p.proname + "#, + ) + .bind(&function_schemas) + .fetch_all(pool) + .await?, + ); + } + + let type_schemas = unique_schemas(&|object_type| object_type == ObjectType::Type); + if !type_schemas.is_empty() { + rows.extend( + sqlx::query_as::<_, AclRow>( + r#" + SELECT + NULL::text AS grantee, + acl.privilege_type, + n.nspname::text AS schema_name, + t.typname::text AS object_name, + 'type' AS obj_type + FROM pg_type t + JOIN pg_namespace n ON n.oid = t.typnamespace + CROSS JOIN LATERAL aclexplode( + COALESCE(t.typacl, acldefault('T'::"char", t.typowner)) + ) AS acl + WHERE n.nspname = ANY($1) + AND t.typname NOT LIKE '\_%' + AND t.typtype <> 'p' + AND acl.grantee = 0 + ORDER BY n.nspname, t.typname + "#, + ) + .bind(&type_schemas) + .fetch_all(pool) + .await?, + ); + } + + let schema_names: Vec = scopes + .iter() + .filter(|scope| scope.object_type == ObjectType::Schema) + .filter_map(|scope| scope.name.clone()) + .collect::>() + .into_iter() + .collect(); + if !schema_names.is_empty() { + rows.extend( + sqlx::query_as::<_, AclRow>( + r#" + SELECT + NULL::text AS grantee, + acl.privilege_type, + NULL::text AS schema_name, + n.nspname::text AS object_name, + 'schema' AS obj_type + FROM pg_namespace n + CROSS JOIN LATERAL aclexplode( + COALESCE(n.nspacl, acldefault('n'::"char", n.nspowner)) + ) AS acl + WHERE n.nspname = ANY($1) + AND acl.grantee = 0 + ORDER BY n.nspname + "#, + ) + .bind(&schema_names) + .fetch_all(pool) + .await?, + ); + } + + if scopes + .iter() + .any(|scope| scope.object_type == ObjectType::Database) + { + rows.extend( + sqlx::query_as::<_, AclRow>( + r#" + SELECT + NULL::text AS grantee, + acl.privilege_type, + NULL::text AS schema_name, + db.datname::text AS object_name, + 'database' AS obj_type + FROM pg_database db + CROSS JOIN LATERAL aclexplode( + COALESCE(db.datacl, acldefault('d'::"char", db.datdba)) + ) AS acl + WHERE db.datname = current_database() + AND acl.grantee = 0 + "#, + ) + .fetch_all(pool) + .await?, + ); + } + + let mut grants: BTreeMap = BTreeMap::new(); + for row in rows { + let Some(privilege) = acl_char_to_privilege(&row.privilege_type) else { + continue; + }; + let Some(object_type) = obj_type_str_to_object_type(&row.obj_type) else { + continue; + }; + + if !public_scopes_cover( + scopes, + object_type, + row.schema_name.as_deref(), + &row.object_name, + privilege, + ) { + continue; + } + + let (schema, name) = match object_type { + ObjectType::Schema | ObjectType::Database => (None, Some(row.object_name.clone())), + _ => (row.schema_name.clone(), Some(row.object_name.clone())), + }; + grants + .entry(GrantKey { + role: Grantee::Public, + object_type, + schema, + name, + }) + .or_insert_with(|| GrantState { + privileges: BTreeSet::new(), + }) + .privileges + .insert(privilege); + } + + Ok(grants) +} + +/// Whether some declared PUBLIC scope covers this object and names this +/// privilege. +fn public_scopes_cover( + scopes: &[PublicObjectScope], + object_type: ObjectType, + schema_name: Option<&str>, + object_name: &str, + privilege: Privilege, +) -> bool { + scopes.iter().any(|scope| { + if scope.object_type != object_type || !scope.privileges.contains(&privilege) { + return false; + } + match object_type { + ObjectType::Schema => scope.name.as_deref() == Some(object_name), + ObjectType::Database => scope.name.as_deref().is_none_or(|name| name == object_name), + _ => { + scope.schema.as_deref() == schema_name + && scope + .name + .as_deref() + .is_some_and(|name| name == "*" || name == object_name) + } + } + }) +} + fn unsatisfied_wildcard_grants( grants: &BTreeMap, inventory: &BTreeMap<(ObjectType, String), BTreeSet>, @@ -644,7 +928,7 @@ fn unsatisfied_wildcard_grants( let mut missing_privileges = BTreeSet::new(); for object_name in object_names { let key = GrantKey { - role: wildcard.role.clone(), + role: Grantee::parse(&wildcard.role), object_type: wildcard.object_type, schema: Some(wildcard.schema.clone()), name: Some(object_name.clone()), @@ -903,7 +1187,7 @@ fn detect_unsatisfiable_wildcards( } let key = GrantKey { - role: wildcard.role.clone(), + role: Grantee::parse(&wildcard.role), object_type: wildcard.object_type, schema: Some(wildcard.schema.clone()), name: Some(object_name.clone()), @@ -971,7 +1255,7 @@ fn insert_vacuous_wildcard( wildcard: &WildcardGrantPattern, ) { let wildcard_key = GrantKey { - role: wildcard.role.clone(), + role: Grantee::parse(&wildcard.role), object_type: wildcard.object_type, schema: Some(wildcard.schema.clone()), name: Some("*".to_string()), @@ -1006,7 +1290,7 @@ fn normalize_wildcard_grants( for object_name in object_names { let key = GrantKey { - role: wildcard.role.clone(), + role: Grantee::parse(&wildcard.role), object_type: wildcard.object_type, schema: Some(wildcard.schema.clone()), name: Some(object_name.clone()), @@ -1025,7 +1309,7 @@ fn normalize_wildcard_grants( } let wildcard_key = GrantKey { - role: wildcard.role.clone(), + role: Grantee::parse(&wildcard.role), object_type: wildcard.object_type, schema: Some(wildcard.schema.clone()), name: Some("*".to_string()), @@ -1040,7 +1324,7 @@ fn normalize_wildcard_grants( for object_name in object_names { let key = GrantKey { - role: wildcard.role.clone(), + role: Grantee::parse(&wildcard.role), object_type: wildcard.object_type, schema: Some(wildcard.schema.clone()), name: Some(object_name.clone()), @@ -1268,7 +1552,7 @@ pub async fn fetch_database_privileges( }; let key = GrantKey { - role: grantee.clone(), + role: Grantee::Role(grantee.clone()), object_type: ObjectType::Database, schema: None, name: Some(row.object_name.clone()), @@ -1544,7 +1828,7 @@ mod tests { fn diagnostics_ignore_non_grantable_object_that_already_has_privilege() { let grants = BTreeMap::from([( GrantKey { - role: "app-editor".to_string(), + role: "app-editor".into(), object_type: ObjectType::Function, schema: Some("app".to_string()), name: Some("f2()".to_string()), @@ -1573,7 +1857,7 @@ mod tests { let grants = BTreeMap::from([ ( GrantKey { - role: "app-editor".to_string(), + role: "app-editor".into(), object_type: ObjectType::Function, schema: Some("app".to_string()), name: Some("f1()".to_string()), @@ -1584,7 +1868,7 @@ mod tests { ), ( GrantKey { - role: "app-editor".to_string(), + role: "app-editor".into(), object_type: ObjectType::Function, schema: Some("app".to_string()), name: Some("f2()".to_string()), @@ -1614,7 +1898,7 @@ mod tests { }; let grants = BTreeMap::from([( GrantKey { - role: "app-editor".to_string(), + role: "app-editor".into(), object_type: ObjectType::Table, schema: Some("app".to_string()), name: Some("widgets".to_string()), @@ -1692,7 +1976,7 @@ mod tests { let mut grants = BTreeMap::new(); grants.insert( GrantKey { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), object_type: ObjectType::Table, schema: Some("inventory".to_string()), name: Some("widgets".to_string()), @@ -1703,7 +1987,7 @@ mod tests { ); grants.insert( GrantKey { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), object_type: ObjectType::Table, schema: Some("inventory".to_string()), name: Some("orders".to_string()), @@ -1733,7 +2017,7 @@ mod tests { let wildcard = normalized .get(&GrantKey { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), object_type: ObjectType::Table, schema: Some("inventory".to_string()), name: Some("*".to_string()), @@ -1743,7 +2027,7 @@ mod tests { let specific = normalized .get(&GrantKey { - role: "inventory-editor".to_string(), + role: "inventory-editor".into(), object_type: ObjectType::Table, schema: Some("inventory".to_string()), name: Some("widgets".to_string()), @@ -1772,7 +2056,7 @@ mod tests { let result = normalize_wildcard_grants(grants, &inventory, &wildcards); let wildcard_key = GrantKey { - role: "accounts-editor".to_string(), + role: "accounts-editor".into(), object_type: ObjectType::Sequence, schema: Some("accounts".to_string()), name: Some("*".to_string()), @@ -1807,7 +2091,7 @@ mod tests { let result = normalize_wildcard_grants(grants, &inventory, &wildcards); let wildcard_key = GrantKey { - role: "accounts-editor".to_string(), + role: "accounts-editor".into(), object_type: ObjectType::Function, schema: Some("accounts".to_string()), name: Some("*".to_string()), @@ -1829,7 +2113,7 @@ mod tests { let mut grants = BTreeMap::new(); grants.insert( GrantKey { - role: "app".to_string(), + role: "app".into(), object_type: ObjectType::Sequence, schema: Some("public".to_string()), name: Some("seq1".to_string()), @@ -1840,7 +2124,7 @@ mod tests { ); grants.insert( GrantKey { - role: "app".to_string(), + role: "app".into(), object_type: ObjectType::Sequence, schema: Some("public".to_string()), name: Some("seq2".to_string()), @@ -1870,7 +2154,7 @@ mod tests { let result = normalize_wildcard_grants(grants, &inventory, &wildcards); let wildcard_key = GrantKey { - role: "app".to_string(), + role: "app".into(), object_type: ObjectType::Sequence, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -1929,7 +2213,7 @@ mod tests { assert!( !result.contains_key(&GrantKey { - role: "app".to_string(), + role: "app".into(), object_type: ObjectType::Function, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -1938,7 +2222,7 @@ mod tests { ); assert!( !result.contains_key(&GrantKey { - role: "app".to_string(), + role: "app".into(), object_type: ObjectType::Type, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -1947,7 +2231,7 @@ mod tests { ); assert!( !result.contains_key(&GrantKey { - role: "app".to_string(), + role: "app".into(), object_type: ObjectType::Sequence, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -2127,6 +2411,8 @@ mod tests { privilege_schemas: vec![schema.to_string()], include_database_privileges: false, wildcard_grants: vec![], + public_object_scopes: vec![], + default_priv_scopes: vec![], } } diff --git a/crates/pgroles-inspect/tests/diff_property_live.rs b/crates/pgroles-inspect/tests/diff_property_live.rs index 5fc69f8..e7a0c62 100644 --- a/crates/pgroles-inspect/tests/diff_property_live.rs +++ b/crates/pgroles-inspect/tests/diff_property_live.rs @@ -49,6 +49,15 @@ //! functions, no types, no database privileges. That keeps object bootstrap //! simple; wildcard/function coverage lives in the pure harness and the //! targeted live tests. **Coverage boundary, not an accident.** +//! * PUBLIC grantees are never generated: PUBLIC ACL entries are +//! database-global state with no schema prefix to isolate them, so +//! concurrent seeds would corrupt them for each other. +//! * `ensure: absent` assertions are never generated; absence semantics are +//! covered by targeted live tests in `crates/pgroles-cli/tests/cli.rs`. +//! * Global-scope default privileges are never generated: like PUBLIC ACLs, +//! global `pg_default_acl` rows are database-global state with no schema +//! prefix to isolate them, so concurrent seeds would corrupt them for +//! each other. //! * Relation grants (tables/sequences) only target the base schemas present //! in *both* graphs: pgroles manages grants, not tables, so an object must //! exist before a grant on it can execute — and a schema created by the @@ -84,8 +93,9 @@ use sqlx::{Executor, PgPool}; use pgroles_core::diff::{Change, diff}; use pgroles_core::manifest::{ExpandedManifest, ExpandedSchema, ObjectType, Privilege}; use pgroles_core::model::{ - DefaultPrivKey, DefaultPrivState, GrantKey, GrantState, MembershipEdge, RoleAttribute, - RoleGraph, RoleState, SchemaState, default_schema_owner_privileges, + DefaultPrivKey, DefaultPrivState, DefaultPrivilegeScope, GrantKey, GrantState, Grantee, + MembershipEdge, RoleAttribute, RoleGraph, RoleState, SchemaState, + default_schema_owner_privileges, }; use pgroles_core::sql::{quote_ident, render_statements}; use pgroles_inspect::{InspectConfig, inspect}; @@ -330,7 +340,7 @@ fn gen_grants(rng: &mut Rng, graph: &mut RoleGraph, names: &Names) { let role_names: Vec = graph.roles.keys().cloned().collect(); let schema_names: Vec = graph.schemas.keys().cloned().collect(); for _ in 0..rng.usize(7) { - let role = pick(rng, &role_names).clone(); + let role = Grantee::Role(pick(rng, &role_names).clone()); let (key, privileges) = match rng.usize(3) { 0 => { let schema = pick(rng, &schema_names).clone(); @@ -386,9 +396,11 @@ fn gen_default_privileges(rng: &mut Rng, graph: &mut RoleGraph, names: &Names) { graph.default_privileges.insert( DefaultPrivKey { owner, - schema: pick(rng, &names.base_schemas).clone(), + scope: DefaultPrivilegeScope::Schema { + schema: pick(rng, &names.base_schemas).clone(), + }, on_type, - grantee, + grantee: Grantee::Role(grantee), }, DefaultPrivState { privileges }, ); @@ -477,10 +489,10 @@ fn remove_role(graph: &mut RoleGraph, role: &str) { state.owner_privileges = default_schema_owner_privileges(&survivor); } } - graph.grants.retain(|key, _| key.role != role); + graph.grants.retain(|key, _| key.role.as_str() != role); graph .default_privileges - .retain(|key, _| key.owner != role && key.grantee != role); + .retain(|key, _| key.owner != role && key.grantee.as_str() != role); graph .memberships .retain(|edge| edge.role != role && edge.member != role); @@ -594,7 +606,7 @@ fn derive_current(rng: &mut Rng, desired: &RoleGraph, names: &Names) -> RoleGrap for _ in 0..rng.usize(3) { c.grants.insert( GrantKey { - role: pick(rng, ¤t_roles).clone(), + role: Grantee::Role(pick(rng, ¤t_roles).clone()), object_type: ObjectType::Table, schema: Some(pick(rng, &names.base_schemas).clone()), name: Some(pick(rng, &names.tables).clone()), @@ -644,9 +656,11 @@ fn derive_current(rng: &mut Rng, desired: &RoleGraph, names: &Names) -> RoleGrap c.default_privileges.insert( DefaultPrivKey { owner, - schema: pick(rng, &names.base_schemas).clone(), + scope: DefaultPrivilegeScope::Schema { + schema: pick(rng, &names.base_schemas).clone(), + }, on_type: ObjectType::Table, - grantee, + grantee: Grantee::Role(grantee), }, DefaultPrivState { privileges: [Privilege::Select].into_iter().collect(), @@ -722,10 +736,9 @@ fn strip_owner_schema_grants(current: &mut RoleGraph, desired: &mut RoleGraph) { .collect(); graph.grants.retain(|key, _| { !(key.object_type == ObjectType::Schema - && key - .name - .as_ref() - .is_some_and(|name| owners.contains(&(name.clone(), key.role.clone())))) + && key.name.as_ref().is_some_and(|name| { + owners.contains(&(name.clone(), key.role.as_str().to_string())) + })) }); } } @@ -863,7 +876,7 @@ fn apply_changes(graph: &RoleGraph, changes: &[Change]) -> RoleGraph { state.owner_privileges = [Privilege::Create, Privilege::Usage].into_iter().collect(); g.grants.remove(&GrantKey { - role: owner.clone(), + role: Grantee::Role(owner.clone()), object_type: ObjectType::Schema, schema: None, name: Some(name.clone()), @@ -927,14 +940,14 @@ fn apply_changes(graph: &RoleGraph, changes: &[Change]) -> RoleGraph { } Change::SetDefaultPrivilege { owner, - schema, + scope, on_type, grantee, privileges, } => { let key = DefaultPrivKey { owner: owner.clone(), - schema: schema.clone(), + scope: scope.clone(), on_type: *on_type, grantee: grantee.clone(), }; @@ -950,14 +963,14 @@ fn apply_changes(graph: &RoleGraph, changes: &[Change]) -> RoleGraph { } Change::RevokeDefaultPrivilege { owner, - schema, + scope, on_type, grantee, privileges, } => { let key = DefaultPrivKey { owner: owner.clone(), - schema: schema.clone(), + scope: scope.clone(), on_type: *on_type, grantee: grantee.clone(), }; @@ -1048,9 +1061,10 @@ fn filter_prefix(graph: &RoleGraph, prefix: &str) -> RoleGraph { let mut g = graph.clone(); g.roles.retain(|name, _| name.starts_with(prefix)); g.schemas.retain(|name, _| name.starts_with(prefix)); - g.grants.retain(|key, _| key.role.starts_with(prefix)); + g.grants + .retain(|key, _| key.role.as_str().starts_with(prefix)); g.default_privileges - .retain(|key, _| key.owner.starts_with(prefix) && key.grantee.starts_with(prefix)); + .retain(|key, _| key.owner.starts_with(prefix) && key.grantee.as_str().starts_with(prefix)); g.memberships .retain(|edge| edge.role.starts_with(prefix) && edge.member.starts_with(prefix)); g diff --git a/crates/pgroles-operator/src/crd.rs b/crates/pgroles-operator/src/crd.rs index da4e391..5ab8f89 100644 --- a/crates/pgroles-operator/src/crd.rs +++ b/crates/pgroles-operator/src/crd.rs @@ -11,7 +11,8 @@ use std::collections::{BTreeMap, BTreeSet}; use pgroles_core::bounds::*; use pgroles_core::manifest::{ - DefaultPrivilege, Grant, Membership, ObjectType, Privilege, RoleRetirement, SchemaBinding, + DefaultPrivilege, Ensure, Grant, Membership, ObjectType, Privilege, RoleRetirement, + SchemaBinding, }; /// Valid PostgreSQL SSL modes for connection params. @@ -672,6 +673,10 @@ pub struct DefaultPrivilegeGrantSpec { #[schemars(length(min = 1, max = MAX_PRIVILEGES))] pub privileges: Vec, pub on_type: ObjectType, + /// Whether the privilege must be present or absent. Matches the + /// top-level `default_privileges` entries, which carry the same field. + #[serde(default, skip_serializing_if = "Ensure::is_present")] + pub ensure: Ensure, } /// A concrete role definition (CRD-compatible version). @@ -1733,17 +1738,34 @@ impl DatabaseIdentity { pub struct OwnershipClaims { pub roles: BTreeSet, pub schemas: BTreeSet, + /// Databases whose PUBLIC privileges this policy asserts. + /// + /// A database-level rule names no schema, and PUBLIC cannot be claimed as + /// a role, so such a rule would otherwise claim nothing at all. PUBLIC is + /// one shared surface per database, so two policies writing it conflict + /// even when their roles and schemas are disjoint. Database grants to a + /// named role are already covered by the role claim, and are deliberately + /// not claimed here — two teams granting CONNECT to their own roles on a + /// shared database do not conflict. + pub public_databases: BTreeSet, } impl OwnershipClaims { pub fn overlaps(&self, other: &Self) -> bool { - !self.roles.is_disjoint(&other.roles) || !self.schemas.is_disjoint(&other.schemas) + !self.roles.is_disjoint(&other.roles) + || !self.schemas.is_disjoint(&other.schemas) + || !self.public_databases.is_disjoint(&other.public_databases) } pub fn overlap_summary(&self, other: &Self) -> String { let overlapping_roles: Vec<_> = self.roles.intersection(&other.roles).cloned().collect(); let overlapping_schemas: Vec<_> = self.schemas.intersection(&other.schemas).cloned().collect(); + let overlapping_databases: Vec<_> = self + .public_databases + .intersection(&other.public_databases) + .cloned() + .collect(); let mut parts = Vec::new(); if !overlapping_roles.is_empty() { @@ -1752,6 +1774,12 @@ impl OwnershipClaims { if !overlapping_schemas.is_empty() { parts.push(format!("schemas: {}", overlapping_schemas.join(", "))); } + if !overlapping_databases.is_empty() { + parts.push(format!( + "PUBLIC on databases: {}", + overlapping_databases.join(", ") + )); + } parts.join("; ") } @@ -2069,6 +2097,7 @@ fn build_policy_manifest<'a>( role: dp.role.clone(), privileges: dp.privileges.clone(), on_type: dp.on_type, + ensure: dp.ensure, }) .collect(), config: spec.config.clone(), @@ -2214,14 +2243,29 @@ impl PostgresPolicySpec { let mut roles: BTreeSet = expanded.roles.into_iter().map(|r| r.name).collect(); let mut schemas: BTreeSet = self.schemas.iter().map(|s| s.name.clone()).collect(); + // A database-level PUBLIC rule names no schema and no claimable role. + let public_databases: BTreeSet = manifest + .grants + .iter() + .filter(|g| g.object.object_type == ObjectType::Database && g.role == "PUBLIC") + .map(|g| g.object.name.clone().unwrap_or_default()) + .collect(); roles.extend(manifest.retirements.into_iter().map(|r| r.role)); - roles.extend(manifest.grants.iter().map(|g| g.role.clone())); + // PUBLIC is a pseudo-role, not a role this policy may claim. + roles.extend( + manifest + .grants + .iter() + .map(|g| g.role.clone()) + .filter(|role| role != "PUBLIC"), + ); roles.extend( manifest .default_privileges .iter() - .flat_map(|dp| dp.grant.iter().filter_map(|grant| grant.role.clone())), + .flat_map(|dp| dp.grant.iter().filter_map(|grant| grant.role.clone())) + .filter(|role| role != "PUBLIC"), ); roles.extend(manifest.memberships.iter().map(|m| m.role.clone())); roles.extend( @@ -2241,14 +2285,31 @@ impl PostgresPolicySpec { _ => g.object.schema.clone(), }), ); - schemas.extend( - manifest - .default_privileges - .iter() - .map(|dp| dp.schema.clone()), - ); + // Global-scope entries have no schema; their claim is the owner role. + for dp in &manifest.default_privileges { + match dp.resolved_scope() { + Ok(scope) => match scope.schema() { + Some(schema) => { + schemas.insert(schema.to_string()); + } + // An entry that omits `owner` still resolves to one, so it + // has to claim the same role the reconcile will act as. + None => { + if let Some(owner) = dp.owner.as_ref().or(manifest.default_owner.as_ref()) { + roles.insert(owner.clone()); + } + } + }, + // expand_manifest above already rejected invalid scopes. + Err(_) => continue, + } + } - Ok(OwnershipClaims { roles, schemas }) + Ok(OwnershipClaims { + roles, + schemas, + public_databases, + }) } } @@ -3162,6 +3223,54 @@ mod tests { assert!(claims.schemas.contains("inventory")); } + #[test] + fn a_global_default_privilege_claims_the_implicit_default_owner() { + let spec = PostgresPolicySpec { + connection: ConnectionSpec { + secret_ref: Some(SecretReference { + name: "pg-secret".to_string(), + }), + secret_key: Some("DATABASE_URL".to_string()), + params: None, + require_physical_identity: None, + }, + interval: "5m".to_string(), + suspend: false, + mode: PolicyMode::Apply, + reconciliation_mode: CrdReconciliationMode::default(), + default_owner: Some("app_owner".to_string()), + profiles: std::collections::HashMap::new(), + schemas: vec![], + roles: vec![], + grants: vec![], + // No `owner`, so the claim has to come from `default_owner`. + default_privileges: vec![DefaultPrivilege { + owner: None, + schema: None, + scope: Some(pgroles_core::manifest::DefaultPrivilegeScopeSpec { + scope_type: pgroles_core::manifest::DefaultPrivilegeScopeType::Global, + schema: None, + }), + grant: vec![pgroles_core::manifest::DefaultPrivilegeGrant { + role: Some("reader".to_string()), + privileges: vec![pgroles_core::manifest::Privilege::Select], + on_type: ObjectType::Table, + ensure: pgroles_core::manifest::Ensure::Present, + }], + }], + memberships: vec![], + retirements: vec![], + approval: None, + }; + + let claims = spec.ownership_claims().unwrap(); + assert!( + claims.roles.contains("app_owner"), + "expected the resolved default owner to be claimed, got {:?}", + claims.roles + ); + } + #[test] fn ownership_overlap_summary_reports_roles_and_schemas() { let mut left = OwnershipClaims::default(); @@ -4180,6 +4289,78 @@ params: assert!(summary.is_empty()); } + /// Build a spec whose only content is one database-level PUBLIC grant. + fn public_connect_spec( + database: &str, + ensure: pgroles_core::manifest::Ensure, + ) -> PostgresPolicySpec { + PostgresPolicySpec { + connection: ConnectionSpec { + secret_ref: Some(SecretReference { + name: "pg-secret".to_string(), + }), + secret_key: Some("DATABASE_URL".to_string()), + params: None, + require_physical_identity: None, + }, + interval: "5m".to_string(), + suspend: false, + mode: PolicyMode::Apply, + reconciliation_mode: CrdReconciliationMode::default(), + default_owner: None, + profiles: std::collections::HashMap::new(), + schemas: vec![], + roles: vec![], + grants: vec![Grant { + role: "PUBLIC".to_string(), + privileges: vec![pgroles_core::manifest::Privilege::Connect], + object: pgroles_core::manifest::ObjectTarget { + object_type: ObjectType::Database, + schema: None, + name: Some(database.to_string()), + }, + ensure, + }], + default_privileges: vec![], + memberships: vec![], + retirements: vec![], + approval: None, + } + } + + #[test] + fn contradictory_public_database_rules_are_detected_as_conflicting() { + use pgroles_core::manifest::Ensure; + // Disjoint in roles and schemas: both claim nothing but PUBLIC on the + // same database, and they assert opposite states for it. + let present = public_connect_spec("mydb", Ensure::Present) + .ownership_claims() + .unwrap(); + let absent = public_connect_spec("mydb", Ensure::Absent) + .ownership_claims() + .unwrap(); + + assert!(present.roles.is_disjoint(&absent.roles)); + assert!(present.schemas.is_disjoint(&absent.schemas)); + assert!( + present.overlaps(&absent), + "two policies writing PUBLIC on the same database must conflict" + ); + assert!(present.overlap_summary(&absent).contains("mydb")); + } + + #[test] + fn public_rules_on_different_databases_do_not_conflict() { + use pgroles_core::manifest::Ensure; + let left = public_connect_spec("orders", Ensure::Present) + .ownership_claims() + .unwrap(); + let right = public_connect_spec("billing", Ensure::Present) + .ownership_claims() + .unwrap(); + assert!(!left.overlaps(&right)); + } + #[test] fn ownership_claims_partial_role_overlap() { let mut left = OwnershipClaims::default(); diff --git a/crates/pgroles-operator/src/plan.rs b/crates/pgroles-operator/src/plan.rs index d0ca00e..fa2f7bd 100644 --- a/crates/pgroles-operator/src/plan.rs +++ b/crates/pgroles-operator/src/plan.rs @@ -27,7 +27,7 @@ use crate::crd::{ }; use crate::k8s_names::{LabelValue, truncate_name_prefix}; use crate::reconciler::ReconcileError; -use pgroles_core::approval::{APPROVAL_EFFECT_ENCODING_V2, TargetIdentity}; +use pgroles_core::approval::{APPROVAL_EFFECT_ENCODING_V3, TargetIdentity}; /// Result of plan creation — distinguishes genuinely new plans from /// deduplication hits so callers can decide whether to emit events. @@ -708,7 +708,7 @@ pub async fn create_or_update_plan( last_error: None, sql_hash: Some(sql_hash), change_digest: Some(change_digest.clone()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), target_physical_identity: target_identity.physical.clone(), target_logical_fingerprint: target_identity.logical.clone(), physical_identity_available: Some(target_identity.has_physical()), @@ -1543,7 +1543,7 @@ pub(crate) fn plan_matches_digest( status: &crate::crd::PostgresPolicyPlanStatus, change_digest: &str, ) -> bool { - status.change_digest_encoding.as_deref() == Some(APPROVAL_EFFECT_ENCODING_V2) + status.change_digest_encoding.as_deref() == Some(APPROVAL_EFFECT_ENCODING_V3) && status.change_digest.as_deref() == Some(change_digest) } @@ -3347,7 +3347,7 @@ mod tests { fn grant_change(role: &str) -> pgroles_core::diff::Change { pgroles_core::diff::Change::Grant { - role: role.to_string(), + role: pgroles_core::model::Grantee::parse(role), privileges: [pgroles_core::manifest::Privilege::Select] .into_iter() .collect(), @@ -3468,7 +3468,7 @@ mod tests { let pending = PostgresPolicyPlanStatus { phase: PlanPhase::Pending, change_digest: Some(planned.clone()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), revalidated_generation: Some(1), ..Default::default() }; @@ -3499,7 +3499,7 @@ mod tests { let pending = PostgresPolicyPlanStatus { phase: PlanPhase::Pending, change_digest: Some(old_digest.clone()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; // At most one plan may await a decision, so a pending plan is retired @@ -3517,7 +3517,7 @@ mod tests { let approved = PostgresPolicyPlanStatus { phase: PlanPhase::Approved, change_digest: Some(approved_digest.clone()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; @@ -3616,7 +3616,7 @@ mod tests { let current = PostgresPolicyPlanStatus { change_digest: Some(digest.clone()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; assert!(plan_matches_digest(¤t, &digest)); @@ -3638,10 +3638,22 @@ mod tests { let different_effects = PostgresPolicyPlanStatus { change_digest: Some("sha256:0000".to_string()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; assert!(!plan_matches_digest(&different_effects, &digest)); + + // A plan carrying the same digest string under v2 encodes + // default-privilege scope differently, so it is not comparable and + // must supersede instead of being accepted. + let previous_encoding = PostgresPolicyPlanStatus { + change_digest: Some(digest.clone()), + change_digest_encoding: Some( + pgroles_core::approval::APPROVAL_EFFECT_ENCODING_V2.to_string(), + ), + ..Default::default() + }; + assert!(!plan_matches_digest(&previous_encoding, &digest)); } /// A pending plan whose effects disappear before anyone decides on it must @@ -3654,7 +3666,7 @@ mod tests { let pending = PostgresPolicyPlanStatus { phase: PlanPhase::Pending, change_digest: Some(planned), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; let empty_digest = digest_for(&[], &versions); @@ -3669,7 +3681,7 @@ mod tests { let empty_plan = PostgresPolicyPlanStatus { phase: PlanPhase::Pending, change_digest: Some(empty_digest.clone()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; assert_eq!( @@ -3686,7 +3698,7 @@ mod tests { let pending = PostgresPolicyPlanStatus { phase: PlanPhase::Pending, change_digest: Some(digest_for(&original, &versions)), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; @@ -3726,7 +3738,7 @@ mod tests { let approved = PostgresPolicyPlanStatus { phase: PlanPhase::Approved, change_digest: Some(approved_digest.clone()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; @@ -3775,7 +3787,7 @@ mod tests { let approved = PostgresPolicyPlanStatus { change_digest: Some(digest_for(&changes, &versions)), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), target_physical_identity: test_target_identity().physical, target_logical_fingerprint: test_target_identity().logical, physical_identity_available: Some(true), @@ -3841,7 +3853,7 @@ mod tests { let approved = PostgresPolicyPlanStatus { phase: PlanPhase::Approved, change_digest: Some(digest_for(&[grant_change("reporting")], &versions)), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; let empty_digest = digest_for(&[], &versions); @@ -3857,7 +3869,7 @@ mod tests { let approved_empty = PostgresPolicyPlanStatus { phase: PlanPhase::Approved, change_digest: Some(empty_digest.clone()), - change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V2.to_string()), + change_digest_encoding: Some(APPROVAL_EFFECT_ENCODING_V3.to_string()), ..Default::default() }; assert_eq!( diff --git a/crates/pgroles-operator/src/reconciler.rs b/crates/pgroles-operator/src/reconciler.rs index d9dae13..0b2c9ad 100644 --- a/crates/pgroles-operator/src/reconciler.rs +++ b/crates/pgroles-operator/src/reconciler.rs @@ -153,6 +153,9 @@ pub enum ReconcileError { #[error("{0}")] UnsatisfiableWildcardGrant(String), + #[error("{0}")] + ExecutorAuthority(String), + #[error("{0}")] ConflictingPolicy(String), @@ -478,6 +481,7 @@ fn retry_class_for_reconcile_error(error: &ReconcileError) -> RetryClass { | ReconcileError::InvalidSpec(_) | ReconcileError::MissingDatabaseObjects(_) | ReconcileError::UnsatisfiableWildcardGrant(_) + | ReconcileError::ExecutorAuthority(_) | ReconcileError::ConflictingPolicy(_) | ReconcileError::UnsafeRoleDrops(_) | ReconcileError::EmptyPasswordSecret { .. } @@ -615,7 +619,14 @@ fn referenced_schema_names( } } for dp in &expanded.default_privileges { - names.insert(dp.schema.clone()); + if let Some(schema) = &dp.schema { + names.insert(schema.clone()); + } + if let Some(spec) = &dp.scope + && let Some(schema) = &spec.schema + { + names.insert(schema.clone()); + } } names } @@ -737,9 +748,10 @@ async fn reconcile_apply( } "InvalidSpec" => ctx.observability.record_invalid_spec(), "ConflictingPolicy" => ctx.observability.record_policy_conflict(), - "ApplyFailed" | "MissingDatabaseObject" | "UnsatisfiableWildcardGrant" => { - ctx.observability.record_apply_result("error") - } + "ApplyFailed" + | "MissingDatabaseObject" + | "UnsatisfiableWildcardGrant" + | "ExecutorAuthority" => ctx.observability.record_apply_result("error"), _ => {} } reconcile_guard.record_result("error", error_reason); @@ -1295,6 +1307,32 @@ async fn apply_under_lock( )); } + // Planned default-privilege changes and PUBLIC revokes need owner + // authority the executor may lack; a PUBLIC revoke without it silently + // no-ops and the controller would flap. + // + // Only executing is blocked. Producing and reviewing a plan needs no + // authority, and failing here instead would leave an operator with a + // Degraded policy and nothing to look at. + let authority_issues = + pgroles_inspect::preflight_authority_issues(pool, &changes, ¤t).await?; + let authority_block: Option = if authority_issues.is_empty() { + None + } else { + let message = authority_issues + .iter() + .map(ToString::to_string) + .collect::>() + .join("\n"); + tracing::warn!( + name, + namespace, + issues = %message, + "executor lacks the authority to apply these changes; planning continues, execution is blocked" + ); + Some(message) + }; + let summary = summarize_changes(&changes); let sql_ctx = detect_sql_context(pool, &inspect_config).await?; @@ -1524,6 +1562,14 @@ async fn apply_under_lock( // Re-fetch after approval status update. let plan = plans_api.get(&plan_name).await?; emit_plan_event(ctx, resource, &plan, PlanEventType::Approved).await; + + // The plan exists and is reviewable; this is the point past + // which it would execute, so this is where missing authority + // stops it. + if let Some(message) = &authority_block { + return Err(ReconcileError::ExecutorAuthority(message.clone())); + } + emit_plan_event(ctx, resource, &plan, PlanEventType::ApplyStarted).await; // Generated Secrets are created here, after approval and before @@ -1914,6 +1960,13 @@ async fn apply_under_lock( Api::namespaced(ctx.kube_client.clone(), namespace); let plan = plans_api.get(¤t_plan.name_any()).await?; + // The decision is recorded and the plan stays + // reviewable; this is the point past which it would + // execute, so this is where missing authority stops it. + if let Some(message) = &authority_block { + return Err(ReconcileError::ExecutorAuthority(message.clone())); + } + emit_plan_event(ctx, resource, &plan, PlanEventType::ApplyStarted).await; // Generated Secrets are created here, after the human @@ -3200,6 +3253,7 @@ impl ReconcileError { ReconcileError::ApprovalDigest(_) => "ApprovalDigestFailed", ReconcileError::ConflictingPolicy(_) => "ConflictingPolicy", ReconcileError::UnsatisfiableWildcardGrant(_) => "UnsatisfiableWildcardGrant", + ReconcileError::ExecutorAuthority(_) => "ExecutorAuthority", ReconcileError::LockContention(_, _) => "LockContention", ReconcileError::RequestIndexNotReady(_) => "RequestIndexNotReady", ReconcileError::Context(context) => match context.as_ref() { @@ -3787,7 +3841,7 @@ mod tests { accumulate_summary( &mut summary, &Change::Grant { - role: "test".to_string(), + role: "test".into(), object_type: pgroles_core::manifest::ObjectType::Schema, schema: None, name: Some("public".to_string()), @@ -3849,7 +3903,7 @@ mod tests { owner: Some("inventory_owner".to_string()), }, Change::Grant { - role: "test".to_string(), + role: "test".into(), object_type: pgroles_core::manifest::ObjectType::Schema, schema: None, name: Some("public".to_string()), @@ -3936,7 +3990,7 @@ mod tests { accumulate_summary( &mut summary, &Change::Grant { - role: "r1".to_string(), + role: "r1".into(), object_type: pgroles_core::manifest::ObjectType::Table, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -3948,7 +4002,7 @@ mod tests { accumulate_summary( &mut summary, &Change::Revoke { - role: "r1".to_string(), + role: "r1".into(), object_type: pgroles_core::manifest::ObjectType::Table, schema: Some("public".to_string()), name: Some("*".to_string()), @@ -3960,9 +4014,11 @@ mod tests { accumulate_summary( &mut summary, &Change::SetDefaultPrivilege { - schema: "public".to_string(), + scope: pgroles_core::model::DefaultPrivilegeScope::Schema { + schema: "public".to_string(), + }, owner: "owner".to_string(), - grantee: "r1".to_string(), + grantee: "r1".into(), on_type: pgroles_core::manifest::ObjectType::Table, privileges: [pgroles_core::manifest::Privilege::Select] .into_iter() @@ -3972,9 +4028,11 @@ mod tests { accumulate_summary( &mut summary, &Change::RevokeDefaultPrivilege { - schema: "public".to_string(), + scope: pgroles_core::model::DefaultPrivilegeScope::Schema { + schema: "public".to_string(), + }, owner: "owner".to_string(), - grantee: "r1".to_string(), + grantee: "r1".into(), on_type: pgroles_core::manifest::ObjectType::Table, privileges: [pgroles_core::manifest::Privilege::Select] .into_iter() @@ -4167,6 +4225,7 @@ mod tests { schemas: Vec::new(), roles: Vec::new(), grants: vec![Grant { + ensure: pgroles_core::manifest::Ensure::Present, role: "app".into(), privileges: vec![Privilege::Usage], object: ObjectTarget { @@ -4191,6 +4250,7 @@ mod tests { schemas: Vec::new(), roles: Vec::new(), grants: vec![Grant { + ensure: pgroles_core::manifest::Ensure::Present, role: "app".into(), privileges: vec![Privilege::Select], object: ObjectTarget { @@ -4216,9 +4276,11 @@ mod tests { roles: Vec::new(), grants: Vec::new(), default_privileges: vec![DefaultPrivilege { + scope: None, owner: Some("app_owner".into()), - schema: "reporting".into(), + schema: Some("reporting".to_string()), grant: vec![DefaultPrivilegeGrant { + ensure: pgroles_core::manifest::Ensure::Present, role: Some("app".into()), privileges: vec![Privilege::Select], on_type: ObjectType::Table, @@ -4241,6 +4303,7 @@ mod tests { roles: Vec::new(), grants: vec![ Grant { + ensure: pgroles_core::manifest::Ensure::Present, role: "app".into(), privileges: vec![Privilege::Usage], object: ObjectTarget { @@ -4250,6 +4313,7 @@ mod tests { }, }, Grant { + ensure: pgroles_core::manifest::Ensure::Present, role: "app".into(), privileges: vec![Privilege::Select], object: ObjectTarget { @@ -4260,9 +4324,11 @@ mod tests { }, ], default_privileges: vec![DefaultPrivilege { + scope: None, owner: Some("app_owner".into()), - schema: "shared".into(), + schema: Some("shared".to_string()), grant: vec![DefaultPrivilegeGrant { + ensure: pgroles_core::manifest::Ensure::Present, role: Some("app".into()), privileges: vec![Privilege::Select], on_type: ObjectType::Table, @@ -4285,6 +4351,7 @@ mod tests { schemas: Vec::new(), roles: Vec::new(), grants: vec![Grant { + ensure: pgroles_core::manifest::Ensure::Present, role: "app".into(), privileges: vec![Privilege::Connect], object: ObjectTarget { @@ -4367,6 +4434,7 @@ mod tests { roles: Vec::new(), grants: vec![ Grant { + ensure: pgroles_core::manifest::Ensure::Present, role: "app".into(), privileges: vec![Privilege::Usage], object: ObjectTarget { @@ -4376,6 +4444,7 @@ mod tests { }, }, Grant { + ensure: pgroles_core::manifest::Ensure::Present, role: "app".into(), privileges: vec![Privilege::Select], object: ObjectTarget { diff --git a/docs/src/pages/docs/adoption.md b/docs/src/pages/docs/adoption.md index 6823f1f..216ed3b 100644 --- a/docs/src/pages/docs/adoption.md +++ b/docs/src/pages/docs/adoption.md @@ -116,21 +116,40 @@ pgroles can create schemas that are explicitly declared under `schemas:`. Schema ## PUBLIC privilege caveats -PostgreSQL grants certain default privileges to the `PUBLIC` pseudo-role on every database (e.g. `CONNECT`, `TEMPORARY`). pgroles **does not inspect or manage PUBLIC grants**. +PostgreSQL grants some privileges to the `PUBLIC` pseudo-role on every database, +such as `CONNECT` and `TEMPORARY` on the database, `EXECUTE` on every function, +and `USAGE` on every type. Several of these have no ACL entry behind them, so +they are invisible until you look for them. -This means: +pgroles manages a `PUBLIC` privilege only where a rule names it. Until you write +one, `PUBLIC` behaves exactly as it did before: -- A role may have effective privileges not visible in `pgroles inspect` output -- A manifest that omits `TEMPORARY` does not guarantee the role lacks `TEMPORARY` — it may still inherit it from `PUBLIC` -- `additive` mode showing "no changes needed" does not mean effective privileges or existing role attributes match the manifest exactly +- A role may hold effective privileges that `pgroles inspect` does not list + among its grants. +- A manifest that omits `TEMPORARY` does not prove the role lacks it, because it + may still reach it through `PUBLIC`. +- `additive` mode reporting "no changes needed" does not mean effective + privileges match the manifest. -If least-privilege enforcement is important, you may need to manually revoke unwanted `PUBLIC` grants: +To close a gap, assert it: -```sql -REVOKE TEMPORARY ON DATABASE mydb FROM PUBLIC; -REVOKE CREATE ON SCHEMA public FROM PUBLIC; +```yaml +grants: + - role: PUBLIC + ensure: absent + privileges: [TEMPORARY] + object: { type: database, name: mydb } + - role: PUBLIC + ensure: absent + privileges: [CREATE] + object: { type: schema, name: public } ``` -{% callout type="warning" title="PUBLIC is outside pgroles scope" %} -pgroles intentionally excludes PUBLIC from inspection and management. Revoking PUBLIC grants is a manual, database-level decision that should be made carefully — it affects all roles, not just those managed by pgroles. +Read [Grants](/docs/grants#public) for how `PUBLIC` rules behave, and +[Default privileges](/docs/default-privileges#removing-public-privileges) for +removing the built-in `EXECUTE` on future functions. + +{% callout type="warning" title="Revoking from PUBLIC affects every role" %} +A `PUBLIC` revoke reaches every role in the database, not only the ones your +policy manages. Decide these deliberately, and check the plan before applying. {% /callout %} diff --git a/docs/src/pages/docs/cli.md b/docs/src/pages/docs/cli.md index 80eb767..6fb5c12 100644 --- a/docs/src/pages/docs/cli.md +++ b/docs/src/pages/docs/cli.md @@ -355,6 +355,8 @@ pgroles apply --database-url postgres://localhost/mydb --mode additive Additive mode filters out: `ALTER ROLE`, `COMMENT ON ROLE`, `REVOKE`, `REVOKE DEFAULT PRIVILEGE`, `REMOVE MEMBER`, `ALTER SCHEMA ... OWNER TO ...`, `DROP ROLE`, `DROP OWNED`, `REASSIGN OWNED`, and `TERMINATE SESSIONS`. +Because additive mode never revokes, it also ignores every `ensure: absent` rule. The rest of the plan still applies, and the run does not fail. Use `adopt` or `authoritative` when you need those assertions enforced. + If additive mode skips a schema ownership transfer, pgroles also defers owner-bound follow-up steps such as schema-owner privilege repair and `ALTER DEFAULT PRIVILEGES FOR ROLE ...` for that owner context. For brownfield roles that already exist, additive mode intentionally leaves role attributes and comments unchanged. That means a pre-existing `LOGIN NOINHERIT` role can stay that way during adoption even if a minimal manifest would otherwise imply `NOLOGIN INHERIT`. @@ -367,7 +369,7 @@ Manage declared roles fully (including revoking excess grants within their scope pgroles apply --database-url postgres://localhost/mydb --mode adopt ``` -Adopt mode filters out: `DROP ROLE`, `DROP OWNED`, `REASSIGN OWNED`, and `TERMINATE SESSIONS`. Revokes and membership removals for managed roles still apply. +Adopt mode filters out: `DROP ROLE`, `DROP OWNED`, `REASSIGN OWNED`, and `TERMINATE SESSIONS`. Revokes and membership removals for managed roles still apply. `ensure: absent` rules apply in this mode. {% callout type="note" title="Adoption path" %} A common adoption path is: start with `--mode additive` to verify the manifest produces the right grants, then move to `--mode adopt` to start revoking excess grants within managed roles, and finally switch to `--mode authoritative` when you're confident the manifest is complete. diff --git a/docs/src/pages/docs/default-privileges.md b/docs/src/pages/docs/default-privileges.md index 069e074..d5b8859 100644 --- a/docs/src/pages/docs/default-privileges.md +++ b/docs/src/pages/docs/default-privileges.md @@ -42,6 +42,128 @@ ALTER DEFAULT PRIVILEGES FOR ROLE "app_owner" GRANT SELECT, USAGE ON SEQUENCES TO "analytics"; ``` +## Scope: one schema or the whole database + +The `schema:` field above is shorthand. The long form names a scope explicitly: + +```yaml +default_privileges: + # These two entries mean the same thing. + - owner: app_owner + schema: public + grant: [...] + + - owner: app_owner + scope: { type: schema, schema: public } + grant: [...] +``` + +Global scope has no schema at all. It sets the owner's defaults for every +schema in the database: + +```yaml +default_privileges: + - owner: app_owner + scope: { type: global } + grant: + - role: analytics + privileges: [SELECT] + on_type: table +``` + +```sql +ALTER DEFAULT PRIVILEGES FOR ROLE "app_owner" + GRANT SELECT ON TABLES TO "analytics"; +``` + +An entry must set either `schema` or `scope`, never both and never neither. + +PostgreSQL layers the two. The global rule applies everywhere, and a +schema-scoped rule adds to it for that one schema. A schema-scoped rule cannot +subtract a privilege the global layer grants, which matters when you remove +`PUBLIC EXECUTE`. See [Removing PUBLIC privileges](#removing-public-privileges). + +Global scope accepts `on_type: schema` as well as table, sequence, function, +and type. Schema scope accepts everything except `schema`, because a schema is +not contained in another schema. PostgreSQL has no database-level default +privileges, so `on_type: database` is rejected in both. + +`pg_default_acl` records views and materialized views as plain tables, so +declare `on_type: table` for them. pgroles rejects `on_type: view` here rather +than emit a rule that could never converge. + +{% callout type="warning" title="Global rules reach every schema" %} +A global rule affects every schema in the database, including ones no policy +manages. `pgroles diff` counts global changes on their own line so they stand +out in a plan. +{% /callout %} + +## Removing PUBLIC privileges + +PostgreSQL grants `EXECUTE` on every new function to `PUBLIC`, and it does so +without writing an ACL entry. Granting `EXECUTE` to the roles you intend +therefore does not stop anyone else from calling the function. + +Write `ensure: absent` to assert that a privilege must not exist: + +```yaml +grants: + # Existing routines. + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: privileged_api, name: "*" } + +default_privileges: + # Future routines, everywhere this owner creates them. + - owner: function_owner + scope: { type: global } + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function + + # Defend one schema against a later schema-scoped re-grant. + - owner: function_owner + scope: { type: schema, schema: privileged_api } + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function +``` + +The global rule is what removes PostgreSQL's built-in default. The +schema-scoped rule cannot do that on its own, because schema defaults add to +the global layer instead of subtracting from it. Its job is to remove a +schema-scoped `GRANT ... TO PUBLIC` if one appears later. + +The inverse works too. Remove the privilege globally, then hand it back in one +schema: + +```yaml +default_privileges: + - owner: function_owner + scope: { type: global } + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function + - owner: function_owner + scope: { type: schema, schema: public_api } + grant: + - role: PUBLIC + privileges: [EXECUTE] + on_type: function +``` + +Read [Grants](/docs/grants) for `ensure: absent` on existing objects, and for +what pgroles does and does not manage about `PUBLIC`. The complete worked +manifest is +[examples/security-definer-api.yaml](https://github.com/hardbyte/pgroles/blob/main/examples/security-definer-api.yaml). + ## Owner context The `owner` field specifies which role's object creation triggers the default grant. This is typically the role that creates tables, such as `app_migrator` or `app_owner`. diff --git a/docs/src/pages/docs/grants.md b/docs/src/pages/docs/grants.md index b8655e3..1bfd14f 100644 --- a/docs/src/pages/docs/grants.md +++ b/docs/src/pages/docs/grants.md @@ -127,3 +127,76 @@ This is equivalent to granting `SELECT, INSERT, UPDATE` on all tables. ## Convergent revocation Privileges present in the database but absent from the manifest are revoked. If a role has `DELETE` on a table but your manifest only grants `SELECT`, pgroles will generate a `REVOKE DELETE` statement. + +Revocation is driven by the manifest as a whole, so removing a grant entry is +enough to revoke it. `PUBLIC` is the exception, described below. + +## Asserting a privilege is absent + +Some privileges exist without anyone granting them. PostgreSQL gives `EXECUTE` +on every function to `PUBLIC`, `USAGE` on every type to `PUBLIC`, and `CONNECT` +plus `TEMPORARY` on the database to `PUBLIC`. There is no ACL entry to delete, +so leaving them out of the manifest changes nothing. + +`ensure: absent` states that a privilege must not exist: + +```yaml +grants: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: privileged_api, name: "*" } +``` + +```sql +REVOKE EXECUTE ON ALL ROUTINES IN SCHEMA "privileged_api" FROM PUBLIC; +``` + +Every grant entry carries `ensure`, which defaults to `present`. An absent rule +revokes only the privileges it lists, and only where they are actually held. If +nothing holds them, it plans nothing. + +`ensure: absent` is not a PostgreSQL deny. It controls one ACL edge. A role that +also reaches the object through membership or ownership still reaches it. + +Use `ensure: absent` on the objects that exist today, and pair it with a global +default-privilege rule for the objects created tomorrow. See +[Default privileges](/docs/default-privileges#removing-public-privileges). + +## PUBLIC + +`PUBLIC` is the PostgreSQL pseudo-role that every role belongs to. Write it as +the exact uppercase value `PUBLIC` in a `role:` field: + +```yaml +grants: + - role: PUBLIC + privileges: [USAGE] + object: { type: schema, name: public_api } +``` + +The name is reserved. pgroles rejects a manifest that declares a role, a +membership, a retirement, a schema owner, or a default-privilege owner called +`PUBLIC`. A role whose name merely resembles the keyword is an ordinary role and +is quoted as one in the generated SQL. + +pgroles manages a `PUBLIC` privilege only where a rule names it. This differs +from ordinary roles: + +- A `PUBLIC` privilege no rule mentions is left alone, even in authoritative + mode. Databases are full of `PUBLIC` grants that extensions and PostgreSQL + itself created, and revoking them wholesale would break things pgroles was + never asked to manage. +- Deleting a `present` rule for `PUBLIC` therefore does not revoke anything. + Change it to `ensure: absent` when you want the privilege gone. + +`pgroles generate` never emits `PUBLIC` rules or `ensure: absent`. A privilege +that happens to be missing today is not evidence you want pgroles to keep it +missing. + +{% callout title="Migrations still matter" %} +Reconciliation is not part of your migration transaction. A function created +between two pgroles runs carries the built-in `PUBLIC EXECUTE` until the next +run. Apply the global default rule before the migration that creates the +function, or revoke inside the migration itself. +{% /callout %} diff --git a/docs/src/pages/docs/manifest-reference.md b/docs/src/pages/docs/manifest-reference.md index 74bdd1b..88e6d0b 100644 --- a/docs/src/pages/docs/manifest-reference.md +++ b/docs/src/pages/docs/manifest-reference.md @@ -235,6 +235,23 @@ Supported object `type` values: `table`, `view`, `materialized_view`, `sequence` pgroles also accepts a quoted legacy `"on"` key when parsing older manifests, but `object` is the supported spelling for new manifests and generated output. +Each entry also accepts `ensure`: + +| Field | Default | Description | +|---|---|---| +| `role` | required | Grantee. The exact value `PUBLIC` means the PostgreSQL pseudo-role | +| `ensure` | `present` | `present` grants the privileges; `absent` revokes them where held | + +```yaml +grants: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: { type: function, schema: privileged_api, name: "*" } +``` + +See [Grants](/docs/grants#asserting-a-privilege-is-absent) for what `absent` does and does not promise. + ## default_privileges Default privileges configure what happens when new objects are created: @@ -257,6 +274,30 @@ default_privileges: If `owner` is omitted, the top-level `default_owner` is used. +| Field | Default | Description | +|---|---|---| +| `owner` | `default_owner` | The role whose newly created objects get these defaults | +| `schema` | — | Shorthand for `scope: {type: schema, schema: ...}` | +| `scope` | — | `{type: schema, schema: NAME}` or `{type: global}`. Set exactly one of `schema` and `scope` | +| `grant[].role` | required | Grantee; `PUBLIC` means the pseudo-role | +| `grant[].ensure` | `present` | `absent` revokes the default where it exists | +| `grant[].on_type` | required | `table`, `sequence`, `function`, `type`, or `schema` (global scope only) | + +Global scope omits the `IN SCHEMA` clause and applies to every schema in the database: + +```yaml +default_privileges: + - owner: function_owner + scope: { type: global } + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function +``` + +`on_type: database` is rejected because PostgreSQL has no database-level default privileges. `on_type: schema` is global-only. Declare views and materialized views as `table`, which is how `pg_default_acl` records them. + ## memberships Memberships declare which roles are members of other roles: diff --git a/docs/src/pages/docs/tooling.md b/docs/src/pages/docs/tooling.md index 2923ac0..fb623fe 100644 --- a/docs/src/pages/docs/tooling.md +++ b/docs/src/pages/docs/tooling.md @@ -128,6 +128,6 @@ export DATABASE_URL=postgres://postgres:testpassword@localhost:5432/pgroles_test - Do not grant broad runtime privileges just because schema changes need them. Split schema-owner and runtime credentials. - Do not keep permanent `GRANT` policy in old setup scripts and also manage the same grants with pgroles. Pick pgroles as the source of truth once adopted. -- Do not depend on `PUBLIC` grants for application access. pgroles does not manage `PUBLIC`, so those privileges stay outside the manifest. +- Do not depend on `PUBLIC` grants for application access. pgroles manages a `PUBLIC` privilege only where a rule names it, so anything you leave undeclared stays outside the manifest. - Do not give CI or code generation jobs superuser access when read-only catalog/schema access is sufficient. - Do not use one bundle fragment per file unless the scope boundary is real. Bundle files should reflect ownership, not just directory layout. diff --git a/examples/security-definer-api.yaml b/examples/security-definer-api.yaml new file mode 100644 index 0000000..2ba630e --- /dev/null +++ b/examples/security-definer-api.yaml @@ -0,0 +1,78 @@ +# Locking down a SECURITY DEFINER API schema. +# +# PostgreSQL grants EXECUTE on every new function to PUBLIC, and it does so +# without writing an ACL entry. Granting EXECUTE to the roles you intend +# therefore does not stop anyone else from calling the function, which matters +# most for SECURITY DEFINER routines that run with their owner's rights. +# +# Removing that access takes three rules, because PostgreSQL keeps default +# privileges in two layers: an owner-wide global layer, and per-schema +# additions on top of it. A schema rule can add a privilege but never subtract +# one the global layer grants. +# +# Apply this before the migration that creates the routines. Reconciliation is +# not part of your migration transaction, so a routine created between two runs +# carries the built-in PUBLIC EXECUTE until the next one. + +roles: + # Owns the API schema and its routines. Runs the SECURITY DEFINER bodies. + - name: function_owner + login: false + + # The only role meant to call the API. + - name: allowed_executor + login: false + +grants: + # Existing routines must not be callable merely through PUBLIC. + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + object: + type: function + schema: privileged_api + name: "*" + + # Callers still need to resolve names in the schema. + - role: allowed_executor + privileges: [USAGE] + object: + type: schema + name: privileged_api + + # This assumes every routine in privileged_api is an intended API. + - role: allowed_executor + privileges: [EXECUTE] + object: + type: function + schema: privileged_api + name: "*" + +default_privileges: + # Remove PostgreSQL's built-in PUBLIC EXECUTE for every routine + # function_owner creates from now on, in any schema. Only the global layer + # can do this. + - owner: function_owner + scope: + type: global + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function + + # New routines in this schema go to the caller. The absent rule here does + # not remove the built-in default — the global rule above already did that. + # It removes a schema-scoped GRANT ... TO PUBLIC if someone adds one later. + - owner: function_owner + scope: + type: schema + schema: privileged_api + grant: + - role: PUBLIC + ensure: absent + privileges: [EXECUTE] + on_type: function + - role: allowed_executor + privileges: [EXECUTE] + on_type: function diff --git a/k8s/crd.yaml b/k8s/crd.yaml index 28ca0fb..5e336df 100644 --- a/k8s/crd.yaml +++ b/k8s/crd.yaml @@ -332,12 +332,20 @@ "default": [], "description": "One-off default privileges.", "items": { - "description": "Default privilege configuration.", + "description": "Default privilege configuration.\n\nThe scope rules are also expressed as CEL so the API server rejects a bad\nentry at apply time. `resolved_scope` enforces the same rules for the CLI,\nwhich has no admission step.", "properties": { "grant": { "items": { "description": "A single default privilege grant entry.", "properties": { + "ensure": { + "description": "Whether the listed privileges must exist or must not exist.\n\n`absent` asserts one ACL edge only. It plans a REVOKE when the privilege is\nfound live, and it says nothing about access the grantee may still have\nthrough role membership or ownership.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "on_type": { "description": "PostgreSQL object types that can have privileges granted on them.", "enum": [ @@ -376,7 +384,7 @@ "type": "array" }, "role": { - "description": "The role receiving the default privilege. Only used in top-level default_privileges\n(in profiles, the role is determined by expansion).", + "description": "The role receiving the default privilege. Only used in top-level default_privileges\n(in profiles, the role is determined by expansion). The exact-uppercase\nvalue `PUBLIC` means the PostgreSQL PUBLIC pseudo-role.", "maxLength": 63, "minLength": 1, "nullable": true, @@ -400,16 +408,54 @@ "type": "string" }, "schema": { + "description": "Schema shorthand, equivalent to `scope: {type: schema, schema: ...}`.\nExactly one of `schema` and `scope` must be set.", "maxLength": 63, "minLength": 1, + "nullable": true, "type": "string" + }, + "scope": { + "description": "Where the defaults apply: one schema, or owner-wide (global). Global\nscope renders `ALTER DEFAULT PRIVILEGES` without an `IN SCHEMA` clause.", + "nullable": true, + "properties": { + "schema": { + "description": "Schema name. Required for `type: schema`, forbidden for `type: global`.", + "maxLength": 63, + "minLength": 1, + "nullable": true, + "type": "string" + }, + "type": { + "description": "The kind of default-privilege scope.", + "enum": [ + "global", + "schema" + ], + "type": "string" + } + }, + "required": [ + "type" + ], + "type": "object", + "x-kubernetes-validations": [ + { + "message": "`schema` is required when type is `schema` and forbidden when type is `global`", + "rule": "has(self.schema) == (self.type == 'schema')" + } + ] } }, "required": [ - "grant", - "schema" + "grant" ], - "type": "object" + "type": "object", + "x-kubernetes-validations": [ + { + "message": "exactly one of `schema` and `scope` must be set", + "rule": "has(self.schema) != has(self.scope)" + } + ] }, "maxItems": 512, "type": "array" @@ -420,6 +466,14 @@ "items": { "description": "A concrete grant on a specific object or wildcard.", "properties": { + "ensure": { + "description": "Whether the listed privileges must exist or must not exist.\n\n`absent` asserts one ACL edge only. It plans a REVOKE when the privilege is\nfound live, and it says nothing about access the grantee may still have\nthrough role membership or ownership.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "object": { "description": "Target object for a grant.", "properties": { @@ -481,6 +535,7 @@ "type": "array" }, "role": { + "description": "The grantee. The exact-uppercase value `PUBLIC` means the PostgreSQL\nPUBLIC pseudo-role; any other value is an ordinary role name.", "maxLength": 63, "minLength": 1, "type": "string" @@ -579,6 +634,14 @@ "items": { "description": "Default privilege grant within a profile.", "properties": { + "ensure": { + "description": "Whether the privilege must be present or absent. Matches the\ntop-level `default_privileges` entries, which carry the same field.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "on_type": { "description": "PostgreSQL object types that can have privileges granted on them.", "enum": [ diff --git a/k8s/postgrespolicycandidate-crd.yaml b/k8s/postgrespolicycandidate-crd.yaml index c25a1e7..77d162e 100644 --- a/k8s/postgrespolicycandidate-crd.yaml +++ b/k8s/postgrespolicycandidate-crd.yaml @@ -69,12 +69,20 @@ "default_privileges": { "description": "One-off default privileges.", "items": { - "description": "Default privilege configuration.", + "description": "Default privilege configuration.\n\nThe scope rules are also expressed as CEL so the API server rejects a bad\nentry at apply time. `resolved_scope` enforces the same rules for the CLI,\nwhich has no admission step.", "properties": { "grant": { "items": { "description": "A single default privilege grant entry.", "properties": { + "ensure": { + "description": "Whether the listed privileges must exist or must not exist.\n\n`absent` asserts one ACL edge only. It plans a REVOKE when the privilege is\nfound live, and it says nothing about access the grantee may still have\nthrough role membership or ownership.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "on_type": { "description": "PostgreSQL object types that can have privileges granted on them.", "enum": [ @@ -113,7 +121,7 @@ "type": "array" }, "role": { - "description": "The role receiving the default privilege. Only used in top-level default_privileges\n(in profiles, the role is determined by expansion).", + "description": "The role receiving the default privilege. Only used in top-level default_privileges\n(in profiles, the role is determined by expansion). The exact-uppercase\nvalue `PUBLIC` means the PostgreSQL PUBLIC pseudo-role.", "maxLength": 63, "minLength": 1, "nullable": true, @@ -137,16 +145,54 @@ "type": "string" }, "schema": { + "description": "Schema shorthand, equivalent to `scope: {type: schema, schema: ...}`.\nExactly one of `schema` and `scope` must be set.", "maxLength": 63, "minLength": 1, + "nullable": true, "type": "string" + }, + "scope": { + "description": "Where the defaults apply: one schema, or owner-wide (global). Global\nscope renders `ALTER DEFAULT PRIVILEGES` without an `IN SCHEMA` clause.", + "nullable": true, + "properties": { + "schema": { + "description": "Schema name. Required for `type: schema`, forbidden for `type: global`.", + "maxLength": 63, + "minLength": 1, + "nullable": true, + "type": "string" + }, + "type": { + "description": "The kind of default-privilege scope.", + "enum": [ + "global", + "schema" + ], + "type": "string" + } + }, + "required": [ + "type" + ], + "type": "object", + "x-kubernetes-validations": [ + { + "message": "`schema` is required when type is `schema` and forbidden when type is `global`", + "rule": "has(self.schema) == (self.type == 'schema')" + } + ] } }, "required": [ - "grant", - "schema" + "grant" ], - "type": "object" + "type": "object", + "x-kubernetes-validations": [ + { + "message": "exactly one of `schema` and `scope` must be set", + "rule": "has(self.schema) != has(self.scope)" + } + ] }, "maxItems": 512, "type": "array" @@ -156,6 +202,14 @@ "items": { "description": "A concrete grant on a specific object or wildcard.", "properties": { + "ensure": { + "description": "Whether the listed privileges must exist or must not exist.\n\n`absent` asserts one ACL edge only. It plans a REVOKE when the privilege is\nfound live, and it says nothing about access the grantee may still have\nthrough role membership or ownership.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "object": { "description": "Target object for a grant.", "properties": { @@ -217,6 +271,7 @@ "type": "array" }, "role": { + "description": "The grantee. The exact-uppercase value `PUBLIC` means the PostgreSQL\nPUBLIC pseudo-role; any other value is an ordinary role name.", "maxLength": 63, "minLength": 1, "type": "string" @@ -298,6 +353,14 @@ "items": { "description": "Default privilege grant within a profile.", "properties": { + "ensure": { + "description": "Whether the privilege must be present or absent. Matches the\ntop-level `default_privileges` entries, which carry the same field.", + "enum": [ + "present", + "absent" + ], + "type": "string" + }, "on_type": { "description": "PostgreSQL object types that can have privileges granted on them.", "enum": [ diff --git a/skills/pgroles-policy/SKILL.md b/skills/pgroles-policy/SKILL.md index f0bbdc2..50f7f6f 100644 --- a/skills/pgroles-policy/SKILL.md +++ b/skills/pgroles-policy/SKILL.md @@ -56,7 +56,8 @@ changes remain in the plan: - `additive`: creates and additions only. It filters revokes, membership removals, existing schema-owner transfers, existing-role rewrites, and role retirement. It also omits role comments. Configured password updates are a - deliberate exception. + deliberate exception. Because it never revokes, it silently ignores every + `ensure: absent` rule without failing the run. - `adopt`: authoritative convergence except role drops and their retirement steps. Revokes and membership removals still occur. - `authoritative`: retains every change computed within pgroles' managed @@ -99,12 +100,24 @@ ACL becomes visible, authoritative reconciliation can revoke privileges not declared for that managed role. Declare the ordinary object privileges an owner must retain when applications use that role for DML. +PostgreSQL materializes an object's whole ACL, owner entry included, the first +time anything is granted or revoked on it. Any first-time grant does this, and +so does revoking `EXECUTE` from PUBLIC. Expect one extra convergence pass on +such objects, and declare the owner privileges that must survive it. + Ownership rights such as altering or dropping an object, and the owner's implicit grant options, remain PostgreSQL behavior outside the object ACL model. -Default privileges are per creating role, schema, and object type. A default -owner declaration does not retroactively grant existing objects and does not -cover objects created by another role. +Default privileges are per creating role, scope, and object type, where a scope +is one schema or the owner-wide global layer. A default owner declaration does +not retroactively grant existing objects and does not cover objects created by +another role. + +Schema defaults add to the global layer and cannot subtract from it. Removing +PostgreSQL's built-in `PUBLIC EXECUTE` on functions therefore needs a global +rule with `ensure: absent`; a schema-scoped one only removes a schema-scoped +re-grant. Global rules reach every schema in the database, so only the bundle +document that owns the owner role may declare them. ## Privilege Review @@ -118,8 +131,11 @@ Review effective and transitive privileges, not role names alone. schema-wide wildcards as security-sensitive. - Owner, definer, and other group roles may carry much broader inherited access than the new membership suggests. -- PUBLIC and column-level grants are outside desired-state reconciliation. Read - inspection warnings and review them separately. +- Column-level grants are outside desired-state reconciliation. Read inspection + warnings and review them separately. +- PUBLIC is reconciled only where a rule names it. A privilege PUBLIC holds that + no rule mentions is left alone in every mode, so deleting a `present` PUBLIC + rule does not revoke anything — switch it to `ensure: absent` instead. ## Safe Removal