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