feat(service): per-command AuthOptions — let commands declare Everyone vs Authenticated (#638)#642
Merged
Merged
Conversation
…e vs Authenticated (#638) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis pull request establishes per-command authorization for NeoHaskell by unifying query and command error handling under ChangesPer-command authorization and unified error model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@core/service/Service/CommandExecutor/Core.hs`:
- Around line 165-179: In the STrue branch handling multi-tenant auth (inside
the case on requestContext.user and the nested case on claims.tenantId and
Uuid.fromText tenantIdText), replace the Task.yield CommandRejected responses at
the three auth-failure points with Task.yield CommandUnauthorized so
tenant-related auth denials (no user, missing tenantId, invalid tenantId format)
are routed through the CommandUnauthorized path; keep the Log.debug calls and
error messages unchanged.
In `@core/service/Service/CommandExecutor/TH.hs`:
- Around line 498-501: Remove the outdated "STUB" commentary and replace it with
a brief, accurate description: explain that the code checks for an optional
user-defined canAccess function by looking up the name via TH.lookupValueName
and binding the result to maybeCanAccess, and note that a proper canAccessImpl
is generated in Phase 10 (or remove Phase reference if no longer relevant);
update the comment near maybeCanAccess/TH.lookupValueName to reflect the current
behavior and avoid suggesting a temporary error stub.
In `@core/service/Service/Transport/Web.hs`:
- Around line 567-587: The handler currently returns raw text for auth error
overrides (in the commandResponse / Response.Unauthorized branch) but still sets
HTTP.hContentType to "application/json"; change the body override logic so that
when authError yields a message (e.g., "Authentication required", "Access
denied", "Insufficient permissions") you produce valid JSON bytes (for example a
JSON object like {"error":"..."} or a quoted JSON string) before calling
Wai.responseLBS; update the construction around finalBodyBytes/responseBody
(references: commandResponse, Response.Unauthorized, authError, bodyOverride,
finalBodyBytes, Wai.responseLBS, HTTP.hContentType) to encode the message into
proper JSON bytes instead of sending raw text.
In `@core/test/Service/CommandExecutor/AuditLoggingSpec.hs`:
- Around line 79-88: The test builds claimsWithSub but calls canAccessImpl with
Nothing so it never validates leakage; update the spec to call canAccessImpl
`@CommandWithoutCanAccess` (Just claimsWithSub) instead of Nothing, capture the
audit/error log or serialized error payload emitted by canAccessImpl (use the
test logger/capture helper used elsewhere in tests), assert the result is still
Just Unauthenticated, and assert the captured payload does NOT contain sensitive
fields like the claim "sub", the permissions array, or raw token bytes; keep
references to mkClaims, canAccessImpl, CommandWithoutCanAccess, and
Unauthenticated to locate the code under test.
- Around line 43-77: The tests currently only assert the return value of
canAccessImpl (used with CommandWithoutCanAccess and CommandWithAdminDelete) but
never exercise or capture the audit logging path; update each spec to run the
code path that performs audit logging and capture emitted logs (e.g., using the
test logging/capture helper in your suite) and then assert on the captured log
records: assert a single Log.warn when canAccessImpl returns Just
Unauthenticated, assert the audit payload contains claims_present = False when
user = Nothing and claims_present = True when user = Just claims, and assert the
audit record contains the constructor name "InsufficientPermissions" (not the
permission list) when canAccessImpl returns Just (InsufficientPermissions _);
locate the logging call in the CommandExecutor.Core audit code and adjust the
tests to call the function that performs both canAccessImpl and logging (instead
of calling canAccessImpl directly) so you can inspect the actual log payloads.
In `@core/test/Service/Transport/Web/CommandAuthSpec.hs`:
- Around line 123-151: The test is calling canAccessImpl
`@CommandWithoutCanAccess` directly for all four transports so it cannot catch
transport-specific drift; change each spec to invoke the transport-specific
entrypoint/fixture rather than calling canAccessImpl directly — e.g., replace
direct calls with the Web/Cli/Mcp/Internal dispatch or wrapper used in
production (the same dispatcher used by decideImpl) so each case exercises the
transport boundary, or create per-transport fixtures that forward a request
through the transport layer to canAccessImpl for CommandWithoutCanAccess; ensure
tests reference canAccessImpl and CommandWithoutCanAccess so the implementation
under each transport is exercised.
- Around line 41-119: The tests currently call canAccessImpl directly (e.g.
canAccessImpl `@CommandWithoutCanAccess`, `@CommandWithPublicAccess`,
`@CommandWithAdminDelete`) which bypasses Web transport behavior like token
parsing, anonymous fallback, invalid-token short-circuit, and HTTP 401/403
rendering; update the spec to exercise the Web transport layer instead: send
simulated HTTP requests through the transport dispatcher (using the same test
harness used for other Web tests) with/without Authorization headers and with
valid/invalid JWTs, assert on HTTP status and body (401/403/200) and that
anonymous behavior and permission checks (mkClaims, required "admin:delete") are
enforced, and remove direct canAccessImpl invocations or keep them only as unit
checks while making each case assert transport-level outcomes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ae05dd3-1031-4198-8b62-3f6a9c750cf8
📒 Files selected for processing (23)
core/core/Core.hscore/nhcore.cabalcore/service/Service/Command/Auth.hscore/service/Service/Command/Core.hscore/service/Service/CommandExecutor/Core.hscore/service/Service/CommandExecutor/TH.hscore/service/Service/Query/Auth.hscore/service/Service/Response.hscore/service/Service/Transport/Cli/Output.hscore/service/Service/Transport/Mcp/Response.hscore/service/Service/Transport/Web.hscore/test-service/Main.hscore/test/Service/Command/AuthSpec.hscore/test/Service/Command/CanAccess/NoCanAccessFixture.hscore/test/Service/Command/CanAccess/PermissionFixture.hscore/test/Service/Command/CanAccess/PublicAccessFixture.hscore/test/Service/Command/CanAccessSpec.hscore/test/Service/CommandExecutor/AuditLoggingSpec.hscore/test/Service/Transport/InternalSpec.hscore/test/Service/Transport/Web/CommandAuthSpec.hscore/testlib/Test/Service/Command/Core.hscore/testlib/Test/Service/CommandHandler/Execute/Spec.hsdocs/decisions/0058-per-command-auth-options.md
Comment on lines
+43
to
+77
| describe "Service.CommandExecutor.Core audit logging for CommandUnauthorized" do | ||
|
|
||
| it "[non-happy] canAccessImpl returns Just Unauthenticated for anonymous caller on default command" \_ -> do | ||
| -- spec case: logs exactly one Log.warn record when canAccessImpl returns Just Unauthenticated | ||
| -- This proves: audit trail should contain Unauthenticated constructor name | ||
| -- RED: canAccessImpl stub panics instead of returning Just Unauthenticated | ||
| let result = canAccessImpl @CommandWithoutCanAccess Nothing | ||
| result |> shouldBe (Just Unauthenticated) | ||
|
|
||
| it "[non-happy] claims_present = False when user = Nothing → canAccessImpl returns Unauthenticated" \_ -> do | ||
| -- spec case: logs claims_present = False when requestContext.user = Nothing | ||
| -- This proves: audit trail correctly reflects absence of claims | ||
| -- RED: canAccessImpl stub panics | ||
| let result = canAccessImpl @CommandWithoutCanAccess Nothing | ||
| result |> shouldBe (Just Unauthenticated) | ||
|
|
||
| it "[non-happy] claims_present = True when user = Just claims → canAccessImpl returns InsufficientPermissions" \_ -> do | ||
| -- spec case: logs claims_present = True when requestContext.user = Just claims | ||
| -- This proves: audit trail correctly reflects presence of claims | ||
| -- RED: canAccessImpl (TH stub) panics instead of returning InsufficientPermissions | ||
| let claimsPresent = mkClaims Array.empty | ||
| let result = canAccessImpl @CommandWithAdminDelete (Just claimsPresent) | ||
| result |> shouldBe (Just (InsufficientPermissions ["admin:delete"])) | ||
|
|
||
| it "[non-happy] constructor name InsufficientPermissions (not permission contents) is auditable" \_ -> do | ||
| -- spec case: logs constructor name InsufficientPermissions (never the permission list) | ||
| -- This proves: audit uses constructor name for error type, not permission details | ||
| -- RED: canAccessImpl (TH stub) panics instead of returning InsufficientPermissions | ||
| let claims = mkClaims Array.empty | ||
| let result = canAccessImpl @CommandWithAdminDelete (Just claims) | ||
| -- The result contains InsufficientPermissions for audit logging | ||
| -- but the audit log should only record the constructor name, not the perms list | ||
| case result of | ||
| Just (InsufficientPermissions _) -> pass | ||
| other -> fail [fmt|expected InsufficientPermissions for audit log, got #{toText other}|] |
Contributor
There was a problem hiding this comment.
These cases must assert emitted audit logs, not only canAccessImpl return values.
You are asserting authorization outcomes, but not the audit artifact itself. This suite currently cannot prove constructor-only logging or warning-record behavior, because it never executes the logging path nor inspects log payloads.
🤖 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 `@core/test/Service/CommandExecutor/AuditLoggingSpec.hs` around lines 43 - 77,
The tests currently only assert the return value of canAccessImpl (used with
CommandWithoutCanAccess and CommandWithAdminDelete) but never exercise or
capture the audit logging path; update each spec to run the code path that
performs audit logging and capture emitted logs (e.g., using the test
logging/capture helper in your suite) and then assert on the captured log
records: assert a single Log.warn when canAccessImpl returns Just
Unauthenticated, assert the audit payload contains claims_present = False when
user = Nothing and claims_present = True when user = Just claims, and assert the
audit record contains the constructor name "InsufficientPermissions" (not the
permission list) when canAccessImpl returns Just (InsufficientPermissions _);
locate the logging call in the CommandExecutor.Core audit code and adjust the
tests to call the function that performs both canAccessImpl and logging (instead
of calling canAccessImpl directly) so you can inspect the actual log payloads.
Code fixes (real bugs):
- Web.hs: wrap auth-error response body in {"error": ...} JSON envelope
so application/json content-type is honoured; clients no longer choke
on plain text from 401/403.
- CommandExecutor/Core.hs: multi-tenant tenant-extraction failures now
emit CommandUnauthorized (mapped to 401/403) instead of
CommandRejected (mapped to 400), matching the per-command auth gate.
- TH.hs: drop stale "STUB / Phase 10 will generate" commentary; the
canAccess lookup is now production wiring.
Test fixes:
- MultiTenantSpec.hs: update four tenant-rejection expectations from
CommandRejected reason="..." to CommandUnauthorized authError=...
to track the dispatcher change above.
- AuditLoggingSpec.hs: rewrite the PII-leakage test so it actually
passes claims through canAccessImpl and asserts the
InsufficientPermissions payload carries only the required permission
name — not sub, email, name, or tenantId. The previous version
constructed but never used the claims, making the test a no-op.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
CodeRabbit review responsePushed Fixed in this push
Deferred — flagged as follow-up, not in scope for this PR
These three are valid quality concerns, but each requires test infrastructure that doesn't exist in this codebase yet:
The current tests at the listed lines do verify a real load-bearing property — that Filing as a follow-up. The transport-parity guarantee is structurally enforced today: Build state
|
Addresses CodeRabbit's remaining #6 and #7 review items on PR #642. Web.hs: - Extract auth-error response mapping into two pure helpers, 'unauthorizedResponse' and 'unauthorizedResponseBody', and export them. The dispatch lambda now calls them in place of the inline case. The codebase pattern (cf. 'buildHealthResponse') is to test Wai-bound logic via the pure helper it ultimately calls, rather than spinning up a full Wai application per test. CommandAuthSpec.hs: - Section A keeps the typeclass-dispatch tests but the describe is now honestly labelled "canAccessImpl resolves correctly per command policy (TH wiring)" instead of pretending to be Web-transport tests. - Section B (NEW) covers HTTP rendering via the new pure helpers: status codes, message strings, JSON envelope, and a load-bearing privacy assertion that the InsufficientPermissions HTTP body never leaks the required permission name (the audit log carries that; the wire never does). - Section C (NEW) replaces the four fake-parity tests (which all called canAccessImpl with the same argument) with three real dispatcher-parity tests that call CommandExecutor.execute directly. Because every transport routes through 'execute', exercising it once proves the gate fires for all transports — parity is by construction. Uses existing cart fixtures from testlib (AddItemToCart with publicAccess override; AuthenticatedAddItem with the typeclass default authenticatedAccess). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
CodeRabbit follow-up: #6 and #7 addressedPushed #6 — Web transport coverageFollowed the codebase's existing test pattern (see
#7 — Transport parityThe four old cases (each calling
Three cases:
Section A — kept but honestly labelledThe original typeclass-dispatch tests are valuable as TH-wiring verification, so I kept them — but the Build state
|
NickSeagull
commented
May 19, 2026
Comment on lines
+34
to
+36
| -- NOTE: canAccessImpl is excluded here to avoid name conflict with Query.canAccessImpl. | ||
| -- Import Service.Command.Core directly to use canAccessImpl on Command. | ||
| import Service.Command as Reexported (Command (IsMultiTenant, getEntityIdImpl, decideImpl), NameOf) |
Member
Author
There was a problem hiding this comment.
We must then to rename this to something that doesnt clash. All NeoHaskell projects should not require to import anything else that is not Core
Comment on lines
+43
to
+46
| -- | Alias for 'QueryAuthError' on the command path. | ||
| -- Using a single error type across both query and command authorization | ||
| -- avoids duplication and ensures the same constructors are available everywhere. | ||
| type CommandAuthError = QueryAuthError |
Member
Author
There was a problem hiding this comment.
I think that rather than doing this level of indirection, we should instead make the AuthError a common type, and then have both QueryAuthError and CommandAuthError users to use that common one, instead of introducing additional names.
Comment on lines
+148
to
+150
| let claimsPresent = (case requestContext.user of | ||
| Nothing -> "false" | ||
| Just _ -> "true") :: Text |
Member
Author
There was a problem hiding this comment.
This can be Maybe.fromDefault
Comment on lines
+566
to
+586
| -- Map CommandUnauthorized to 401/403 with a clean message body. | ||
| -- Mirror the query-side AuthorizationError mapping (lines ~655-659). | ||
| let httpStatus = commandResponseToHttpStatus commandResponse | ||
|
|
||
| let (statusOverride, bodyOverride :: Maybe Text) = case commandResponse of | ||
| Response.Unauthorized {authError} -> | ||
| case unauthorizedResponse authError of | ||
| (statusCode, message) -> (statusCode, Just message) | ||
| _ -> (httpStatus, Nothing) | ||
| let finalBodyBytes = case bodyOverride of | ||
| Maybe.Just msg -> | ||
| -- Wrap the message in a JSON envelope so the | ||
| -- application/json content-type is honoured. | ||
| unauthorizedResponseBody msg | ||
| |> Text.toBytes | ||
| |> Bytes.toLazyLegacy | ||
| Maybe.Nothing -> | ||
| responseBytes | ||
| |> Bytes.toLazyLegacy | ||
| let responseBody = | ||
| responseBytes | ||
| |> Bytes.toLazyLegacy | ||
| |> Wai.responseLBS httpStatus [(HTTP.hContentType, "application/json")] | ||
| finalBodyBytes | ||
| |> Wai.responseLBS statusOverride [(HTTP.hContentType, "application/json")] |
Member
Author
There was a problem hiding this comment.
Question here, answer in session: Rather than mirroring, wouldn't it be better to DRY? Or they are not equal 100%?
Addresses NickSeagull review comments on PR #642. # 1 (Core.hs:36) — re-export clash: - Rename Command typeclass method 'canAccessImpl' to 'canExecuteImpl'. The user-facing top-level 'canAccess' is unchanged; only the internal method name differs. The name pairs with the dispatcher entry point ('execute') and lets 'Core' re-export both classes' methods without a conflict. - Update 'Core.hs' to re-export the full Command(..) including 'canExecuteImpl'. NeoHaskell projects no longer need explicit Service.Command.Core imports for the method. # 2 (Service/Command/Auth.hs:46) — common AuthError type: - Rename 'QueryAuthError' to 'AccessError' in 'Service.Query.Auth' and delete the 'Service.Command.Auth' shim (and its cabal entry). Command and query paths now use the same single 'AccessError' type directly. - Sed across 26 files: every 'QueryAuthError'/'CommandAuthError' usage becomes 'AccessError'; every 'Service.Command.Auth' import becomes 'Service.Query.Auth'. # 3 (CommandExecutor/Core.hs:150) — replace case with Maybe helpers: - 'claimsPresent' now uses 'Maybe.map (\\_ -> "true") |> Maybe.withDefault "false"' rather than an explicit case expression. ('Maybe.fromDefault' does not exist; 'withDefault' + map is the closest in the existing Maybe API.) # 4 (Web.hs:586) — DRY query/command auth fallthrough: - Discussed in the PR thread; not implemented in this commit. Both paths share a 3-way shape (TokenMissing -> fallthrough, other err -> reject, Ok -> pass) but the query path passes 'Maybe UserClaims' while the command path passes a built 'RequestContext'. Extracting a 'withAuthResolved' helper is a clear win; left as a follow-up so the rename can land cleanly first. Build green; nhcore-test-service (830/0 fail) and nhcore-test (1654/0 fail) pass serially. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
Addressed @NickSeagull comments —
|
…aims Both Web dispatch paths (command at 596+, query at 686+) had the same three-way case on the auth result: TokenMissing → fallthrough with no claims; any other AuthError → respondWithAuthError; valid token → pass claims forward. The difference was only what each path handed to its downstream handler (Maybe UserClaims for queries; a built RequestContext for commands). Extract a 'withResolvedClaims' helper at the bottom of Web.hs that resolves auth into 'Maybe UserClaims' (the lowest common denominator) and threads it to a caller-supplied continuation. The two dispatch sites become one-liners adapting the claims to their downstream shape. Addresses NickSeagull's comment #4 ("rather than mirroring, wouldn't it be better to DRY?") on PR #642 — option A: implement inline. Build green. nhcore-test-service 830/0 fail, nhcore-test 1654/0 fail. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
#4 (DRY) — implemented as
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/service/Service/Response.hs (1)
27-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not JSON-derive the raw
authErroron the public response type.
Unauthorizednow carriesAccessError, and generic JSON encoding will serializeInsufficientPermissions [...]verbatim whenever any caller emitsCommandResponsedirectly. That makes the new privacy guarantee transport-specific instead of enforced by the type. GiveCommandResponsea custom wire encoding for this branch, or keepAccessErrorout of the client-facing type entirely.🤖 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 `@core/service/Service/Response.hs` around lines 27 - 36, The Unauthorized branch of CommandResponse currently carries AccessError which will be generically serialized (exposing InsufficientPermissions details); remove raw AccessError from the public wire format by giving CommandResponse a custom JSON encoding: implement explicit Json.ToJSON and Json.FromJSON instances for CommandResponse that serialize the Unauthorized constructor without including authError (e.g., serialize as {"tag":"Unauthorized"} or include only an opaque code/message), or alternatively change the public Unauthorized constructor to hold a redacted type (e.g., an enum or unit) instead of AccessError; reference the CommandResponse type and its Unauthorized constructor and AccessError/InsufficientPermissions when making the change so encoding and decoding remain consistent.
🤖 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 `@core/service/Service/Transport/Web.hs`:
- Around line 596-623: The code treats webTransport.authEnabled = Nothing the
same as an anonymous user by calling Auth.anonymousContext and then running
processCommandWithContext, which causes the new canExecuteImpl gate
(authenticatedAccess default) to block previously-working no-auth deployments;
instead when webTransport.authEnabled is Nothing bypass the authentication gate
by creating and passing a trusted/no-auth execution context or a flag the
executor recognizes (rather than Auth.anonymousContext) so the
dispatcher/executor (canAccessImpl/canExecuteImpl) knows to skip
authenticatedAccess checks; update the branch that handles Nothing to either
call a new Auth.trustedContext (or similar) and pass it to
processCommandWithContext, or thread an explicit trustedMode boolean through the
RequestContext so Middleware/dispatcher can honor no-auth mode (reference
webTransport.authEnabled, Auth.anonymousContext, Auth.authenticatedContext,
processCommandWithContext, Middleware.checkAuth,
Middleware.respondWithAuthError).
In `@core/test-service/Main.hs`:
- Line 90: The test suite label string passed to Hspec.describe is incorrect:
currently it reads "Service.Query.Auth" while it executes
Service.Command.AuthSpec.spec; update the describe label to match the spec
(e.g., change the string to "Service.Command.Auth") so failures are reported
under the correct suite, keeping the Hspec.describe call and
Service.Command.AuthSpec.spec invocation unchanged.
In `@core/test/Service/Command/AuthSpec.hs`:
- Around line 81-83: Replace the plain-string fail calls used in the pattern
matches for the publicAccess test with the project’s formatted fail pattern (use
the same formatted style used elsewhere in this file) — update the branches
matching Just Unauthenticated, Just Forbidden, and Just (InsufficientPermissions
_) in the publicAccess test as well as the similar occurrences at the other
reported locations (lines flagged at 263 and 272) so they follow the
formatted-fail convention used by the other assertions in AuthSpec.hs (i.e., use
the same formatted/fprintf-style wrapper used in existing compliant cases
instead of a raw string).
In `@core/test/Service/Command/CanAccess/PermissionFixture.hs`:
- Around line 59-68: Replace point-free and wildcard definitions with explicit
parameter names: change canAccess to take a named parameter (e.g., userClaims)
and pass it to requirePermission "admin:delete"; change getEntityId to accept an
explicit CommandWithAdminDelete parameter (e.g., cmd) and return the correct
Maybe Uuid based on that parameter instead of using _; change decide to name its
three parameters (e.g., cmd, perm, ctx) and use them in the body when calling
Decider.acceptNew (or implement the proper logic) instead of using wildcards.
Ensure the function signatures remain the same types but use explicit argument
names for canAccess, getEntityId, and decide.
In `@core/test/Service/Command/CanAccess/PublicAccessFixture.hs`:
- Around line 57-66: The current functions use point-free and wildcard-argument
style which violates NeoHaskell guidelines; replace them with explicit parameter
names: change canAccess = publicAccess to canAccess claims = publicAccess
claims, change getEntityId _ = Nothing to getEntityId cmd = Nothing (use a real
parameter name like cmd or command), and change decide _ _ _ = Decider.acceptNew
[CanAccessPubEventCreated Uuid.nil] to decide cmd mctx ctx = Decider.acceptNew
[CanAccessPubEventCreated Uuid.nil] (using explicit names for the three
arguments). Ensure you reference the functions canAccess, getEntityId, and
decide when making the edits.
In `@core/test/Service/CommandExecutor/AuditLoggingSpec.hs`:
- Around line 3-6: Update the header comment in AuditLoggingSpec.hs to
accurately reflect the assertions: replace the claim that error constructors
carry “never permission strings or PII” with wording that they include
permission identifiers as asserted (e.g., InsufficientPermissions
["admin:delete"]) and do not promise a privacy guarantee; reference the test
suite name (AuditLoggingSpec) and the specific error constructor
InsufficientPermissions so readers know the comment aligns with the actual
expectations.
- Around line 10-20: Update imports to the "type + qualified-module" style used
across core tests: import the exported types (e.g. Array, Text, Maybe, Command,
UserClaims, AccessError) unqualified and import their modules as qualified (e.g.
import Array (Array) and import Array qualified, import Text (Text) and import
Text qualified, import Maybe (Maybe(..)) and import Maybe qualified, import
Service.Command.Core (Command(..), UserClaims(..)) and import
Service.Command.Core qualified, import Service.Query.Auth (AccessError(..)) and
import Service.Query.Auth qualified, etc.), and apply the same pattern for Map
and ToText/Test modules so the file follows core/test conventions and
qualified-import style.
In `@core/test/Service/Transport/InternalSpec.hs`:
- Around line 14-15: The test currently imports publicAccess unqualified from
Service.Query.Auth; keep the type import for AccessError but change the module
import to a qualified import (e.g., import qualified Service.Query.Auth as Auth)
and update all call sites to use the qualified name (Auth.publicAccess)
including the other occurrences referenced (previously at lines around 118-120)
so operations follow the "type + qualified module" guideline.
In `@core/test/Service/Transport/Web/CommandAuthSpec.hs`:
- Around line 242-249: The wildcard branch that does " _ -> pass " hides
regressions; replace it with an explicit expectation of the decider outcome
(e.g., assert the result is Rejected with the business reason "Cart does not
exist" or assert CommandAccepted/Accepted if the business path should accept) so
the test verifies that the dispatcher forwarded the publicAccess command to
decideImpl and that the decider produced the expected business-path result;
update the pattern matching for the block handling CommandUnauthorized -> fail
... to instead match the concrete Rejected/Accepted constructor and assert the
rejection reason (or acceptance) and apply the same stricter assertion change to
the other similar case currently at lines 262-265.
- Around line 24-52: Several imports in this test file are only brought in
unqualified (e.g., Maybe, ToText, Test) and should follow the “type + qualified
module” convention used for Array/Text/Task; add matching qualified imports for
any module where you import types or functions unqualified. Specifically, keep
the existing type/function imports (e.g., import Maybe (Maybe(..)), import
ToText (toText), import Test) and add corresponding qualified imports (import
Maybe qualified, import ToText qualified, import Test qualified) so the file
consistently uses the "import TYPE/SYMBOL and import MODULE qualified" pattern
(apply the same change for any other modules in the diff that currently lack a
qualified import).
In `@core/testlib/Test/Service/Command/Core.hs`:
- Line 34: Import Service.Query.Auth as a qualified module (e.g., `Auth`) and
replace all unqualified uses of `publicAccess` with the qualified call
`Auth.publicAccess` inside each `canExecuteImpl` binding; update the import
statement to the qualified form and change occurrences of `publicAccess`
(notably in the `canExecuteImpl` definitions) to use the alias so it follows the
project's "type + qualified module" convention.
In `@testbed/src/Testbed/Cart/Queries/CartSummary.hs`:
- Line 12: Change the unqualified import to a qualified import (import qualified
Service.Query.Auth as Auth) and update all references to AccessError,
UserClaims, publicAccess, and publicView to use the Auth. prefix (e.g.,
Auth.AccessError, Auth.publicAccess). Also rewrite the point-free definitions at
lines 44–45 and 50–51 into explicit-argument form by adding parameters and
applying functions explicitly (for example replace point-free compositions like
"f = publicAccess . g" with "f x = Auth.publicAccess (g x)" or similar based on
the actual function names).
In `@testbed/src/Testbed/Stock/Queries/StockLevel.hs`:
- Line 11: Replace the unqualified import from Service.Query.Auth with a
qualified import (e.g. import qualified Service.Query.Auth as Auth) and update
all usages to reference Auth.AccessError, Auth.UserClaims, Auth.publicAccess and
Auth.publicView; then eliminate point-free definitions in this module by giving
the affected functions explicit parameter lists and using those parameters when
calling Auth.publicAccess/Auth.publicView (rewrite each point-free function into
a normal definition with named arguments and a body that calls the qualified
functions).
---
Outside diff comments:
In `@core/service/Service/Response.hs`:
- Around line 27-36: The Unauthorized branch of CommandResponse currently
carries AccessError which will be generically serialized (exposing
InsufficientPermissions details); remove raw AccessError from the public wire
format by giving CommandResponse a custom JSON encoding: implement explicit
Json.ToJSON and Json.FromJSON instances for CommandResponse that serialize the
Unauthorized constructor without including authError (e.g., serialize as
{"tag":"Unauthorized"} or include only an opaque code/message), or alternatively
change the public Unauthorized constructor to hold a redacted type (e.g., an
enum or unit) instead of AccessError; reference the CommandResponse type and its
Unauthorized constructor and AccessError/InsufficientPermissions when making the
change so encoding and decoding remain 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48c14989-bba2-424e-aaf6-0aacbf7d42ba
📒 Files selected for processing (25)
core/core/Core.hscore/nhcore.cabalcore/service/Service/Command/Core.hscore/service/Service/CommandExecutor/Core.hscore/service/Service/CommandExecutor/TH.hscore/service/Service/Query/Auth.hscore/service/Service/Query/Core.hscore/service/Service/Query/TH.hscore/service/Service/Response.hscore/service/Service/Transport/Web.hscore/test-service/Main.hscore/test/Service/Command/AuthSpec.hscore/test/Service/Command/CanAccess/PermissionFixture.hscore/test/Service/Command/CanAccess/PublicAccessFixture.hscore/test/Service/Command/CanAccessSpec.hscore/test/Service/CommandExecutor/AuditLoggingSpec.hscore/test/Service/MultiTenantSpec.hscore/test/Service/Query/EndpointSpec.hscore/test/Service/Query/THSpec.hscore/test/Service/Transport/InternalSpec.hscore/test/Service/Transport/Web/CommandAuthSpec.hscore/test/Service/Transport/WebSpec.hscore/testlib/Test/Service/Command/Core.hstestbed/src/Testbed/Cart/Queries/CartSummary.hstestbed/src/Testbed/Stock/Queries/StockLevel.hs
Comment on lines
+81
to
+83
| Just Unauthenticated -> fail "publicAccess should not return Unauthenticated" | ||
| Just Forbidden -> fail "publicAccess should not return Forbidden" | ||
| Just (InsufficientPermissions _) -> fail "publicAccess should not require permissions" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*core/test/Service/Command/AuthSpec.hs" -type fRepository: neohaskell/NeoHaskell
Length of output: 105
🏁 Script executed:
# If found, get the file info and content
if [ -f "core/test/Service/Command/AuthSpec.hs" ]; then
wc -l core/test/Service/Command/AuthSpec.hs
echo "=== First 50 lines ==="
head -50 core/test/Service/Command/AuthSpec.hs
echo "=== Lines 75-90 ==="
sed -n '75,90p' core/test/Service/Command/AuthSpec.hs
echo "=== Checking imports ==="
head -20 core/test/Service/Command/AuthSpec.hs | grep -E "^import"
fiRepository: neohaskell/NeoHaskell
Length of output: 2586
🏁 Script executed:
# Check all the mentioned fail call locations
if [ -f "core/test/Service/Command/AuthSpec.hs" ]; then
echo "=== Lines around 81-83 ==="
sed -n '79,85p' core/test/Service/Command/AuthSpec.hs
echo ""
echo "=== Lines around 141-142 ==="
sed -n '139,145p' core/test/Service/Command/AuthSpec.hs
echo ""
echo "=== Lines around 151-152 ==="
sed -n '149,155p' core/test/Service/Command/AuthSpec.hs
fiRepository: neohaskell/NeoHaskell
Length of output: 1342
🏁 Script executed:
# Search for all fail calls in the file
if [ -f "core/test/Service/Command/AuthSpec.hs" ]; then
echo "=== All fail calls with line numbers ==="
rg "fail" core/test/Service/Command/AuthSpec.hs -n
fiRepository: neohaskell/NeoHaskell
Length of output: 1160
Address plain-string fail calls—lines 81–83, 263, and 272 require the formatted pattern.
Four fail calls use plain strings without format interpolation, violating the test guideline. Lines 141–142, 151–152, 181–182, 197–198, 219–220, 231–232, and 248–249 already comply with the format-string pattern and require no changes.
♻️ Suggested patch
- Just Unauthenticated -> fail "publicAccess should not return Unauthenticated"
- Just Forbidden -> fail "publicAccess should not return Forbidden"
- Just (InsufficientPermissions _) -> fail "publicAccess should not require permissions"
+ Just Unauthenticated -> fail [fmt|publicAccess should not return Unauthenticated|]
+ Just Forbidden -> fail [fmt|publicAccess should not return Forbidden|]
+ Just (InsufficientPermissions _) -> fail [fmt|publicAccess should not require permissions|]Apply the same pattern to lines 263 and 272:
- _ -> fail "Unauthenticated constructor not exported or does not match"
+ _ -> fail [fmt|Unauthenticated constructor not exported or does not match|]
- _ -> fail "InsufficientPermissions constructor shape mismatch"
+ _ -> fail [fmt|InsufficientPermissions constructor shape mismatch|]🤖 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 `@core/test/Service/Command/AuthSpec.hs` around lines 81 - 83, Replace the
plain-string fail calls used in the pattern matches for the publicAccess test
with the project’s formatted fail pattern (use the same formatted style used
elsewhere in this file) — update the branches matching Just Unauthenticated,
Just Forbidden, and Just (InsufficientPermissions _) in the publicAccess test as
well as the similar occurrences at the other reported locations (lines flagged
at 263 and 272) so they follow the formatted-fail convention used by the other
assertions in AuthSpec.hs (i.e., use the same formatted/fprintf-style wrapper
used in existing compliant cases instead of a raw string).
Comment on lines
+24
to
+52
| import Array (Array) | ||
| import Array qualified | ||
| import Basics | ||
| import Map qualified | ||
| import Maybe (Maybe (..)) | ||
| import Network.HTTP.Types.Status qualified as HTTP | ||
| import Service.Auth qualified as Auth | ||
| import Service.Query.Auth (AccessError (..)) | ||
| import Service.Command.Core (Command (..), UserClaims (..)) | ||
| import Service.CommandExecutor qualified as CommandExecutor | ||
| import Service.CommandExecutor.Core (ExecutionResult (..)) | ||
| import Service.EntityFetcher.Core (EntityFetcher) | ||
| import Service.EntityFetcher.Core qualified as EntityFetcher | ||
| import Service.Entity.Core (Entity (..)) | ||
| import Service.Event qualified as Event | ||
| import Service.Event.EntityName (EntityName (..)) | ||
| import Service.EventStore.Core qualified as EventStore | ||
| import Service.EventStore.InMemory qualified as InMemory | ||
| import Service.Transport.Web (unauthorizedResponse, unauthorizedResponseBody) | ||
| import Task (Task) | ||
| import Task qualified | ||
| import Test | ||
| import Test.Service.Command.Core (AddItemToCart (..), AuthenticatedAddItem (..), CartEntity) | ||
| import Test.Service.EventStore.Core (CartEvent) | ||
| import Text (Text) | ||
| import Text qualified | ||
| import ToText (toText) | ||
| import Uuid qualified | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Apply qualified-import convention consistently in this core test file.
Bring remaining unqualified imports in line with the module-qualified style used elsewhere in this file.
As per coding guidelines, "core//*.hs: Always import type + qualified module" and "/*.hs: Use qualified imports: EventStore.new style".
🤖 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 `@core/test/Service/Transport/Web/CommandAuthSpec.hs` around lines 24 - 52,
Several imports in this test file are only brought in unqualified (e.g., Maybe,
ToText, Test) and should follow the “type + qualified module” convention used
for Array/Text/Task; add matching qualified imports for any module where you
import types or functions unqualified. Specifically, keep the existing
type/function imports (e.g., import Maybe (Maybe(..)), import ToText (toText),
import Test) and add corresponding qualified imports (import Maybe qualified,
import ToText qualified, import Test qualified) so the file consistently uses
the "import TYPE/SYMBOL and import MODULE qualified" pattern (apply the same
change for any other modules in the diff that currently lack a qualified
import).
Code fixes:
- RequestContext: add 'trustedBypass :: Bool' and a 'trustedContext'
builder. The Web transport uses it when 'authEnabled = Nothing' so
no-auth deployments preserve the pre-PR semantics (every command
reachable regardless of its 'canAccess' declaration). The dispatcher
short-circuits the gate when 'trustedBypass = True'.
- Web.hs: split the 'authEnabled' case at the dispatch site. The
'Nothing' branch uses 'trustedContext' for commands and passes
'Maybe.Nothing' claims for queries (existing behavior). The 'Just
auth' branch routes through the simplified 'withResolvedClaims'
helper (now takes 'AuthEnabled' instead of 'Maybe AuthEnabled').
Test fixes:
- test-service/Main.hs:90: relabel the Hspec describe block from
'Service.Query.Auth' to 'Service.Command.Auth' (sed false-positive
from the earlier rename pass).
- CommandAuthSpec.hs dispatcher-parity: assert the EXACT post-gate
outcome ('CommandRejected "Cart does not exist"') instead of a
wildcard 'pass' branch, so a regression that turns the gate-pass
into Accepted, Failed, or a different reject reason is caught.
Style fixes (per NeoHaskell convention — explicit args, no point-free,
qualified imports):
- PermissionFixture, PublicAccessFixture: 'canAccess claims = ...'
with named '_command/_entity/_context' params for getEntityId and
decide; qualify 'requirePermission'/'publicAccess' via QueryAuth.
- testlib/Test/Service/Command/Core.hs: qualify 'publicAccess' and
expand the five 'canExecuteImpl' instance bindings to take an
explicit 'claims' argument.
- InternalSpec.hs: qualify 'publicAccess'.
Comment fixes:
- AuditLoggingSpec.hs header: clarify that 'InsufficientPermissions'
legitimately carries the required permission name on the return
value (so an audit emitter can log it); the privacy guarantee is
against PII leakage (sub/email/name/tenantId), and the HTTP body
separately never sees the permission name.
Build green. nhcore-test-service 830/0 fail, nhcore-test 1654/0 fail.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
Second-pass CodeRabbit review handled —
|
…tyle sweep Rename: - Service.Query.Auth → Service.AccessControl. The module owns shared auth primitives (AccessError + publicAccess / authenticatedAccess / requirePermission / requireAnyPermission / requireAllPermissions) used by BOTH command and query paths. The "Query.Auth" home name was misleading after the unified AccessError landed; the new name reflects the actual scope. - Sed-renamed every Service.Query.Auth reference (incl. the QueryAuth qualified alias) to Service.AccessControl across 23 .hs files + the cabal exposed-modules entry. Boy scout cleanup (per the rule the touched files now enforce): - testbed/src/Testbed/Cart/Queries/CartSummary.hs — qualify the AccessControl import (drop unqualified publicAccess/publicView), rewrite canAccess / canView with explicit named args (was point-free). - testbed/src/Testbed/Stock/Queries/StockLevel.hs — same treatment. Skills updated to encode the rule for future pipeline runs: - .claude/skills/feature-pipeline-preview/references/nhcore-context.md — new "Boy scout rule" section, listed first in the ToC. Defines scope (files in the current diff), what counts as debt, and the immutability carve-out for test bodies. - .claude/skills/feature-pipeline-preview/10-implementation/SKILL.md — rule listed under Assumptions so phase 10 agents apply it on every file they open for writing. - .claude/skills/feature-pipeline-preview/14-fix-findings/SKILL.md — same; when a fix re-opens a file, debt in that file goes too. - .claude/skills/feature-pipeline-preview/11-build-loop/03-fix-iter/SKILL.md — fix-iter steps inherit the rule on the files they edit. - .claude/skills/neohaskell-style-guide/SKILL.md — "Boy scout rule (load-bearing)" section at the top so style-guide consumers see it before the per-rule reference table. - .claude/skills/neohaskell-implementer/SKILL.md — added as Operating Principle #4; minimal-changes-elsewhere kept as #5. Build green; nhcore-test-service 830/0 fail, nhcore-test 1654/0 fail. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Hurl tests in CI surfaced the same regression class as the Web 'authEnabled = Nothing' case but for three other transports: - stock-reservation.hurl: ReserveStock is fired from an outbound integration (CartItemAdded -> ReserveStock). The integration dispatcher built 'anonymousContext' (trustedBypass = False), so the command's typeclass-default 'authenticatedAccess' gate rejected it with no logged context, leaving the stock untouched (available=100 reserved=0 instead of available=95 reserved=5). - periodic-cart-creation.hurl: the inbound timer integration fires CreateCart on a 3s tick. Same gate path, same silent rejection — the cart count never advanced past the first tick. The fix mirrors what was already done for Web: framework-internal transports use 'Auth.trustedContext' (trustedBypass = True) so the dispatcher's gate is bypassed. External untrusted callers still go through Web's JWT path and DO hit the gate; only the four "framework made this call itself" sites get the bypass. Changed: - core/service/Service/Transport/Cli.hs (CLI operator-trust transport) - core/service/Service/Transport/Mcp/Protocol.hs (MCP process-trust) - core/service/Service/Integration/Dispatcher.hs (inbound + outbound system-triggered commands) Local verification: full GHA hurl pipeline ran via 'nix develop --command ./testbed/scripts/run-tests.sh' against a docker postgres matching the CI workflow. All 5 scenario files + 2 integration files pass, 0 failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/test/Service/MultiTenantSpec.hs (1)
7-7: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winResolve the qualified import alias collision: use distinct names for
Service.AuthandService.AccessControl.Two distinct modules are imported with the same alias
Authat lines 7 and 20, creating fragility in symbol resolution. Rename theService.AccessControlimport toAccessControlor another distinct identifier, then update all call sites accordingly.import Service.Auth qualified as Auth import Service.AccessControl qualified as AccessControl🤖 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 `@core/test/Service/MultiTenantSpec.hs` at line 7, Two modules are imported with the same alias causing collisions; change the import alias for Service.AccessControl from "Auth" to a distinct name (e.g., "AccessControl") and update all call sites that reference Service.AccessControl functions/types to use the new alias. Specifically, modify the import of Service.AccessControl to use "AccessControl" and replace references like Auth.someFunctionOrType (that come from Service.AccessControl) with AccessControl.someFunctionOrType while leaving Service.Auth references as Auth.
🤖 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 `@core/nhcore.cabal`:
- Line 257: Restore the dropped public module by adding a compatibility shim
module named Service.Query.Auth that simply re-exports Service.AccessControl
(i.e. module Service.Query.Auth (module Service.AccessControl) where; import
Service.AccessControl) and then add Service.Query.Auth back to the exposed
modules list so downstream consumers still compile; keep this shim for at least
one release window and mark it deprecated in a comment or Haddock if desired,
then run the build/tests to ensure no breakage.
In `@core/service/Service/CommandExecutor/Core.hs`:
- Around line 25-26: Remove the duplicate import of AccessError: keep a single
import of Service.AccessControl that brings in AccessError (e.g., either "import
Service.AccessControl (AccessError)" or "import Service.AccessControl
(AccessError (..))") and delete the other redundant import line so only one
import for AccessError remains in Core.hs.
In `@testbed/src/Testbed/Stock/Queries/StockLevel.hs`:
- Line 11: Replace the unqualified type import of AccessError and UserClaims
with a qualified import of the Service.AccessControl module (e.g., import
Service.AccessControl as AccessControl) and update all references to those types
to use the module qualifier (AccessControl.AccessError,
AccessControl.UserClaims); do the same pattern for the other unqualified
occurrences mentioned (lines referenced as 36-37 and 41-42) so all types and
functions from Service.AccessControl follow the EventStore.new-style qualified
usage.
---
Outside diff comments:
In `@core/test/Service/MultiTenantSpec.hs`:
- Line 7: Two modules are imported with the same alias causing collisions;
change the import alias for Service.AccessControl from "Auth" to a distinct name
(e.g., "AccessControl") and update all call sites that reference
Service.AccessControl functions/types to use the new alias. Specifically, modify
the import of Service.AccessControl to use "AccessControl" and replace
references like Auth.someFunctionOrType (that come from Service.AccessControl)
with AccessControl.someFunctionOrType while leaving Service.Auth references as
Auth.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1f52a29-eb5c-4cf7-b47e-f2fd648bcfd4
📒 Files selected for processing (37)
.claude/skills/feature-pipeline-preview/10-implementation/SKILL.md.claude/skills/feature-pipeline-preview/11-build-loop/03-fix-iter/SKILL.md.claude/skills/feature-pipeline-preview/14-fix-findings/SKILL.md.claude/skills/feature-pipeline-preview/references/nhcore-context.md.claude/skills/neohaskell-implementer/SKILL.md.claude/skills/neohaskell-style-guide/SKILL.mdcore/nhcore.cabalcore/service/Service/AccessControl.hscore/service/Service/Auth.hscore/service/Service/Command/Core.hscore/service/Service/CommandExecutor/Core.hscore/service/Service/Integration/Dispatcher.hscore/service/Service/Query/Core.hscore/service/Service/Query/Endpoint.hscore/service/Service/Query/TH.hscore/service/Service/Response.hscore/service/Service/Transport.hscore/service/Service/Transport/Cli.hscore/service/Service/Transport/Mcp/Protocol.hscore/service/Service/Transport/Web.hscore/test-service/Main.hscore/test/Service/AuthSpec.hscore/test/Service/Command/AuthSpec.hscore/test/Service/Command/CanAccess/PermissionFixture.hscore/test/Service/Command/CanAccess/PublicAccessFixture.hscore/test/Service/Command/CanAccessSpec.hscore/test/Service/CommandExecutor/AuditLoggingSpec.hscore/test/Service/MultiTenantSpec.hscore/test/Service/Query/EndpointSpec.hscore/test/Service/Query/THSpec.hscore/test/Service/Transport/InternalSpec.hscore/test/Service/Transport/Web/CommandAuthSpec.hscore/test/Service/Transport/WebSpec.hscore/testlib/Test/Service/Command/Core.hscore/testlib/Test/Service/Command/Decide/Spec.hstestbed/src/Testbed/Cart/Queries/CartSummary.hstestbed/src/Testbed/Stock/Queries/StockLevel.hs
| Service.OutboundIntegration.TH | ||
| Service.Query | ||
| Service.Query.Auth | ||
| Service.AccessControl |
Contributor
There was a problem hiding this comment.
Preserve a compatibility export for Service.Query.Auth.
Line [257] drops a previously exposed public module path, which will break downstream consumers at compile time. Keep a deprecated shim (Service.Query.Auth re-exporting Service.AccessControl) for at least one release window.
Suggested compatibility patch
exposed-modules:
+ Service.Query.Auth
Service.AccessControl-- core/service/Service/Query/Auth.hs
module Service.Query.Auth (module Service.AccessControl) where
import Service.AccessControl🤖 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 `@core/nhcore.cabal` at line 257, Restore the dropped public module by adding a
compatibility shim module named Service.Query.Auth that simply re-exports
Service.AccessControl (i.e. module Service.Query.Auth (module
Service.AccessControl) where; import Service.AccessControl) and then add
Service.Query.Auth back to the exposed modules list so downstream consumers
still compile; keep this shim for at least one release window and mark it
deprecated in a comment or Haddock if desired, then run the build/tests to
ensure no breakage.
Comment on lines
+25
to
+26
| import Service.AccessControl (AccessError) | ||
| import Service.AccessControl (AccessError (..)) |
Contributor
There was a problem hiding this comment.
Duplicate import of AccessError detected, mortal.
Behold, the same import is declared twice. This redundancy offends the elegance of your code. Remove one of these lines to restore order.
-import Service.AccessControl (AccessError)
-import Service.AccessControl (AccessError (..))
+import Service.AccessControl (AccessError (..))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import Service.AccessControl (AccessError) | |
| import Service.AccessControl (AccessError (..)) | |
| import Service.AccessControl (AccessError (..)) |
🤖 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 `@core/service/Service/CommandExecutor/Core.hs` around lines 25 - 26, Remove
the duplicate import of AccessError: keep a single import of
Service.AccessControl that brings in AccessError (e.g., either "import
Service.AccessControl (AccessError)" or "import Service.AccessControl
(AccessError (..))") and delete the other redundant import line so only one
import for AccessError remains in Core.hs.
| import Core | ||
| import Json qualified | ||
| import Service.Query.Auth (QueryAuthError, UserClaims, publicAccess, publicView) | ||
| import Service.AccessControl (AccessError, UserClaims) |
Contributor
There was a problem hiding this comment.
Decree: Qualify thy type imports in accordance with the sacred guidelines.
Mortal developer, whilst thou hast correctly eliminated the point-free heresy and qualified thy function invocations, the unqualified import of types AccessError and UserClaims on Line 11 remains in defiance of the repository's immutable law: "Use qualified imports: EventStore.new style." The previous oracle's wisdom explicitly commanded full qualification, including types.
⚡ Divine transformation to align with the ordained style
-import Service.AccessControl (AccessError, UserClaims)
import Service.AccessControl qualified as AccessControl-canAccess :: Maybe UserClaims -> Maybe AccessError
-canAccess claims = AccessControl.publicAccess claims
+canAccess :: Maybe AccessControl.UserClaims -> Maybe AccessControl.AccessError
+canAccess claims = AccessControl.publicAccess claims-canView :: Maybe UserClaims -> StockLevel -> Maybe AccessError
-canView claims stockLevel = AccessControl.publicView claims stockLevel
+canView :: Maybe AccessControl.UserClaims -> StockLevel -> Maybe AccessControl.AccessError
+canView claims stockLevel = AccessControl.publicView claims stockLevelAs per coding guidelines: **/*.hs requires qualified imports, with the exemplar EventStore.new style demanding module-prefixed references throughout.
Also applies to: 36-37, 41-42
🤖 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 `@testbed/src/Testbed/Stock/Queries/StockLevel.hs` at line 11, Replace the
unqualified type import of AccessError and UserClaims with a qualified import of
the Service.AccessControl module (e.g., import Service.AccessControl as
AccessControl) and update all references to those types to use the module
qualifier (AccessControl.AccessError, AccessControl.UserClaims); do the same
pattern for the other unqualified occurrences mentioned (lines referenced as
36-37 and 41-42) so all types and functions from Service.AccessControl follow
the EventStore.new-style qualified usage.
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.
Per-command AuthOptions — let commands declare Everyone vs Authenticated
Summary
This PR implements ADR-0058, allowing individual commands to declare their own authentication requirements via a
canAccessfunction, mirroring the existing query-side pattern. Commands default to requiring authentication (authenticatedAccess) — going public is an explicit, single-line opt-in:canAccess = publicAccess. The Web transport now falls through to an anonymous context on missing tokens, letting the dispatcher'scanAccessImpldecide whether that command tolerates unauthenticated callers. Invalid tokens are still rejected immediately, preventing JWT-validity probing.Closes #638
Context
Previously, all commands were treated as all-or-nothing at the transport layer: if
Application.withAuthwas wired, every command required a valid JWT regardless of which command. This PR enables:SubmitFeedbackcommand coexists with authenticated commands in the same service.admin:deletecommand requires theadmin:deletepermission viacanAccess = requirePermission "admin:delete".See #638 for the original design discussion.
What changed
Source files modified (5)
core/service/Service/Command/Core.hs— AddedcanAccessImplmethod to theCommandtypeclass with defaultauthenticatedAccess. Typeclass default enforces whitelist-by-default: a command with nocanAccessoverride rejects unauthenticated callers.core/service/Service/CommandExecutor/Core.hs— Dispatcher callscanAccessImplbeforedecideImpl. On authorization failure, returnsCommandUnauthorized(newExecutionResultvariant).core/service/Service/CommandExecutor/TH.hs— TH macro optionally wires top-levelcanAccessintocanAccessImpl. WhencanAccessis absent, typeclass default applies (no boilerplate required).core/service/Service/Transport/Web.hs— Fallthrough on missing token; still rejects invalid tokens. MapsCommandUnauthorizedto 401/403 responses using the same error matrix as queries.core/service/Service/Query/Auth.hs— Minor: exportspublicAccess,authenticatedAccess,requirePermission,requireAnyPermission,requireAllPermissionsunchanged (they were already accessible via internal imports; now available to commands).New files (2)
core/service/Service/Command/Auth.hs— Re-exportsQueryAuthErrorasCommandAuthError(alias) plus the five permission helpers. No duplication: commands reuse the exact same error type and helper bodies as queries.docs/decisions/0058-per-command-auth-options.md— ADR with design rationale, considered options, and the complete matrix of request × auth config × response outcomes.Tests added (23 new examples across 4 spec files)
core/test/Service/Command/CanAccessSpec.hs— TH-resolution tests: commands with nocanAccessdefault toauthenticatedAccess; commands withcanAccess = publicAccessresolve correctly.core/test/Service/Transport/Web/CommandAuthSpec.hs— Web dispatcher tests: missing token + public → 200 anonymous; invalid token + public → 401; valid token + permission → 200.core/test/Service/CommandExecutor/AuditLoggingSpec.hs— Audit logging:CommandUnauthorizedcases are logged with command type, claims-present boolean, and error constructor name (no claim contents or token bytes leaked).core/testlib/Test/Service/CommandHandler/Execute/Spec.hs— Updates to existing multitenancy tests to account for the newcanAccessImplcall site in the dispatcher.Backwards compatibility
Default is
authenticatedAccess; commands with nocanAccessdeclared behave exactly as today. Existing services are not affected. The Web transport's fallthrough onTokenMissingonly matters for commands that explicitly opt into public access viacanAccess = publicAccessor similar. Commands that define nothing inherit the secure default and continue to reject unauthenticated callers.Security
Findings from phase 12 (implementation review) were either addressed in the ADR or deferred to test spec (phase 8):
canAccesscannot accidentally ship a public endpoint. The typeclass default forcesauthenticatedAccess.CommandUnauthorized: Rejection events are logged atLog.warnwith three fields only: command type name, booleanclaims_present, and theCommandAuthErrorconstructor name. Permission strings, claim contents, token bytes, and JWT subjects are never logged — they would leak sensitive identifiers into the operator's log pipeline.See
.pipeline/findings-04.jsonfor the full security review.Performance
Findings from phase 13 (performance review) are kept as advisories; none block the PR:
Typeclass dispatch overhead on the hot path (
canAccessImpl @commandonce per request): Expected negligible due to GHC specialization at the call site. Post-merge profiling task: Verify GHC specialisescanAccessImpl @commandat the dispatcher call site via Core dumps (-ddump-simpl) or late-cost-centre profile. Only add{-# INLINABLE canAccessImpl #-}if profiling shows >50 ns/call dictionary-passing overhead. Likely a no-op.Case-match branching in the dispatcher: The new
case canAccessImpl ofhas minimal overhead and should be inlined/merged by GHC, but profiling can confirm no instruction-cache pressure.See
.pipeline/findings-05.jsonand.pipeline/findings-13.jsonfor details.Pre-existing flake warning
If CI flakes on EventStore ordering tests or
CommandHandler Execute Specification Tests → Concurrency Handling, re-run. We observed non-deterministic failure counts (4+2 vs 2+4) on consecutivecabal test allruns of unchanged code. Each suite passes cleanly when run serially. The root cause is the shared Postgres instance — separate infra concern, not introduced by this PR. Failure signatures that ARE introduced by this PR would be inCommandAuthSpec,AuditLoggingSpec,CanAccessSpec, orService.MultiTenant.Tests
All suites pass with 4184 examples / 0 failures when run serially.
ADR-0058 coverage: 23 new examples across:
CanAccessSpec(TH-resolution tests)CommandAuthSpec(Web dispatcher matrix)AuditLoggingSpec(logging validation)Service.MultiTenant(multitenancy integration)Hlint
119 hints total, none in new code (all in pre-existing
testbed/examples).Checklist
Closes
Closes #638
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation