diff --git a/cmd/doctor_solution.go b/cmd/doctor_solution.go index c0070a47..a1f73241 100644 --- a/cmd/doctor_solution.go +++ b/cmd/doctor_solution.go @@ -55,6 +55,14 @@ Every rule is one that silently broke something. The two worth knowing: renders into a mesh allow rule straight to that module's service, past the check. +Permissions are judged by reading the contract through Core's own loader, +which refuses a role granting a scope no binding backs and no ceiling covers +("role X grants documents:read, which is neither within the module's own scope +ceilings nor a scope one of its bindings spells out"). That is the +missing-permission class, caught before install rather than at the first +refused call. What a composition's values resolve each binding to is NOT +judged here: that needs the composition, not the directory. + Exit codes: 0 every judged solution conforms (warnings allowed) 1 at least one check failed, or the command itself failed @@ -71,7 +79,8 @@ module_contract_unreadable, owner_aware_missing, module_package_missing, module_package_unreadable, module_package_publishes_shell, frontend_declaration_missing, frontend_declaration_unreadable, page_shell_missing, module_interface_missing, entry_endpoint_not_exported, -page_shell_exported_too_widely.`, +page_shell_exported_too_widely, consumed_module_without_binding, +page_shell_has_no_app.`, Example: ` # Judge every solution in the workspace. codefly doctor solution diff --git a/cmd/doctor_solution_test.go b/cmd/doctor_solution_test.go index 6474b5ae..51821d6d 100644 --- a/cmd/doctor_solution_test.go +++ b/cmd/doctor_solution_test.go @@ -52,6 +52,7 @@ func TestEveryDiagnosticCodeIsDocumented(t *testing.T) { solutionshape.CodeFrontendMissing, solutionshape.CodeFrontendUnreadable, solutionshape.CodePageServiceMissing, solutionshape.CodeInterfaceMissing, solutionshape.CodeEntryNotExported, solutionshape.CodePageExportedTooWidely, + solutionshape.CodeConsumedWithoutBinding, solutionshape.CodePageShellHasNoApp, } { if !strings.Contains(DoctorSolutionCmd.Long, code) { t.Errorf("diagnostic code %q is not documented in the command's help", code) diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 6cc49f0c..966ccb69 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -2505,6 +2505,14 @@ Every rule is one that silently broke something. The two worth knowing: renders into a mesh allow rule straight to that module's service, past the check. +Permissions are judged by reading the contract through Core's own loader, +which refuses a role granting a scope no binding backs and no ceiling covers +("role X grants documents:read, which is neither within the module's own scope +ceilings nor a scope one of its bindings spells out"). That is the +missing-permission class, caught before install rather than at the first +refused call. What a composition's values resolve each binding to is NOT +judged here: that needs the composition, not the directory. + Exit codes: 0 every judged solution conforms (warnings allowed) 1 at least one check failed, or the command itself failed @@ -2521,7 +2529,8 @@ module_contract_unreadable, owner_aware_missing, module_package_missing, module_package_unreadable, module_package_publishes_shell, frontend_declaration_missing, frontend_declaration_unreadable, page_shell_missing, module_interface_missing, entry_endpoint_not_exported, -page_shell_exported_too_widely. +page_shell_exported_too_widely, consumed_module_without_binding, +page_shell_has_no_app. ``` ``` diff --git a/docs/commands.md b/docs/commands.md index b78bbfeb..554ae783 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -2465,6 +2465,7 @@ Every rule is one that silently broke something. The two worth knowing: | | | |---|---| | `owner_aware_missing` | The authority issues a person a capability at a solution **only** when its module contract declares `owner_aware: true`, and refuses by name otherwise. Without it the solution receives no owner-selected scope and can act for nobody — and nothing fails at build time to tell you. | +| `consumed_module_without_binding` | A module named in `api.consumes` that the contract declares no binding for. A **warning**: correct when the module is reached with the viewer's own credential (wiki reaches robin, tracing and annotations that way), a bug when the solution calls it on a person's behalf — and the two look identical until a call is refused. | | `module_edge_declared` | A solution reaches every module through the **host's door** — a saas host's gateway and frontend, or the authority's hop beside the authority interface. A `service-dependencies` entry naming anything else renders into a mesh allow rule straight to that module's service, past the authority's check. | Exit codes: `0` every judged solution conforms (warnings allowed), `1` at least @@ -2472,6 +2473,22 @@ one check failed. `--json` prints a versioned report per module (`{schema_version, module, dir, is_solution, status, checks[]}`), so a consumer repository can gate on it instead of hand-rolling the equivalent. +**Permissions.** The contract is read through Core's own loader, which refuses +a role granting a scope no binding backs and no ceiling covers — *"role +`installer` grants `documents:read`, which is neither within the module's own +scope ceilings nor a scope one of its bindings spells out"*. That is the +missing-permission class, caught before install rather than at the first +refused call. What a composition's values resolve each binding to is not +judged: that needs the composition, not the directory. + +`page_shell_has_no_app` is worth knowing about: conforming the fleet to one +shape added the `web` shell's *manifest* to solutions whose page had never been +written, because writing one would have been inventing product in a conforming +change. Those solutions are correctly shaped and `codefly run service web` has +nothing to start on them. A declared service with no source is otherwise +invisible — every manifest reads fine, and the first sign is a run that does +nothing. + A warning is not a failure: it names something worth seeing that does not make the solution wrong — publishing the page's shell, a headless solution with no page declaration, a solution with no shell at all. diff --git a/pkg/solutionshape/check.go b/pkg/solutionshape/check.go index fdefe8bb..74e72206 100644 --- a/pkg/solutionshape/check.go +++ b/pkg/solutionshape/check.go @@ -76,6 +76,8 @@ const ( CodePageServiceMissing = "page_shell_missing" CodeInterfaceMissing = "module_interface_missing" CodeEntryNotExported = "entry_endpoint_not_exported" + CodeConsumedWithoutBinding = "consumed_module_without_binding" + CodePageShellHasNoApp = "page_shell_has_no_app" CodePageExportedTooWidely = "page_shell_exported_too_widely" ) @@ -124,10 +126,10 @@ func Inspect(moduleName, dir string) *Report { return report } - report.checkManifest(raw) + consumed := report.checkManifest(raw) module := report.checkModule(dir) report.checkEntryService(dir) - report.checkContract(dir) + report.checkContract(dir, consumed) report.checkPackage(dir) report.checkFrontend(dir) if module != nil { @@ -139,13 +141,13 @@ func Inspect(moduleName, dir string) *Report { // checkManifest holds the solution manifest to the reader every render, sync // and authority derivation actually uses, then to the one rule those readers // do not enforce: a consumed entry with no route name. -func (r *Report) checkManifest(raw []byte) { +func (r *Report) checkManifest(raw []byte) []string { loaded, err := manifest.LoadConsumption(raw) if err != nil { r.fail(CodeManifestUnreadable, fmt.Sprintf("%s is not readable by core's consumption reader: %v", manifest.FileName, err), "every render, sync and authority derivation reads the manifest this way, so nothing can resolve what this solution consumes") - return + return nil } r.ok(CodeManifestUnreadable, fmt.Sprintf("%s declares %d consumed binding(s)", manifest.FileName, len(loaded.API.Consumes))) @@ -158,12 +160,21 @@ func (r *Report) checkManifest(raw []byte) { r.fail(CodeConsumedAPIUnnamed, fmt.Sprintf("api.consumes entry %q names no route (`as`)", consumed.ID), "give it `as: `; the runtime addresses the module by that name and an entry without one is unroutable") - return + return nil } } if len(loaded.API.Consumes) > 0 { r.ok(CodeConsumedAPIUnnamed, "every consumed API names its route") } + seen := map[string]bool{} + var modules []string + for index := range loaded.API.Consumes { + if module := loaded.API.Consumes[index].Module; module != "" && !seen[module] { + seen[module] = true + modules = append(modules, module) + } + } + return modules } // checkModule holds the module manifest's service-entry, which is load-bearing @@ -189,6 +200,7 @@ func (r *Report) checkModule(dir string) *resources.Module { if hasService(module, PageService) { r.ok(CodePageServiceMissing, "the page's shell is declared") + r.checkPageShellHasAnApp(dir) } else { r.warn(CodePageServiceMissing, fmt.Sprintf("the module declares no %q service", PageService), @@ -265,7 +277,7 @@ func (r *Report) checkEntryService(dir string) { } // checkContract holds the contract to the declaration that fails silently. -func (r *Report) checkContract(dir string) { +func (r *Report) checkContract(dir string, consumed []string) { if _, err := os.Stat(filepath.Join(dir, modulecontract.FileName)); errors.Is(err, os.ErrNotExist) { r.fail(CodeContractMissing, fmt.Sprintf("no %s", modulecontract.FileName), "it declares the principal every credential the solution mints is issued to; without it the solution has no declared identity to issue against") @@ -278,6 +290,7 @@ func (r *Report) checkContract(dir string) { return } r.ok(CodeContractMissing, fmt.Sprintf("the contract declares principal %q", contract.Principal)) + r.checkConsumedModulesHaveAuthority(consumed, contract) // The one that fails silently, and the reason this checker exists. if contract.OwnerAware { @@ -454,3 +467,88 @@ func interfaceKey(reference string) string { } return key } + +// WHAT IS DELIBERATELY NOT CHECKED HERE, and why it would be wrong to try. +// +// "Can a person holding role R actually call module M?" is the +// missing-permission question, and it cannot be answered from these documents +// alone. A role reaches another module's authority two ways: by naming a +// binding in role.Bindings, and by declaring that module's resource kinds in +// role.Scopes — which is how wiki and proposals do it, granting `documents` +// and `documents.collection` with no RoleBinding at all. A binding's own kinds +// are composition SLOTS, so which kinds it covers is not known until the +// composition supplies values. +// +// A first draft of this package failed any binding no role named by id. It +// reported wiki and proposals — the two solutions that have been running +// longest — as broken, which is how the second mechanism was found. The real +// answer needs modulecontract.Contract.Resolve(values) against a composition's +// configuration, which is a composition-scoped check and not a +// directory-scoped one. + +// checkConsumedModulesHaveAuthority reports a module the solution consumes for +// which the contract declares no binding. This is NOT always wrong — a module +// reached with the viewer's own credential, or on nobody's behalf, asks for no +// binding — which is why it warns rather than fails. It is reported because +// the two declarations live in different files, so nothing else puts them side +// by side, and "the binding was never added" and "this one needs none" look +// identical until a call is refused. +func (r *Report) checkConsumedModulesHaveAuthority(consumes []string, contract *modulecontract.Contract) { + if len(consumes) == 0 { + return + } + bound := map[string]bool{} + for i := range contract.Bindings { + for _, audience := range contract.Bindings[i].Calls { + for _, call := range audience { + bound[call] = true + } + } + bound[contract.Bindings[i].ID] = true + } + var unbound []string + for _, module := range consumes { + if !bound[module] { + unbound = append(unbound, module) + } + } + if len(unbound) == 0 { + r.ok(CodeConsumedWithoutBinding, "every consumed module has a binding") + return + } + r.warn(CodeConsumedWithoutBinding, + fmt.Sprintf("consumed with no binding in the contract: %s", strings.Join(unbound, ", ")), + "correct when the module is reached with the VIEWER'S own credential or on nobody's behalf; a bug when the solution calls it on a person's behalf, and then the call is refused at runtime with nothing failing earlier. Confirm each one") +} + +// checkPageShellHasAnApp reports a shell that is declared and has no app +// behind it — a service.codefly.yaml naming a Next.js service with no +// package.json beside it. +// +// It is a real state and not a hypothetical: conforming the fleet to one shape +// added the shell's MANIFEST to several solutions whose page had never been +// written, because writing one would have been inventing product in a +// conforming change. The declaration is right and the gap is real, so +// `codefly run service web` has nothing to start on those. +// +// A warning, not a failure: the solution is correctly shaped, and nothing is +// broken until someone runs the shell. It is reported because a declared +// service with no source is otherwise invisible — every manifest reads fine, +// and the first sign is a run that does nothing. +func (r *Report) checkPageShellHasAnApp(dir string) { + shell := filepath.Join(dir, "services", PageService) + if _, err := os.Stat(filepath.Join(shell, "service.codefly.yaml")); err != nil { + // No manifest at all is the module declaring a service it does not + // have, which the entry-service rules already speak to. + return + } + for _, marker := range []string{"package.json", "next.config.ts", "next.config.js"} { + if _, err := os.Stat(filepath.Join(shell, marker)); err == nil { + r.ok(CodePageShellHasNoApp, "the page's shell has an app behind it") + return + } + } + r.warn(CodePageShellHasNoApp, + fmt.Sprintf("the %q shell is declared with no app behind it", PageService), + "`codefly run service "+PageService+"` has nothing to start. Write the shell, or drop the service from module.codefly.yaml — a declaration with no source is the one state neither reading the manifests nor running the gates reveals") +} diff --git a/pkg/solutionshape/check_test.go b/pkg/solutionshape/check_test.go index 28d44590..a48156f1 100644 --- a/pkg/solutionshape/check_test.go +++ b/pkg/solutionshape/check_test.go @@ -3,6 +3,7 @@ package solutionshape import ( "os" "path/filepath" + "strings" "testing" ) @@ -320,3 +321,78 @@ endpoints: t.Fatal("a backend that presents no identity was accepted") } } + +// A consumed module with no binding is reported, but as a warning: it is +// correct when the module is reached with the viewer's own credential, which +// is what wiki does for robin, tracing and annotations. Failing it would fail +// the reference solution. +func TestAConsumedModuleWithNoBindingWarnsRatherThanFails(t *testing.T) { + report := Inspect("wiki", conformingSolution(t)) + check := checkFor(t, report, CodeConsumedWithoutBinding) + if check.Status != StatusWarn { + t.Fatalf("a consumed module with no binding was %s, want warn", check.Status) + } + if !strings.Contains(check.Message, "documents") { + t.Fatalf("the warning does not name the module: %q", check.Message) + } + if !report.Conforms() { + t.Fatal("a warning was treated as a failure") + } +} + +// THE MISSING-PERMISSION CLASS IS CAUGHT, and it is core that catches it: a +// role may grant another module's resource kind only when one of the +// contract's bindings spells that scope out, or it is within the module's own +// ceilings. Running the contract through core's loader is what surfaces it — +// which is the whole reason this package reads every document with the reader +// that owns it instead of parsing YAML itself. +func TestARoleGrantingAnUnbackedScopeIsReported(t *testing.T) { + dir := conformingSolution(t) + write(t, filepath.Join(dir, "module.contract.codefly.yaml"), `schema: codefly/module-contract/v1 +principal: wiki +owner_aware: true +namespaces: [] +queues: [] +scope_ceilings: + - resource_kind: wiki.page + actions: [read, write] +roles: + - name: installer + scopes: + - resource_kind: wiki.page + actions: [read, write] + - resource_kind: documents + actions: [read] +bindings: [] +`) + report := Inspect("wiki", dir) + check := checkFor(t, report, CodeContractUnreadable) + if check.Status != StatusFail { + t.Fatalf("a role granting a scope no binding backs was %s, want fail", check.Status) + } + if !strings.Contains(check.Message, "documents:read") { + t.Fatalf("the failure does not name the unbacked grant: %q", check.Message) + } +} + +// A shell declared with no app behind it is the state conforming the fleet +// created: the manifest was added, the page was never written. It warns rather +// than fails — the solution is correctly shaped and nothing breaks until +// someone runs the shell — but it is reported, because a declared service with +// no source is invisible to every other reading. +func TestAPageShellWithNoAppWarns(t *testing.T) { + dir := conformingSolution(t) + report := Inspect("wiki", dir) + if checkFor(t, report, CodePageShellHasNoApp).Status != StatusWarn { + t.Fatal("a shell with no app behind it was not reported") + } + if !report.Conforms() { + t.Fatal("a warning was treated as a failure") + } + + // With an app beside it, the finding goes away. + write(t, filepath.Join(dir, "services", PageService, "package.json"), `{"name":"web","private":true}`) + if checkFor(t, Inspect("wiki", dir), CodePageShellHasNoApp).Status != StatusOK { + t.Fatal("a shell with a real app was still reported") + } +}