fix(accounts): ensuring a derived role that is already as declared writes nothing

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 {

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))
+}

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