From ffc20f581f60ee356d35b90045a6d01f837cb28c Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Tue, 27 Oct 2026 05:18:28 -0400 Subject: [PATCH] fix(accounts): ensuring a derived role that is already as declared writes nothing Every reconcile pass converges the fixture's installations, and an installation that cannot converge yet (a solution whose install step is not acknowledged, so its teams wait) is retried each pass. The retry resolves the installation's solution role through EnsureDerivedRole, which upserted the role and deleted and re-inserted every permission. roles and role_permissions carry BEFORE triggers that move the organization's authorization revision on every row they see, an upsert that changes nothing included, so each pass moved the revision about nine times per role and every capability minted before it was refused ("authorization revision superseded: minted at revision 2473, issuer is at 2495"). EnsureDerivedRole now reads the role and its permissions and writes only what differs: a missing role, a changed description, a permission removed or added. Idle reconcile passes and idle fixture convergence leave the revision where it is; a real change still moves it. The fold of the envelope revision into module installation revisions (#1257) is not the cause: reconcile passes alone left the revision unchanged. Co-Authored-By: Claude Opus 5.5 --- .../host_interface_conformance_test.go | 28 ++++++ .../pkg/infra/derived_role_revision_test.go | 79 ++++++++++++++++ .../infra/postgres_derived_module_access.go | 93 +++++++++++++------ 3 files changed, 174 insertions(+), 26 deletions(-) create mode 100644 module/services/accounts/code/pkg/infra/derived_role_revision_test.go diff --git a/module/services/accounts/code/pkg/adapters/host_interface_conformance_test.go b/module/services/accounts/code/pkg/adapters/host_interface_conformance_test.go index 3a63a8d51..e3f95bbbe 100644 --- a/module/services/accounts/code/pkg/adapters/host_interface_conformance_test.go +++ b/module/services/accounts/code/pkg/adapters/host_interface_conformance_test.go @@ -816,6 +816,34 @@ func runHostInterfaceFixture(t *testing.T) { // Beyond the suite: what the core changes is in the audit record, in the // transaction that changed it, naming the organization. + // A capability is minted at the organization's authorization revision and + // refused once it moves, so the host must move it only for a real change. + // Idle reconcile passes, and fixture convergence retrying an installation + // that cannot converge yet (a solution whose install step is not + // acknowledged, so its teams wait), leave it where it is; a real change of + // a role moves it. + t.Run("idle reconcile passes and fixture convergence leave the authorization revision alone", func(t *testing.T) { + revision := func() int64 { + t.Helper() + var r int64 + require.NoError(t, store.WithControlPlane(ctx, func(ctx context.Context) error { + return storetx.Tx(ctx).QueryRow(ctx, `SELECT revision FROM public.organization_authorization_revisions WHERE org_id = $1`, fixtureOrganization).Scan(&r) + })) + return r + } + _ = installations.Converge(ctx) // the product's own records of the declared installations: a real change + settled := revision() + for range 3 { + _ = reconciler.RunOnce(ctx) + _ = installations.Converge(ctx) + } + require.Equal(t, settled, revision(), "an idle pass moved the authorization revision, superseding every capability minted before it") + require.NoError(t, store.WithOrgTx(ctx, fixtureOrganization, func(ctx context.Context) error { + return store.CreateRole(ctx, &gen.Role{Id: business.NewIDString(), OrgId: fixtureOrganization, Name: "Revision probe", Permissions: []*gen.Permission{{Resource: "example-records", Action: "read"}}}) + })) + require.Greater(t, revision(), settled, "a real change of a role moves it") + }) + t.Run("every change of authority is audited", func(t *testing.T) { var count int require.NoError(t, store.WithControlPlane(ctx, func(ctx context.Context) error { diff --git a/module/services/accounts/code/pkg/infra/derived_role_revision_test.go b/module/services/accounts/code/pkg/infra/derived_role_revision_test.go new file mode 100644 index 000000000..fb056cfa8 --- /dev/null +++ b/module/services/accounts/code/pkg/infra/derived_role_revision_test.go @@ -0,0 +1,79 @@ +//go:build !pure + +package infra_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "accounts/pkg/business" + gen "accounts/pkg/gen/saas/accounts/v1" + "accounts/pkg/infra/storetx" +) + +// Ensuring a derived role that is already as declared writes nothing, so it +// leaves the organization's authorization revision (what every capability is +// minted at) where it is; a changed description or permission set is written, +// and only what changed. +func TestEnsuringADerivedRoleAsDeclaredMovesNoRevision(t *testing.T) { + userID := seedUser(t) + orgID := seedOrg(t, userID) + revision := func() int64 { + t.Helper() + var r int64 + require.NoError(t, testStore.WithControlPlane(testCtx, func(ctx context.Context) error { + return storetx.Tx(ctx).QueryRow(ctx, `SELECT revision FROM public.organization_authorization_revisions WHERE org_id = $1`, orgID).Scan(&r) + })) + return r + } + permissions := func(roleID string) []string { + t.Helper() + var out []string + require.NoError(t, testStore.WithControlPlane(testCtx, func(ctx context.Context) error { + rows, err := storetx.Tx(ctx).Query(ctx, `SELECT resource || ':' || action FROM role_permissions WHERE role_id = $1`, roleID) + if err != nil { + return err + } + defer rows.Close() + for rows.Next() { + var p string + if err := rows.Scan(&p); err != nil { + return err + } + out = append(out, p) + } + return rows.Err() + })) + return out + } + ensure := func(description string, perms ...*gen.Permission) string { + t.Helper() + var id string + require.NoError(t, testStore.WithOrgTx(testCtx, orgID, func(ctx context.Context) error { + var err error + id, err = testStore.EnsureDerivedRole(ctx, business.DerivedRole{OrgID: orgID, DerivedFrom: "installation=revision-test;role=viewer", + Name: "solution role · revision-test", Description: description, Permissions: perms}) + return err + })) + return id + } + use, read, write := &gen.Permission{Resource: "solution", Action: "use"}, &gen.Permission{Resource: "doc", Action: "read"}, &gen.Permission{Resource: "doc", Action: "write"} + + id := ensure("the viewer role", use, read) + settled := revision() + for range 3 { + require.Equal(t, id, ensure("the viewer role", use, read)) + } + require.Equal(t, settled, revision(), "ensuring a role already as declared wrote it") + + require.Equal(t, id, ensure("the viewer role", use, write)) + require.ElementsMatch(t, []string{"solution:use", "doc:write"}, permissions(id)) + moved := revision() + require.Greater(t, moved, settled, "a changed permission set is written") + + require.Equal(t, id, ensure("the viewer role, renamed", use, write)) + require.Greater(t, revision(), moved, "a changed description is written") + require.ElementsMatch(t, []string{"solution:use", "doc:write"}, permissions(id)) +} diff --git a/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go b/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go index 888132efc..f03bce5f4 100644 --- a/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go +++ b/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go @@ -95,36 +95,77 @@ func (s *PostgresStore) EnsureDerivedRole(ctx context.Context, role business.Der return "", fmt.Errorf("a derived role must name what it was derived from") } - var roleID string - // The conflict target is the partial unique index on derived_from, so two - // grants of the same cause serialize on the row rather than both inserting. - // - // Only the DESCRIPTION is refreshed. `app_tenant` holds UPDATE on that one - // column (migration 5), column-scoped on purpose, and the derived role's name - // is the provenance precisely so that it never needs rewriting — see - // business.derivedRoleName. - if err := executor.QueryRow(ctx, ` - INSERT INTO roles (id, name, description, built_in, org_id, derived_from, created_at) - VALUES ($1, $2, $3, false, $4, $5, CURRENT_TIMESTAMP) - ON CONFLICT (derived_from) WHERE derived_from IS NOT NULL - DO UPDATE SET description = EXCLUDED.description - RETURNING id::text`, - business.NewIDString(), role.Name, role.Description, role.OrgID, role.DerivedFrom, - ).Scan(&roleID); err != nil { - return "", w.Wrapf(err, "failed to write the derived role") + // Every write to roles or role_permissions moves the organization's + // authorization revision (their BEFORE triggers fire even on an upsert + // that changes nothing), and every capability minted before it is then + // refused. A role already as declared is therefore read, never written: + // only a role that is missing, or whose description or permissions + // differ, is touched, and only by what differs. + var roleID, description string + err := executor.QueryRow(ctx, `SELECT id::text, COALESCE(description, '') FROM roles WHERE derived_from = $1`, role.DerivedFrom).Scan(&roleID, &description) + switch { + case errors.Is(err, pgx.ErrNoRows): + // The conflict target is the partial unique index on derived_from, so + // two grants of the same cause serialize on the row rather than both + // inserting. Only the DESCRIPTION is refreshed: `app_tenant` holds + // UPDATE on that one column (migration 5), column-scoped on purpose, + // and the derived role's name is the provenance precisely so that it + // never needs rewriting — see business.derivedRoleName. + if err := executor.QueryRow(ctx, ` + INSERT INTO roles (id, name, description, built_in, org_id, derived_from, created_at) + VALUES ($1, $2, $3, false, $4, $5, CURRENT_TIMESTAMP) + ON CONFLICT (derived_from) WHERE derived_from IS NOT NULL + DO UPDATE SET description = EXCLUDED.description + RETURNING id::text`, + business.NewIDString(), role.Name, role.Description, role.OrgID, role.DerivedFrom, + ).Scan(&roleID); err != nil { + return "", w.Wrapf(err, "failed to write the derived role") + } + case err != nil: + return "", w.Wrapf(err, "failed to read the derived role") + case description != role.Description: + if _, err := executor.Exec(ctx, `UPDATE roles SET description = $2 WHERE id = $1`, roleID, role.Description); err != nil { + return "", w.Wrapf(err, "failed to refresh the derived role's description") + } } - if _, err := executor.Exec(ctx, `DELETE FROM role_permissions WHERE role_id = $1`, roleID); err != nil { - return "", w.Wrapf(err, "failed to clear the derived role's permissions") + rows, err := executor.Query(ctx, `SELECT resource, action FROM role_permissions WHERE role_id = $1`, roleID) + if err != nil { + return "", w.Wrapf(err, "failed to read the derived role's permissions") } + held := map[[2]string]bool{} + for rows.Next() { + var resource, action string + if err := rows.Scan(&resource, &action); err != nil { + rows.Close() + return "", w.Wrapf(err, "failed to read the derived role's permissions") + } + held[[2]string{resource, action}] = true + } + rows.Close() + if err := rows.Err(); err != nil { + return "", w.Wrapf(err, "failed to read the derived role's permissions") + } + declared := map[[2]string]bool{} for _, permission := range role.Permissions { - if _, err := executor.Exec(ctx, ` - INSERT INTO role_permissions (role_id, resource, action) - VALUES ($1, $2, $3) ON CONFLICT DO NOTHING`, - roleID, permission.Resource, permission.Action, - ); err != nil { - return "", w.Wrapf(err, "failed to write the derived role's permission %s:%s", - permission.Resource, permission.Action) + declared[[2]string{permission.Resource, permission.Action}] = true + } + for permission := range held { + if declared[permission] { + continue + } + if _, err := executor.Exec(ctx, `DELETE FROM role_permissions WHERE role_id = $1 AND resource = $2 AND action = $3`, + roleID, permission[0], permission[1]); err != nil { + return "", w.Wrapf(err, "failed to remove the derived role's permission %s:%s", permission[0], permission[1]) + } + } + for permission := range declared { + if held[permission] { + continue + } + if _, err := executor.Exec(ctx, `INSERT INTO role_permissions (role_id, resource, action) VALUES ($1, $2, $3)`, + roleID, permission[0], permission[1]); err != nil { + return "", w.Wrapf(err, "failed to write the derived role's permission %s:%s", permission[0], permission[1]) } } return roleID, nil