Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughA new mock HTTP server infrastructure is introduced for testing authorization decisions. The system includes a Go-based server application that manages mock response rules, a health endpoint, and dynamic bulk decision generation. TypeScript test helpers provide type-safe functions to interact with the mock server. An end-to-end test suite for ABAC with external PDP is added, demonstrating the infrastructure in action with various authorization scenarios. Changes
Sequence DiagramsequenceDiagram
actor Test
participant App as Application
participant MockServer as Mock Server
Test->>MockServer: POST /__mock/set-bulk-decision<br/>(permit_values, default_decision)
MockServer-->>Test: Registered dynamic bulk rule
Test->>App: Make authorized request<br/>(with user identity)
activate App
App->>MockServer: POST /GetDecisionBulk<br/>(decision request with user email)
activate MockServer
MockServer->>MockServer: Inspect email against<br/>permit_values
MockServer-->>App: Return DECISION_PERMIT<br/>or DECISION_DENY
deactivate MockServer
App-->>Test: Response (403 if denied,<br/>200 if permitted)
deactivate App
Test->>MockServer: DELETE /__mock/reset
MockServer-->>Test: State cleared
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Labels
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/externalpdp #39913 +/- ##
====================================================
- Coverage 70.34% 70.31% -0.03%
====================================================
Files 3247 3247
Lines 115559 115559
Branches 21056 21017 -39
====================================================
- Hits 81286 81252 -34
- Misses 32209 32239 +30
- Partials 2064 2068 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
✅ Layne — scan passed No security issues found on latest push. |
coderabbitai
bot
added
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/data/mock-server.helper.ts">
<violation number="1" location="apps/meteor/tests/data/mock-server.helper.ts:40">
P2: Check the reset response and fail when the mock-server reset request is unsuccessful; otherwise test runs can silently proceed with stale mocks.</violation>
</file>
<file name="apps/meteor/tests/end-to-end/api/abac.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/abac.ts:2666">
P2: The suite mutates several global ABAC/PDP settings but does not restore them in teardown, which can leak state into later tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }; | ||
|
|
||
| export const mockServerReset = async (): Promise<void> => { | ||
| await fetch(`${MOCK_SERVER_URL}/__mock/reset`, { method: 'DELETE' }); |
There was a problem hiding this comment.
P2: Check the reset response and fail when the mock-server reset request is unsuccessful; otherwise test runs can silently proceed with stale mocks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/data/mock-server.helper.ts, line 40:
<comment>Check the reset response and fail when the mock-server reset request is unsuccessful; otherwise test runs can silently proceed with stale mocks.</comment>
<file context>
@@ -0,0 +1,87 @@
+};
+
+export const mockServerReset = async (): Promise<void> => {
+ await fetch(`${MOCK_SERVER_URL}/__mock/reset`, { method: 'DELETE' });
+};
+
</file context>
| this.timeout(10000); | ||
|
|
||
| await mockServerReset(); | ||
| await updateSetting('ABAC_PDP_Type', 'local'); |
There was a problem hiding this comment.
P2: The suite mutates several global ABAC/PDP settings but does not restore them in teardown, which can leak state into later tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/abac.ts, line 2666:
<comment>The suite mutates several global ABAC/PDP settings but does not restore them in teardown, which can leak state into later tests.</comment>
<file context>
@@ -2614,3 +2623,424 @@ const addAbacAttributesToUserDirectly = async (userId: string, abacAttributes: I
+ this.timeout(10000);
+
+ await mockServerReset();
+ await updateSetting('ABAC_PDP_Type', 'local');
+ await updateSetting('ABAC_Enabled', false);
+ });
</file context>
1 participant
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
The tests here don't connect directly to Virtru or touch any virtru config. That's a black box for us.
The goal is to test our decision enforcement. If server returns DENY, then we act. If it's permit, we do as well.
Further comments
Summary by CodeRabbit