MaaS Admin List All API Keys#6908
Conversation
📝 WalkthroughWalkthroughThis PR makes subscription-related fields optional: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security considerationsCWE-602: Client-Side Enforcement of Server-Side Security Type-guard validation alignment (CWE-20: Improper Input Validation) 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/maas/frontend/src/app/api/subscriptions.ts (1)
13-32:⚠️ Potential issue | 🟠 MajorHarden nested object guards to prevent malformed payload acceptance (CWE-20, Major).
Line [20], Line [28], and Line [31] only check
typeof ... === 'object', which acceptsnulland arrays and does not enforce shape. A malformed BFF response can pass the guard and break downstream consumers when fields are dereferenced.Proposed fix
const isRecord = (v: unknown): v is Record<string, unknown> => !!v && typeof v === 'object'; +const isBillingRate = (v: unknown): v is { perToken: string } => + isRecord(v) && typeof v.perToken === 'string'; + +const isGroupReference = (v: unknown): v is { name: string } => + isRecord(v) && typeof v.name === 'string'; + +const isOwnerSpec = (v: unknown): v is { groups: { name: string }[] } => + isRecord(v) && Array.isArray(v.groups) && v.groups.every(isGroupReference); + +const isTokenMetadata = ( + v: unknown, +): v is { organizationId: string; costCenter: string; labels?: Record<string, string> } => + isRecord(v) && + typeof v.organizationId === 'string' && + typeof v.costCenter === 'string' && + (v.labels === undefined || + (isRecord(v.labels) && Object.values(v.labels).every((value) => typeof value === 'string'))); + const isMaaSSubscriptionRef = (v: unknown): v is ModelSubscriptionRef => isRecord(v) && typeof v.name === 'string' && typeof v.namespace === 'string' && (v.tokenRateLimits === undefined || (Array.isArray(v.tokenRateLimits) && v.tokenRateLimits.every(isTokenRateLimit))) && (v.tokenRateLimitRef === undefined || typeof v.tokenRateLimitRef === 'string') && - (v.billingRate === undefined || typeof v.billingRate === 'object'); + (v.billingRate === undefined || isBillingRate(v.billingRate)); const isMaaSSubscription = (v: unknown): v is MaaSSubscription => isRecord(v) && typeof v.name === 'string' && typeof v.namespace === 'string' && (v.phase === undefined || typeof v.phase === 'string') && (v.priority === undefined || typeof v.priority === 'number') && - typeof v.owner === 'object' && + isOwnerSpec(v.owner) && Array.isArray(v.modelRefs) && v.modelRefs.every(isMaaSSubscriptionRef) && - (v.tokenMetadata === undefined || typeof v.tokenMetadata === 'object') && + (v.tokenMetadata === undefined || isTokenMetadata(v.tokenMetadata)) && (v.creationTimestamp === undefined || typeof v.creationTimestamp === 'string');As per coding guidelines,
**/*.{ts,tsx,js,jsx}: WEB SECURITY (XSS, CSRF Prevention) — “Validate all API responses before rendering”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/maas/frontend/src/app/api/subscriptions.ts` around lines 13 - 32, The type guards isMaaSSubscriptionRef and isMaaSSubscription currently use typeof ... === 'object' which allows nulls and arrays; update these checks to use the existing isRecord predicate (or equivalent explicit null/array checks) so nested objects are validated as plain records: replace (v.billingRate === undefined || typeof v.billingRate === 'object') with (v.billingRate === undefined || isRecord(v.billingRate)), replace typeof v.owner === 'object' with isRecord(v.owner), and replace (v.tokenMetadata === undefined || typeof v.tokenMetadata === 'object') with (v.tokenMetadata === undefined || isRecord(v.tokenMetadata)); ensure you import/retain the isRecord helper and keep the rest of the guard logic intact (functions: isMaaSSubscriptionRef, isMaaSSubscription).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysToolbar.tsx`:
- Around line 114-143: When the username filter UI unmounts the hidden state can
persist; add a useEffect inside ApiKeysToolbar that watches isMaasAdmin and on
unmount or when isMaasAdmin becomes false calls setLocalUsername('') and
onUsernameChange('') to clear any lingering filter state (use a cleanup function
or effect branch). This ensures the username filter (localUsername,
setLocalUsername, onUsernameChange) is reset whenever the ToolbarFilter block is
removed so invisible filtering cannot persist.
---
Outside diff comments:
In `@packages/maas/frontend/src/app/api/subscriptions.ts`:
- Around line 13-32: The type guards isMaaSSubscriptionRef and
isMaaSSubscription currently use typeof ... === 'object' which allows nulls and
arrays; update these checks to use the existing isRecord predicate (or
equivalent explicit null/array checks) so nested objects are validated as plain
records: replace (v.billingRate === undefined || typeof v.billingRate ===
'object') with (v.billingRate === undefined || isRecord(v.billingRate)), replace
typeof v.owner === 'object' with isRecord(v.owner), and replace (v.tokenMetadata
=== undefined || typeof v.tokenMetadata === 'object') with (v.tokenMetadata ===
undefined || isRecord(v.tokenMetadata)); ensure you import/retain the isRecord
helper and keep the rest of the guard logic intact (functions:
isMaaSSubscriptionRef, isMaaSSubscription).
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: dbabbb1d-1129-48d7-8e44-19b2109157ba
📒 Files selected for processing (4)
packages/maas/frontend/src/app/api/subscriptions.tspackages/maas/frontend/src/app/pages/api-keys/AllApiKeysPage.tsxpackages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysToolbar.tsxpackages/maas/frontend/src/app/types/subscriptions.ts
| {isMaasAdmin && ( | ||
| <ToolbarFilter | ||
| labels={filterData.username ? [filterData.username] : []} | ||
| deleteLabel={() => { | ||
| setLocalUsername(''); | ||
| onUsernameChange(''); | ||
| }} | ||
| categoryName="Username" | ||
| > | ||
| <SearchInput | ||
| aria-label="Filter by username" | ||
| placeholder="Filter by username" | ||
| data-testid="username-filter-input" | ||
| value={localUsername} | ||
| onChange={(_event, value) => { | ||
| setLocalUsername(value); | ||
| }} | ||
| onSearch={(_event, value) => onUsernameChange(value)} | ||
| onClear={() => { | ||
| setLocalUsername(''); | ||
| onUsernameChange(''); | ||
| }} | ||
| /> | ||
| </Tooltip> | ||
| </ToolbarFilter> | ||
| <Tooltip | ||
| content="Please enter the full username" | ||
| data-testid="username-filter-tooltip" | ||
| > | ||
| <SearchInput | ||
| aria-label="Filter by username" | ||
| placeholder="Filter by username" | ||
| data-testid="username-filter-input" | ||
| value={localUsername} | ||
| onChange={(_event, value) => { | ||
| setLocalUsername(value); | ||
| }} | ||
| onSearch={(_event, value) => onUsernameChange(value)} | ||
| onClear={() => { | ||
| setLocalUsername(''); | ||
| onUsernameChange(''); | ||
| }} | ||
| /> | ||
| </Tooltip> | ||
| </ToolbarFilter> | ||
| )} |
There was a problem hiding this comment.
Hidden username filter state can persist after role downgrade.
When this block unmounts, a previously set username filter can still affect requests (see packages/maas/frontend/src/app/pages/api-keys/AllApiKeysPage.tsx, Line 41), leaving users with invisible filtering and no direct clear control.
Proposed fix
const ApiKeysToolbar: React.FC<ApiKeysToolbarProps> = ({
setIsModalOpen,
filterData,
localUsername,
setLocalUsername,
onUsernameChange,
onStatusToggle,
onStatusClear,
activeApiKeys,
refresh,
onClearFilters,
isMaasAdmin,
}) => {
const [isStatusSelectOpen, setIsStatusSelectOpen] = React.useState(false);
+
+ React.useEffect(() => {
+ if (!isMaasAdmin && localUsername) {
+ setLocalUsername('');
+ onUsernameChange('');
+ }
+ }, [isMaasAdmin, localUsername, onUsernameChange, setLocalUsername]);
return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/maas/frontend/src/app/pages/api-keys/allKeys/ApiKeysToolbar.tsx`
around lines 114 - 143, When the username filter UI unmounts the hidden state
can persist; add a useEffect inside ApiKeysToolbar that watches isMaasAdmin and
on unmount or when isMaasAdmin becomes false calls setLocalUsername('') and
onUsernameChange('') to clear any lingering filter state (use a cleanup function
or effect branch). This ensures the username filter (localUsername,
setLocalUsername, onUsernameChange) is reset whenever the ToolbarFilter block is
removed so invisible filtering cannot persist.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cypress/cypress/tests/mocked/modelsAsAService/maasApiKeys.cy.ts`:
- Around line 96-101: The test "should not display the username filter for
non-MaaS admins" currently inherits the global beforeEach stub that returns
{allowed: true}; override that by intercepting GET /maas/api/v1/is-maas-admin to
return {allowed: false} (use cy.intercept and give it an alias), call
asProjectAdminUser(), then call apiKeysPage.visit() and cy.wait on the alias to
ensure the frontend has received the response before asserting
apiKeysPage.findFilterInput().should('not.exist') and
apiKeysPage.findUsernameFilterTooltip().should('not.exist').
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c0020bde-19e9-4e2b-84cb-916eec8991f9
📒 Files selected for processing (2)
packages/cypress/cypress/support/commands/odh.tspackages/cypress/cypress/tests/mocked/modelsAsAService/maasApiKeys.cy.ts
packages/cypress/cypress/tests/mocked/modelsAsAService/maasApiKeys.cy.ts
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6908 +/- ##
==========================================
+ Coverage 64.09% 64.46% +0.36%
==========================================
Files 2530 2497 -33
Lines 76695 76945 +250
Branches 19202 19106 -96
==========================================
+ Hits 49161 49603 +442
+ Misses 27534 27342 -192
... and 102 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
/retest |
1 participant
Closes RHOAIENG-51807
Description
The MaaS API already handled this 🤷♀️ the ticket was just to double check
On the dashboard-maas cluster I had the cluster admin make some keys and the user make some keys
cluster admin view:
user view:
and hiding the username search input for non-maas-admins:
(this uses impersonate so that's why all the keys are still showing just ignore that, that call isn't hooked up to the is-maas-admin check)
Screen.Recording.2026-03-26.at.9.47.54.AM.mov
Small fix for list subscriptions, had some required fields that aren't actually required so I was getting some invalid response format issues
@Griffin-Sullivan mentioned he did the same in his PR, doesn't matter which goes first
How Has This Been Tested?
Tested locally
Test Impact
no tests changed
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
New Features
Improvements
Tests