chore: Detailed connection checks for Virtru PDP#40013
chore: Detailed connection checks for Virtru PDP#40013KevLehman merged 8 commits intofeat/externalpdpfrom
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 (11)
WalkthroughThis pull request introduces a health check endpoint for the ABAC Virtru Policy Decision Point (PDP). It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Admin UI)
participant Server as API Server
participant AbacService as ABAC Service
participant PDP as PDP Instance<br/>(Local or Virtru)
Client->>Server: GET /abac/pdp/health
Server->>AbacService: getPDPHealth()
alt PDP configured
AbacService->>PDP: getHealthStatus()
alt Local PDP
PDP->>PDP: Return immediately<br/>(always available)
else Virtru PDP
PDP->>PDP: Check platform /healthz
PDP->>PDP: Verify IdP token generation
PDP->>PDP: Test authorization via GetDecisions
end
PDP-->>AbacService: Success or throw error
AbacService-->>Server: Resolved or rejected
else No PDP configured
AbacService-->>Server: Throw ABAC_PDP_Health_No_PDP
end
alt Success
Server-->>Client: { success: true, available: true,<br/>message: 'ABAC_PDP_Health_OK' }
else Failure
Server-->>Client: { success: false, available: false,<br/>message: error message }
end
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 |
|
There was a problem hiding this comment.
Pull request overview
This PR improves ABAC Virtru PDP setup diagnostics by replacing a simple “available/unavailable” check with a more detailed health check flow (token acquisition + Virtru platform reachability + authenticated request), and exposes that via the REST API with i18n-friendly status keys.
Changes:
- Replace
isPdpAvailable(): Promise<boolean>withgetPdpHealth(): Promise<void>across the ABAC service and PDP interfaces. - Add Virtru PDP
getHealthStatus()that checks/healthzand performs an authenticated request using an OIDC client token. - Update the
/abac/pdp/healthendpoint to return a success payload with a translated message key and add rate limiting.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/i18n/src/locales/en.i18n.json | Adds new ABAC PDP health i18n keys/messages. |
| packages/core-services/src/types/IAbacService.ts | Replaces boolean availability API with getPdpHealth() void API. |
| ee/packages/abac/src/pdp/VirtruPDP.ts | Implements detailed Virtru PDP health checks (token, /healthz, authenticated request). |
| ee/packages/abac/src/pdp/types.ts | Extends IPolicyDecisionPoint with getHealthStatus(). |
| ee/packages/abac/src/pdp/LocalPDP.ts | Adds a no-op getHealthStatus() for the local PDP. |
| ee/packages/abac/src/index.ts | Implements AbacService.getPdpHealth() and updates service behavior accordingly. |
| apps/meteor/ee/server/api/abac/schemas.ts | Updates REST response schemas for the PDP health endpoint. |
| apps/meteor/ee/server/api/abac/index.ts | Updates /abac/pdp/health handler (messages + rate limiting + new error schema). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/externalpdp #40013 +/- ##
====================================================
- Coverage 70.37% 70.37% -0.01%
====================================================
Files 3270 3270
Lines 117050 117076 +26
Branches 21151 21165 +14
====================================================
+ Hits 82376 82392 +16
- Misses 32604 32614 +10
Partials 2070 2070
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KevLehman
force-pushed
the
feat/externalpdp
branch
from
65f09e0 to
7d82278
Compare
KevLehman
force-pushed
the
fix/healthcheck
branch
from
ef91220 to
dfe3264
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 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/ee/server/api/abac/index.ts">
<violation number="1" location="apps/meteor/ee/server/api/abac/index.ts:383">
P2: Guard the catch value before reading `.message`; the `as Error` cast hides non-Error cases and can return an undefined/incorrect message.
(Based on your team's feedback about avoiding unsafe type casts.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| await Abac.getPDPHealth(); | ||
| return API.v1.success({ available: true, message: 'ABAC_PDP_Health_OK' }); | ||
| } catch (err) { | ||
| return API.v1.failure({ available: false, message: (err as Error).message }); |
There was a problem hiding this comment.
P2: Guard the catch value before reading .message; the as Error cast hides non-Error cases and can return an undefined/incorrect message.
(Based on your team's feedback about avoiding unsafe type casts.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/ee/server/api/abac/index.ts, line 383:
<comment>Guard the catch value before reading `.message`; the `as Error` cast hides non-Error cases and can return an undefined/incorrect message.
(Based on your team's feedback about avoiding unsafe type casts.) </comment>
<file context>
@@ -363,15 +364,24 @@ const abacEndpoints = API.v1
+ await Abac.getPDPHealth();
+ return API.v1.success({ available: true, message: 'ABAC_PDP_Health_OK' });
+ } catch (err) {
+ return API.v1.failure({ available: false, message: (err as Error).message });
+ }
},
</file context>
| return API.v1.failure({ available: false, message: (err as Error).message }); | |
| const message = err instanceof Error ? err.message : 'ABAC_PDP_Health_Error'; | |
| return API.v1.failure({ available: false, message }); |
coderabbitai
bot
added
type: feature
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-2037
Steps to test or reproduce
Further comments
AIM: users should know exactly why the connection fails when setting up virtru. For this, we're gonna test the flow we actually use: Get token, then contact virtru with the token.
A dedicated healthz test for virtru is added so we know if the platform is just not available.
Summary by CodeRabbit
Release Notes