feat: complete brokered authenticated reference status - #15
Merged
Conversation
The architecture proof's last unproved boundary was authentication. The shell now owns it end to end: it selects an isolated context, reads a development credential the module never sees, and answers the module's AcquireAccess with a fixture token bound to the declared audience and scope, the context's organization, this invocation, and a near-term expiry. A denial is the shell's own typed problem. The module receipt is the ceiling. An undeclared audience, a permission the installation does not declare, a context with no credential source, and a second request in one command are all refused, and none of them reaches the service. The module is launched with an empty environment, so there is no ambient credential for it to find. The local status service accepts only what it serves: the wrong audience, scope, organization, invocation, or an expired token is refused. A broker denial and a service failure end in different exit classes, so automation can tell "you may not" from "it is broken". The development issuer and the status service are test infrastructure. Both are internal packages, neither is linked into the shell binary, and every token says so on the wire. Closes wso2#7
📝 WalkthroughSummaryCompletes brokered authentication for Key changes
WalkthroughThis PR implements brokered authentication for the reference status module. It adds validated context documents, isolated fixture installation, invocation-bound development tokens, shell and module access-broker protocol messages, and RPC enforcement. The reference module now requests access and calls a local authenticated status service. Tests cover token claims, access denials, service failures, expiry, claim mismatches, credential isolation, and rendered status output. CI and contributor documentation now include reference-module validation. Sequence Diagram(s)sequenceDiagram
participant CLI
participant Shell
participant ReferenceModule
participant StatusService
CLI->>Shell: Run reference status
Shell->>ReferenceModule: Launch with selected context
ReferenceModule->>Shell: Request declared audience and scope
Shell-->>ReferenceModule: Return invocation-bound token
ReferenceModule->>StatusService: GET /status with token
StatusService-->>ReferenceModule: Return status JSON
ReferenceModule-->>Shell: Return semantic status result
Shell-->>CLI: Render status output
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A missing-credential denial could only tell a user what to do by naming the variable to set, and that whole problem was encoded into AccessDenied. The module was therefore handed the credential's source reference, which is one of the three things the proof promises it never receives. A denial is now two statements of one refusal. The module is sent a refusal that names no credential source; the shell keeps the version that does, and reports it when the module returns the refusal as its outcome. Only a refusal this invocation actually issued is restored, so nothing a module invents can make the shell speak for it. Three smaller refusals join it. The broker serves the reserved reference namespace alone, so a product module cannot reach the development issuer behind it. An access request that names no exchange is refused rather than answered into the void. An endpoint that embeds credentials in its URL is refused, and no rejected endpoint is echoed into a problem the shell renders. The contract said an expired token could be reacquired, which the broker has never allowed. It now says what the code does.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep contributor commands aligned with CI.
The new command runs only
go test, while CI also requires formatting validation andgo vet. Document those checks as well, or provide one shared validation command so local and CI results stay consistent.As per path instructions, this feedback is concise and focused on correctness and best practices.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTRIBUTING.md` at line 29, Update the contributor validation command near the reference module entry to include the same formatting validation and go vet checks required by CI, or replace it with the shared validation command used by CI so local verification remains consistent.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/module/broker.go`:
- Around line 88-121: Update streamBroker.Acquire so the response wait through
b.reader.ReadEnvelope is bounded by ctx cancellation or deadline. Ensure
cancellation unblocks or stops the read and returns the context error, while
preserving the existing envelope validation and error handling for successful or
failed reads.
---
Nitpick comments:
In `@CONTRIBUTING.md`:
- Line 29: Update the contributor validation command near the reference module
entry to include the same formatting validation and go vet checks required by
CI, or replace it with the shared validation command used by CI so local
verification remains consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e3581e50-78ac-4faf-90b2-d5cf34ee4895
⛔ Files ignored due to path filters (1)
sdk/protocol/contractv1/contract.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (36)
.github/workflows/pr-checks.ymlCONTRIBUTING.mddocs/examples/authentication-contexts.mddocs/plans/first-cli-vertical-slice.mdexamples/reference-module/cmd/wso2-module-reference/main.goexamples/reference-module/cmd/wso2-module-reference/status.goexamples/reference-module/cmd/wso2-module-reference/status_test.gointernal/app/app.gointernal/app/invoke.gointernal/auth/auth.gointernal/auth/auth_test.gointernal/auth/devtoken/devtoken.gointernal/auth/devtoken/devtoken_test.gointernal/contexts/contexts.gointernal/contexts/contexts_test.gointernal/contexts/fixture/fixture.gointernal/modules/fixture/fixture.gointernal/rpc/broker_test.gointernal/rpc/invocation_id.gointernal/rpc/launch.gointernal/rpc/rpc.gointernal/rpc/session.gointernal/state/state.gointernal/state/state_test.gointernal/statusservice/statusservice.gointernal/statusservice/statusservice_test.gosdk/module/broker.gosdk/module/broker_test.gosdk/module/serve.gosdk/proto/wso2/cli/module/v1/contract.protosdk/protocol/codec.gosdk/testkit/testkit.gotest/acceptance/acceptance_test.gotest/acceptance/broker_test.gotest/acceptance/status_test.gotest/acceptance/testdata/noisymodule/main.go
The fail-closed work landed on main while this branch was open, and the two disagree in one place each. Its shadowing test runs the reference module and expects an answer, which now requires brokered access to the local status service, so it deploys one. Its control-file helper was renamed, so the environment-reporting test calls it by its new name.
A reviewer asked whether a module waiting for the shell's answer can hang, because the wait carries no deadline. It cannot: the shell closes the module's protocol input on every path it can end an invocation by, and that closure ends the wait. Bounding the read on the context instead would leave a framed stream at an unknown position, so the only safe response to a cancelled read is the one already in place.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/acceptance/failclosed_test.go (1)
269-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated fault-test setup.
Nearly every test in this range repeats the same three lines (
buildShell,isolatedStateRoot,installFaultyModule) before callingtryFailingShell. A small helper (e.g.runWithFault(t, fault string, args ...string) failedRun) would remove this duplication across the ~8 call sites without changing behavior.♻️ Proposed helper
+// runWithFault installs the fault fixture selecting the given fault and runs +// the shell against it, returning the run for assertion. +func runWithFault(t *testing.T, fault string, args ...string) failedRun { + t.Helper() + shell := buildShell(t) + stateRoot := isolatedStateRoot(t) + installFaultyModule(t, stateRoot, fault) + return tryFailingShell(t, shell, stateRoot, args...) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/acceptance/failclosed_test.go` around lines 269 - 421, Extract the repeated buildShell, isolatedStateRoot, installFaultyModule, and tryFailingShell setup into a helper such as runWithFault(t, fault string, args ...string) failedRun. Update the affected fault-based tests, including the table-driven cases, to use this helper while preserving each test’s arguments, assertions, and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/acceptance/failclosed_test.go`:
- Around line 269-421: Extract the repeated buildShell, isolatedStateRoot,
installFaultyModule, and tryFailingShell setup into a helper such as
runWithFault(t, fault string, args ...string) failedRun. Update the affected
fault-based tests, including the table-driven cases, to use this helper while
preserving each test’s arguments, assertions, and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8052cb0-d1e9-4195-9491-27cf00da18f9
📒 Files selected for processing (4)
sdk/module/broker.gotest/acceptance/broker_test.gotest/acceptance/failclosed_test.gotest/acceptance/status_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- sdk/module/broker.go
- test/acceptance/status_test.go
- test/acceptance/broker_test.go
sachiniSam
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7. Parent: #3. Increment 4 of
the first CLI vertical slice plan.
What this proves
The architecture proof's last unproved boundary was authentication. The shell
now owns it end to end:
The reference module receives a token and nothing else: not the credential the
shell holds, not a reference to it, and no way to renew what it was given. It
is launched with an empty environment, so there is no ambient credential for it
to find.
Contract
Field numbers 13, 14, and 15 were reserved for this exchange and are now
AcquireAccess,AccessGranted, andAccessDenied.InvocationContextgainsan
endpoint: it tells a module where to call, never that it may.sdk/protocol/contractv1is regenerated with the pinnedbuf.Policy
The module receipt is the ceiling. Refused, without reaching the service:
auth.audience_not_declaredauth.scope_not_declaredauth.context_not_selectedauth.organization_not_selectedauth.method_unsupportedauth.credential_unavailableauth.already_grantedA broker denial exits 77 and a product-service failure exits 75, so automation
can tell "you may not" from "it is broken" without reading text.
Non-production seams
internal/auth/devtoken(the issuer) andinternal/statusservice(the service)are test infrastructure. Both are internal packages with no command behind them,
neither is linked into the shell binary, and every token it mints begins with
wso2-development-token.. The production replacements are a real token exchangeand a real product service.
Tests
47 new tests across unit, contract, and black-box layers, covering denied
access, successful access, claim mismatch, expiry, service failure, a sanitized
module environment, and a canary scan proving no run discloses the source
credential. CI now also builds, vets, and tests the reference module, whose own
tests previously never ran.
Known limit
The status service compares the token's invocation claim with a header from the
same caller. That proves the token belongs to the invocation the caller is
acting as; it is not replay detection, since a replayer sends both. Closing it
needs an identity provider the audience trusts, which is what the production
replacement brings. The limit is documented at the check.