feat(OIDC): API Access tab with trust relationships UI - #8044
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe change adds trust relationship types, RTK Query endpoints, cache handling, and React hooks. Organisation settings now expose a feature-flagged trust relationship section and updated API access navigation. The UI supports listing, creating, editing, deleting, role assignment, administrator permissions, validation errors, and provider selection. GitHub Actions support includes repository and environment selection, audience derivation, claim rules, and workflow setup guidance. Master API key typings are also corrected. Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to This PR adds trust-relationship management and changes API-key settings behavior, but the current version can show stale API-key data after organisation changes or failed refreshes, while trust-relationship editing has bounded issues around preserving existing claims, displaying configured repositories, rejecting blank claims, and keyboard access. These issues should be fixed or explicitly accepted before merging. 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 |
github-actions
Bot
added
the
front-end
khvn26
changed the title
Docker builds report
|
f52d48d to
897e6b5
Compare
github-actions
Bot
added
feature
897e6b5 to
ac0eb55
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
github-actions
Bot
added
feature
Visual Regression19 screenshots compared. See report for details. |
ac0eb55 to
f3d09e9
Compare
github-actions
Bot
removed
feature
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
|
❌ private-cloud · depot-ubuntu-latest-arm-16 — run #19462 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Failed testsfirefox › tests/project-permission-test.pw.ts › Project Permission Tests › Project-level permissions control access to features, environments, audit logs, and segments @enterprise 🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #19462 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19462 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19462 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
❌ private-cloud · depot-ubuntu-latest-16 — run #19374 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/environment-permission-test.pw.ts › Environment Permission Tests › Environment-level permissions control access to features, identities, and segments @enterprise ✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19374 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19374 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19374 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19368 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19368 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19368 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19368 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
|
@themis-blindfold review |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis judgement: 🟠 Fix before mergeSolid new feature: a trust-relationships management UI with a well-scoped RTK Query service, typed requests/responses, a GitHub-specific preset form, a generic OIDC form, and tab plumbing. The GitHub form's role error handling was fixed in earlier review rounds, but the same fix was never applied to its sibling
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Automate: add a component test for Product take: This unlocks OIDC token exchange for CI pipelines — a meaningful capability for teams adopting keyless authentication. The GitHub-specific preset with auto-audience and setup snippet lowers the bar considerably compared to manual OIDC configuration. Big picture: 🧭 Assumptions & unverified claims
A trust relationship is only as strong as its weakest modal. · reviewed at 5a3a2d8 |
talissoncosta
left a comment
talissoncosta
left a comment
There was a problem hiding this comment.
Looks good @khvn26. Left some questions and suggestions.
Non-blocking: the two form components are on the large side and mix data-fetching, form state, and submit logic. Two extractions would help most:
- pull the audience derivation into a pure function so it's unit-testable.
- move the create/update + toast block into a submit hook shared by both forms — they're near-identical today
Size shrinks as a side effect. Happy to leave both as fast-follows.
| <Tooltip title={<span aria-label={provider.label}>{provider.icon}</span>}> | ||
| {`<span class="font-monospace">${issuer}</span>`} | ||
| </Tooltip> |
There was a problem hiding this comment.
Question: is hiding the URL behind the icon intentional (keep the column compact), or should the issuer always be visible like it is for unknown providers?
Suggestion either way: show icon + issuer side by side and drop the hand-built HTML string.
<Row className='gap-1 align-items-center'>
{provider.icon}
<span className='font-monospace'>{issuer}</span>
</Row>
There was a problem hiding this comment.
My initial idea was to omit issuer URL for issuers we know and trust to reassure users that the trust relationship will work correctly with their provider of choice.
Decided to simplify in 24d6587. Icon and issuer now sit side by side and the HTML string is gone.
| setClaimRules((rules) => rules.filter((_, i) => i !== index)) | ||
| } | ||
| > | ||
| <IonIcon icon={closeIcon} /> |
There was a problem hiding this comment.
Do you mind checking if we can use Icon component instead of IconIcon ?
| const alertError = useMemo(() => { | ||
| const error = (createError || updateError) as { data?: any } | undefined | ||
| const data = error?.data | ||
| if (data && typeof data === 'object' && !Array.isArray(data)) { |
There was a problem hiding this comment.
On a non-JSON error body (404 HTML page here) the banner renders the raw HTML — see screenshot. The typeof data === 'object' guard passes a string body straight to ErrorMessage. Route create/update through trustRelationshipErrorMessage so it falls back to a readable message?
There was a problem hiding this comment.
6988d5c — both forms now go through errors.ts.
| <label>Audience</label> | ||
| <Input className='full-width' value={audience} readOnly /> |
There was a problem hiding this comment.
Any reason for not using InputGroup in here ?
| updateTrustRelationship, | ||
| { error: updateError, isLoading: isUpdating }, | ||
| ] = useUpdateTrustRelationshipMutation() | ||
| const fieldErrors = ((createError || updateError) as { data?: any })?.data |
There was a problem hiding this comment.
const fieldErrors = ((createError || updateError) as { data?: any })?.data
const error = (createError || updateError) as { data?: any } | undefinedTwo any casts (lint disallows any), and this rolls its own error extraction while errors.ts already has trustRelationshipErrorMessage. Share one typed helper across both forms?
There was a problem hiding this comment.
6988d5c — errors.ts now exports trustRelationshipFieldErrors and trustRelationshipAlertError. No casts left.
| onChange={(e: InputEvent) => setAudience(Utils.safeParseEventValue(e))} | ||
| placeholder='e.g. https://github.com/YourOrg' | ||
| /> | ||
| <label>Claim matching rules</label> |
There was a problem hiding this comment.
Can you check if you can use FieldLabel? is the standalone label for cases like this.
| setRoles((selected) => [...selected, { id: role.id, name: role.name }]) | ||
| return | ||
| } | ||
| createRoleMasterApiKey(getStore(), { |
There was a problem hiding this comment.
Roles don't go through RTK Query like the rest of the feature — they call the services directly and keep the list in useState, which can drift from the server (no cache, no refetch). The hooks already exist (useCreateRoleMasterApiKeyMutation, useGetRolesMasterAPIKeyWithMasterAPIKeyRolesQuery). Any reason not to use them?
There was a problem hiding this comment.
No good reason — it was copied from AdminAPIKeys. Fixed in bf5392b; also corrected the query's response type.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d1026e3a-e9bf-487d-9541-7b6318be6b6e
📒 Files selected for processing (25)
frontend/common/services/useMasterAPIKeyWithMasterAPIKeyRole.tsfrontend/common/services/useTrustRelationship.tsfrontend/common/types/requests.tsfrontend/common/types/responses.tsfrontend/web/components/AdminAPIKeys.jsfrontend/web/components/pages/organisation-settings/OrganisationSettingsPage.tsxfrontend/web/components/pages/organisation-settings/tabs/APIKeysTab.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/GithubTrustRelationshipForm/GithubTrustRelationshipForm.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/GithubTrustRelationshipForm/index.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/NewTrustRelationshipModal/NewTrustRelationshipModal.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/NewTrustRelationshipModal/index.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationshipModal/TrustRelationshipModal.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationshipModal/index.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationshipPermissionsFields/TrustRelationshipPermissionsFields.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationshipPermissionsFields/index.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationships/TrustRelationships.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationships/index.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/WorkflowSetupSnippet/WorkflowSetupSnippet.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/WorkflowSetupSnippet/index.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/__tests__/errors.test.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/errors.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/hooks/__tests__/useTrustRelationshipRoles.test.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/hooks/useTrustRelationshipRoles.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/index.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/providers.tsx
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 (2)
frontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationshipModal/TrustRelationshipModal.tsx (1)
80-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDiscard whitespace-only claim names before serialisation.
Line 80 accepts a claim containing only spaces. Line 82 then sends it as
claim: ''. The API receives an invalid rule instead of ignoring the empty row.Proposed fix
- .filter((rule) => rule.claim) + .filter((rule) => rule.claim.trim())frontend/web/components/pages/organisation-settings/tabs/trust-relationships/GithubTrustRelationshipForm/GithubTrustRelationshipForm.tsx (1)
245-252: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow the existing repository-name relationship in the selector.
If an edited relationship uses
repository,initialRepoFullNamehas a value butselectedReporemainsnull. The selector then renders its placeholder although the relationship is configured. DisplayinitialRepoFullNameuntil the user selects a repository. KeepselectedRepounset so an unchanged save preserves the existingrepositoryclaim.Proposed fix
value={ selectedRepo ? { label: selectedRepo.full_name, value: selectedRepo.full_name, } + : initialRepoFullName + ? { + label: initialRepoFullName, + value: initialRepoFullName, + } : null }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 67ecfa06-477f-4954-9579-9e7df28c30be
📒 Files selected for processing (12)
frontend/common/services/useTrustRelationship.tsfrontend/web/components/AdminAPIKeys.jsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/GithubTrustRelationshipForm/GithubTrustRelationshipForm.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationshipModal/TrustRelationshipModal.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationshipPermissionsFields/TrustRelationshipPermissionsFields.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/TrustRelationships/TrustRelationships.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/WorkflowSetupSnippet/WorkflowSetupSnippet.tsxfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/WorkflowSetupSnippet/index.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/__tests__/isGithubFormEditable.test.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/github.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/hooks/__tests__/useTrustRelationshipRoles.test.tsfrontend/web/components/pages/organisation-settings/tabs/trust-relationships/providers.tsx
| .catch(() => { | ||
| this.setState({ isLoading: false }) | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Bind API-key state to the active request.
This handler clears only isLoading. It retains the previous apiKeys value. After an organisation change or refresh fails, the component can render the previous organisation's API-key data. An older rejection can also clear isLoading while a newer request is still pending.
Track a request sequence or cancel the previous request. Capture organisationId, ignore stale then and catch callbacks, and clear apiKeys for the active failed request.
Proposed fix
+ fetchRequestId = 0
+
fetch = () => {
- this.setState({ isLoading: true })
+ const requestId = ++this.fetchRequestId
+ const organisationId = this.props.organisationId
+ this.setState({ apiKeys: undefined, isLoading: true })
data
.get(
- `${Project.api}organisations/${this.props.organisationId}/master-api-keys/`,
+ `${Project.api}organisations/${organisationId}/master-api-keys/`,
)
.then((res) => {
+ if (requestId !== this.fetchRequestId) return
this.setState({
apiKeys: res,
isLoading: false,
})
})
.catch(() => {
- this.setState({ isLoading: false })
+ if (requestId !== this.fetchRequestId) return
+ this.setState({ apiKeys: undefined, isLoading: false })
})
}📝 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.
| .catch(() => { | |
| this.setState({ isLoading: false }) | |
| }) | |
| fetchRequestId = 0 | |
| fetch = () => { | |
| const requestId = ++this.fetchRequestId | |
| const organisationId = this.props.organisationId | |
| this.setState({ apiKeys: undefined, isLoading: true }) | |
| data | |
| .get( | |
| `${Project.api}organisations/${organisationId}/master-api-keys/`, | |
| ) | |
| .then((res) => { | |
| if (requestId !== this.fetchRequestId) return | |
| this.setState({ | |
| apiKeys: res, | |
| isLoading: false, | |
| }) | |
| }) | |
| .catch(() => { | |
| if (requestId !== this.fetchRequestId) return | |
| this.setState({ apiKeys: undefined, isLoading: false }) | |
| }) | |
| } |
…s UI The organisation settings API Keys tab becomes API Access and gains a Trust Relationships section behind the trust_relationships feature flag: list, create, edit and delete, with a claim-rules editor and the same admin toggle and RBAC role assignment flow as master API keys, attached via the backing key. Also fixes two latent request types in the master API key role services. beep boop
The GitHub/Other provider cards were div+onClick only, so keyboard and screen-reader users could not start the create flow. Add role="button", tabIndex and an Enter/Space onKeyDown handler. beep boop
addRole/removeRole mutated local role state unconditionally and never inspected the request result, so a failed assignment left the UI showing a role the backend never stored, with no error surfaced. Update local state only on success and toast on failure; also flag when role assignment fails during creation instead of silently swallowing it. beep boop
Adopt the component-folder layout the frontend guideline expects: every new component now lives in its own directory with an index.ts barrel (ComponentName/ComponentName.tsx + index.ts). providers.ts and errors.ts stay as flat modules, being exempt constants/util. Cross-component imports use the components/ alias, as the import-alias lint rule requires for parent-traversal paths; APIKeysTab keeps importing the tab via the unchanged ./trust-relationships path. beep boop
The role add/remove fix from review only reached the GitHub preset form, leaving the freeform modal with optimistic state updates and success toasts on failed API calls. Role behaviour now lives in a single useTrustRelationshipRoles hook used by both forms — including partial failure reporting when assigning roles after creation — with its core extracted for unit tests, so the two forms cannot drift again. beep boop
The list showed the issuer only inside a tooltip, built from a hand-written HTML string. Render the provider badge and the issuer side by side instead. beep boop
…dies A non-JSON response — an HTML error page from a proxy, say — was passed straight to the alert, which rendered its markup. Both forms now share typed helpers that only surface messages they can read, and fall back otherwise. beep boop
Roles called the services directly and mirrored the result in component state, which could drift from the server. Both mutations invalidate the tag the roles query provides, so the cache is now the single source of truth in edit mode; only the not-yet-assigned selection is held locally. Corrects the response type of the roles query along the way — it is a paged list of roles, not a master API key. beep boop
…ip to admin beep boop
…annot render The GitHub form writes back one repository rule and one environment rule, so a relationship carrying both repository selectors, or a repeated claim, lost the extra rules on save — widening the trust. Such relationships now open in the freeform editor. Moves the predicates to `github.ts` so they can be tested without the component. beep boop
4 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to #8040
In this PR, we add a Trust relationships management UI.
The organisation settings "API Keys" tab is renamed to "API Access" and gains a Trust Relationships section, gated by the
trust_relationshipsfeature flag.Tabs now pass their stable key as the slug, and the label-derived legacy slug
?tab=api-keysredirects, like?tab=samlalready did.Creation is preset-based: a provider chooser offers "GitHub Actions" or a freeform "Other OIDC provider", kept for other providers and self-hosted issuers. The GitHub form asks for a repository, picked from the org's GitHub integration if configured, or typed manually otherwise, plus an optional environment. The audience is auto-derived:
https://github.com/<owner>(GitHub's zero-config default) for the first trust relationship per owner, a repo-scoped value once the default is taken.A YAML snippet for
Flagsmith/setup-cliaction is presented in create/edit modals for GitHub trust relationships.The "Is Admin" toggle is now accompanied by "available in enterprise" banner both for Master API Keys and Trust relationships.
Built in the modern RTK style (new
useTrustRelationshipservice, typed requests/responses) rather than extending the legacyAdminAPIKeys.js. Also fixes two latent types surfaced by typed usage:createRoleMasterApiKeywas missing itsbody, and thedeleteMasterAPIKeyWithMasterAPIKeyRoleshelper was annotated with the wrong request type.How did you test this code?
Locally, by hand; no new E2E tests yet.
Screenshots
Trust relationships list:
New trust relationship provider chooser:
GitHub modal:
GitHub setup snippet:
Generic provider modal:
