From 92baf0aab7b7322b25620c1fe2510597db6f708c Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Tue, 27 Oct 2026 12:15:39 -0400 Subject: [PATCH 1/2] fix(fixture-composition): one statement, stated once, inside a fragment too; and apply what can fail first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the review of #42. One statement, one source, was enforced per SOURCE only: one fragment could state the same (organisation, role, kind) twice and silently union `{id: seeded}` with `{all: true}` — the exact widening the rule exists to stop, inside one file, where whoever reads the first statement has no reason to look for a second. `Decode` now refuses a duplicate grant, installation or stand-in module within one document, which also holds the merged document to the rule. `ApplyComposition` wrote the role grants before loading the installations and stand-in modules, so a composition whose installation the fixture then refused had already widened the fixture's roles, against the same function's "nothing is applied unless the whole document is valid". The two applying steps are now ordered: the one that can fail (LoadFixture) runs first, the one that cannot (the role writes) after. The third is an accepted limit, written down rather than fixed: a statement is keyed by organisation, role and kind alone, so a module's fragment CAN grant at another module's kinds — and a composition's fragment does, by design (the robin playground grants its viewer the collection its own seed writes). "One definition per module" is a convention about where a grant is written, not ownership enforcement; both READMEs now say so, with why nothing is lost by it (development fixture only, and a grant at an unserved kind resolves to nothing). The refusal table's prepended valid entry moves to its own kind: one document states each (organisation, role, kind) once, so reusing the cases' kind would have made every case a duplicate and hidden what it tests. Co-Authored-By: Claude Opus 5 --- libraries/fixture-composition/go/README.md | 13 ++++- .../go/fixturecomposition.go | 47 +++++++++++++--- .../go/fixturecomposition_test.go | 56 +++++++++++++++++++ modules/authority/services/local/README.md | 20 ++++++- .../code/pkg/business/state/composition.go | 23 +++++--- .../pkg/business/state/composition_test.go | 31 +++++++++- .../local/code/pkg/business/state/state.go | 2 +- 7 files changed, 172 insertions(+), 20 deletions(-) diff --git a/libraries/fixture-composition/go/README.md b/libraries/fixture-composition/go/README.md index 1b55cbd..1470414 100644 --- a/libraries/fixture-composition/go/README.md +++ b/libraries/fixture-composition/go/README.md @@ -15,9 +15,16 @@ composed module ships next to its code, plus the composition's own - `Merge` reads each `Source` with `Decode` (an error names the source) and combines them. Every statement has exactly one source: a role's grant on one kind, one installation, one stand-in module. A second source stating one is - `ErrConflict`, naming both sources, even when they agree. Sources are merged - in the order of their names, so the result does not depend on the order they - were found in. + `ErrConflict`, naming both sources, even when they agree; so is a second + statement of it **within one document**, which `Decode` refuses, since the + grants of a kind are a union and a file stating one twice widens it exactly + as two files would. Sources are merged in the order of their names, so the + result does not depend on the order they were found in. +- A statement is keyed by organisation, role and kind — never by which module + ships the fragment — so the merge does not enforce that a module grants only + at its own kinds, and a composition's fragment deliberately grants at a + module's (its seeded collection). See the local service's README, "What the + merge does not enforce: ownership". - `Encode` writes YAML that `Decode` reads back unchanged. `go test -race ./...` covers the merge of two module fragments and a diff --git a/libraries/fixture-composition/go/fixturecomposition.go b/libraries/fixture-composition/go/fixturecomposition.go index a8f63f8..2ff172a 100644 --- a/libraries/fixture-composition/go/fixturecomposition.go +++ b/libraries/fixture-composition/go/fixturecomposition.go @@ -48,9 +48,10 @@ var ( actionGrammar = regexp.MustCompile(`^[a-z][a-z0-9_-]*$`) ) -// ErrConflict is two sources stating the same thing: the same role's grant on -// one kind, the same installation, or the same stand-in module. -var ErrConflict = errors.New("fixture-composition: two sources state the same grant") +// ErrConflict is the same thing stated twice — the same role's grant on one +// kind, the same installation, the same stand-in module — whether by two +// sources (Merge) or twice within one document (Decode). +var ErrConflict = errors.New("fixture-composition: one statement, stated twice") // Document is one fixture-composition document. type Document struct { @@ -150,7 +151,19 @@ func Decode(data []byte) (Document, error) { return doc, doc.validate() } +// validate holds one document — a single fragment, or a merged one — to the +// rules that do not depend on the fixture, the one-statement-one-source rule +// included: the same (organisation, role, kind), the same installation and the +// same stand-in module are each stated once in a document, as they are stated +// by one source across documents (Merge). +// +// Within one document that rule is what the cross-source rule exists for. The +// grants of a kind are a UNION — a role holding `documents.collection:read` on +// `{id: seeded}` and again on `{all: true}` reads every collection, the exact +// widening the rule refuses between two fragments — and whoever reads the +// first statement has no reason to look for a second further down the file. func (d Document) validate() error { + grants := map[string]bool{} for _, r := range d.Roles { key := r.Key() switch { @@ -163,24 +176,42 @@ func (d Document) validate() error { if err := CheckScope(sc.Kind, sc.Actions); err != nil { return fmt.Errorf("%s: %w", key, err) } - grants := make([]resources.Grant, 0, len(sc.Resources)) + if grants[key+" "+sc.Kind] { + return fmt.Errorf("%w: %s's grant on %s is stated twice in one document; state it once, with every action and resource it covers", + ErrConflict, key, sc.Kind) + } + grants[key+" "+sc.Kind] = true + compiled := make([]resources.Grant, 0, len(sc.Resources)) for _, m := range sc.Resources { - grants = append(grants, m.Grant()) + compiled = append(compiled, m.Grant()) } - if _, err := resources.Compile(grants); err != nil { + if _, err := resources.Compile(compiled); err != nil { return fmt.Errorf("%s scope %s: %w", key, sc.Kind, err) } } } + installations := map[string]bool{} for _, i := range d.Installations { if i.Target == "" || i.Role == "" { return errors.New("an installation names no target or no role") } + key := i.Organization + "/" + i.Target + if installations[key] { + return fmt.Errorf("%w: the installation of %s at %s is stated twice in one document; state it once", + ErrConflict, i.Target, i.Organization) + } + installations[key] = true } + modules := map[string]bool{} for _, m := range d.Modules { if !kindGrammar.MatchString(m.Module) || len(m.Kinds) == 0 { return fmt.Errorf("stand-in module %q names no valid module or no kind", m.Module) } + if modules[m.Module] { + return fmt.Errorf("%w: stand-in module %s is stated twice in one document; state it once, with every kind it serves", + ErrConflict, m.Module) + } + modules[m.Module] = true for _, k := range m.Kinds { if err := CheckScope(k.Kind, k.Actions); err != nil { return fmt.Errorf("stand-in module %s: %w", m.Module, err) @@ -230,7 +261,9 @@ type Source struct { // owns it is the one a reader goes to: a composition narrowing a module's grant // to a seeded resource must not be silently widened by the module's, and a // module's grant copied into a composition is the drift this exists to end. -// Within one source the document reads as it does alone. +// Within ONE source the same rule holds, and for the same reason — the grants +// of a kind are a union, so a fragment stating one twice widens it exactly as +// two fragments would — which Decode refuses (validate). // // The result does not depend on the order the sources are given in: they are // merged in the order of their names, and two sources of one name are refused. diff --git a/libraries/fixture-composition/go/fixturecomposition_test.go b/libraries/fixture-composition/go/fixturecomposition_test.go index 42798e5..79bfad6 100644 --- a/libraries/fixture-composition/go/fixturecomposition_test.go +++ b/libraries/fixture-composition/go/fixturecomposition_test.go @@ -167,3 +167,59 @@ func TestMergeOfNothingIsTheSchemaAlone(t *testing.T) { t.Fatalf("%q %v", encoded, err) } } + +// The one-statement-one-source rule holds inside a fragment too: the grants of +// a kind are a union, so a fragment stating the same (organisation, role, kind) +// twice widens it exactly as two fragments would — `{id: seeded}` and again +// `{all: true}` reads every resource of the kind, the failure the rule exists +// to stop. The same holds for an installation and a stand-in module. +func TestADocumentStatingOneThingTwiceIsRefused(t *testing.T) { + for name, tc := range map[string]struct{ doc, reason string }{ + "widening grant": {`roles: + - organization: acme + name: viewer + scopes: + - {kind: documents.collection, actions: [read], resources: [{id: seeded}]} + - {kind: documents.collection, actions: [read], resources: [{all: true}]}`, + "acme/viewer's grant on documents.collection is stated twice in one document"}, + "two role entries": {`roles: + - organization: acme + name: viewer + scopes: [{kind: documents, actions: [read]}] + - organization: acme + name: viewer + scopes: [{kind: documents, actions: [write]}]`, + "acme/viewer's grant on documents is stated twice"}, + "installation": {`installations: + - {organization: acme, target: atlas, role: viewer, owner: alex} + - {organization: acme, target: atlas, role: editor, owner: alex}`, + "the installation of atlas at acme is stated twice"}, + "stand-in": {`modules: + - {module: oracle, kinds: [{kind: oracle.answers, actions: [read]}]} + - {module: oracle, kinds: [{kind: oracle.answers, actions: [write]}]}`, + "stand-in module oracle is stated twice"}, + } { + t.Run(name, func(t *testing.T) { + _, err := fixturecomposition.Decode([]byte(head + tc.doc + "\n")) + if !errors.Is(err, fixturecomposition.ErrConflict) || !strings.Contains(err.Error(), tc.reason) { + t.Fatalf("got %v, want a conflict naming %q", err, tc.reason) + } + // The merge names the fragment it came from. + _, err = fixturecomposition.Merge([]fixturecomposition.Source{{Name: "module ledger", Data: []byte(head + tc.doc + "\n")}}) + if !errors.Is(err, fixturecomposition.ErrConflict) || !strings.HasPrefix(err.Error(), "module ledger: ") { + t.Fatalf("got %v, want the conflict named against module ledger", err) + } + }) + } + // One role stating two different kinds, and two roles stating one kind, + // are not duplicates. + for _, doc := range []string{ + `roles: [{organization: acme, name: viewer, scopes: [{kind: documents, actions: [read]}, {kind: robin.conversations, actions: [read]}]}]`, + `roles: [{organization: acme, name: viewer, scopes: [{kind: documents, actions: [read]}]}, {organization: acme, name: editor, scopes: [{kind: documents, actions: [read]}]}]`, + `installations: [{organization: acme, target: atlas, role: viewer, owner: alex}, {organization: globex, target: atlas, role: viewer, owner: zoe}]`, + } { + if _, err := fixturecomposition.Decode([]byte(head + doc + "\n")); err != nil { + t.Errorf("%s: %v", doc, err) + } + } +} diff --git a/modules/authority/services/local/README.md b/modules/authority/services/local/README.md index dec7e17..7eae879 100644 --- a/modules/authority/services/local/README.md +++ b/modules/authority/services/local/README.md @@ -130,9 +130,27 @@ the one parser of the document: refused naming both, even when they agree, so a composition narrowing a module's grant to its seeded collection is never silently widened by the module's, and a module's grant copied into a playground is refused rather - than left to drift; + than left to drift. One **fragment** stating the same one twice is refused + too, and for the same reason: the grants of a kind are a union, so a file + granting `{id: seeded}` and again `{all: true}` widens exactly as two files + would; - the result does not depend on the order fragments are found in. +**What the merge does not enforce: ownership.** A statement is keyed by +organisation, role and kind alone, never by which module ships the fragment, so +a module's fragment *can* grant at another module's kinds — and a composition's +fragment does, by design: the robin playground grants its viewer +`documents.collection` on the collection its own seed writes, a kind the +document store owns. "One definition per module" is therefore a convention +about where a grant is written, enforced only in that two fragments cannot +state the same grant; it is not a check that a module grants on its own kinds. +Nothing is lost by that: these fragments reach the development fixture alone, +never a host, and a grant at a kind no composed module serves resolves to +nothing (the authority intersects every capability with the delivered +manifests), so the failure mode is a refusal, not a widening. A composition +that wanted ownership enforced would have to name each kind's owning module in +the document, which v1 does not. + The authority module's own fragment (`modules/authority/configurations/local/fixture-composition.yaml`) is the schema line alone, so a composition of the authority and nothing else boots diff --git a/modules/authority/services/local/code/pkg/business/state/composition.go b/modules/authority/services/local/code/pkg/business/state/composition.go index 40be0d2..a9e5148 100644 --- a/modules/authority/services/local/code/pkg/business/state/composition.go +++ b/modules/authority/services/local/code/pkg/business/state/composition.go @@ -32,7 +32,9 @@ const CompositionSchema = fixturecomposition.Schema // until its own delivery replaces it. The document is read by the library's // one parser, which holds every rule that does not depend on the fixture; // this checks it against the fixture. Nothing is applied unless the whole -// document is valid. It returns one line per effect, for the boot log. +// document is valid, and the one applying step that can fail runs before the +// one that cannot, so a refused installation leaves every role as it was. It +// returns one line per effect, for the boot log. func (s *State) ApplyComposition(ctx context.Context, data []byte) ([]string, error) { doc, err := fixturecomposition.Decode(data) if err != nil { @@ -46,12 +48,13 @@ func (s *State) ApplyComposition(ctx context.Context, data []byte) ([]string, er if err != nil { return nil, fmt.Errorf("%s: %w", CompositionDocument, err) } - s.mu.Lock() - for key, role := range roles { - s.roles[key] = role - } - s.refreshRolesLocked() - s.mu.Unlock() + // The installations and stand-in modules go first, and the role grants + // after: everything above was validation, which changed nothing, and of the + // two applying steps only this one can fail. Granting first meant a + // composition whose installation was then refused had already widened the + // fixture's roles — the document was part-applied, against what this says. + // (LoadFixture's own internal steps are not individually reversible; a + // failure inside it still leaves what it applied before failing.) if len(doc.Installations) > 0 || len(doc.Modules) > 0 { setup, err := yaml.Marshal(fixtureFile{Installations: doc.Installations, Modules: doc.Modules}) if err != nil { @@ -61,6 +64,12 @@ func (s *State) ApplyComposition(ctx context.Context, data []byte) ([]string, er return nil, fmt.Errorf("%s: %w", CompositionDocument, err) } } + s.mu.Lock() + for key, role := range roles { + s.roles[key] = role + } + s.refreshRolesLocked() + s.mu.Unlock() return append(lines, more...), nil } diff --git a/modules/authority/services/local/code/pkg/business/state/composition_test.go b/modules/authority/services/local/code/pkg/business/state/composition_test.go index bb3447f..d4d45b3 100644 --- a/modules/authority/services/local/code/pkg/business/state/composition_test.go +++ b/modules/authority/services/local/code/pkg/business/state/composition_test.go @@ -130,7 +130,12 @@ modules: // A composition grants concrete permissions to roles the fixture defines, and // a refused document changes nothing. func TestCompositionRefusesByName(t *testing.T) { - valid := `{organization: acme, name: editor, scopes: [{kind: ledger.entry, actions: [read]}]}` + // A valid entry prepended to every roles case below, so a refused later + // entry cannot leave it applied. Its kind differs from the cases' own: + // one document states each (organisation, role, kind) once, so reusing + // ledger.entry here would make every case a duplicate and hide what it + // is actually testing. + valid := `{organization: acme, name: editor, scopes: [{kind: ledger.audit, actions: [read]}]}` for name, tc := range map[string]struct{ doc, reason string }{ "empty": {"", "the document is empty"}, "null": {"null", "not a mapping"}, @@ -185,3 +190,27 @@ func TestTheShippedDefaultAddsNothing(t *testing.T) { t.Fatalf("%v %v", lines, err) } } + +// A document whose installation the fixture refuses leaves every role as it +// was: the applying step that can fail runs before the one that cannot, so +// nothing is part-applied. +func TestARefusedInstallationLeavesTheRolesAlone(t *testing.T) { + k := fixtureOnly(t) + before := k.State.RoleScopes("acme", "editor") + _, err := k.State.ApplyComposition(context.Background(), []byte(head+` +roles: + - organization: acme + name: editor + scopes: + - {kind: ledger.entry, actions: [read, write]} +installations: + - {organization: acme, target: atlas, role: viewer, owner: zoe} +`)) + if err == nil || !strings.Contains(err.Error(), "not a person of acme") { + t.Fatalf("got %v, want the installation refused", err) + } + after := k.State.RoleScopes("acme", "editor") + if !state.Attenuates(before, after) || !state.Attenuates(after, before) { + t.Fatalf("a refused document granted the editor anyway: %v → %v", before, after) + } +} diff --git a/modules/authority/services/local/code/pkg/business/state/state.go b/modules/authority/services/local/code/pkg/business/state/state.go index 007d1d4..ffc500f 100644 --- a/modules/authority/services/local/code/pkg/business/state/state.go +++ b/modules/authority/services/local/code/pkg/business/state/state.go @@ -7,7 +7,6 @@ package state import ( - fixturecomposition "github.com/codefly-dev/authority/libraries/fixture-composition/go" "context" "crypto/ed25519" "errors" @@ -20,6 +19,7 @@ import ( "github.com/codefly-dev/authority/libraries/authority-core/go/domain" "github.com/codefly-dev/authority/libraries/authority-core/go/memstore" "github.com/codefly-dev/authority/libraries/authority-core/go/scope" + fixturecomposition "github.com/codefly-dev/authority/libraries/fixture-composition/go" resources "github.com/codefly-dev/authority/libraries/resources/go" basev0 "github.com/codefly-dev/core/generated/go/codefly/base/v0" "github.com/codefly-dev/core/workloadidentity" From a26fa41de26bd667964e171edb88710e8f9ead0a Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Tue, 27 Oct 2026 13:06:43 -0400 Subject: [PATCH 2/2] test(fixture-composition): the ordering guard needs a document that fails while APPLYING MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestARefusedInstallationLeavesTheRolesAlone could not fail: the refusal it provoked ("not a person of acme") comes from checkComposedSetup, pure validation that runs before either applying step in BOTH orderings, so the scenario never reached the code the fix moves. Reverting only the ordering left it green, and the whole pkg/business/state suite with it — a guard about a name. Replaced by TestADocumentRefusedWhileApplyingLeavesTheRolesAlone, whose document gets past validation and fails INSIDE LoadFixture: a stand-in module named after a fixture person, which checkComposedSetup only checks the kinds of and logs a line for, and which the core refuses when the manifest is applied ("names a person; a module's principal is its own"). Diagnosed by putting the bug back: against the pre-fix ordering the new test fails with the editor holding `ledger.entry approve` from a document that was refused; with the fix the role is untouched. Co-Authored-By: Claude Opus 5 --- .../pkg/business/state/composition_test.go | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/modules/authority/services/local/code/pkg/business/state/composition_test.go b/modules/authority/services/local/code/pkg/business/state/composition_test.go index d4d45b3..47c9a32 100644 --- a/modules/authority/services/local/code/pkg/business/state/composition_test.go +++ b/modules/authority/services/local/code/pkg/business/state/composition_test.go @@ -191,23 +191,34 @@ func TestTheShippedDefaultAddsNothing(t *testing.T) { } } -// A document whose installation the fixture refuses leaves every role as it -// was: the applying step that can fail runs before the one that cannot, so -// nothing is part-applied. -func TestARefusedInstallationLeavesTheRolesAlone(t *testing.T) { +// A document the fixture validates but that fails while being APPLIED leaves +// every role as it was: the applying step that can fail runs before the one +// that cannot. +// +// The document has to fail INSIDE LoadFixture to exercise that, which is the +// whole point of the ordering. A refused installation does not: an owner the +// fixture does not define is caught by checkComposedSetup, pure validation +// that runs before either applying step in both orderings, so such a document +// never reaches the code the ordering moves. A stand-in module named after a +// fixture person does: checkComposedSetup only checks its kinds and logs a +// line, and the core refuses it when the manifest is applied ("names a +// person; a module's principal is its own"). With the role writes first, that +// left the editor holding the grant from a document that was refused. +func TestADocumentRefusedWhileApplyingLeavesTheRolesAlone(t *testing.T) { k := fixtureOnly(t) before := k.State.RoleScopes("acme", "editor") - _, err := k.State.ApplyComposition(context.Background(), []byte(head+` + lines, err := k.State.ApplyComposition(context.Background(), []byte(head+` roles: - organization: acme name: editor scopes: - - {kind: ledger.entry, actions: [read, write]} -installations: - - {organization: acme, target: atlas, role: viewer, owner: zoe} + - {kind: ledger.entry, actions: [approve]} +modules: + - module: maya + kinds: [{kind: maya.things, actions: [read]}] `)) - if err == nil || !strings.Contains(err.Error(), "not a person of acme") { - t.Fatalf("got %v, want the installation refused", err) + if err == nil || !strings.Contains(err.Error(), "names a person") { + t.Fatalf("got %v (%v), want the stand-in module refused while applying", err, lines) } after := k.State.RoleScopes("acme", "editor") if !state.Attenuates(before, after) || !state.Attenuates(after, before) {