feat(node-message-broker): analysis authorization — scope binding + capability check#13
Merged
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds Authup-backed permission checks, shared authz and analysis lookup contracts, HTTP middleware and application wiring for the new checker, and an analysis ownership policy helper with unit tests. ChangesAuthup authorization wiring
Sequence Diagram(s)sequenceDiagram
participant HTTPModule
participant mountPermissionChecker
participant HttpPermissionProvider
participant AuthupPermissionGateway
participant AuthupClient
HTTPModule->>mountPermissionChecker: mount checker when authupClient resolves
mountPermissionChecker->>HttpPermissionProvider: create with CallerIdentity
HttpPermissionProvider->>AuthupPermissionGateway: holds(permission, identity)
AuthupPermissionGateway->>AuthupClient: permission.check(permission, { identity })
AuthupClient-->>AuthupPermissionGateway: status
AuthupPermissionGateway-->>HttpPermissionProvider: boolean grant
HttpPermissionProvider-->>mountPermissionChecker: binding or null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
tada5hi
force-pushed
the
feat/analysis-policy
branch
from
593acbd to
f14a602
Compare
Enforce node-side that an analysis caller is bound to the path analysis. The Hub stays analysis-agnostic; analysis scope is a node-only rule. assertClientOwnsAnalysis(analyses, analysisId, clientId) (core/analysis) asserts the caller's client (from the verified request identity) owns the analysis by matching it against the analysis's client in server-core (IAnalysisClientLookup, wrapping analysis.getOne). A missing client or mismatch throws ForbiddenError. The check is a pure rule over the narrow IAnalysisClientLookup port -- no adapter class, DI module, or injection key needed. The earlier IAnalysisPolicy / AnalysisPolicy / AnalysisPolicyModule were dropped as needless ceremony around a three-line assertion. The ANALYSIS_SELF_MESSAGE_BROKER_USE capability is enforced separately at the route via the request permission checker (useRequestPermissionChecker), backed by Authup -- not read off introspection.permissions (being removed). That route enforcement lands with the S5 controller. Closes #6
tada5hi
force-pushed
the
feat/analysis-policy
branch
from
f14a602 to
eff7bfc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/node-message-broker/src/adapters/analysis/analysis-policy.ts`:
- Around line 31-38: `assertBelongsToAnalysis` in `AnalysisPolicy` is only
checking `clientId`, which widens authorization to client-level and lets any
analysis under the same client pass. Update the policy to accept and validate
the caller’s analysis identity as well (or derive it from trusted claims), then
compare that verified analysis against `analysisId` instead of only matching
`getClientId(analysisId)` to `clientId`. Keep the existing `ForbiddenError`
paths in place, but make the success condition require the exact analysis to
match.
- Around line 36-38: Wrap the `this.analyses.getClientId(analysisId)` lookup in
`AnalysisPolicy` so any rejection is treated the same as a failed authorization
check and results in `ForbiddenError`, not a raw upstream error. Update the
`isAllowed`/policy method that compares `analysisClientId` and `clientId` to
catch lookup failures and normalize them to the existing `ForbiddenError('The
caller does not belong to this analysis.')`, keeping the failure closed behavior
for `AnalysisPolicy` and `getClientId`.
In
`@apps/node-message-broker/test/unit/adapters/analysis/analysis-policy.spec.ts`:
- Around line 30-50: The unit tests in analysis-policy.spec.ts only match
rejection text, so they can pass for the wrong error type; update the
`policy.assertBelongsToAnalysis` assertions in these cases to verify the promise
rejects with `ForbiddenError` as well as the expected message. Use the existing
`setup()` helper and `analysisPolicy`/`policy.assertBelongsToAnalysis` test
cases to keep the denial contract explicit for the “not bound to a client,”
“unknown analysis,” and “does not own the analysis” paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c24d287-34d7-4b32-a832-8e8a47167aa8
📒 Files selected for processing (12)
apps/node-message-broker/src/adapters/analysis/analysis-policy.tsapps/node-message-broker/src/adapters/analysis/index.tsapps/node-message-broker/src/app/builder.tsapps/node-message-broker/src/app/factory.tsapps/node-message-broker/src/app/modules/analysis-policy/constants.tsapps/node-message-broker/src/app/modules/analysis-policy/index.tsapps/node-message-broker/src/app/modules/analysis-policy/module.tsapps/node-message-broker/src/app/modules/core-client/constants.tsapps/node-message-broker/src/app/modules/core-client/module.tsapps/node-message-broker/src/core/analysis/types.tsapps/node-message-broker/test/unit/adapters/analysis/analysis-policy.spec.tsapps/node-message-broker/test/unit/adapters/analysis/fake-analysis-client-lookup.ts
Comment on lines
+31
to
+38
| async assertBelongsToAnalysis(analysisId: string, clientId: string | undefined): Promise<void> { | ||
| if (!clientId) { | ||
| throw new ForbiddenError('The caller is not bound to a client.'); | ||
| } | ||
|
|
||
| const analysisClientId = await this.analyses.getClientId(analysisId); | ||
| if (!analysisClientId || analysisClientId !== clientId) { | ||
| throw new ForbiddenError('The caller does not belong to this analysis.'); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Analysis-level isolation was widened to client-level isolation.
Line 31 only accepts clientId, and Lines 36-37 authorize solely by matching the analysis owner client. That makes two analyses under the same client interchangeable here, so this policy cannot enforce the stated requirement to reject a caller whose analysis does not match :id. Pass the verified analysis identity into the policy (or resolve it from verified claims) and compare that directly.
🤖 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 `@apps/node-message-broker/src/adapters/analysis/analysis-policy.ts` around
lines 31 - 38, `assertBelongsToAnalysis` in `AnalysisPolicy` is only checking
`clientId`, which widens authorization to client-level and lets any analysis
under the same client pass. Update the policy to accept and validate the
caller’s analysis identity as well (or derive it from trusted claims), then
compare that verified analysis against `analysisId` instead of only matching
`getClientId(analysisId)` to `clientId`. Keep the existing `ForbiddenError`
paths in place, but make the success condition require the exact analysis to
match.
Comment on lines
+36
to
+38
| const analysisClientId = await this.analyses.getClientId(analysisId); | ||
| if (!analysisClientId || analysisClientId !== clientId) { | ||
| throw new ForbiddenError('The caller does not belong to this analysis.'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize lookup failures to ForbiddenError.
If this.analyses.getClientId(analysisId) rejects, the raw upstream error escapes instead of the policy failing closed with ForbiddenError. That turns a dependency hiccup into a 500 on an authorization path.
Suggested fix
- const analysisClientId = await this.analyses.getClientId(analysisId);
+ let analysisClientId: string | null;
+ try {
+ analysisClientId = await this.analyses.getClientId(analysisId);
+ } catch {
+ throw new ForbiddenError('The caller does not belong to this analysis.');
+ }
+
if (!analysisClientId || analysisClientId !== clientId) {
throw new ForbiddenError('The caller does not belong to this analysis.');
}📝 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
| const analysisClientId = await this.analyses.getClientId(analysisId); | |
| if (!analysisClientId || analysisClientId !== clientId) { | |
| throw new ForbiddenError('The caller does not belong to this analysis.'); | |
| let analysisClientId: string | null; | |
| try { | |
| analysisClientId = await this.analyses.getClientId(analysisId); | |
| } catch { | |
| throw new ForbiddenError('The caller does not belong to this analysis.'); | |
| } | |
| if (!analysisClientId || analysisClientId !== clientId) { | |
| throw new ForbiddenError('The caller does not belong to this analysis.'); | |
| } |
🤖 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 `@apps/node-message-broker/src/adapters/analysis/analysis-policy.ts` around
lines 36 - 38, Wrap the `this.analyses.getClientId(analysisId)` lookup in
`AnalysisPolicy` so any rejection is treated the same as a failed authorization
check and results in `ForbiddenError`, not a raw upstream error. Update the
`isAllowed`/policy method that compares `analysisClientId` and `clientId` to
catch lookup failures and normalize them to the existing `ForbiddenError('The
caller does not belong to this analysis.')`, keeping the failure closed behavior
for `AnalysisPolicy` and `getClientId`.
Add the capability half of analysis authorization: the node asks Authup whether
the caller holds a named permission over HTTP, instead of reading it off the
token's (soon-removed) introspection permissions.
- AuthupPermissionGateway wraps POST /permissions/:id/check (authenticated as the
node client, caller identity in the body); status==='success' is a grant. Answers
are cached briefly (shared, keyed by permission+identity; coalesces in-flight,
evicts transient failures; ttl<=0 disables).
- HttpPermissionProvider plugs @authup/access's IPermissionProvider seam (grant ->
policy-less binding, deny -> null), the engine's one extension point.
- A per-request middleware overrides the request permission checker so
useRequestPermissionChecker().check({ name }) evaluates through that provider;
mounted between the authorization middleware and the controllers, skipped in tests.
- Register the Authup client (withAuthupHook + withAuthupClient) so token
verification is real and the gateway has a client.
The route-level check({ name: ANALYSIS_SELF_MESSAGE_BROKER_USE }) call lands with
the S5 endpoints.
tada5hi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/node-message-broker/test/unit/core/analysis/policy.spec.ts (1)
23-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the rejection type, not just the message.
The denial cases match only on error text, so they'd still pass for a generic
Error(or a leaked downstream error) carrying the same message. SinceForbiddenErroris the contract this policy enforces, also assert the rejection type.💚 Example for one case
+import { ForbiddenError } from '`@ebec/http`'; @@ - await expect(assertClientOwnsAnalysis(analyses, 'a1', undefined)).rejects.toThrow(/not bound to a client/i); + await expect(assertClientOwnsAnalysis(analyses, 'a1', undefined)).rejects.toBeInstanceOf(ForbiddenError); + await expect(assertClientOwnsAnalysis(analyses, 'a1', undefined)).rejects.toThrow(/not bound to a client/i);🤖 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 `@apps/node-message-broker/test/unit/core/analysis/policy.spec.ts` around lines 23 - 40, The policy tests for assertClientOwnsAnalysis currently only verify the rejection message, so they can pass for the wrong error type. Update the denial-case assertions in policy.spec.ts to also check that assertClientOwnsAnalysis rejects with ForbiddenError, using the same test cases around assertClientOwnsAnalysis and FakeAnalysisClientLookup so the contract is validated by type as well as message.
🤖 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 `@apps/node-message-broker/src/app/modules/http/module.ts`:
- Around line 84-87: Remove the test-environment bypass in module.ts so the
AuthupPermissionGateway is always mounted whenever authupResult.success is true,
using mountPermissionChecker and the AuthupPermissionGateway adapter without the
isTestEnvironment guard. If client resolution is not reliable in production,
fail startup explicitly instead of silently skipping permission gateway setup,
so the authorization path is consistently enforced outside tests.
---
Duplicate comments:
In `@apps/node-message-broker/test/unit/core/analysis/policy.spec.ts`:
- Around line 23-40: The policy tests for assertClientOwnsAnalysis currently
only verify the rejection message, so they can pass for the wrong error type.
Update the denial-case assertions in policy.spec.ts to also check that
assertClientOwnsAnalysis rejects with ForbiddenError, using the same test cases
around assertClientOwnsAnalysis and FakeAnalysisClientLookup so the contract is
validated by type as well as message.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e13ca52c-fc34-4b86-a56e-28e096c4dab3
📒 Files selected for processing (19)
apps/node-message-broker/src/adapters/authz/authup-permission-gateway.tsapps/node-message-broker/src/adapters/authz/http-permission-provider.tsapps/node-message-broker/src/adapters/authz/index.tsapps/node-message-broker/src/adapters/http/middleware/permission-checker.tsapps/node-message-broker/src/app/factory.tsapps/node-message-broker/src/app/modules/core-client/constants.tsapps/node-message-broker/src/app/modules/core-client/module.tsapps/node-message-broker/src/app/modules/http/module.tsapps/node-message-broker/src/core/analysis/index.tsapps/node-message-broker/src/core/analysis/policy.tsapps/node-message-broker/src/core/analysis/types.tsapps/node-message-broker/src/core/authz/index.tsapps/node-message-broker/src/core/authz/types.tsapps/node-message-broker/test/unit/adapters/authz/authup-permission-gateway.spec.tsapps/node-message-broker/test/unit/adapters/authz/fake-permission-check-client.tsapps/node-message-broker/test/unit/adapters/authz/fake-permission-gateway.tsapps/node-message-broker/test/unit/adapters/authz/http-permission-provider.spec.tsapps/node-message-broker/test/unit/core/analysis/fake-analysis-client-lookup.tsapps/node-message-broker/test/unit/core/analysis/policy.spec.ts
✅ Files skipped from review due to trivial changes (4)
- apps/node-message-broker/test/unit/adapters/authz/authup-permission-gateway.spec.ts
- apps/node-message-broker/src/core/analysis/index.ts
- apps/node-message-broker/test/unit/adapters/authz/fake-permission-check-client.ts
- apps/node-message-broker/src/core/authz/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/node-message-broker/src/app/modules/core-client/constants.ts
- apps/node-message-broker/src/app/modules/core-client/module.ts
Comment on lines
+84
to
+87
| if (authupResult.success && !isTestEnvironment) { | ||
| const gateway = new AuthupPermissionGateway({ client: authupResult.data }); | ||
| mountPermissionChecker(app, gateway); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how withAuthupClient registers AuthupClientInjectionKey and whether it can fail silently.
rg -nP -C4 'withAuthupClient|AuthupClientInjectionKey' apps/node-message-broker/srcRepository: PrivateAIM/node
Length of output: 2575
🏁 Script executed:
#!/bin/bash
# Check how mountMiddlewares is called and if authorization is always included.
sed -n '40,70p' apps/node-message-broker/src/app/modules/http/module.tsRepository: PrivateAIM/node
Length of output: 1613
🏁 Script executed:
#!/bin/bash
# Find the mountMiddlewares function implementation to see how it handles undefined authupClient.
rg -n "function mountMiddlewares|^export.*mountMiddlewares" apps/node-message-broker/src --type tsRepository: PrivateAIM/node
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Search for mountMiddlewares definition (it might be named differently or in a different file)
rg -n "mountMiddlewares" apps/node-message-broker/src --type ts -A 2 --context 5Repository: PrivateAIM/node
Length of output: 2448
🏁 Script executed:
#!/bin/bash
# Search in the node_modules or the privateaim package to find the implementation of mountMiddlewares.
# Since it's imported from `@privateaim/server-http-kit`, check if that local package exists in the repo.
fd "server-http-kit" --type d || fd "server-http" --type dRepository: PrivateAIM/node
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Look for tsconfig paths or check root for the internal packages
ls -la packages/ 2>/dev/null || ls -la apps/Repository: PrivateAIM/node
Length of output: 319
🏁 Script executed:
#!/bin/bash
# Try to find where the internal libs are located (maybe in root)
find . -maxdepth 2 -type d -name "*server*" 2>/dev/null | head -10
# Also search for the token verifier to understand the "fallback" logic
rg "createAuthupTokenVerifier" apps/node-message-broker/src --type ts -A 5Repository: PrivateAIM/node
Length of output: 1381
🏁 Script executed:
#!/bin/bash
# Search for files containing "mountMiddlewares" to see if there's a local implementation or if we can infer behavior.
# Also check imports from '`@privateaim/server-kit`' to see what's available.
rg "export.*mountMiddlewares" --type tsRepository: PrivateAIM/node
Length of output: 153
Remove the isTestEnvironment gate for the AuthupPermissionGateway mount to enforce strict authorization in production.
If authupClient fails to resolve in a production environment, the current logic skips mounting the new permission checker, leaving the application reliant solely on the basic token verifier. This bypasses the required permission gateway logic defined by the AuthupPermissionGateway adapter. The authorization configuration passed to mountMiddlewares accepts an undefined client, which likely defaults to a legacy mode or fails to enrich permissions, violating the principle that the new permission system must be mandatory outside of test environments. Remove the conditional check && !isTestEnvironment (or check else throw) to ensure AuthupPermissionGateway is always mounted when a valid client is resolved, or fail the startup process if the client resolution logic is unreliable.
🤖 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 `@apps/node-message-broker/src/app/modules/http/module.ts` around lines 84 -
87, Remove the test-environment bypass in module.ts so the
AuthupPermissionGateway is always mounted whenever authupResult.success is true,
using mountPermissionChecker and the AuthupPermissionGateway adapter without the
isTestEnvironment guard. If client resolution is not reliable in production,
fail startup explicitly instead of silently skipping permission gateway setup,
so the authorization path is consistently enforced outside tests.
Contributor
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
…nials Denial tests now assert the rejection is a ForbiddenError (not just the message), so a generic or leaked downstream error with matching text cannot pass them. Also document the 1:1 analysis<->client invariant on assertClientOwnsAnalysis: each analysis has a dedicated client, so matching the caller's client to the analysis owner is exact analysis-level isolation.
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
1 participant
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.
Summary
Node-side authorization for inbound analysis callers (issue #6). The Hub stays analysis-agnostic, so both checks live on the node. Two separable concerns:
ANALYSIS_SELF_MESSAGE_BROKER_USE) — evaluated against Authup over HTTP, not read from the token's (soon-removed) introspection permissions.Analysis scope
assertClientOwnsAnalysis(analyses, analysisId, clientId)(incore/analysis/): rejects a caller with no client, and binds the caller to the analysis — the caller's client (from the verified request identity) must equal the analysis's owning client in server-core (analysis.getOne(:id).client_id, via the narrowIAnalysisClientLookupport). A pure rule over one infra-isolating port — no policy class/adapter/DI module.Capability
A custom HTTP permission binding plugged into Authup's evaluator, so
useRequestPermissionChecker(event).check({ name })resolves the decision against the Hub:AuthupPermissionGateway— wrapsPOST /permissions/:id/check(authenticated as the node client; the caller's identity rides in the body, which the endpoint honors over the bearer).status === 'success'→ grant. Answers are cached for a few seconds (shared, keyed by permission + identity; coalesces in-flight calls, evicts transient failures,ttl<=0disables).HttpPermissionProvider— implements@authup/access'sIPermissionProvider(the engine's one extension seam): grant → policy-less binding, deny →null.setRequestPermissionChecker(...), mounted between the authorization middleware and the controllers; skipped in tests.withAuthupHook()+withAuthupClient()).Background: the framework hard-codes its permission provider from
introspection.permissionswith no injection hook, so once that field is removed every real check would deny. The supported path is to override the request permission checker with a customIPermissionProvider— confirmed by deep-reading@authup/access/@privateaim/server-(http-)kitand the Authup server-core source.Behavioral note
Registering the Authup client flips
mountAuthorizationMiddlewareout of fake mode → real token verification for all routes in non-test (previously effectively unauthenticated). Tests stay dry-run/grant-all and the override is skipped, so the suite is network-free.Scope note
This delivers the authorization building blocks. The route-level
check({ name: ANALYSIS_SELF_MESSAGE_BROKER_USE })+assertClientOwnsAnalysis(...)call sites land with the S5 send/participants endpoints.Tests (54 passing)
success; deny otherwise; identity mapped into the body; caches within TTL; keyed by permission+identity; transient failure not cached;ttl<=0disablesbuild(tsdown + tsc),eslint, andnx test node-message-brokerall clean.Closes #6
Summary by CodeRabbit