From 0825d37b0096465040de8212775a9cb55a1d36fa Mon Sep 17 00:00:00 2001 From: Brian Thorne Date: Sun, 8 Nov 2026 02:28:51 +1300 Subject: [PATCH] fix(queue-storage): refresh SET-only lane repairs on 0.6 --- CHANGELOG.md | 1 + .../v023_install_queue_storage_substrate.sql | 40 ++++-- awa-model/src/migrations.rs | 5 +- .../v0614_alpha1_lane_sequence_repair.sql | 83 +++++++++++++ awa/tests/lane_ownership_roles_test.rs | 117 ++++++++++++++++++ docs/security.md | 12 ++ docs/upgrade-0.6.13-to-0.6.14.md | 12 +- 7 files changed, 253 insertions(+), 17 deletions(-) create mode 100644 awa/tests/fixtures/v0614_alpha1_lane_sequence_repair.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a7935542..76f240f48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ order; binaries can roll back to 0.6.13 with the patch retained. See the ### Fixed +- Lane-sequence repair completes missing runtime grants when the migration role can `SET ROLE` to the schema owner without inheriting its privileges, and restores its prior role afterward. `awa migrate` refreshes an older installed repair helper even when the versionless provisioning patch was already applied. - Schema-only admin clients use installed ready-slot numbers for point reads, cancellation and retained-body recovery, including rings larger than the default. - New queue lanes and their sequences belong to the schema owner and receive the head table's grants, regardless of which runtime creates them. Migration diff --git a/awa-model/migrations/v023_install_queue_storage_substrate.sql b/awa-model/migrations/v023_install_queue_storage_substrate.sql index 4625ebada..57f56c7f7 100644 --- a/awa-model/migrations/v023_install_queue_storage_substrate.sql +++ b/awa-model/migrations/v023_install_queue_storage_substrate.sql @@ -511,13 +511,12 @@ AS $problems$ AND NOT candidate.rolsuper AND pg_has_role(candidate.oid, lane_sequence.relowner, 'USAGE') AND (lane_sequence.relowner = lane_sequence.nspowner - OR (pg_has_role(candidate.oid, lane_sequence.nspowner, 'USAGE') - AND pg_has_role( + OR pg_has_role( candidate.oid, lane_sequence.nspowner, CASE WHEN current_setting('server_version_num')::int >= 160000 THEN 'SET' ELSE 'MEMBER' END - ))) + )) ORDER BY candidate.rolname ) FROM lane_sequence @@ -555,6 +554,9 @@ AS $repair$ DECLARE v_problem RECORD; v_remaining RECORD; + v_original_role TEXT := current_setting('role'); + v_reassigned BOOLEAN := false; + v_role_changed BOOLEAN := false; v_busy BOOLEAN := false; v_set_mode TEXT := CASE WHEN current_setting('server_version_num')::int >= 160000 THEN 'SET' ELSE 'MEMBER' END; @@ -576,6 +578,7 @@ BEGIN AND pg_has_role(current_user, to_regrole(quote_ident(v_problem.expected_owner)), v_set_mode) THEN BEGIN EXECUTE v_problem.reassign_sql; + v_reassigned := true; EXCEPTION WHEN insufficient_privilege THEN NULL; END; @@ -585,14 +588,29 @@ BEGIN FROM awa.lane_sequence_problems(p_schema, p_sequence) AS problem; IF FOUND AND cardinality(v_remaining.missing_grants) > 0 - AND pg_has_role(current_user, to_regrole(quote_ident(v_remaining.owner)), 'USAGE') THEN - FOREACH v_statement IN ARRAY v_remaining.missing_grants LOOP - BEGIN - EXECUTE v_statement; - EXCEPTION WHEN insufficient_privilege THEN - NULL; - END; - END LOOP; + AND (pg_has_role(current_user, to_regrole(quote_ident(v_remaining.owner)), 'USAGE') + OR v_reassigned) THEN + BEGIN + IF v_reassigned + AND NOT pg_has_role(current_user, to_regrole(quote_ident(v_remaining.owner)), 'USAGE') THEN + PERFORM set_config('role', v_remaining.owner, true); + v_role_changed := true; + END IF; + + FOREACH v_statement IN ARRAY v_remaining.missing_grants LOOP + BEGIN + EXECUTE v_statement; + EXCEPTION WHEN insufficient_privilege THEN + NULL; + END; + END LOOP; + + IF v_role_changed THEN + PERFORM set_config('role', v_original_role, true); + END IF; + EXCEPTION WHEN insufficient_privilege THEN + NULL; + END; END IF; EXCEPTION WHEN lock_not_available THEN v_busy := true; diff --git a/awa-model/src/migrations.rs b/awa-model/src/migrations.rs index d6b4a6734..549dcabef 100644 --- a/awa-model/src/migrations.rs +++ b/awa-model/src/migrations.rs @@ -307,7 +307,10 @@ macro_rules! definer_lane_provisioning_pending_sql { WHERE to_regprocedure('awa.lane_sequence_problems(text,text)') IS NULL \ UNION ALL \ SELECT 'awa.repair_lane_sequence' \ - WHERE to_regprocedure('awa.repair_lane_sequence(text,text)') IS NULL \ + WHERE NOT EXISTS ( \ + SELECT 1 FROM pg_proc AS p \ + WHERE p.oid = to_regprocedure('awa.repair_lane_sequence(text,text)') \ + AND p.prosrc LIKE '%set_config(''role'', v_original_role, true)%') \ UNION ALL \ SELECT 'awa.refresh_lane_sequence_helpers' \ WHERE EXISTS ( \ diff --git a/awa/tests/fixtures/v0614_alpha1_lane_sequence_repair.sql b/awa/tests/fixtures/v0614_alpha1_lane_sequence_repair.sql new file mode 100644 index 000000000..70bcfefcc --- /dev/null +++ b/awa/tests/fixtures/v0614_alpha1_lane_sequence_repair.sql @@ -0,0 +1,83 @@ +CREATE OR REPLACE FUNCTION awa.repair_lane_sequence(p_schema TEXT, p_sequence TEXT) +RETURNS TABLE ( + sequence_name TEXT, + owner TEXT, + expected_owner TEXT, + outcome TEXT, + missing_grants TEXT[], + reassign_sql TEXT, + repair_roles TEXT[] +) +LANGUAGE plpgsql +SECURITY INVOKER +SET search_path = pg_catalog, pg_temp +SET lock_timeout = '300ms' +AS $repair$ +DECLARE + v_problem RECORD; + v_remaining RECORD; + v_busy BOOLEAN := false; + v_set_mode TEXT := CASE WHEN current_setting('server_version_num')::int >= 160000 + THEN 'SET' ELSE 'MEMBER' END; + v_statement TEXT; +BEGIN + SELECT * INTO v_problem + FROM awa.lane_sequence_problems(p_schema, p_sequence) AS problem; + IF NOT FOUND THEN + RETURN; + END IF; + + BEGIN + -- ALTER ... OWNER TO needs the current owner's privileges, SET ROLE to + -- the new owner, and the new owner's CREATE on the schema (which a + -- schema owner has). Check first so an unrelated owner is skipped + -- without waiting for the lock. + IF v_problem.reassign_sql IS NOT NULL + AND pg_has_role(current_user, to_regrole(quote_ident(v_problem.owner)), 'USAGE') + AND pg_has_role(current_user, to_regrole(quote_ident(v_problem.expected_owner)), v_set_mode) THEN + BEGIN + EXECUTE v_problem.reassign_sql; + EXCEPTION WHEN insufficient_privilege THEN + NULL; + END; + END IF; + + SELECT * INTO v_remaining + FROM awa.lane_sequence_problems(p_schema, p_sequence) AS problem; + IF FOUND + AND cardinality(v_remaining.missing_grants) > 0 + AND pg_has_role(current_user, to_regrole(quote_ident(v_remaining.owner)), 'USAGE') THEN + FOREACH v_statement IN ARRAY v_remaining.missing_grants LOOP + BEGIN + EXECUTE v_statement; + EXCEPTION WHEN insufficient_privilege THEN + NULL; + END; + END LOOP; + END IF; + EXCEPTION WHEN lock_not_available THEN + v_busy := true; + END; + + SELECT * INTO v_remaining + FROM awa.lane_sequence_problems(p_schema, p_sequence) AS problem; + sequence_name := v_problem.sequence_name; + expected_owner := v_problem.expected_owner; + IF FOUND THEN + owner := v_remaining.owner; + missing_grants := v_remaining.missing_grants; + reassign_sql := v_remaining.reassign_sql; + repair_roles := v_remaining.repair_roles; + outcome := CASE WHEN v_busy THEN 'busy' + WHEN cardinality(v_remaining.missing_grants) = 0 THEN 'owner_unchanged' + ELSE 'unrepaired' END; + ELSE + owner := v_problem.expected_owner; + missing_grants := ARRAY[]::TEXT[]; + reassign_sql := NULL; + repair_roles := ARRAY[]::TEXT[]; + outcome := 'repaired'; + END IF; + RETURN NEXT; +END +$repair$; diff --git a/awa/tests/lane_ownership_roles_test.rs b/awa/tests/lane_ownership_roles_test.rs index 026511f77..f7b85eea0 100644 --- a/awa/tests/lane_ownership_roles_test.rs +++ b/awa/tests/lane_ownership_roles_test.rs @@ -776,6 +776,123 @@ async fn migration_repairs_lanes_it_may_and_reports_the_rest() -> TestResult { cluster.teardown().await } +#[tokio::test] +async fn set_only_schema_owner_membership_repairs_legacy_lanes_and_restores_role() -> TestResult { + let cluster = Cluster::new(Schema::BeforeProvisioning, false).await?; + let queue = "set_only_owner_lane"; + assert_eq!(enqueue(&cluster.migrator, queue, 1, 1).await?, 1); + let sequences = cluster.lane_sequence_owners(queue).await?; + assert_eq!(sequences.len(), 2); + for (sequence, owner) in &sequences { + assert_eq!(owner, &cluster.migrator_name); + cluster + .migrator + .execute(sqlx::raw_sql(&sql(format!( + "REVOKE ALL ON SEQUENCE awa.{sequence} FROM {}", + cluster.runtime_name + )))) + .await?; + } + for (_, _, statement) in migrations::schema_patch_sql(migrations::SCHEMA_PATCHES) { + cluster.owner.execute(sqlx::raw_sql(&statement)).await?; + } + cluster + .owner + .execute(sqlx::raw_sql(include_str!( + "fixtures/v0614_alpha1_lane_sequence_repair.sql" + ))) + .await?; + let pending = migrations::pending_schema_patches(&cluster.owner).await?; + assert_eq!(pending.len(), 1, "{pending:?}"); + assert_eq!(pending[0].name, "definer_lane_provisioning"); + let report = migrations::run_with_report(&cluster.owner).await?; + assert!(report.complete(), "{:?}", report.patches); + assert!(migrations::pending_schema_patches(&cluster.owner) + .await? + .is_empty()); + cluster + .owner + .execute(sqlx::raw_sql(&sql(format!( + "GRANT USAGE, CREATE ON SCHEMA awa TO {migrator}; \ + GRANT EXECUTE ON FUNCTION awa.repair_lane_sequence(text, text) TO {migrator}", + migrator = cluster.migrator_name + )))) + .await?; + cluster + .admin + .execute(sqlx::raw_sql(&sql(format!( + "GRANT {owner} TO {migrator} WITH INHERIT FALSE; \ + GRANT {owner} TO {migrator} WITH SET TRUE", + owner = cluster.owner_name, + migrator = cluster.migrator_name + )))) + .await?; + + let mut conn = cluster.migrator.acquire().await?; + let (inherits_owner, can_set_owner, can_create): (bool, bool, bool) = sqlx::query_as( + "SELECT pg_has_role(current_user, $1, 'USAGE'), \ + pg_has_role(current_user, $1, 'SET'), \ + has_schema_privilege(current_user, 'awa', 'USAGE') \ + AND has_schema_privilege(current_user, 'awa', 'CREATE')", + ) + .bind(&cluster.owner_name) + .fetch_one(&mut *conn) + .await?; + assert!(!inherits_owner); + assert!(can_set_owner); + assert!(can_create); + let role_before: (String, String) = + sqlx::query_as("SELECT current_user::text, current_setting('role')") + .fetch_one(&mut *conn) + .await?; + assert_eq!(role_before.0, cluster.migrator_name); + + let problems = storage::lane_sequence_problems(&mut *conn, "awa").await?; + assert_eq!(problems.len(), 2, "{problems:?}"); + for problem in &problems { + assert_eq!(problem.owner, cluster.migrator_name); + assert!(!problem.missing_grants.is_empty(), "{problem:?}"); + assert!( + problem.repair_roles.contains(&cluster.migrator_name), + "{problem:?}" + ); + } + let repaired = storage::repair_lane_sequences(&mut conn, "awa").await?; + assert_eq!(repaired.len(), 2, "{repaired:?}"); + for repair in &repaired { + assert_eq!(repair.outcome, LaneSequenceOutcome::Repaired, "{repair:?}"); + assert_eq!(repair.owner, cluster.owner_name); + assert!(repair.missing_grants.is_empty(), "{repair:?}"); + assert!(repair.reassign_sql.is_none(), "{repair:?}"); + } + let role_after: (String, String) = + sqlx::query_as("SELECT current_user::text, current_setting('role')") + .fetch_one(&mut *conn) + .await?; + assert_eq!(role_after, role_before); + assert!(storage::lane_sequence_problems(&mut *conn, "awa") + .await? + .is_empty()); + drop(conn); + + for (sequence, _) in sequences { + let runtime_grants_complete: bool = sqlx::query_scalar( + "SELECT has_sequence_privilege($1, $2, 'USAGE') \ + AND has_sequence_privilege($1, $2, 'SELECT') \ + AND has_sequence_privilege($1, $2, 'UPDATE')", + ) + .bind(&cluster.runtime_name) + .bind(format!("awa.{sequence}")) + .fetch_one(&cluster.owner) + .await?; + assert!(runtime_grants_complete, "{sequence}"); + } + cluster.assert_lanes_owned_by_owner(queue, 1).await?; + assert_eq!(enqueue(&cluster.app_b, queue, 1, 1).await?, 1); + assert_eq!(drain(&cluster.app_b, queue).await?, 2); + cluster.teardown().await +} + /// Reassigning a lane sequence waits on any open enqueue or claim on the lane, /// so the repair bounds that wait and holds each sequence's lock only for its /// own short transaction. A held sequence is skipped and reported; the rest diff --git a/docs/security.md b/docs/security.md index 2b9848ec7..68a5dcf5c 100644 --- a/docs/security.md +++ b/docs/security.md @@ -209,6 +209,18 @@ Which role can repair a sequence: The schema owner holds neither the migration role's nor an application role's privileges, so a repair running as `awa_owner` cannot reassign sequences those roles created. That includes the migration role connected with `PGOPTIONS='-c role=awa_owner'`. Run the repair as the migration role itself, without `PGOPTIONS`, as above. `awa migrate` and `--repair` retry each sequence the effective role could not repair as the login role, so `awa migrate` run with `PGOPTIONS='-c role=awa_owner'`, as documented, still repairs the migration role's sequences. +For a migrator with SET-only membership in `awa_owner`, grant schema access and helper execution explicitly before running the repair. Run as the schema owner: + +```sql +GRANT USAGE ON SCHEMA awa TO awa_migrator; +GRANT EXECUTE ON FUNCTION + awa.lane_sequence_problems(TEXT, TEXT), + awa.repair_lane_sequence(TEXT, TEXT) + TO awa_migrator; +``` + +For a custom queue-storage schema, also grant `USAGE` on that schema. The migrator must still hold the old sequence owner's privileges and be allowed to `SET ROLE` to the schema owner; inherited privileges from the schema owner are not required. + For each sequence that remains, the output lists `repair_roles`, the login roles other than superusers that can repair it, and the statements to run. A busy sequence only needs another `--repair`. When `repair_roles` is empty, which is usual for a sequence an application role created on managed PostgreSQL, either: - run `missing_grants` as the current owner. Every runtime role can then use the lane, although the owner stays unchanged; or diff --git a/docs/upgrade-0.6.13-to-0.6.14.md b/docs/upgrade-0.6.13-to-0.6.14.md index b18fa715e..7f07fe0bc 100644 --- a/docs/upgrade-0.6.13-to-0.6.14.md +++ b/docs/upgrade-0.6.13-to-0.6.14.md @@ -5,17 +5,17 @@ ## Upgrade 1. Install the 0.6.14 CLI. -2. Check lane ownership. This is read-only and exits 2 while lanes need repair: +2. Run `awa migrate` once, with the schema owner as the effective role, while workers keep running: ```bash - awa --database-url "$AWA_DATABASE_URL" storage lane-sequences + PGOPTIONS='-c role=awa_owner' \ + awa --database-url "$AWA_MIGRATOR_DATABASE_URL" migrate ``` -3. Run `awa migrate` once, with the schema owner as the effective role, while workers keep running: +3. Check lane ownership after the patch installs its diagnostic helper. This is read-only and exits 2 while lanes need repair: ```bash - PGOPTIONS='-c role=awa_owner' \ - awa --database-url "$AWA_MIGRATOR_DATABASE_URL" migrate + awa --database-url "$AWA_DATABASE_URL" storage lane-sequences ``` 4. Read the output. The `definer_lane_provisioning` patch reports `applied`, followed by how many lane sequences were repaired. Any left over are listed in warnings: a busy one needs another run of `awa storage lane-sequences --repair` as the migration role (without `PGOPTIONS`), and the others name the roles that can repair them (see [Check and repair lane ownership](security.md#check-and-repair-lane-ownership)). The migration exit status does not depend on them. @@ -27,6 +27,8 @@ The schema stays at v40. The `definer_lane_provisioning` patch installs `provisi Once the patch commits, `awa migrate` repairs existing lane sequences, each in its own short transaction. A sequence not owned by the schema owner is reassigned if the role holds its current owner's privileges and can `SET ROLE` to the schema owner. Missing grants are added if the role holds the owner's privileges. Each sequence is tried as the effective role and then as the login role. Reassigning waits at most 300 ms for the sequence's lock, which an open enqueue or claim on that lane holds, and holds it only for that sequence's transaction. A sequence that is busy, or that neither role may change, is reported and left as it was. +If an earlier build already applied the patch, run `awa migrate` again with the updated CLI. It detects the older installed repair helper and reapplies the repeatable patch before attempting repairs. + The patch: - only replaces functions; it takes no sequence or table locks;