EDM-2373: Support multiple authentication providers#378
Conversation
| // For 500/501 errors, return the status code for detection | ||
| if (response.status === 500 || response.status === 501) { | ||
| // The UI proxy should convert alert errors 401 to 501, but we guard against 401 here as well. | ||
| const isAlertsDisabled = response.status === 500 || response.status === 501 || response.status === 401; |
There was a problem hiding this comment.
Now rather than 401 we should be getting proper statusCode 403 when the user does not have permissions, but since it's a bit unstable I'd rather keep converting 401 to 501 and disabling alerts with 401.
| } | ||
|
|
||
| func getAAPAuthHandler(authURL string, internalAuthURL *string) (*AAPAuthHandler, error) { | ||
| func getAAPAuthHandler(provider *v1alpha1.AuthProvider, aapSpec *v1alpha1.AapProviderSpec) (*AAPAuthHandler, error) { |
There was a problem hiding this comment.
aap is still WIP, the clientId should be retrieved from the config, etc.
|
|
||
| const secretAppClientSecret = 'CLIENT-SECRET-1'; | ||
|
|
||
| const githubValues = { |
There was a problem hiding this comment.
I need to remove this, but it's just a quick way to fill in the details and start testing.
WalkthroughAdds a provider-driven authentication feature across frontend and backend: admin UI for managing auth providers (CRUD, test, details), provider-aware auth handlers with PKCE and token-data model changes, a test-connection endpoint, many new TS types/UI form helpers, YAML/editor and RBAC updates, and removal of two proxy config vars and their docs entries. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FE as Frontend (Login)
participant Proxy as Backend (proxy/auth)
participant IDP as Identity Provider
participant Test as TestAuthHandler
User->>FE: GET /login
FE->>Proxy: GET /api/auth/config
Proxy-->>FE: list of providers
alt Token (k8s) provider (direct token)
User->>FE: submit token
FE->>Proxy: POST /login?provider=k8s (token)
Proxy-->>FE: session/token response
FE->>User: redirect to /
else OAuth/OIDC/OpenShift provider (PKCE)
FE->>FE: generate PKCE (code_challenge, verifier)
FE->>Proxy: request login redirect (state + code_challenge)
Proxy->>IDP: redirect user to provider auth endpoint
IDP-->>Proxy: callback with code & state
Proxy->>Proxy: exchange code via backend/API server (PKCE)
Proxy-->>FE: set session cookie / token data
FE->>User: redirect to /
end
Note over User,Test: Admin tests provider connection
User->>Test: POST /test-auth-provider-connection (config)
Test->>Test: discovery & endpoint reachability checks (SSRF-safe)
Test-->>User: field-level validation results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus during review:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/auth/auth.go (1)
289-315: Token provider POST login ignoresio.ReadAllerrorsIn the K8s token login branch:
body, err := io.ReadAll(r.Body) err = json.Unmarshal(body, &loginParams) if err != nil { ... }If
io.ReadAllfails, its error is discarded and only the unmarshal error is seen, which can hide I/O issues and potentially mis‑classify bad requests vs server problems. Consider the same pattern you already use in the non‑K8s branch:- body, err := io.ReadAll(r.Body) - err = json.Unmarshal(body, &loginParams) - if err != nil { + body, err := io.ReadAll(r.Body) + if err != nil { + log.GetLogger().WithError(err).Warn("Failed to read request body") + w.WriteHeader(http.StatusBadRequest) + return + } + if err := json.Unmarshal(body, &loginParams); err != nil { log.GetLogger().WithError(err).Warn("Failed to unmarshal request body") w.WriteHeader(http.StatusBadRequest) return }This keeps diagnostics consistent across branches.
♻️ Duplicate comments (4)
apps/ocp-plugin/src/utils/apiCalls.ts (1)
56-71: 401-as-disabled behavior is consistent with existing alerts handlingTreating
401like500/501for alerts (returningError('${response.status}')) aligns with the comment and existing pattern for signaling “alerts disabled” to callers. The special-case 404 handling is preserved, and all other statuses still go throughgetErrorMsgFromAlertsApiResponse, so behavior remains predictable.libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx (1)
32-56: Remove or behind-flag the hard-coded “test provider” presets before shippingThe blue‑bordered “Test provider” section plus
githubValues/googleValuesconstants are clearly a temporary testing harness, including fake client IDs/secrets and canned org/role settings.Leaving this in production:
- Clutters the UX for real users.
- Risks accidental use of placeholder config.
- Conflicts with your own earlier note about removing it.
I’d either:
- Strip this block and related constants entirely, or
- Hide it behind a dev‑only flag (e.g., build‑time env) if you still need it locally.
Also applies to: 135-203
proxy/auth/aap.go (1)
53-105: AAP handler follows the new pattern but hard‑codesclientId, which should eventually come from config/specThe AAP changes align with the other providers:
getAAPAuthHandlernow derives URLs fromAapProviderSpec(internal vs external) and builds both external and internal clients.getClientenforces a non‑emptyclientIdand wires PKCE‑friendlyosincli.ClientConfig.GetToken,RefreshToken,GetLoginRedirectURL, andLogoutall follow the shared patterns.However,
getAAPAuthHandlercurrently hard‑codes:clientId := "flightctl-ui"while comments and past reviews indicate this should eventually be sourced from configuration / CRD. Combined with
getClientIdFromProviderConfiginauth.gostill returning an error forProviderTypeAAP, this leaves AAP in an incomplete, WIP state for the main login/refresh paths.Non‑blocking for now if AAP isn’t exposed to users yet, but before enabling it in production you’ll want to:
- Add a
clientIdfield to the AAP provider spec or a dedicated config source.- Plumb that value into both
getAAPAuthHandlerandgetClientIdFromProviderConfig.- Remove or adjust the hard‑coded
"flightctl-ui"default.Tagging as a duplicate of the earlier clientId comment so it stays on your radar.
Also applies to: 107-181
proxy/auth/auth.go (1)
53-151: Provider lookup / instantiation logic is solid, but AAP cannot yet participate in BFF token flowsThe
findProviderConfig+getProviderInstancepipeline is a good abstraction and the OpenShift vs vanilla K8s split viaExternalOpenShiftApiUrlis reasonable. One important gap:
getClientIdFromProviderConfigcurrently returns an error forProviderTypeAAPand for non‑OpenShift K8s. BecauseLogin(POST) andRefreshrely ongetClientIdFromProviderConfigfor all non‑K8s‑token providers, AAP providers effectively cannot use the new backend BFF token exchange at all; they will consistently hit the"AAP providers client_id needs to be retrieved"path and surface a 500.If AAP isn’t meant to be enabled yet, that’s acceptable as an intentional limitation, but consider either:
- Explicitly rejecting AAP providers at configuration time with a clearer message, or
- Wiring
ClientIdfrom the AAP provider spec / config intogetClientIdFromProviderConfigas soon as the backend surface is ready.Otherwise the behavior is confusing: AAP looks like a first‑class provider type but is unusable in the main login/refresh paths.
🧹 Nitpick comments (43)
proxy/bridge/handler.go (1)
60-89: Refactor to eliminate code duplication and verify the 501 status code choice.This function duplicates most of
createReverseProxy. Consider extracting the common proxy setup logic and passing the 401 handling behavior as a parameter or callback.Additionally, converting 401 to 501 "Not Implemented" may mislead clients and monitoring tools. The comment mentions "disabled", but 501 typically indicates unsupported HTTP methods, not disabled services.
Suggested refactor to reduce duplication:
+type responseModifier func(*http.Response) error + +func createReverseProxyWithModifier(apiURL string, modifier responseModifier) (*url.URL, *httputil.ReverseProxy) { + target, err := url.Parse(apiURL) + if err != nil { + log.GetLogger().WithError(err).Errorf("Failed to parse URL '%s'", apiURL) + os.Exit(1) + } + proxy := httputil.NewSingleHostReverseProxy(target) + proxy.ModifyResponse = func(r *http.Response) error { + filterHeaders := []string{ + "Access-Control-Allow-Headers", + "Access-Control-Allow-Methods", + "Access-Control-Allow-Origin", + "Access-Control-Expose-Headers", + } + for _, h := range filterHeaders { + r.Header.Del(h) + } + + if modifier != nil { + return modifier(r) + } + return nil + } + return target, proxy +} + func createReverseProxy(apiURL string) (*url.URL, *httputil.ReverseProxy) { - target, err := url.Parse(apiURL) - if err != nil { - log.GetLogger().WithError(err).Errorf("Failed to parse URL '%s'", apiURL) - os.Exit(1) - } - proxy := httputil.NewSingleHostReverseProxy(target) - proxy.ModifyResponse = func(r *http.Response) error { - filterHeaders := []string{ - "Access-Control-Allow-Headers", - "Access-Control-Allow-Methods", - "Access-Control-Allow-Origin", - "Access-Control-Expose-Headers", - } - for _, h := range filterHeaders { - r.Header.Del(h) - } - + return createReverseProxyWithModifier(apiURL, func(r *http.Response) error { // If the backend returns 401 Unauthorized, clear the session cookie // This handles the case where the token in the cookie has expired if r.StatusCode == http.StatusUnauthorized { r.Header.Set("Clear-Site-Data", `"cookies"`) log.GetLogger().Debug("Backend returned 401, clearing session cookies") } - return nil - } - return target, proxy + }) } func createAlertsReverseProxy(apiURL string) (*url.URL, *httputil.ReverseProxy) { - target, err := url.Parse(apiURL) - if err != nil { - log.GetLogger().WithError(err).Errorf("Failed to parse URL '%s'", apiURL) - os.Exit(1) - } - proxy := httputil.NewSingleHostReverseProxy(target) - proxy.ModifyResponse = func(r *http.Response) error { - filterHeaders := []string{ - "Access-Control-Allow-Headers", - "Access-Control-Allow-Methods", - "Access-Control-Allow-Origin", - "Access-Control-Expose-Headers", - } - for _, h := range filterHeaders { - r.Header.Del(h) - } - + return createReverseProxyWithModifier(apiURL, func(r *http.Response) error { // For alerts API, we may sometimes receive 401 instead of 403. // To prevent login out the user, we convert 401 to 501. if r.StatusCode == http.StatusUnauthorized { r.StatusCode = http.StatusNotImplemented r.Status = "501 Not Implemented" log.GetLogger().Debug("Alerts API returned 401, converting to 501 (disabled)") } - return nil - } - return target, proxy + }) }Regarding the 501 status code:
Please verify that 501 "Not Implemented" is the appropriate choice. Consider these alternatives:
- 503 Service Unavailable if alerts are temporarily disabled
- 403 Forbidden if the user lacks permissions for alerts
- 404 Not Found if the alerts feature is completely disabled/unavailable
You may also want to check whether clients and monitoring systems correctly interpret this status code as "alerts disabled" rather than "server doesn't support this method."
libs/ui-components/src/components/form/NameField.tsx (1)
75-83: Disabled helper text wiring looks correct and non-breaking.Plumbing
helperTextinto the disabled path viaDefaultHelperTextis consistent with the existing Formik setup, avoids leakinghelperTextintoTextInputvia...rest, and keeps the publicNameFieldPropssurface unchanged.libs/ui-components/src/utils/patch.ts (1)
23-54: Boolean-safe JSON patch behavior is an improvementGuarding the remove case with
typeof newValue !== 'boolean'ensures boolean fields are never dropped when toggled tofalse, but are instead patched viaadd/replace, which is more intuitive for flags. One small follow-up you might consider is documenting (or asserting) thatencodeB64is only used for string values, since we now more often pass non-stringnewValuethrough this helper as well.libs/types/models/AuthDynamicRoleAssignment.ts (1)
8-21: Separator field is useful, but consider regenerating from OpenAPIAdding
separator?: string(with the detailed JSDoc) matches the dynamic role semantics used elsewhere and should make UI code safer and clearer. Since this file is tagged as generated byopenapi-typescript-codegen, it would be better long term to update the OpenAPI schema and regenerate types so this property doesn’t get lost on the next codegen run.libs/ui-components/src/components/Login/TokenLoginForm.tsx (1)
33-99: Submit flow and error handling look solid; consider reusing shared utilitiesThe overall flow (clearing local storage, posting a trimmed token, parsing JSON/non‑JSON errors, setting an expiry, then redirecting) is clean and defensive. Two small follow-ups you might consider:
- Reuse a shared
nowInSecondshelper (and, if one exists elsewhere, a sharedEXPIRATIONstorage key) instead of re‑defining them locally to avoid drift with other login flows.- If other callers rely on the same
login?provider=endpoint, double‑check thatproxyFetchsets the appropriate JSONContent-Typeheader so the backend can reliably parse{ token }.Neither is blocking, but they would help keep auth behavior consistent across providers.
libs/ui-components/src/components/form/ListItemField.tsx (1)
9-120: Harden ListItemField against undefined initial values and simplify async usageNice reusable component; the LabelGroup + EditableLabelControl pattern fits well. Two small robustness refactors to consider:
useField<string[]>(name)will givevalueasundefinedif the form’sinitialValuesforget to initialize this field to[];items.map(...)would then throw. You could defensively normalize with something likeconst items = Array.isArray(field.value) ? field.value : [];to make the component safer in future forms.updateItems,onDelete,onAdd, andonEditare markedasyncandawait setItems(...), but Formik’ssetValueis synchronous and returns void. Droppingasync/awaithere would simplify the code without changing behavior.These are non-blocking refinements that make the component more resilient and clearer.
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (1)
471-483: Consider avoidingthenproperties in Yup.whenconfig to satisfynoThenPropertyBiome flags the
thenkeys in the.when('roleAssignmentType', { is, then })configs as suspicious. You can express the same logic using the function form ofwhen, which avoids athenproperty entirely and keeps the intent clear.- roleClaimPath: Yup.array() - .of(Yup.string()) - .when('roleAssignmentType', { - is: RoleAssignmentType.Dynamic, - then: (schema) => schema.min(1, t('At least one claim path segment is required for dynamic role assignment')), - }), + roleClaimPath: Yup.array() + .of(Yup.string()) + .when('roleAssignmentType', (roleAssignmentType, schema) => + roleAssignmentType === RoleAssignmentType.Dynamic + ? schema.min(1, t('At least one claim path segment is required for dynamic role assignment')) + : schema, + ), @@ - staticRoles: Yup.array() - .of(Yup.string()) - .when('roleAssignmentType', { - is: RoleAssignmentType.Static, - then: (schema) => schema.min(1, t('At least one role is required for static role assignment')), - }), + staticRoles: Yup.array() + .of(Yup.string()) + .when('roleAssignmentType', (roleAssignmentType, schema) => + roleAssignmentType === RoleAssignmentType.Static + ? schema.min(1, t('At least one role is required for static role assignment')) + : schema, + ),Please double-check against the Yup docs for your exact version to confirm the
whenbuilder signature.proxy/auth/api_client.go (1)
217-227: Optional: fall back tosubwhenpreferred_usernameis missingRight now a missing
preferred_usernameis treated as an error even ifsubis present. Depending on backend behavior, it might be more robust to fall back tosub(or another stable identifier) instead of failing outright.- if userInfoResp.PreferredUsername == nil { - return "", fmt.Errorf("userinfo response missing preferred_username") - } - - username := *userInfoResp.PreferredUsername + var username string + if userInfoResp.PreferredUsername != nil { + username = *userInfoResp.PreferredUsername + } else if userInfoResp.Sub != nil { + username = *userInfoResp.Sub + } else { + return "", fmt.Errorf("userinfo response missing preferred_username and sub") + }libs/ui-components/src/components/AuthProvider/AuthProviderDetails/RoleAssigmentDetails.tsx (1)
14-15: Typo in component name (RoleAssigmentDetailsvsRoleAssignmentDetails)The component/file name is missing an “n” (“Assigment”). Not a functional issue, but it may cause minor confusion when searching the codebase.
If feasible, consider renaming the file and component to
RoleAssignmentDetailsand updating imports accordingly.Also applies to: 53-53
proxy/middleware/auth.go (1)
17-21: UsingGetAuthToken()and guarding on empty tokens is appropriateSwitching to
tokenData.GetAuthToken()and only injecting the header when non-empty aligns this middleware with the new token abstraction.As a minor tweak, you might prefer
r.Header.Set(common.AuthHeaderKey, "Bearer "+token)instead ofAddto avoid any chance of multipleAuthorizationheaders if something upstream already set one.apps/standalone/src/app/routes.tsx (2)
358-399: Auth gating is fine, but consider avoiding directwindowaccess in renderThe authentication gating logic is straightforward and correctly skips redirects when
authEnabledis false orusernameis present. However, you might want to eventually:
- Derive
pathnamefromuseLocation()instead ofwindow.locationto align with react-router and make testing easier.- Consider performing the redirect in an effect (
useEffect) using the router’s navigation APIs instead of mutatingwindow.locationinside render, unless a full page reload is explicitly required.This is non-blocking and can be deferred.
406-409: Duplicate/callbackroute definitions may be unnecessaryThere is a top-level
/callbackroute here and another/callbackroute returned fromgetAppRoutes. They currently redirect to different targets ('/'vs'/overview'). It’s worth double‑checking that you really need both and that the chosen redirect target is intentional for all callback flows; otherwise, consider consolidating to a single canonical/callbackroute.libs/ui-components/src/components/AuthProvider/CreateAuthProvider/Oauth2ProviderFields.tsx (1)
7-21: OAuth2 fields component is straightforward; consider tightening input semanticsThe three required fields map cleanly to
OAuth2ProviderSpec(authorizationUrl,tokenUrl,userinfoUrl) and use translations consistently. IfTextFieldsupports it, consider setting an appropriatetype(e.g.,"url") for these fields to get native browser validation and better UX around URL entry.libs/ui-components/src/utils/k8sProvider.ts (1)
4-22: Tighten JWT format check and make K8s provider detection more robustA few small edge cases worth considering here:
isValidJwtTokenFormat(Lines 4–12): the base64url regex^[A-Za-z0-9_-]+$will reject segments that include=padding. Some JWT libraries emit padded base64url; if you expect those, consider allowing=at the end, or document that this is intentionally strict.nowInSeconds(Line 14): for epoch seconds,Math.floor(Date.now() / 1000)is more conventional and avoids rounding into the future by up to 0.5s, which can matter for expiry comparisons.isK8sTokenProvider(Lines 16–21): the comment says OpenShift providers areproviderType === K8swithexternalOpenShiftApiUrlset and different fromapiUrl. The current predicate (externalOpenShiftApiUrl === apiUrl) will returnfalsefor K8s providers whereexternalOpenShiftApiUrlis simply unset. If “plain K8s” is represented asexternalOpenShiftApiUrlabsent/empty, you may want something like:export const isK8sTokenProvider = (provider: AuthProvider): boolean => { if (provider.spec.providerType !== ProviderType.K8s) { return false; } const { externalOpenShiftApiUrl, apiUrl } = provider.spec; // Treat as OpenShift only when an explicit, different external OCP URL is configured return !externalOpenShiftApiUrl || externalOpenShiftApiUrl === apiUrl; };Can you confirm how
externalOpenShiftApiUrlis populated for non-OCP K8s providers and whether the undefined/empty case should be treated as a K8s token provider?apps/standalone/src/app/components/Login/LoginPageLayout.tsx (1)
4-38: Consider reusing shared theme utility and guarding for non-browser environmentsTwo minor points here:
updateThemeClassduplicates the logic fromlibs/ui-components/src/hooks/useThemePreferences.ts. Since that helper is already exported and pure, you could import and reuse it instead of re-implementing, to keep theme handling consistent.useBrowserThemecallswindow.matchMediaand touchesdocument.documentElementduring render/effect. That’s fine for a purely browser-based app, but if this component is ever rendered in a non-DOM environment (SSR or certain tests), you may want quick guards aroundtypeof window !== 'undefined'/typeof document !== 'undefined'.Not blocking, but worth considering for consistency and resilience.
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/AuthOrganizationAssignment.tsx (1)
11-99: Organization assignment logic looks solid; consider DRY’ing prefix/suffix fieldsThe three org-assignment modes (Static/Dynamic/Per user) are wired correctly to
AuthProviderFormValuesand the UX text is clear.The only nit is the duplicated
orgNamePrefix/orgNameSuffixFormGroups in the Dynamic and PerUser branches. If this section grows, you might want to extract a small helper component for the shared prefix/suffix fields to reduce repetition, but it’s not required for correctness.apps/standalone/src/app/utils/apiCalls.ts (1)
58-60: 401 handling and alerts-disabled semantics: double-check for redirect loops and intentA few behavioral points around the new 401 handling:
redirectToLogin(Lines 58–60) is now always called on 401 inhandleApiJSONResponse(Lines 73–75), regardless of the current route. If any code on/loginitself usesfetchData/fetchWithRetryand gets a 401 (e.g., a misconfigured public endpoint), this can result in a redirect loop. Consider restoring a simple guard:if (response.status === 401 && window.location.pathname !== '/login') { redirectToLogin(); }
- After
redirectToLogin()you stillthrow new Error(...). The page will be navigating away anyway, so this error mainly adds noise to logs. If you want to avoid that, you couldreturn Promise.reject()or early-return after redirect; otherwise it’s acceptable but a bit redundant.- In
handleAlertsJSONResponse(Lines 90–94), treating401like500/501and surfacing it as “alerts disabled” is a reasonable defensive fallback, but it assumes any 401 from the alerts proxy should not trigger a login redirect. Given the earlier pattern for organizations (501 => disabled), it would be good to confirm with the backend/proxy contract that a real auth failure on alerts should never leak through as a bare 401 here.If the intent is exactly as above, the implementation is fine; otherwise, a small tweak to separate “disabled” vs “unauthorized” cases might be warranted.
Also applies to: 73-75, 90-94
libs/ui-components/src/components/Login/ProviderSelector.tsx (1)
21-28: Tighten provider key generation and duplicate-name details logicThis component is generally well structured; a few small refinements would make it more robust:
- Key generation (Lines 21–28):
getProviderKeyfalls back to literal'issuer'/'clientId'when those fields don’t exist, so two non-OAuth2/OIDC providers with the samemetadata.namewill share the same React key. If possible, include something guaranteed unique, e.g.metadata.uidor a stable URL:const getProviderKey = (provider: AuthProvider): string => { const name = provider.metadata.name as string; const issuerUrl = 'issuer' in provider.spec ? provider.spec.issuer : ''; const clientId = 'clientId' in provider.spec ? provider.spec.clientId : ''; const providerType = provider.spec.providerType; return `${providerType}-${name}-${issuerUrl}-${clientId}`; };
- Type narrowing for details (Lines 80–88): casting
provider.spec as DynamicAuthProviderSpecis a bit misleading when a non-dynamic provider shares a display name; you’ll still hit this block, but those specs may not haveauthorizationUrl/issuer/clientId. You could instead use the existing type guard directly on the wider spec type:const spec = provider.spec; if (isOAuth2Provider(spec)) { details = spec.authorizationUrl || spec.clientId || ''; } else { details = 'issuer' in spec ? spec.issuer || spec.clientId || '' : spec.clientId || ''; }
- Avoid empty
<small>(Lines 80–82, 101): ifdetailsends up as an empty string (e.g., non-dynamic provider), you still render<small>{details}</small>. Consider guarding ondetails:{isDuplicateName && details && <small>{details}</small>}These are all relatively small changes but will make duplicate handling and rendering more predictable.
Also applies to: 39-53, 76-104
libs/ui-components/src/components/common/PageNavigation.tsx (1)
64-108: Gate the Settings button onisAdminto match RBAC intentRight now:
shouldShowNavigation = showOrganizationSelection || isAdmin;- But the Settings cog is always rendered whenever the nav is shown.
That means a non‑admin user who just happens to have multiple organizations will see the “Settings” button and can navigate to the auth providers page, even if they don’t have update/create rights (backend should still block, but UX becomes confusing and exposes an entry point that looks admin‑only).
To align with the “isAdmin === canUpdate/create auth providers” intent from earlier discussion, consider wrapping the Settings toolbar item in an
isAdmincheck:- {showOrganizationSelection && ( + {showOrganizationSelection && ( <ToolbarItem> <OrganizationDropdown organizationName={currentOrgDisplayName} onSwitchOrganization={() => { setShowOrganizationModal(true); }} /> </ToolbarItem> )} - <ToolbarItem> - <Tooltip content={t('Manage authentication providers')}> - <Button - variant="link" - aria-label={t('Settings')} - onClick={() => navigate(ROUTE.AUTH_PROVIDERS)} - icon={<CogIcon />} - > - {t('Settings')} - </Button> - </Tooltip> - </ToolbarItem> + {isAdmin && ( + <ToolbarItem> + <Tooltip content={t('Manage authentication providers')}> + <Button + variant="link" + aria-label={t('Settings')} + onClick={() => navigate(ROUTE.AUTH_PROVIDERS)} + icon={<CogIcon />} + > + {t('Settings')} + </Button> + </Tooltip> + </ToolbarItem> + )}That keeps
shouldShowNavigationbehavior (single‑org admins still see the bar) while making the Settings control itself consistently admin‑only.libs/ui-components/src/components/AuthProvider/AuthProviderDetails/DeleteAuthProviderModal.tsx (1)
16-90: Consider closing the modal automatically on successful delete
handleDeletecurrently:await remove(`authproviders/${authProviderId}`); onDeleteSuccess();but never calls
onClose. In the details page,onDeleteSuccessnavigates away so the modal unmounts anyway, but in other potential callers (e.g. a list page that stays on the same route) it’s easy to forget to both refetch and explicitly close the modal.You could make the component safer to reuse by closing on success:
const handleDelete = async () => { setError(undefined); setIsLoading(true); try { await remove(`authproviders/${authProviderId}`); - onDeleteSuccess(); + onDeleteSuccess(); + onClose(); } catch (e) { setError(getErrorMessage(e)); } finally { setIsLoading(false); } };This preserves current behavior on the details page (it will still navigate) while ensuring the modal always dismisses after a successful delete.
apps/standalone/src/app/components/Login/LoginPage.tsx (1)
50-53: Minor: clean up provider name interpolation stringThe translation string has a trailing space and slightly inconsistent spacing around the variable (
'{{ providerName}} '). Consider tightening it so localization doesn’t depend on incidental whitespace:- t('Failed to initiate login with {{ providerName}} ', { + t('Failed to initiate login with {{ providerName }}', { providerName: getProviderDisplayName(provider, t) || (provider.metadata.name as string), }),apps/standalone/src/app/context/AuthContext.ts (1)
49-102: Callback and user‑info error handling is much more robust; consider centralizing response parsingThe changes around the
/callbackflow and/infohandling look good:
- Requiring both
codeandstatebefore POSTing to${loginAPI}avoids ambiguous or insecure callback handling.- Provider name extraction from
stateand passing the full encodedstatethrough to the backend aligns with the documentedstateformat and PKCE fallback comments.- For both the login POST and
/infocalls, you now try to extract a detailed error message from JSON or text responses and bail out early, which prevents later logic from running on failed auth.One small clean‑up opportunity: the error‑body parsing logic for the 401 branch and the generic
resp.status !== 200branch is nearly identical. If you find yourself reusing this pattern in more places, it might be worth factoring into a small helper likeasync readErrorMessage(resp, defaultMessage)to keep the main control flow tighter.Please verify that backend error payloads for
${loginAPI}and${loginAPI}/infoconsistently use an{ error?: string }shape (or plain text) so this parsing logic always yields meaningful messages.Also applies to: 115-157, 166-169
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx (1)
63-76: Minor text polish for empty stateTwo tiny text nits you might want to tidy up:
- Title: “Add your first authentication providers” → “Add your first authentication provider”
- Body: “Oauth2” → “OAuth2” for consistency with other labels.
This keeps the copy consistent and a bit more polished.
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProvider.tsx (1)
29-47: Create/edit flow and RBAC wrapper look correct; consider basing title onauthProviderIdThe create/edit page wiring looks good overall:
- When
authProviderIdis present, you fetch details, show a spinner while loading, and surface errors inline via an Alert.CreateAuthProviderFormgets the existing provider in edit mode and navigates back to the list or details on close/success.CreateAuthProviderWithPermissionscorrectly checks CREATE vs PATCH viauseAccessReviewand wraps the inner component inPageWithPermissions.One minor UX suggestion: the page title is currently driven by
!!authProviderDetails, so if fetching an existing provider fails you’ll show “Add authentication provider” while the breadcrumb still includes the ID and an error alert explains that details couldn’t be retrieved. Basing the title onauthProviderIdinstead would keep the edit vs create mode more semantically consistent even when the fetch fails.- let title: string; - if (!!authProviderDetails) { - title = t('Edit authentication provider'); - } else { - title = t('Add authentication provider'); - } + const title = authProviderId + ? t('Edit authentication provider') + : t('Add authentication provider');Not critical, but it would make error states a bit clearer to users.
Also applies to: 55-88, 121-135
libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetailsTab.tsx (3)
49-56: Guard the DynamicAuthProviderSpec cast / orgAssignment access
authProvider.specis force-cast toDynamicAuthProviderSpec, andorganizationAssignmentis accessed without checks. If a non-OIDC/OAuth2 provider ever reaches this component (e.g., k8s), this will blow up at runtime.Consider either:
- Narrowing the prop type to a dynamic provider-specific type, or
- Adding a runtime guard on
spec.providerTypebefore casting/usingorganizationAssignment.
96-99: Use the human‑readable provider type label in the configuration titleThe title uses
spec.providerTypedirectly, which will likely render raw enum values such as"oidc"/"oauth2":{t('{{ providerType }} configuration', { providerType: spec.providerType })}You already have
getProviderTypeLabel; wiring that in here would keep the title consistent with the “Type” field:- {t('{{ providerType }} configuration', { providerType: spec.providerType })} + {t('{{ providerType }} configuration', { + providerType: getProviderTypeLabel(spec.providerType, t), + })}
196-230: Defensive handling fororgAssignment.claimPathand optional org name prefix/suffixFor dynamic org assignment you assume
orgAssignment.claimPathis always present:{orgAssignment.type === OrgAssignmentType.Dynamic && ( ... {orgAssignment.claimPath.map(...)} )}If the backend ever omits
claimPath(or it’s accidentallynull), this will throw. Same fororganizationNamePrefix/organizationNameSuffixyou later render as strings but default to'N/A'.Consider normalizing with safe defaults:
- {orgAssignment.claimPath.map((pathSegment, index) => ( + {(orgAssignment.claimPath ?? []).map((pathSegment, index) => ( <Label key={`${pathSegment}-${index}`}>{pathSegment}</Label> ))}and similar
?? ''defaults where appropriate before rendering.libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx (3)
229-235: ClarifyClient secretrequiredness for OAuth2 PKCE vs confidential clients
Client secretis always markedisRequiredin the UI:<FormGroup label={t('Client secret')} isRequired> <TextField name="clientSecret" ... type="password" /> </FormGroup>Given the new PKCE flows and support for public OAuth2 clients without a secret, this visual required marker may be misleading if the validation schema allows an empty secret for those cases.
Consider:
- Tying
isRequiredto the same condition you use inauthProviderSchema, or- Adjusting the label/helper text to “Required for confidential clients” if secrets are optional for some provider types.
282-315: Test connection payload may be too minimal (no client secret / scopes)
handleTestConnectiononly sends:const requestBody = { providerType: values.providerType, issuer: values.issuer, authorizationUrl: values.authorizationUrl, tokenUrl: values.tokenUrl, userinfoUrl: values.userinfoUrl, clientId: values.clientId, };If the backend’s
test-auth-provider-connectionendpoint needsclientSecretand/orscopesto properly validate a configuration (especially for confidential clients), the test button could give false negatives or incomplete diagnostics.It’s probably worth confirming the API contract and, if needed, extending the payload:
clientId: values.clientId, + clientSecret: values.clientSecret, + scopes: values.scopes,
351-379: Edit flow passes staleauthProvidertoonSuccessIn the edit path you compute patches and send them, but on success you call:
await patch<AuthProvider>(`authproviders/${authProvider.metadata.name}`, patches); onSuccess(authProvider);This hands the caller the original object, not the updated resource or at least a locally updated copy. If callers rely on the returned
AuthProviderfor UI state, they’ll be out of sync with what the backend now has.Consider either:
- Using the response from
patch(if it returns the updated resource), or- Manually merging
valuesintoauthProviderbefore passing it toonSuccess.Example:
- const updated = await patch<AuthProvider>(..., patches); - onSuccess(authProvider); + const updated = await patch<AuthProvider>(..., patches); + onSuccess(updated);(Adjust to match your
patchhelper’s return type.)proxy/auth/oauth2.go (1)
26-56: PKCE‑only client config: verify behavior for confidential OAuth2 clients
getOAuth2AuthHandlerenforces required fields and then builds an osin client with:ClientSecret: "", // Not needed with PKCE for public clients SendClientSecretInParams: false, Scope: scope,and never uses
oauth2Spec.ClientSecret.That’s perfect for pure PKCE/public clients, but if you intend to support confidential OAuth2 clients that do require a client secret, they’ll currently be misconfigured:
- No secret is sent on token exchange.
- There’s no early validation that a secret was erroneously provided.
I’d suggest either:
- Explicitly scoping this handler to PKCE/public clients and rejecting specs that include a client secret, or
- Extending the config to optionally include
oauth2Spec.ClientSecret(and configuring osin accordingly) when your provider type indicates a confidential client.proxy/auth/k8s.go (1)
27-32: Consider populatingTokenData.ProviderusingproviderNameYou now capture
providerNameinTokenAuthProvider, butValidateTokenbuilds:tokenData := TokenData{ IDToken: token, RefreshToken: "", }Given
TokenDatahas aProviderfield (and you’re moving toward multiple providers), setting it here would make downstream logic more robust:tokenData := TokenData{ IDToken: token, RefreshToken: "", + Provider: t.providerName, }If you don’t plan to use
Providerfor K8s flows, you could also drop theproviderNamefield entirely to keep the struct minimal.Also applies to: 78-85
proxy/auth/openshift.go (1)
35-98: OpenShift handler wiring is sound; consider future configurability and clearer namingThe handler follows the same PKCE /
exchangeToken/refreshOAuthToken/loginRedirectpatterns as the other providers, and the discovery‑based logout is a nice touch. A couple of non‑blocking points:
authURL(local var) is really the API server base URL and is then copied toapiServerURL, while the struct fieldauthURLstores the authorize endpoint. Renaming the local variable or the struct field later would reduce confusion when debugging.clientIdand scope ("user:full") are currently hard‑coded. Once the CRD/spec grows fields for OpenShift client IDs and scopes, this function will be the place to thread them through instead of relying on the default service account and static scope.No blockers from this file.
Also applies to: 100-169
proxy/auth/auth.go (3)
196-205: Customer‑token provider detection via*TokenAuthProvideris simple and effectiveUsing a concrete type check in
isProviderWithCustomerTokencleanly separates K8s token providers (direct validation, no PKCE/BFF) from OAuth‑style providers, and it’s reused consistently inLoginandRefresh. As long as we don’t introduce additional customer‑token provider types without extending this helper, this remains easy to reason about.
395-457: Refresh routing via provider type is correct; error responses could be slightly more consistentThe refresh handler correctly:
- Short‑circuits when auth is disabled (Teapot) or session cookie parsing fails.
- Requires a provider in the session, resolves it via
getProviderInstance, and distinguishes customer‑token K8s providers (refresh unsupported) from OAuth‑style providers.- Requires a refresh token, resolves
client_idfrom the provider config, and performs a backend/tokenrefresh viaexchangeTokenWithApiServer, translating the result intoTokenData.Functionally this is fine. The only small improvement would be to use
respondWithError(with a message) instead of raww.WriteHeader(http.StatusInternalServerError)in the error paths where provider resolution or client_id extraction fails, to keep responses uniform for callers.
608-645: getAuthInfo behavior is fine; log message is slightly misleading
getAuthInfocorrectly:
- Uses the API TLS config to GET
/api/v1/auth/config.- Interprets HTTP 418 as “auth disabled” and returns
(nil, nil)to signal that.- Propagates all other errors and unmarshals a valid
AuthConfigJSON on success.Minor nit: the log message
"Failed to read terminal session response"is clearly copy‑pasted and should mention “auth config response” instead to avoid confusion when debugging.libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (1)
1-96: Type guards and form model look consistent with backend contractsThis module is well‑structured:
OrgAssignmentType/RoleAssignmentTypeand theis*type guards correctly discriminate on thetypefield for the imported assignment/spec unions.AuthProviderFormValuescaptures the key OIDC/OAuth2 fields plus org/role assignment data and maps cleanly onto what the backend types expose.FieldValidationandFieldValidationResultmirror the Go structs inproxy/bridge/testauth.go(field names and optional notes/value), keeping the test‑connection pipeline strongly typed on both sides.Only minor polish you might consider later:
DEFAULT_USERNAME_CLAIM/DEFAULT_ROLE_CLAIMare flat strings while the corresponding form fields (usernameClaim,roleClaimPath) are string arrays; a tiny helper to convert those constants into path arrays (e.g.[DEFAULT_USERNAME_CLAIM]) would make the defaults plug‑and‑play.proxy/auth/oidc.go (1)
37-124: OIDC handler refactor matches the new BFF/PKCE model; consider trimming unused internalAuthURL pathThe OIDC handler now:
- Validates
IssuerandClientIdfromOIDCProviderSpec.- Discovers endpoints via
/.well-known/openid-configurationand builds clients withgetOIDCClient, including dynamically built scopes (openid profile emailplusorganization:*when enabled).- Stores
tokenEndpoint,clientId, andproviderNameon the handler and uses them inGetTokenandGetLoginRedirectURL.- Defers user info to the API server via the same “GetUserInfo should be retrieved through the flightctl API” contract used by other providers.
- Adds
client_idto the logout URL, aligning with how the backend and IdPs typically expect end‑session requests.Functionally this looks good and consistent with the rest of the refactor. The only minor clean‑up is that
internalAuthURLis now alwaysnil, so the branch that rebuilds endpoints viareplaceBaseURLand re‑creates an external client is currently unreachable. If internal vs external OIDC endpoints are no longer supported, you could remove that code path to reduce cognitive load; otherwise, wiringinternalAuthURLfrom spec when/if it appears again would make the behavior clearer.Based on learnings
Also applies to: 146-206
proxy/auth/common.go (3)
335-343: Add a timeout to the HTTP client used for token exchangeThe custom
http.Clientfor the token exchange inherits the zero Timeout, so a slow or wedged token endpoint could hang a request indefinitely on this path.Consider setting a reasonable timeout (e.g., 10–30 seconds, or reusing a shared client with standard timeouts) to keep the proxy responsive:
httpClient := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: tlsConfig, - }, + Transport: &http.Transport{ + TLSClientConfig: tlsConfig, + }, + Timeout: 10 * time.Second, }
691-751: PKCE verifier cookie handling is sound; consider scope and log levelStoring the
code_verifierin an HttpOnly cookie with SameSite=Lax and a short MaxAge, and then clearing it with a MaxAge -1, is consistent with the redirect‑based PKCE flow. A couple of minor considerations:
getPKCEVerifierCookielogs a Warn when the cookie is missing or empty, but for the “fallback via state” path that can be an expected condition; using Debug for the missing‑cookie case would reduce noise.- Cookie
Path=/is fine, but if only the callback endpoint needs it, narrowing the path could slightly reduce exposure in case of other endpoints or subapps reading cookies.Neither is a blocker, just small polish items.
189-308: Remove unreachable reflection fallback; all call sites provide explicit config valuesThe reflection code (lines 203–275) that attempts to extract
tokenURL,clientID, andredirectURIfromosincli.Client.Configis never executed. All four call sites inopenshift.go,oidc.go,oauth2.go, andaap.gopass explicit, non-empty values for these parameters, so the condition at line 203 is never true. SimplifyexchangeToken()by removing the unused reflection fallback entirely and requiring explicit configuration, or remove the optional parameters altogether if they are always expected to be provided. This eliminates the brittle reflection dependency and clarifies intent.proxy/bridge/testauth.go (2)
47-91: TestConnection handler structure is solid; consider minor hardeningThe handler correctly:
- Restricts to POST.
- Validates JSON into
TestConnectionRequest.- Constructs an
http.Clientwith yourtls.Configand a 10s timeout.- Dispatches by
providerTypeand returns a JSONresultsarray.You might optionally handle
json.NewEncoder(w).Encode(response)errors (e.g., log at Warn) to cover rare cases where the connection drops while writing, but behavior today is acceptable for this diagnostic endpoint.
93-225: OIDC discovery and endpoint checks are good; normalize issuer URL to avoid false mismatchesThe OIDC branch does the right things: calls
issuer/.well-known/openid-configuration, validates required endpoints, and then probes the discovered URLs.One improvement: user‑supplied issuers often vary only by a trailing
/compared to the discovery document. Today you:
- Build
discoveryUrlstraight fromreq.Issuer.- Compare
discovery.Issuer != req.Issuerand record a note, but don’t treat it as an error.To reduce noisy notes and potential confusion, consider normalizing both values before comparison and when building the discovery URL:
- discoveryUrl := fmt.Sprintf("%s/.well-known/openid-configuration", req.Issuer) + issuer := strings.TrimRight(req.Issuer, "/") + discoveryUrl := fmt.Sprintf("%s/.well-known/openid-configuration", issuer) ... - if discovery.Issuer != req.Issuer { + if strings.TrimRight(discovery.Issuer, "/") != issuer { issuerNotes = append(issuerNotes, fmt.Sprintf("Discovery document issuer (%s) does not match provided issuer (%s)", discovery.Issuer, req.Issuer)) }This keeps the “issuer mismatch” note focused on genuinely different values instead of benign trailing slashes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (2)
177-215: Wrap switch-case declarations in blocks to satisfynoSwitchDeclarations.As noted in previous reviews,
constdeclarations insideswitchcases share lexical scope across cases, which violatesnoSwitchDeclarations. Wrap the case bodies that declare variables (aClaimPath,bClaimPath) in blocks.Apply this diff:
switch (a.type) { case OrgAssignmentType.Static: if (!isOrgAssignmentStatic(a) || !isOrgAssignmentStatic(b)) { return false; } return (a.organizationName || '') === (b.organizationName || ''); - case OrgAssignmentType.Dynamic: + case OrgAssignmentType.Dynamic: { if (!isOrgAssignmentDynamic(a) || !isOrgAssignmentDynamic(b)) { return false; } const aClaimPath = a.claimPath || []; const bClaimPath = b.claimPath || []; if (aClaimPath.length !== bClaimPath.length) { return false; } if (!aClaimPath.every((val, idx) => val === bClaimPath[idx])) { return false; } return ( (a.organizationNamePrefix || '') === (b.organizationNamePrefix || '') && (a.organizationNameSuffix || '') === (b.organizationNameSuffix || '') ); + } case OrgAssignmentType.PerUser: if (!isOrgAssignmentPerUser(a) || !isOrgAssignmentPerUser(b)) { return false; } return ( (a.organizationNamePrefix || '') === (b.organizationNamePrefix || '') && (a.organizationNameSuffix || '') === (b.organizationNameSuffix || '') );
221-255: Wrap switch-case declarations in blocks to satisfynoSwitchDeclarations.As noted in previous reviews, wrap the case bodies that declare variables (
aRoles,bRoles,aClaimPath,bClaimPath) in blocks to isolate their scope.Apply this diff:
switch (a.type) { - case RoleAssignmentType.Static: + case RoleAssignmentType.Static: { if (!isRoleAssignmentStatic(a) || !isRoleAssignmentStatic(b)) { return false; } const aRoles = a.roles || []; const bRoles = b.roles || []; if (aRoles.length !== bRoles.length) { return false; } // Compare roles arrays (order matters for roles) return aRoles.every((val, idx) => val === bRoles[idx]); - case RoleAssignmentType.Dynamic: + } + case RoleAssignmentType.Dynamic: { if (!isRoleAssignmentDynamic(a) || !isRoleAssignmentDynamic(b)) { return false; } const aClaimPath = a.claimPath || []; const bClaimPath = b.claimPath || []; if (aClaimPath.length !== bClaimPath.length) { return false; } // Compare claim path arrays (order matters) if (!aClaimPath.every((val, idx) => val === bClaimPath[idx])) { return false; } return (a.separator || DEFAULT_ROLE_SEPARATOR) === (b.separator || DEFAULT_ROLE_SEPARATOR); + } default: return false; }apps/standalone/src/app/components/Login/LoginPage.tsx (1)
15-20: Add response validation and error handling toredirectToProviderLogin.This concern was previously raised and remains unaddressed. The function doesn't validate the HTTP response status or check if the
urlfield exists in the response body. On 4xx/5xx errors or malformed responses, the code will attempt to redirect toundefined, bypassing error handlers.Apply this diff to add proper validation:
const redirectToProviderLogin = async (provider: AuthProvider) => { // Backend generates PKCE parameters and stores code_verifier in a cookie const response = await fetch(`${loginAPI}?provider=${provider.metadata.name}`); + + if (!response.ok) { + throw new Error(`Login redirect failed with status ${response.status}`); + } + - const { url } = (await response.json()) as { url: string }; + const { url } = (await response.json()) as { url?: string }; + if (!url) { + throw new Error('Login redirect URL missing in response'); + } + window.location.href = url; };
🧹 Nitpick comments (4)
libs/ui-components/src/utils/k8sProvider.ts (1)
11-11: PreferMath.floor()for Unix timestamps.Using
Math.round()can produce timestamps up to 0.5 seconds in the future, which may cause edge-case issues with JWT validation (e.g., token appears issued before it actually was).Math.floor()is the conventional choice for Unix timestamps.Apply this diff:
-export const nowInSeconds = () => Math.round(Date.now() / 1000); +export const nowInSeconds = () => Math.floor(Date.now() / 1000);libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (2)
100-101: Consider adding a fallback fororganizationName.For consistency with line 150 in
getOrgAssignment, which usesvalues.orgName || '', consider adding a fallback here to handle potential undefined values defensively.Apply this diff:
} else { orgAssignmentType = OrgAssignmentType.Static; - orgName = orgAssignment.organizationName; + orgName = orgAssignment.organizationName || ''; }
257-272: Remove unnecessary type assertions.
AuthStaticRoleAssignmentandAuthDynamicRoleAssignmentare already subtypes ofAuthRoleAssignment, so the explicit casts on lines 263 and 270 are unnecessary.Apply this diff:
if (values.roleAssignmentType === RoleAssignmentType.Static) { const staticAssignment: AuthStaticRoleAssignment = { type: RoleAssignmentType.Static, roles: values.staticRoles || [], }; - return staticAssignment as AuthRoleAssignment; + return staticAssignment; } else { const dynamicAssignment: AuthDynamicRoleAssignment = { type: RoleAssignmentType.Dynamic, claimPath: values.roleClaimPath || [], separator: values.roleSeparator || DEFAULT_ROLE_SEPARATOR, }; - return dynamicAssignment as AuthRoleAssignment; + return dynamicAssignment; }proxy/auth/common.go (1)
190-309: Consider refactoring reflection-based config extraction.The reflection code (lines 203-276 for config fields, 278-309 for TLS) extracts internal osincli.Client configuration. While functional, this approach is fragile:
- Breaks if osincli changes internal field names or structure
- The code tries both
Configandconfig,TokenUrlandtokenUrlto handle variations- Relies on unexported fields
Consider requesting these values as explicit parameters to
exchangeTokenfrom all call sites, or wrapping osincli.Client in an interface with accessor methods.The sensitive logging on lines 322-368 has already been flagged and acknowledged as temporary for debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
proxy/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
apps/standalone/src/app/components/Login/LoginPage.tsx(1 hunks)libs/types/index.ts(4 hunks)libs/types/models/AppType.ts(1 hunks)libs/types/models/ApplicationPort.ts(1 hunks)libs/types/models/ApplicationResourceLimits.ts(1 hunks)libs/types/models/ApplicationResources.ts(1 hunks)libs/types/models/ApplicationVolume.ts(1 hunks)libs/types/models/AuthProviderSpec.ts(1 hunks)libs/types/models/ImageApplicationProviderSpec.ts(1 hunks)libs/types/models/ImageMountVolumeProviderSpec.ts(1 hunks)libs/types/models/K8sProviderSpec.ts(0 hunks)libs/types/models/MountVolumeProviderSpec.ts(1 hunks)libs/types/models/OpenShiftProviderSpec.ts(1 hunks)libs/types/models/VolumeMount.ts(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/RoleAssignmentSection.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts(1 hunks)libs/ui-components/src/utils/k8sProvider.ts(1 hunks)proxy/auth/api_client.go(1 hunks)proxy/auth/auth.go(7 hunks)proxy/auth/common.go(3 hunks)proxy/auth/openshift.go(1 hunks)proxy/go.mod(1 hunks)
💤 Files with no reviewable changes (1)
- libs/types/models/K8sProviderSpec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-components/src/components/AuthProvider/CreateAuthProvider/RoleAssignmentSection.tsx
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
📚 Learning: 2025-11-20T07:33:34.630Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/utils/authProvider.ts:5-18
Timestamp: 2025-11-20T07:33:34.630Z
Learning: In the flightctl-ui repository, for AuthProvider objects, the metadata.name field is always present at runtime as it serves as the ID of the authprovider. The OpenAPI schema incorrectly defines it as optional (name?: string in ObjectMeta), but the type assertion `as string` is appropriate when accessing provider.metadata.name since it's guaranteed to exist.
Applied to files:
libs/types/models/AuthProviderSpec.tslibs/types/models/OpenShiftProviderSpec.tslibs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts
📚 Learning: 2025-10-10T11:35:04.458Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-10-10T11:35:04.458Z
Learning: In the flightctl-ui repository, TypeScript types are generated using hey-api/openapi-ts and output to a single index.ts file (previously used openapi-typescript-codegen with one file per type under /libs/types/models).
Applied to files:
libs/types/models/OpenShiftProviderSpec.tslibs/types/index.ts
📚 Learning: 2025-10-29T16:47:29.614Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
Applied to files:
libs/types/models/AppType.tslibs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.tsproxy/auth/api_client.goproxy/auth/common.go
📚 Learning: 2025-03-24T12:33:54.002Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: proxy/auth/oidc.go:17-19
Timestamp: 2025-03-24T12:33:54.002Z
Learning: OIDC (OpenID Connect) providers return usernames in the `preferred_username` field, as implemented in the OIDCUserInfo struct in proxy/auth/oidc.go.
Applied to files:
proxy/auth/auth.goproxy/auth/common.go
📚 Learning: 2025-02-17T08:41:57.993Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-02-17T08:41:57.993Z
Learning: All files in `libs/types/models` are auto-generated using OpenAPI Generator from the schema defined in https://github.com/flightctl/flightctl/blob/main/api/v1alpha1/openapi.yaml. Changes to these files should be made in the schema instead of modifying the generated files directly.
Applied to files:
libs/types/index.tsproxy/auth/api_client.goproxy/go.mod
📚 Learning: 2025-03-20T12:34:33.199Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/types/models/FileSpec.ts:8-8
Timestamp: 2025-03-20T12:34:33.199Z
Learning: Files in the types directory are autogenerated from the OpenAPI spec in the flightctl project and should not be directly modified.
Applied to files:
proxy/auth/api_client.go
📚 Learning: 2025-06-04T06:41:15.969Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 308
File: libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts:204-204
Timestamp: 2025-06-04T06:41:15.969Z
Learning: In the flightctl-ui codebase, volume functionality is not yet implemented in the backend. TODO comments related to volume preservation (like in toAPIApplication function) are intentional placeholders for when backend volume support is added.
Applied to files:
proxy/auth/api_client.go
📚 Learning: 2025-10-21T13:53:29.934Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 357
File: proxy/auth/auth.go:228-237
Timestamp: 2025-10-21T13:53:29.934Z
Learning: In proxy/auth/auth.go, the token cleanup cycles in `startTokenCleanup` and `cleanupExpiredTokens` are designed for memory management of unretrieved tokens, not as a primary security control. The single-use deletion in `getSingleUseApiTokenMapping` provides the security guarantee. TTL enforcement on retrieval is intentionally not implemented because tokens are expected to be retrieved immediately after creation.
Applied to files:
proxy/auth/api_client.go
🧬 Code graph analysis (16)
libs/types/models/MountVolumeProviderSpec.ts (1)
libs/types/models/VolumeMount.ts (1)
VolumeMount(8-13)
libs/types/models/AuthProviderSpec.ts (2)
libs/types/index.ts (1)
OpenShiftProviderSpec(143-143)libs/types/models/OpenShiftProviderSpec.ts (1)
OpenShiftProviderSpec(8-49)
libs/types/models/ImageMountVolumeProviderSpec.ts (1)
libs/types/models/VolumeMount.ts (1)
VolumeMount(8-13)
libs/types/models/VolumeMount.ts (1)
libs/types/index.ts (1)
VolumeMount(187-187)
libs/types/models/ApplicationPort.ts (1)
libs/types/index.ts (1)
ApplicationPort(10-10)
apps/standalone/src/app/components/Login/LoginPage.tsx (4)
libs/types/models/AuthProvider.ts (1)
AuthProvider(10-21)apps/standalone/src/app/utils/apiCalls.ts (1)
loginAPI(17-17)libs/ui-components/src/utils/authProvider.ts (1)
getProviderDisplayName(5-18)libs/types/models/AuthConfig.ts (1)
AuthConfig(6-23)
libs/types/models/OpenShiftProviderSpec.ts (1)
libs/types/index.ts (1)
OpenShiftProviderSpec(143-143)
libs/types/models/ApplicationResources.ts (2)
libs/types/index.ts (2)
ApplicationResources(13-13)ApplicationResourceLimits(12-12)libs/types/models/ApplicationResourceLimits.ts (1)
ApplicationResourceLimits(8-17)
libs/types/models/ApplicationVolume.ts (1)
libs/types/models/ImageVolumeProviderSpec.ts (1)
ImageVolumeProviderSpec(6-8)
libs/types/models/ApplicationResourceLimits.ts (1)
libs/types/index.ts (1)
ApplicationResourceLimits(12-12)
proxy/auth/auth.go (5)
proxy/auth/common.go (8)
AuthProvider(70-76)ProviderTypeOpenShift(31-31)ProviderTypeK8s(27-27)ProviderTypeOIDC(28-28)ProviderTypeAAP(29-29)ProviderTypeOAuth2(30-30)LoginParameters(65-68)ParseSessionCookie(95-120)proxy/auth/k8s.go (2)
TokenAuthProvider(17-21)TokenLoginParameters(23-25)proxy/config/config.go (1)
BaseUiUrl(16-16)libs/types/models/TokenRequest.ts (1)
TokenRequest(8-37)libs/types/models/TokenResponse.ts (1)
TokenResponse(8-33)
libs/types/models/ImageApplicationProviderSpec.ts (3)
libs/types/models/ApplicationVolumeProviderSpec.ts (1)
ApplicationVolumeProviderSpec(6-11)libs/types/models/ApplicationPort.ts (1)
ApplicationPort(8-8)libs/types/models/ApplicationResources.ts (1)
ApplicationResources(9-11)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (5)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (8)
AuthProviderFormValues(56-82)DEFAULT_ROLE_SEPARATOR(23-23)isOrgAssignmentDynamic(35-37)isOrgAssignmentPerUser(39-41)isRoleAssignmentStatic(48-50)isRoleAssignmentDynamic(52-54)isOAuth2Provider(28-29)isOrgAssignmentStatic(31-33)libs/ui-components/src/types/extraTypes.ts (1)
DynamicAuthProviderSpec(82-82)libs/types/models/AuthDynamicRoleAssignment.ts (1)
AuthDynamicRoleAssignment(8-21)libs/ui-components/src/utils/patch.ts (1)
appendJSONPatch(23-54)libs/ui-components/src/components/form/validations.ts (1)
validKubernetesDnsSubdomain(151-166)
proxy/auth/api_client.go (7)
libs/types/models/TokenRequest.ts (1)
TokenRequest(8-37)libs/types/models/TokenResponse.ts (1)
TokenResponse(8-33)proxy/config/config.go (1)
FctlApiUrl(10-10)proxy/log/log.go (1)
GetLogger(17-19)proxy/auth/auth.go (1)
UserInfoResponse(19-21)libs/types/models/UserInfoResponse.ts (1)
UserInfoResponse(9-30)proxy/auth/common.go (1)
TokenData(40-55)
proxy/auth/openshift.go (5)
proxy/auth/common.go (3)
AuthProvider(70-76)LoginParameters(65-68)TokenData(40-55)libs/types/models/OpenShiftProviderSpec.ts (1)
OpenShiftProviderSpec(8-49)proxy/bridge/common.go (1)
GetAuthTlsConfig(40-65)proxy/config/config.go (1)
BaseUiUrl(16-16)proxy/log/log.go (1)
GetLogger(17-19)
proxy/auth/common.go (2)
proxy/log/log.go (1)
GetLogger(17-19)proxy/config/config.go (2)
BaseUiUrl(16-16)TlsCertPath(15-15)
🪛 Biome (2.1.2)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts
[error] 192-192: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 193-193: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 231-231: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 232-232: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 242-242: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 243-243: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 476-476: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 483-483: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build ocp plugin
🔇 Additional comments (37)
libs/types/models/ApplicationVolume.ts (1)
6-11: Syntactic type alias change is safe and keeps semantics identicalRewriting the alias as
{ ... } & ImageVolumeProviderSpec(without the wrapping parentheses) does not change the resulting type and stays consistent with typical TS style. No issues from this change.libs/ui-components/src/utils/k8sProvider.ts (1)
1-9: LGTM! JWT format validation is correctly implemented.The function properly validates the three-part structure of JWT tokens and uses the correct base64url character set. The requirement for non-empty parts is a good security practice for production use.
libs/types/models/AppType.ts (1)
11-11: Resolve: This is a preparatory update from the OpenAPI spec, intentionally included and requires no immediate UI handler changes.This change follows the same pattern as PR #371 where
AppTypeQuadletwas added as a preparatory enum member before backend implementation. The codebase currently supports only one application type value ("compose"), and the UI has not yet implemented per-type handlers. TheAppTypeContainerenum member is automatically generated from OpenAPI specs and will be integrated when backend support becomes available.libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (1)
56-534: Well-structured utility module with comprehensive validation.The implementation is solid:
- Proper handling of different provider types (OIDC vs OAuth2) and assignment types (Static, Dynamic, PerUser)
- Deep equality checks for org and role assignments prevent unnecessary patches
- Conditional validation schema adapts to form state
- Defensive programming with fallbacks (
|| [],?? true)The patch generation logic correctly handles provider type transitions and secret updates.
apps/standalone/src/app/components/Login/LoginPage.tsx (1)
32-98: Well-structured provider selection and initialization logic.The implementation properly handles:
- Prevention of multiple concurrent redirects with the
isRedirectingflag- Error handling in both manual provider selection and auto-redirect paths
- Appropriate user feedback via error messages
- Proper filtering and sorting of enabled providers
libs/types/models/ApplicationPort.ts (1)
1-8: Generated type definition looks correct.This auto-generated type properly defines the ApplicationPort format with clear documentation. Based on learnings, changes to these files should be made in the OpenAPI schema rather than directly.
libs/types/models/VolumeMount.ts (1)
1-14: Generated type definition looks correct.This auto-generated type properly defines the VolumeMount structure with clear documentation for the mount path configuration.
libs/types/models/ImageMountVolumeProviderSpec.ts (1)
1-14: Generated type definition looks correct.This auto-generated type properly composes ImageVolumeSource and VolumeMount types to define an image-backed volume mount specification.
libs/types/models/ApplicationResources.ts (1)
1-12: Generated type definition looks correct.This auto-generated type properly defines resource constraints with an optional ApplicationResourceLimits field.
libs/types/models/MountVolumeProviderSpec.ts (1)
1-12: Generated type definition looks correct.This auto-generated type properly defines a named volume mount configuration using the VolumeMount type.
libs/types/models/AuthProviderSpec.ts (1)
9-10: OpenShift provider type properly integrated.The union type correctly includes OpenShiftProviderSpec alongside the existing provider types, enabling OpenShift-based authentication flows in the UI.
proxy/go.mod (1)
8-8: The flightctl dependency commit exists with no known security issues, but manual verification of breaking changes is recommended.The commit
1919682154b5754b9767e08ec5e5255131d873bdexists in the upstream flightctl/flightctl repository and corresponds to a merge PR about helm chart deployment changes. No recent security advisories were detected. However, review the actual code changes in PR #1961 to ensure this commit doesn't introduce breaking changes to the API or exported functionality that would impact this proxy module.libs/types/models/ImageApplicationProviderSpec.ts (1)
5-17: LGTM! Type additions align with OpenAPI schema.The new
portsandresourcesfields appropriately extendImageApplicationProviderSpecwith container port mappings and resource specifications. The type definitions are properly imported and maintain type safety.Based on learnings
libs/types/models/ApplicationResourceLimits.ts (1)
1-17: LGTM! Clean type definition for resource limits.The type correctly models CPU and memory limits with clear documentation of expected formats (cores for CPU, Podman units for memory).
Based on learnings
libs/types/models/OpenShiftProviderSpec.ts (1)
1-49: LGTM! Well-structured OpenShift provider specification.The type appropriately models OpenShift OAuth provider configuration with a discriminator field (
providerType: 'openshift') and comprehensive OAuth2 fields. The OpenShift-specificclusterControlPlaneUrlfield enables proper cluster URL derivation.Based on learnings
proxy/auth/api_client.go (3)
22-103: Token exchange implementation looks solid.The function properly handles:
- TLS configuration
- Request marshaling and error handling
- HTTP status checks (418 for auth not configured)
- OAuth2 error response parsing
- Appropriate error propagation
The sensitive logging on lines 46-47 and 60-61 has already been flagged and acknowledged as temporary for debugging.
106-191: User info retrieval looks correct with proper username normalization.The function appropriately:
- Fetches user info from the backend API
- Handles error responses (418, non-200 status)
- Strips the Kubernetes service account prefix when present (lines 186-188)
The sensitive logging on lines 124-133 and 146-147 has already been flagged and acknowledged as temporary for debugging.
194-219: LGTM! Token response conversion is correct.The function properly:
- Maps API response fields to internal TokenData structure
- Handles optional fields appropriately
- Converts expires_in from int to int64
- Sets both IDToken and AccessToken for OIDC compatibility (lines 199-204)
libs/types/index.ts (1)
10-187: LGTM! Export additions align with new model types.The new exports properly expose the newly added types: application resources/ports, volume mounts, OpenShift provider spec, and authentication-related types (TokenRequest, TokenResponse, UserInfoResponse).
Based on learnings
proxy/auth/auth.go (9)
53-66: LGTM! Provider lookup is straightforward.The function correctly searches the provider array by metadata.name and returns appropriate errors when providers are not configured or not found.
68-146: Provider instantiation logic is well-structured.The function appropriately:
- Fetches latest auth config
- Uses discriminator pattern to determine provider type
- Creates appropriate provider instance per type
- Returns both provider and config to avoid duplicate API calls
The switch statement covers all provider types (OpenShift, K8s, OIDC, AAP, OAuth2) with proper error handling for unknown types.
148-185: Client ID extraction handles all provider types correctly.The function appropriately:
- Uses discriminator to determine provider type
- Extracts client_id from the right spec field per provider
- Returns clear errors for AAP (needs retrieval) and K8s (doesn't use endpoint)
- Validates OpenShift ClientId is non-empty (lines 175-177)
209-263: LGTM! PKCE initialization for GET /login is correct.The code appropriately:
- Extracts provider from query parameter
- Handles token providers differently (no PKCE needed)
- Generates PKCE verifier and challenge
- Stores verifier in cookie
- Embeds verifier in state as fallback
- Returns login redirect URL
PKCE generation failure correctly returns 500 with clear error message (lines 241-246).
264-386: POST /login token exchange flow looks solid.The implementation correctly:
- Handles K8s token providers separately (direct validation)
- Retrieves PKCE verifier from cookie with fallback to state (lines 326-346)
- Requires PKCE verifier (fails with clear error if missing)
- Clears PKCE cookie after use (line 356)
- Exchanges code for token via backend API
- Converts response to internal TokenData
The PKCE verifier retrieval logic with cookie → state fallback is well-designed for resilience.
388-449: Token refresh implementation is correct.The function appropriately:
- Returns 418 if auth disabled
- Parses session cookie for provider and refresh token
- Gets provider instance
- Rejects refresh for K8s token providers (no refresh support)
- Extracts client_id from provider config
- Exchanges refresh token via backend API
- Handles OAuth errors appropriately
480-528: GetUserInfo routing through backend is well-implemented.The function correctly:
- Returns 418 if auth disabled
- Parses session cookie
- Validates provider is specified
- Clears session and returns 401 on missing token/provider
- Routes ALL providers to backend API (line 510-511)
- Clears session on userinfo failure (appropriate auth failure treatment)
- Validates username is non-empty
The unified backend routing simplifies the flow and ensures consistent behavior.
544-598: Logout implementation handles all cases correctly.The function appropriately:
- Returns 418 if auth disabled
- Always clears cookies (lines 555-558, 568-571, 583-584) even on errors
- Calls provider-specific Logout if provider and token available
- Returns redirect URL if provider supplies one
- Uses Clear-Site-Data header for robust cookie clearing
The unconditional cookie clearing ensures logout always succeeds from the proxy perspective.
600-637: LGTM! Auth config retrieval is simplified.The function now returns only
(*v1alpha1.AuthConfig, error), removing the extra URL pointer. This simplifies the signature and aligns with how the config is used throughout the file.proxy/auth/openshift.go (2)
37-128: OpenShift handler initialization is well-designed.The function appropriately:
- Derives apiServerURL from ClusterControlPlaneUrl with fallback to AuthorizationUrl base (lines 40-59)
- Strips
/oauth/authorizepath when deriving from AuthorizationUrl (lines 50-52)- Provides sensible defaults for authURL and tokenURL based on apiServerURL
- Requires ClientId (lines 79-83) - correct, as this is mandatory for OAuth
- Sets default scope "user:full" with override support (lines 90-94)
- Configures OAuth client with TLS (lines 107-114)
The URL derivation logic with fallbacks ensures flexibility while maintaining correctness.
146-195: Logout with discovery is robustly implemented.The function appropriately:
- Discovers logout URL from
.well-known/oauth-authorization-server(line 149)- Uses TLS config for discovery request
- Handles all error cases gracefully (returns empty string, allows logout to proceed)
- Parses issuer from discovery and constructs logout URL
- Falls back to no redirect URL on any failure
The graceful error handling ensures logout always succeeds even if discovery fails.
proxy/auth/common.go (7)
25-38: LGTM! Provider constants and defaults are well-defined.Using backend v1alpha1 constants for provider types (lines 26-31) ensures consistency across the codebase. The default claim constants follow OAuth2/OIDC standards.
40-63: TokenData refactoring correctly separates ID and access tokens.The split into
IDTokenandAccessTokenfields with clear usage documentation (lines 41-51) properly handles the different token types across providers:
- OIDC/K8s: Use IDToken for API auth
- OAuth2/AAP: Use AccessToken for API auth
The
GetAuthToken()method (lines 57-63) provides the correct precedence: IDToken first, then AccessToken.
65-76: Interface updates align with PKCE flow.Adding
CodeVerifiertoLoginParameters(line 67) and updatingAuthProviderinterface methods (lines 72, 75) correctly supports the PKCE authentication flow.
458-512: LGTM! Login redirect with PKCE and state encoding is correct.The function appropriately:
- Generates authorization URL via osincli
- Embeds provider name in state parameter (line 478)
- Optionally encodes code_verifier in state as fallback (lines 479-484)
- Adds PKCE parameters (code_challenge, code_challenge_method) to query (lines 506-507)
- Logs missing code_challenge as an error (lines 499-503)
The dual storage (cookie + state) for code_verifier provides resilience.
665-752: PKCE cookie management is well-implemented.The functions correctly:
- Use provider-specific cookie names (lines 694, 712, 741)
- Set
SameSite=Lax(not Strict) to allow cookies on OAuth redirect (line 700, 749)- Set 10-minute expiry matching authorization code TTL (line 702)
- Log cookie operations for debugging (lines 706-707, 720-721)
- Handle missing cookies gracefully (lines 725-727)
The
SameSite=Laxsetting is crucial for OAuth callback flows.
668-690: PKCE verifier and challenge generation follows RFC 7636.
generateCodeVerifier: Usescrypto/rand(secure randomness), generates 32 bytes, encodes as base64url (lines 671-680)generateCodeChallenge: Uses SHA-256, encodes as base64url (lines 685-689)Both implementations correctly follow RFC 7636 PKCE specification.
761-787: State parameter parsing correctly handles multiple formats.
extractCodeVerifierFromState(lines 764-787) appropriately:
- Validates state starts with expected provider prefix
- Handles format with encoded verifier:
provider:<name>:<base64url>- Handles legacy format without verifier:
provider:<name>- Decodes base64url-encoded verifier with error handling
The backward compatibility ensures smooth transitions.
celdrake
force-pushed
the
EDM-2373-support-multiple-auth-providers
branch
from
6f78b5e to
63acf96
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-components/src/components/form/SwitchField.tsx (1)
6-31: Fix API mismatch: FormGroup useslabelHelp, notlabelIcon.PatternFly's FormGroup component does not support the
labelIconprop—it was renamed tolabelHelp(use the FormGroupLabelHelp element).Line 25 passes
labelIconto FormGroup, but this prop is unsupported and will not work. Update the implementation to uselabelHelpor the FormGroupLabelHelp element as per current PatternFly API.
♻️ Duplicate comments (5)
libs/ui-components/src/components/AuthProvider/TestConnectionModal/TestConnectionModal.tsx (1)
27-40: Handle empty results and use danger severity for invalid rows.As noted in a previous review, when
results = [],every()returnstrueandsome()returnsfalse, showing a false success. Additionally, usingstatus="warning"for invalid rows doesn't clearly convey failure severity.Also applies to: 69-70
proxy/auth/api_client.go (1)
46-62: Temporary debug logging contains sensitive data.As noted in previous review and acknowledged by @celdrake, the logging at these lines includes sensitive authentication material (tokens, auth headers, request/response bodies). This is temporary for debugging purposes and tracked for removal.
Also applies to: 125-147
proxy/auth/aap.go (1)
53-72: AAPAuthHandler wiring is sound; hard-coded clientId remains a known TODOThe AAP handler correctly derives URLs from the new spec (internal vs external), reuses
GetAuthTlsConfig, uses PKCE for token acquisition, and routes logout/refresh through the internal URL andinternalClient. The one notable gap is the hard‑codedclientId := "flightctl-ui"/data.Set("client_id", a.clientId): as you already noted in earlier discussions, this should eventually come from config or the provider spec once the backend exposes it. The current structure (getClienttaking an explicit clientId) makes that easy to swap in later.Also applies to: 78-92, 94-105, 107-135, 137-147, 149-173, 175-181
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx (1)
32-77: Test provider block + radio ids need cleanup before shippingThe “Test provider” section with hard‑coded GitHub/Google presets is very handy during development, but in its current form it will surface in production (blue border, fake client IDs/secrets, etc.). On top of that, both GitHub radios share the same
id="github-test-provider-radio", which is an accessibility issue and can confuse label/ARIA associations.If you plan to keep this helper, I’d strongly suggest:
- Either hiding it behind a dev/feature flag or removing it entirely before release.
- Giving each radio its own stable id (e.g.,
github-test-provider-radio-with-secret/...-without-secret).- Having
onClearTestProviderfully reset the preset state (e.g.,setValues(initialValues)and clear anytestProviderfield) instead of merging{ ...values, ...initialValues }, which currently leaves extra fields liketestProvideruntouched.Also applies to: 129-141, 143-203
proxy/auth/common.go (1)
190-450: exchangeToken PKCE flow is solid, but logging still exposes raw token responsesThe PKCE enforcement, reflection-based extraction of token URL/client ID/redirect URI and TLS config, and flexible parsing of JSON vs form-encoded responses are all reasonable. However, several logs in this function still include the full token response body or parsed values:
- Info log with
Body previewlogs the start of the token JSON/form body.- Warn logs on non-200 responses and JSON/form parse errors log
string(body).- The debug log for form-encoded responses logs the full
url.Values, which will usually includeaccess_token,id_token, and possiblyrefresh_token.These logs leak high-value secrets into log storage and were already flagged in an earlier review; they should not ship into any non-dev environment. Recommend:
- Stop logging the raw body/values; only log non-sensitive metadata (status code, content type, length, and key names).
- Avoid including
string(body)in returned error strings if those errors are ever logged or surfaced.For example:
- log.GetLogger().Warnf("Token exchange failed with status %d. Response body: %s", resp.StatusCode, string(body)) - return ret, nil, fmt.Errorf("token exchange failed with status %d: %s", resp.StatusCode, string(body)) + log.GetLogger().Warnf("Token exchange failed with status %d", resp.StatusCode) + return ret, nil, fmt.Errorf("token exchange failed with status %d", resp.StatusCode) - bodyPreview := string(body) - if len(bodyPreview) > 200 { - bodyPreview = bodyPreview[:200] + "..." - } - log.GetLogger().Infof("Token response Content-Type: %s, Body length: %d, Body preview: %s", contentType, len(body), bodyPreview) + log.GetLogger().Infof("Token response Content-Type: %s, Body length: %d", contentType, len(body)) - log.GetLogger().Warnf("Failed to parse form-encoded token response. Status: %d, Content-Type: %s, Body: %s", resp.StatusCode, contentType, string(body)) + log.GetLogger().Warnf("Failed to parse form-encoded token response. Status: %d, Content-Type: %s, Body redacted", resp.StatusCode, contentType) - log.GetLogger().Warnf("Failed to parse token response as JSON. Status: %d, Content-Type: %s, Body: %s", resp.StatusCode, contentType, string(body)) + log.GetLogger().Warnf("Failed to parse token response as JSON. Status: %d, Content-Type: %s, Body redacted", resp.StatusCode, contentType) - log.GetLogger().Debugf("Parsed form-encoded response values: %v", values) + log.GetLogger().Debugf("Parsed form-encoded response keys: %v", getMapKeys(tokenResponse))(and similarly for any other logs that include the full body or token-bearing values).
🧹 Nitpick comments (13)
proxy/bridge/handler.go (1)
37-58: 401 handling split between general and alerts proxy looks correct; duplication is acceptableThe new response handling does what the comments describe:
- For the main reverse proxy, a 401 now triggers
Clear-Site-Data: "cookies"plus a debug log, which is an appropriate way to force session cookie cleanup when the backend token has expired.- For the alerts proxy, using a dedicated
createAlertsReverseProxyand converting 401 → 501 avoids logging the user out when Alertmanager returns 401 instead of 403, while still clearly signaling “feature not implemented/disabled” to the frontend.Functionally this looks solid. There is some duplication between
createReverseProxyandcreateAlertsReverseProxy(URL parsing, CORS header stripping) that could be factored into a shared helper, but that’s purely an optional clean‑up and not required for this PR.Also applies to: 60-89, 111-119
apps/standalone/src/app/utils/apiCalls.ts (1)
58-60: Login redirect centralization looks good; consider mirroring the AuthContext guardThe synchronous
redirectToLoginand unconditional 401 redirect inhandleApiJSONResponseare fine. If any API calls are made from the/loginroute, you may want to add the samewindow.location.pathname !== '/login'guard used inAuthContextto avoid redundant navigations, but this is optional.Also applies to: 73-77
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (1)
29-47: Label helpers work; consider future-proofing provider labels
getAssignmentTypeLabel/getProviderTypeLabelare straightforward and correctly wired to i18n for the known types. Today, anything outsideOIDC/OAuth2falls back to'N/A'. IfProviderTypegrows (e.g., additional dynamic providers), it would be safer to either:
- Narrow the type used here to just the dynamic provider subset, or
- Add explicit cases for new provider types instead of relying on the default.
This keeps the UI from silently showing
'N/A'for newly introduced provider kinds.Also applies to: 49-61
libs/ui-components/src/components/Login/TokenLoginForm.tsx (1)
35-35: Remove duplicate function definition.The
nowInSecondsfunction is already exported fromlibs/ui-components/src/utils/k8sProvider.ts(line 10). Import it instead of redefining it here.Apply this diff:
import { isValidJwtTokenFormat } from '../../utils/k8sProvider'; +import { nowInSeconds } from '../../utils/k8sProvider'; import { useTranslation } from '../../hooks/useTranslation'; import { useFetch } from '../../hooks/useFetch'; import { getErrorMessage } from '../../utils/error'; import FlightCtlForm from '../../components/form/FlightCtlForm'; const EXPIRATION = 'expiration'; -const nowInSeconds = () => Math.floor(Date.now() / 1000); - type TokenLoginFormProps = {apps/standalone/src/app/components/Login/LoginPage.tsx (1)
15-27: Consider adding credentials option for robustness.The response validation has been properly implemented addressing the previous review feedback. While the backend uses
SameSite: Laxcookies (makingcredentials: 'include'not strictly necessary for same-origin), adding it would improve robustness across different deployment scenarios.Apply this diff if desired:
const redirectToProviderLogin = async (provider: AuthProvider) => { - const response = await fetch(`${loginAPI}?provider=${provider.metadata.name}`); + const response = await fetch(`${loginAPI}?provider=${provider.metadata.name}`, { + credentials: 'include', + }); if (!response.ok) {libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProvider.tsx (1)
29-37: Solid create/edit flow; minor UX polish around title/edit stateThe create/edit wiring, loading/error states, and permission gating all look good. One small UX nit: the page title (and final breadcrumb item) is driven by
!!authProviderDetails, so on an edit route you'll briefly see “Add authentication provider” until the fetch completes, and you’ll also see the “Add” title if the fetch fails, even though the user is clearly on an edit URL. You might consider basing the title onauthProviderId(or an explicitisEditflag) and using the fetched data only for display values, so the mode is unambiguous even on errors or slow loads.Also applies to: 55-88, 89-101, 121-137
proxy/auth/oauth2.go (1)
25-56: OAuth2AuthHandler aligns well with PKCE/BFF modelThe handler setup (required-field validation, TLS via
GetAuthTlsConfig, osincli client config with PKCE and no client secret, and delegating user info to the API server) is consistent with the rest of the auth stack and looks correct. If you want more actionable operator errors, you could split the combined required-field check into per‑field validations so the message can say exactly which field is missing, but that’s purely QoL.Also applies to: 63-79, 81-103
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx (2)
264-281: Form action wiring looks good; consider richer test payload if backend needs it
CreateAuthProviderFormContentnicely centralizes submit vs. test‑connection behavior, with sensible disabled states for the primary/test buttons (dirtygating for new providers, etc.). The test‑connection request currently only sends a minimal subset of fields (providerType,issuer, auth URLs,clientId); if the backend ever needsscopes,clientSecret, or organization/role mapping to perform a more realistic connectivity/claims check, this would be the place to extend the payload. For now the structure is clean and easy to evolve.Also applies to: 282-316, 324-341
351-380: Patch path ignores server response; consider returning updated provider to onSuccessIn the edit path you compute patches and call:
await patch<AuthProvider>(`authproviders/${authProvider.metadata.name}`, patches); onSuccess(authProvider);Given
patchis already typed to return anAuthProvider, you could propagate the updated object:const updated = await patch<AuthProvider>(`authproviders/${authProvider.metadata.name}`, patches); onSuccess(updated);Right now
onSuccessonly usesmetadata.name, so this is harmless, but passing the fresh object makes the component more future‑proof if callers later want to reflect updated spec/displayName without a re‑fetch.proxy/auth/auth.go (1)
27-51: AuthHandler initialization and config retrieval look correct, with room for future optimizationStoring
apiTlsConfigand an initialauthConfigDatasnapshot while havinggetProviderInstancealways callgetAuthInfokeeps the “auth enabled/disabled” flag cheap to check while ensuring provider definitions are fresh per request. The TLS‑backedgetAuthInfoimplementation is straightforward and handles the418 Teapot“auth disabled” case cleanly. If this endpoint ever becomes high‑traffic, you might consider a small cache/TTL forauthConfigto avoid repeated unmarshals on bursts of logins, but there’s no correctness issue as written.Also applies to: 600-637
proxy/auth/openshift.go (1)
146-195: Consider adding timeout/context to logout discovery HTTP client
Logoutuses a freshhttp.Clientwith a customTransportbut noTimeoutor context, so a slow or hung.well-known/oauth-authorization-serverendpoint could tie up the calling goroutine longer than necessary. Consider reusing a shared client with a sane timeout or wiring acontext.Contextfrom the handler so this remote call is bounded.proxy/auth/common.go (2)
25-38: Provider/claim constants are good; could be true constantsCentralising provider type strings and default claim names is helpful. As a minor hardening step, you could switch the provider type vars to
const(or a dedicated typed const) to prevent accidental runtime mutation, but this is non-blocking.
668-708: PKCE cookies and Clear-Site-Data behaviorThe PKCE helper functions look solid overall:
generateCodeVerifier/generateCodeChallengefollow RFC 7636 (base64url, no padding).- PKCE verifier cookies are HttpOnly, SameSite=Lax (needed for redirects), and time-bound to 10 minutes.
- Logging only records cookie names and verifier length, not the verifier itself.
For
clearSessionCookie, note that relying solely onClear-Site-Data: "cookies"may behave differently across browsers and typically requires a secure context; if you need deterministic logout semantics, you may eventually want to resurrect explicit cookie clearing alongside this header once EDM-2612 is resolved.Also applies to: 710-737, 739-759
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
proxy/go.sumis excluded by!**/*.sum
📒 Files selected for processing (76)
CONFIGURATION.md(0 hunks)apps/ocp-plugin/src/components/AppContext/AppContext.tsx(1 hunks)apps/ocp-plugin/src/utils/apiCalls.ts(1 hunks)apps/standalone/src/app/components/Login/LoginPage.tsx(1 hunks)apps/standalone/src/app/components/Login/LoginPageLayout.tsx(1 hunks)apps/standalone/src/app/context/AuthContext.ts(5 hunks)apps/standalone/src/app/routes.tsx(4 hunks)apps/standalone/src/app/utils/apiCalls.ts(3 hunks)libs/ansible/src/const.ts(1 hunks)libs/i18n/locales/en/translation.json(7 hunks)libs/types/index.ts(4 hunks)libs/types/models/AppType.ts(1 hunks)libs/types/models/ApplicationPort.ts(1 hunks)libs/types/models/ApplicationResourceLimits.ts(1 hunks)libs/types/models/ApplicationResources.ts(1 hunks)libs/types/models/ApplicationVolume.ts(1 hunks)libs/types/models/AuthDynamicRoleAssignment.ts(1 hunks)libs/types/models/AuthProviderSpec.ts(1 hunks)libs/types/models/ImageApplicationProviderSpec.ts(1 hunks)libs/types/models/ImageMountVolumeProviderSpec.ts(1 hunks)libs/types/models/K8sProviderSpec.ts(0 hunks)libs/types/models/MountVolumeProviderSpec.ts(1 hunks)libs/types/models/OpenShiftProviderSpec.ts(1 hunks)libs/types/models/TokenRequest.ts(1 hunks)libs/types/models/TokenResponse.ts(1 hunks)libs/types/models/UserInfoResponse.ts(1 hunks)libs/types/models/VolumeMount.ts(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetails.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetailsTab.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderDetails/DeleteAuthProviderModal.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderDetails/RoleAssigmentDetails.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderRow.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/AuthOrganizationAssignment.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/AuthProviderHelperText.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProvider.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/Oauth2ProviderFields.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/RoleAssignmentSection.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts(1 hunks)libs/ui-components/src/components/AuthProvider/TestConnectionModal/TestConnectionModal.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/useAuthProviders.ts(1 hunks)libs/ui-components/src/components/DetailsPage/DetailsPage.tsx(1 hunks)libs/ui-components/src/components/Login/ProviderSelector.tsx(1 hunks)libs/ui-components/src/components/Login/TokenLoginForm.tsx(1 hunks)libs/ui-components/src/components/OverviewPage/Cards/Alerts/AlertsCard.tsx(1 hunks)libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx(3 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(4 hunks)libs/ui-components/src/components/form/ListItemField.tsx(1 hunks)libs/ui-components/src/components/form/NameField.tsx(2 hunks)libs/ui-components/src/components/form/SwitchField.tsx(2 hunks)libs/ui-components/src/hooks/useAlertsEnabled.ts(1 hunks)libs/ui-components/src/hooks/useAppContext.tsx(1 hunks)libs/ui-components/src/hooks/useNavigate.tsx(2 hunks)libs/ui-components/src/types/extraTypes.ts(2 hunks)libs/ui-components/src/types/rbac.ts(1 hunks)libs/ui-components/src/utils/authProvider.ts(1 hunks)libs/ui-components/src/utils/k8sProvider.ts(1 hunks)libs/ui-components/src/utils/patch.ts(1 hunks)proxy/app.go(1 hunks)proxy/auth/aap.go(6 hunks)proxy/auth/api_client.go(1 hunks)proxy/auth/auth.go(7 hunks)proxy/auth/common.go(3 hunks)proxy/auth/k8s.go(4 hunks)proxy/auth/oauth.go(0 hunks)proxy/auth/oauth2.go(1 hunks)proxy/auth/oidc.go(9 hunks)proxy/auth/openshift.go(1 hunks)proxy/bridge/handler.go(2 hunks)proxy/bridge/testauth.go(1 hunks)proxy/config/config.go(0 hunks)proxy/go.mod(1 hunks)proxy/middleware/auth.go(1 hunks)proxy/middleware/organization.go(1 hunks)
💤 Files with no reviewable changes (4)
- proxy/config/config.go
- libs/types/models/K8sProviderSpec.ts
- proxy/auth/oauth.go
- CONFIGURATION.md
🚧 Files skipped from review as they are similar to previous changes (33)
- libs/types/models/VolumeMount.ts
- proxy/middleware/organization.go
- libs/ui-components/src/utils/patch.ts
- proxy/app.go
- libs/ui-components/src/components/OverviewPage/Cards/Alerts/AlertsCard.tsx
- libs/types/models/TokenResponse.ts
- libs/ui-components/src/components/AuthProvider/useAuthProviders.ts
- libs/ui-components/src/components/form/NameField.tsx
- libs/ui-components/src/components/AuthProvider/AuthProviderDetails/RoleAssigmentDetails.tsx
- libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx
- libs/ui-components/src/hooks/useAlertsEnabled.ts
- libs/ui-components/src/components/AuthProvider/AuthProviderDetails/DeleteAuthProviderModal.tsx
- libs/ui-components/src/components/AuthProvider/CreateAuthProvider/Oauth2ProviderFields.tsx
- libs/ui-components/src/components/form/ListItemField.tsx
- libs/types/models/AuthProviderSpec.ts
- libs/types/models/MountVolumeProviderSpec.ts
- libs/types/models/AppType.ts
- apps/ocp-plugin/src/components/AppContext/AppContext.tsx
- libs/ui-components/src/components/AuthProvider/AuthProviderRow.tsx
- libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetailsTab.tsx
- libs/ui-components/src/types/extraTypes.ts
- libs/ansible/src/const.ts
- libs/types/models/ApplicationResources.ts
- libs/ui-components/src/components/Login/ProviderSelector.tsx
- libs/ui-components/src/utils/authProvider.ts
- libs/types/models/UserInfoResponse.ts
- libs/types/models/ImageMountVolumeProviderSpec.ts
- libs/ui-components/src/utils/k8sProvider.ts
- libs/types/models/ApplicationResourceLimits.ts
- libs/types/models/TokenRequest.ts
- libs/ui-components/src/hooks/useAppContext.tsx
- proxy/bridge/testauth.go
- proxy/middleware/auth.go
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
📚 Learning: 2025-02-18T12:04:42.579Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
Applied to files:
libs/ui-components/src/components/Login/TokenLoginForm.tsx
📚 Learning: 2025-02-17T08:52:50.747Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 218
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx:109-111
Timestamp: 2025-02-17T08:52:50.747Z
Learning: When wrapping a Button component (like `deleteAction`) inside another Button in React/PatternFly, use `component="a"` on the outer Button to avoid accessibility issues caused by button nesting. Include `tabIndex={0}` to ensure proper keyboard navigation.
Applied to files:
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx
📚 Learning: 2025-11-17T15:53:01.194Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 376
File: libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx:274-275
Timestamp: 2025-11-17T15:53:01.194Z
Learning: In PR #376 (flightctl/flightctl-ui), the permission system was updated to fetch all user permissions once at login via the Flight Control API. The `checkPermissions` function from `usePermissionsContext` is a pure calculation on cached permission arrays, not an async fetch, so loading state handling is generally not required at individual component call sites.
Applied to files:
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsxlibs/ui-components/src/components/common/PageNavigation.tsx
📚 Learning: 2025-10-29T16:47:29.614Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
Applied to files:
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsxproxy/auth/api_client.golibs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.tsproxy/auth/common.go
📚 Learning: 2025-02-17T08:41:57.993Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-02-17T08:41:57.993Z
Learning: All files in `libs/types/models` are auto-generated using OpenAPI Generator from the schema defined in https://github.com/flightctl/flightctl/blob/main/api/v1alpha1/openapi.yaml. Changes to these files should be made in the schema instead of modifying the generated files directly.
Applied to files:
proxy/go.modproxy/auth/api_client.go
📚 Learning: 2025-04-10T12:22:38.239Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 266
File: libs/ansible/src/const.ts:20-20
Timestamp: 2025-04-10T12:22:38.239Z
Learning: In the flightctl-ui project, `appRoutes` is defined as a `Record<ROUTE, string>` across multiple applications, requiring all applications to define entries for all route enum values, even when certain routes (like COMMAND_LINE_TOOLS) are only meant to be used in specific applications (like Standalone). For routes not applicable to a particular application, placeholder values are used with explanatory comments.
Applied to files:
apps/standalone/src/app/routes.tsxlibs/ui-components/src/hooks/useNavigate.tsx
📚 Learning: 2025-03-20T12:34:33.199Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/types/models/FileSpec.ts:8-8
Timestamp: 2025-03-20T12:34:33.199Z
Learning: Files in the types directory are autogenerated from the OpenAPI spec in the flightctl project and should not be directly modified.
Applied to files:
proxy/auth/api_client.go
📚 Learning: 2025-06-04T06:41:15.969Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 308
File: libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts:204-204
Timestamp: 2025-06-04T06:41:15.969Z
Learning: In the flightctl-ui codebase, volume functionality is not yet implemented in the backend. TODO comments related to volume preservation (like in toAPIApplication function) are intentional placeholders for when backend volume support is added.
Applied to files:
proxy/auth/api_client.go
📚 Learning: 2025-10-21T13:53:29.934Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 357
File: proxy/auth/auth.go:228-237
Timestamp: 2025-10-21T13:53:29.934Z
Learning: In proxy/auth/auth.go, the token cleanup cycles in `startTokenCleanup` and `cleanupExpiredTokens` are designed for memory management of unretrieved tokens, not as a primary security control. The single-use deletion in `getSingleUseApiTokenMapping` provides the security guarantee. TTL enforcement on retrieval is intentionally not implemented because tokens are expected to be retrieved immediately after creation.
Applied to files:
proxy/auth/api_client.go
📚 Learning: 2025-09-17T12:24:09.201Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 346
File: libs/ui-components/src/components/common/OrganizationGuard.tsx:54-62
Timestamp: 2025-09-17T12:24:09.201Z
Learning: In the Flight Control UI proxy, organization feature detection uses a 501 (Not Implemented) status code to explicitly signal when organizations are disabled via the ORGANIZATIONS_ENABLED setting. Any other status code (including errors like 404, 500) should be treated as organizations being enabled. The logic `organizationsEnabled = response.status !== 501` is correct and more resilient than checking `response.ok`.
Applied to files:
apps/ocp-plugin/src/utils/apiCalls.tsapps/standalone/src/app/utils/apiCalls.ts
📚 Learning: 2025-11-20T07:33:34.656Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/utils/authProvider.ts:5-18
Timestamp: 2025-11-20T07:33:34.656Z
Learning: In the flightctl-ui repository, for AuthProvider objects, the metadata.name field is always present at runtime as it serves as the ID of the authprovider. The OpenAPI schema incorrectly defines it as optional (name?: string in ObjectMeta), but the type assertion `as string` is appropriate when accessing provider.metadata.name since it's guaranteed to exist.
Applied to files:
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.tslibs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts
📚 Learning: 2025-03-24T12:33:54.002Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: proxy/auth/oidc.go:17-19
Timestamp: 2025-03-24T12:33:54.002Z
Learning: OIDC (OpenID Connect) providers return usernames in the `preferred_username` field, as implemented in the OIDCUserInfo struct in proxy/auth/oidc.go.
Applied to files:
proxy/auth/oidc.goproxy/auth/auth.goproxy/auth/common.go
📚 Learning: 2025-10-10T11:35:04.458Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-10-10T11:35:04.458Z
Learning: In the flightctl-ui repository, TypeScript types are generated using hey-api/openapi-ts and output to a single index.ts file (previously used openapi-typescript-codegen with one file per type under /libs/types/models).
Applied to files:
libs/types/index.ts
🧬 Code graph analysis (29)
libs/ui-components/src/components/Login/TokenLoginForm.tsx (4)
libs/ui-components/src/utils/k8sProvider.ts (2)
nowInSeconds(11-11)isValidJwtTokenFormat(2-9)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/organizationStorage.ts (1)
ORGANIZATION_STORAGE_KEY(1-1)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProvider.tsx (5)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useNavigate.tsx (2)
useNavigate(49-72)Link(86-99)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(123-123)libs/ui-components/src/components/common/PermissionsContext.tsx (1)
usePermissionsContext(105-111)
libs/types/models/ImageApplicationProviderSpec.ts (3)
libs/types/models/ApplicationVolumeProviderSpec.ts (1)
ApplicationVolumeProviderSpec(6-11)libs/types/models/ApplicationPort.ts (1)
ApplicationPort(8-8)libs/types/models/ApplicationResources.ts (1)
ApplicationResources(9-11)
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx (5)
libs/ui-components/src/components/Table/Table.tsx (1)
ApiSortTableColumn(16-24)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useNavigate.tsx (1)
useNavigate(49-72)libs/ui-components/src/components/common/PermissionsContext.tsx (1)
usePermissionsContext(105-111)libs/ui-components/src/components/AuthProvider/useAuthProviders.ts (1)
useAuthProviders(4-18)
libs/ui-components/src/components/AuthProvider/TestConnectionModal/TestConnectionModal.tsx (3)
proxy/bridge/testauth.go (1)
FieldValidationResult(32-37)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (1)
FieldValidationResult(91-96)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
libs/types/models/ApplicationPort.ts (1)
libs/types/index.ts (1)
ApplicationPort(10-10)
libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetails.tsx (5)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(123-123)libs/ui-components/src/hooks/useFetchPeriodically.ts (1)
useFetchPeriodically(8-79)libs/ui-components/src/hooks/useNavigate.tsx (2)
useNavigate(49-72)Route(45-45)libs/ui-components/src/components/common/PermissionsContext.tsx (1)
usePermissionsContext(105-111)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProviderForm.tsx (5)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (2)
AuthProviderFormValues(56-82)FieldValidationResult(91-96)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (4)
getInitValues(63-151)authProviderSchema(471-548)getAuthProviderPatches(433-469)getAuthProvider(288-326)
libs/ui-components/src/components/common/PageNavigation.tsx (4)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useNavigate.tsx (1)
useNavigate(49-72)libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)libs/ui-components/src/components/common/PermissionsContext.tsx (1)
usePermissionsContext(105-111)
apps/standalone/src/app/routes.tsx (1)
apps/standalone/src/app/context/AuthContext.ts (1)
AuthContext(21-26)
apps/standalone/src/app/components/Login/LoginPageLayout.tsx (1)
libs/ui-components/src/hooks/useThemePreferences.ts (1)
updateThemeClass(33-39)
apps/standalone/src/app/components/Login/LoginPage.tsx (5)
libs/types/models/AuthProvider.ts (1)
AuthProvider(10-21)apps/standalone/src/app/utils/apiCalls.ts (1)
loginAPI(17-17)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/authProvider.ts (1)
getProviderDisplayName(5-20)libs/types/models/AuthConfig.ts (1)
AuthConfig(6-23)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/AuthOrganizationAssignment.tsx (2)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (1)
AuthProviderFormValues(56-82)
libs/types/models/ApplicationVolume.ts (1)
libs/types/models/ImageVolumeProviderSpec.ts (1)
ImageVolumeProviderSpec(6-8)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/RoleAssignmentSection.tsx (5)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/form/FieldHelperText.tsx (1)
DefaultHelperText(17-25)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (1)
AuthProviderFormValues(56-82)libs/ui-components/src/components/common/WithHelperText.tsx (1)
FormGroupWithHelperText(51-64)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/AuthProviderHelperText.tsx (2)
RoleClaimHelperText(45-65)RoleSeparatorHelperText(67-78)
proxy/auth/oauth2.go (3)
proxy/auth/common.go (3)
AuthProvider(70-76)LoginParameters(65-68)TokenData(40-55)proxy/bridge/common.go (1)
GetAuthTlsConfig(40-65)proxy/config/config.go (1)
BaseUiUrl(16-16)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/AuthProviderHelperText.tsx (1)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)
proxy/auth/api_client.go (6)
libs/types/models/TokenRequest.ts (1)
TokenRequest(8-37)libs/types/models/TokenResponse.ts (1)
TokenResponse(8-33)proxy/config/config.go (1)
FctlApiUrl(10-10)proxy/log/log.go (1)
GetLogger(17-19)libs/types/models/UserInfoResponse.ts (1)
UserInfoResponse(9-30)proxy/auth/common.go (1)
TokenData(40-55)
proxy/bridge/handler.go (2)
proxy/log/log.go (1)
GetLogger(17-19)proxy/config/config.go (1)
AlertManagerApiUrl(13-13)
apps/standalone/src/app/context/AuthContext.ts (1)
apps/standalone/src/app/utils/apiCalls.ts (2)
loginAPI(17-17)redirectToLogin(58-60)
proxy/auth/k8s.go (2)
proxy/auth/common.go (2)
TokenData(40-55)AuthProvider(70-76)proxy/bridge/common.go (1)
GetTlsConfig(13-38)
libs/types/models/OpenShiftProviderSpec.ts (1)
libs/types/index.ts (1)
OpenShiftProviderSpec(143-143)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (2)
libs/types/models/AuthDynamicRoleAssignment.ts (1)
AuthDynamicRoleAssignment(8-21)proxy/bridge/testauth.go (2)
FieldValidation(17-21)FieldValidationResult(32-37)
proxy/auth/oidc.go (2)
proxy/auth/common.go (2)
AuthProvider(70-76)TokenData(40-55)proxy/config/config.go (2)
IsOrganizationsEnabled(36-38)BaseUiUrl(16-16)
proxy/auth/openshift.go (3)
proxy/auth/common.go (3)
AuthProvider(70-76)LoginParameters(65-68)TokenData(40-55)proxy/bridge/common.go (1)
GetAuthTlsConfig(40-65)proxy/config/config.go (1)
BaseUiUrl(16-16)
proxy/auth/aap.go (3)
proxy/auth/common.go (2)
AuthProvider(70-76)TokenData(40-55)proxy/bridge/common.go (1)
GetAuthTlsConfig(40-65)proxy/config/config.go (1)
BaseUiUrl(16-16)
proxy/auth/auth.go (7)
proxy/auth/common.go (8)
AuthProvider(70-76)ProviderTypeOpenShift(31-31)ProviderTypeK8s(27-27)ProviderTypeOIDC(28-28)ProviderTypeAAP(29-29)ProviderTypeOAuth2(30-30)LoginParameters(65-68)ParseSessionCookie(95-120)libs/types/models/AuthProvider.ts (1)
AuthProvider(10-21)libs/types/models/AuthConfig.ts (1)
AuthConfig(6-23)proxy/auth/k8s.go (2)
TokenAuthProvider(17-21)TokenLoginParameters(23-25)proxy/config/config.go (1)
BaseUiUrl(16-16)libs/types/models/TokenRequest.ts (1)
TokenRequest(8-37)libs/types/models/TokenResponse.ts (1)
TokenResponse(8-33)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (6)
libs/types/index.ts (8)
AuthOrganizationAssignment(23-23)AuthRoleAssignment(28-28)AuthProviderSpec(27-27)AuthProvider(25-25)AuthStaticRoleAssignment(30-30)AuthDynamicRoleAssignment(22-22)PatchRequest(147-147)OAuth2ProviderSpec(139-139)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (8)
AuthProviderFormValues(56-82)DEFAULT_ROLE_SEPARATOR(23-23)isOrgAssignmentDynamic(35-37)isOrgAssignmentPerUser(39-41)isRoleAssignmentStatic(48-50)isRoleAssignmentDynamic(52-54)isOAuth2Provider(28-29)isOrgAssignmentStatic(31-33)libs/ui-components/src/types/extraTypes.ts (1)
DynamicAuthProviderSpec(82-82)libs/types/models/AuthDynamicRoleAssignment.ts (1)
AuthDynamicRoleAssignment(8-21)libs/ui-components/src/utils/patch.ts (1)
appendJSONPatch(23-54)libs/ui-components/src/components/form/validations.ts (1)
validKubernetesDnsSubdomain(151-166)
proxy/auth/common.go (2)
proxy/log/log.go (1)
GetLogger(17-19)proxy/config/config.go (2)
BaseUiUrl(16-16)TlsCertPath(15-15)
🪛 Biome (2.1.2)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts
[error] 490-490: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 497-497: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Lint
- GitHub Check: Build ocp plugin
- GitHub Check: Build
| // loginRedirect generates the OAuth login redirect URL with provider state and PKCE parameters | ||
| // If codeChallenge is empty, it will be generated automatically | ||
| // codeVerifier can be optionally passed to encode it in the state parameter as a fallback | ||
| func loginRedirect(client *osincli.Client, providerName string, codeChallenge string, codeVerifier string) string { | ||
| authorizeRequest := client.NewAuthorizeRequest(osincli.CODE) | ||
| authURL := authorizeRequest.GetAuthorizeUrl() | ||
|
|
||
| // Log the redirect_uri used in authorization request for debugging | ||
| // The redirect_uri is included in the authorization URL query parameters | ||
| parsedAuthURL, _ := url.Parse(authURL.String()) | ||
| redirectURI := parsedAuthURL.Query().Get("redirect_uri") | ||
| if redirectURI == "" { | ||
| // Fallback: use the configured redirect URI from config | ||
| redirectURI = config.BaseUiUrl + "/callback" | ||
| } | ||
| log.GetLogger().Infof("Authorization request for provider %s: redirect_uri=%s", providerName, redirectURI) | ||
|
|
||
| // Include provider name in state parameter so callback can identify which provider to use | ||
| // Format: "provider:<providerName>" to match frontend expectations | ||
| // If codeVerifier is provided, encode it in state as a fallback if cookie fails | ||
| state := fmt.Sprintf("provider:%s", providerName) | ||
| if codeVerifier != "" { | ||
| // Encode code_verifier in state as base64url for fallback retrieval | ||
| // Format: "provider:<providerName>:<base64url_encoded_verifier>" | ||
| encodedVerifier := b64.URLEncoding.WithPadding(b64.NoPadding).EncodeToString([]byte(codeVerifier)) | ||
| state = fmt.Sprintf("provider:%s:%s", providerName, encodedVerifier) | ||
| } | ||
|
|
||
| // Parse the URL and add state parameter | ||
| parsedURL, err := url.Parse(authURL.String()) | ||
| if err != nil { | ||
| // If parsing fails, return original URL | ||
| return authURL.String() | ||
| } | ||
|
|
||
| // Add state to query parameters | ||
| query := parsedURL.Query() | ||
| query.Set("state", state) | ||
|
|
||
| // PKCE is required - codeChallenge must be provided | ||
| // Note: codeChallenge should be generated by the caller (auth.go) before calling this function | ||
| if codeChallenge == "" { | ||
| log.GetLogger().Error("PKCE code_challenge is required but not provided - this should not happen") | ||
| // This is a programming error - codeChallenge should always be provided by auth.go | ||
| // The flow will fail when the provider requires PKCE or when token exchange happens | ||
| } | ||
|
|
||
| // Add PKCE parameters (required) | ||
| query.Set("code_challenge", codeChallenge) | ||
| query.Set("code_challenge_method", "S256") | ||
|
|
||
| parsedURL.RawQuery = query.Encode() | ||
|
|
||
| return parsedURL.String() | ||
| } |
There was a problem hiding this comment.
Encoding code_verifier into the state parameter weakens PKCE’s protection
loginRedirect optionally embeds the raw code_verifier (base64url-encoded) into the state parameter (provider:<name>:<encoded_verifier>), and extractCodeVerifierFromState decodes it. Because state travels over the front channel and is returned alongside the authorization code, any attacker who can intercept the redirect (the attack PKCE is meant to mitigate) will now have both code and the code_verifier, effectively defeating PKCE for those flows.
Given you already store the verifier in an HttpOnly, SameSite=Lax cookie, I’d strongly recommend:
- Removing the verifier-embedding in
stateentirely, and relying solely on the cookie for verifier storage; or - Gating this fallback behind a dedicated debug flag and never enabling it in production, and ensuring
stateis never logged or sent to third parties.
As-is, this is a meaningful security regression relative to a standard PKCE setup.
Also applies to: 761-783
🤖 Prompt for AI Agents
In proxy/auth/common.go around lines 458-512 (and also address similar logic at
761-783): the code embeds the code_verifier into the front-channel state
parameter (provider:<name>:<encoded_verifier>), which defeats PKCE if an
attacker intercepts the redirect; remove the verifier-embedding from state
entirely and rely solely on the HttpOnly, SameSite cookie for storing the
code_verifier, ensuring state only contains provider identification (e.g.,
"provider:<name>"); update any code-paths that parse state (e.g.,
extractCodeVerifierFromState) to no longer expect or decode a verifier from
state and instead read the verifier from the cookie, and remove or guard any
logging that might expose state values.
There was a problem hiding this comment.
I'll be looking at this in a follow-up
There was a problem hiding this comment.
@celdrake, understood! Since this is a security concern that will be addressed later, would you like me to create a GitHub issue to track this follow-up work? I can include:
- A summary of the PKCE security concern (code_verifier in state parameter)
- Reference to this PR and comment thread
- Recommendation to rely solely on the HttpOnly cookie mechanism
Just let me know if you'd like me to open an issue for tracking.
🧠 Learnings used
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
There was a problem hiding this comment.
I already created a task for it.
There was a problem hiding this comment.
@celdrake, perfect—thank you for tracking it! I appreciate you ensuring this security concern is captured for future work. 🐰
🧠 Learnings used
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
There was a problem hiding this comment.
@ori-amizur, absolutely! Here's a concrete fix to address the PKCE security issue:
Changes needed in proxy/auth/common.go:
1. Update loginRedirect function (remove verifier embedding in state):
func loginRedirect(client *osincli.Client, providerName string, codeChallenge string, codeVerifier string) string {
authorizeRequest := client.NewAuthorizeRequest(osincli.CODE)
authURL := authorizeRequest.GetAuthorizeUrl()
// Log the redirect_uri used in authorization request for debugging
parsedAuthURL, _ := url.Parse(authURL.String())
redirectURI := parsedAuthURL.Query().Get("redirect_uri")
if redirectURI == "" {
redirectURI = config.BaseUiUrl + "/callback"
}
log.GetLogger().Infof("Authorization request for provider %s: redirect_uri=%s", providerName, redirectURI)
// Include provider name in state parameter so callback can identify which provider to use
// Format: "provider:<providerName>"
// NOTE: We do NOT encode code_verifier in state for security reasons.
// The verifier is stored securely in an HttpOnly cookie instead.
state := fmt.Sprintf("provider:%s", providerName)
// Parse the URL and add state parameter
parsedURL, err := url.Parse(authURL.String())
if err != nil {
return authURL.String()
}
// Add state to query parameters
query := parsedURL.Query()
query.Set("state", state)
// PKCE is required - codeChallenge must be provided
if codeChallenge == "" {
log.GetLogger().Error("PKCE code_challenge is required but not provided - this should not happen")
}
// Add PKCE parameters (required)
query.Set("code_challenge", codeChallenge)
query.Set("code_challenge_method", "S256")
parsedURL.RawQuery = query.Encode()
return parsedURL.String()
}2. Simplify extractCodeVerifierFromState (no longer extract verifier):
// extractProviderNameFromState extracts provider name from state parameter
// State format: "provider:<providerName>"
func extractProviderNameFromState(state string) string {
prefix := "provider:"
if !strings.HasPrefix(state, prefix) {
return ""
}
return strings.TrimPrefix(state, prefix)
}3. Update any code that calls extractCodeVerifierFromState to rely only on the cookie:
// In your callback handler (likely in auth.go):
// Remove this:
// codeVerifier := extractCodeVerifierFromState(state, providerName)
// Use only the cookie mechanism:
codeVerifier, err := getPKCEVerifierCookie(r, providerName)
if err != nil || codeVerifier == "" {
// Handle error - verifier cookie not found or invalid
respondWithError(w, http.StatusBadRequest, "PKCE verification failed: code_verifier not found")
return
}Why these changes improve security:
-
Removes front-channel exposure: The
code_verifierno longer travels through the browser's address bar where it can be intercepted, logged, or leaked via Referer headers. -
Relies on secure storage: The HttpOnly, SameSite=Lax cookie provides browser-enforced security that prevents JavaScript access and CSRF attacks.
-
Maintains PKCE integrity: The separation between the public
code_challenge(in authorization request) and privatecode_verifier(in secure cookie) is now properly maintained. -
Cleaner architecture: State parameter now has a single, clear purpose: identifying which provider is being used.
🧠 Learnings used
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
There was a problem hiding this comment.
@coderabbitai in my code base the function extractProviderNameFromState doesn't exist. Where did you take it from ?
There was a problem hiding this comment.
@ori-amizur, great catch! I apologize for the confusion. extractProviderNameFromState doesn't currently exist in your codebase—I was proposing it as a new function to replace the current extractCodeVerifierFromState function as part of the security fix.
Here's the current situation in your codebase (proxy/auth/common.go lines 761-783):
// extractCodeVerifierFromState extracts code_verifier from state parameter if it was encoded there
// State format: "provider:<providerName>:<base64url_encoded_verifier>"
// Also handles legacy format: "provider:<providerName>" (no verifier)
func extractCodeVerifierFromState(state string, providerName string) string {
prefix := fmt.Sprintf("provider:%s", providerName)
if !strings.HasPrefix(state, prefix) {
return ""
}
// Check if state has the encoded verifier (format: "provider:<name>:<encoded>")
if strings.HasPrefix(state, prefix+":") {
encodedVerifier := strings.TrimPrefix(state, prefix+":")
if encodedVerifier == "" {
return ""
}
// Decode base64url encoded verifier
decoded, err := b64.URLEncoding.WithPadding(b64.NoPadding).DecodeString(encodedVerifier)
if err != nil {
log.GetLogger().Debugf("Failed to decode code_verifier from state: %v", err)
return ""
}
return string(decoded)
}
// Legacy format without verifier
return ""
}Here's what you should actually do:
Option 1: Modify the existing function (simpler approach)
// extractProviderNameFromState extracts provider name from state parameter
// State format: "provider:<providerName>"
// NOTE: We no longer embed code_verifier in state for security reasons
func extractProviderNameFromState(state string) string {
prefix := "provider:"
if !strings.HasPrefix(state, prefix) {
return ""
}
// Extract just the provider name (everything after "provider:")
providerName := strings.TrimPrefix(state, prefix)
// If there was a verifier encoded (legacy format with ":"), take only the first part
if idx := strings.Index(providerName, ":"); idx > 0 {
providerName = providerName[:idx]
}
return providerName
}Then in your callback handler where you currently call extractCodeVerifierFromState, change to:
// Extract provider name from state
providerName := extractProviderNameFromState(state)
if providerName == "" {
respondWithError(w, http.StatusBadRequest, "Invalid state parameter")
return
}
// Get verifier from cookie ONLY (remove any fallback to state)
codeVerifier, err := getPKCEVerifierCookie(r, providerName)
if err != nil || codeVerifier == "" {
respondWithError(w, http.StatusBadRequest, "PKCE verification failed")
return
}The key changes are:
- Rename
extractCodeVerifierFromState→extractProviderNameFromState - Remove the verifier decoding logic (the security issue)
- Update callers to only get the verifier from the cookie, never from state
Does this clarify the approach?
🧠 Learnings used
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
There was a problem hiding this comment.
@coderabbitai could you verify the fix in #384?
Seems correct to me but please check
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch PR #384 details including title, description, and files changed
gh pr view 384 --repo flightctl/flightctl-ui --json title,body,files,state,url
echo "---"
echo "Fetching diff for PR #384..."
gh pr diff 384 --repo flightctl/flightctl-ui
Length of output: 12620
@celdrake, I've reviewed PR #384 and the fix looks excellent! ✅ It correctly addresses all the PKCE security concerns. Here's my verification:
✅ Security Issues Fixed
-
Verifier removed from state parameter: The
loginRedirectfunction no longer embedscode_verifierin the state parameter. State now only contains"provider:<name>"format with a clear security comment. -
Cookie-only mechanism: The callback handler in
auth.gonow relies solely on the cookie to retrieve the verifier, with proper error handling if the cookie is missing. -
Function renamed and simplified:
extractCodeVerifierFromState→extractProviderNameFromStatenow only extracts the provider name and includes legacy format handling. -
Consistent interface changes: All implementations (OIDC, OAuth2, AAP, OpenShift, K8s) updated to remove the
codeVerifierparameter fromGetLoginRedirectURL. -
Frontend compatibility: Added
credentials: 'include'to the fetch request inLoginPage.tsxto ensure cookies are properly sent.
🔍 Minor Observation
There's a small logging inconsistency in proxy/auth/auth.go around line 352:
loginParams.CodeVerifier = codeVerifier
log.GetLogger().Infof("Retrieved PKCE verifier from state parameter for provider %s", providerName)The log message says "from state parameter" but the code actually retrieves it from the cookie now. Consider updating to:
log.GetLogger().Infof("Retrieved PKCE verifier from cookie for provider %s", providerName)This is purely cosmetic and doesn't affect security—just helps with accurate debugging logs.
✅ Overall Assessment
The fix is complete and correct. It successfully:
- Eliminates front-channel exposure of the verifier
- Maintains PKCE security model integrity
- Handles errors gracefully
- Includes backward compatibility for legacy state formats
Great work on the security fix! 🐰
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
🧠 Learnings used
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 376
File: libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx:274-275
Timestamp: 2025-11-17T15:53:01.194Z
Learning: In PR #376 (flightctl/flightctl-ui), the permission system was updated to fetch all user permissions once at login via the Flight Control API. The `checkPermissions` function from `usePermissionsContext` is a pure calculation on cached permission arrays, not an async fetch, so loading state handling is generally not required at individual component call sites.
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.991Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
63acf96 to
18bc90f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx (1)
153-157: Consider reusing CreateAuthProviderButton for consistency.The toolbar button lacks the
VERB.CREATEpermission check thatCreateAuthProviderButton(lines 49-62) provides. While the page itself is LIST-gated, users with LIST but not CREATE permission would see a non-functional button here.Apply this diff to reuse the permission-aware button:
- <ToolbarItem> - <Button variant="primary" onClick={() => navigate(ROUTE.AUTH_PROVIDER_CREATE)}> - {t('Add authentication provider')} - </Button> - </ToolbarItem> + <ToolbarItem> + <CreateAuthProviderButton /> + </ToolbarItem>
🧹 Nitpick comments (3)
proxy/bridge/testauth.go (1)
92-93: Consider checking error return from JSON encoding.While JSON encoding of these structs is unlikely to fail, checking the error improves robustness and makes error scenarios explicit.
Apply this diff:
w.Header().Set("Content-Type", "application/json") -json.NewEncoder(w).Encode(response) +if err := json.NewEncoder(w).Encode(response); err != nil { + log.GetLogger().Errorf("Failed to encode test auth response: %v", err) +}apps/standalone/src/app/components/Login/LoginPageLayout.tsx (1)
6-29: Browser theme hook reuse looks good; consider an optional guard for non‑DOM environmentsUsing
updateThemeClass(document.documentElement, ...)here correctly reuses the shared theming helper and removes the earlier duplication, and the media query listener is wired/cleaned up properly. The only nit is that this assumeswindowandmatchMediaare always available; if this component ever renders in a non‑browser/SSR context, a small guard (e.g., checkingtypeof window !== 'undefined'andwindow.matchMedia) would make it more robust. Based on learningsapps/standalone/src/app/components/Login/LoginPage.tsx (1)
39-63: Consider preserving provider selection UI after redirect failuresWhen a non‑K8s provider redirect fails in
handleProviderSelect, you seterrorand clearuserSelectedProvider; because the render short‑circuits onerror, the user sees only the Alert and loses the provider list and any way to retry without reloading the page.Consider keeping the selector visible with an inline error (or a “Try again” action) so users can switch providers or retry the same one after a transient failure, instead of being forced to reload. For example, you could store a separate
redirectErrorand render it aboveProviderSelectorwhile leaving the mainerrorreserved for fatal config‑load issues.Also applies to: 115-123, 154-160
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/standalone/src/app/components/Login/LoginPage.tsx(1 hunks)apps/standalone/src/app/components/Login/LoginPageLayout.tsx(1 hunks)libs/i18n/locales/en/translation.json(7 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetails.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderRow.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProvider.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts(1 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(4 hunks)libs/ui-components/src/types/extraTypes.ts(2 hunks)libs/ui-components/src/utils/authProvider.ts(1 hunks)proxy/bridge/testauth.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProvider.tsx
- libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts
- libs/ui-components/src/components/AuthProvider/AuthProviderRow.tsx
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
📚 Learning: 2025-11-17T15:53:01.194Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 376
File: libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx:274-275
Timestamp: 2025-11-17T15:53:01.194Z
Learning: In PR #376 (flightctl/flightctl-ui), the permission system was updated to fetch all user permissions once at login via the Flight Control API. The `checkPermissions` function from `usePermissionsContext` is a pure calculation on cached permission arrays, not an async fetch, so loading state handling is generally not required at individual component call sites.
Applied to files:
libs/ui-components/src/components/common/PageNavigation.tsxlibs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx
📚 Learning: 2025-11-20T09:24:41.950Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Applied to files:
libs/ui-components/src/components/common/PageNavigation.tsxlibs/ui-components/src/types/extraTypes.tslibs/ui-components/src/utils/authProvider.tslibs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetails.tsxlibs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsxapps/standalone/src/app/components/Login/LoginPage.tsx
📚 Learning: 2025-11-20T07:33:34.656Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/utils/authProvider.ts:5-18
Timestamp: 2025-11-20T07:33:34.656Z
Learning: In the flightctl-ui repository, for AuthProvider objects, the metadata.name field is always present at runtime as it serves as the ID of the authprovider. The OpenAPI schema incorrectly defines it as optional (name?: string in ObjectMeta), but the type assertion `as string` is appropriate when accessing provider.metadata.name since it's guaranteed to exist.
Applied to files:
libs/ui-components/src/types/extraTypes.tslibs/ui-components/src/utils/authProvider.ts
📚 Learning: 2025-02-17T08:41:57.993Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-02-17T08:41:57.993Z
Learning: All files in `libs/types/models` are auto-generated using OpenAPI Generator from the schema defined in https://github.com/flightctl/flightctl/blob/main/api/v1alpha1/openapi.yaml. Changes to these files should be made in the schema instead of modifying the generated files directly.
Applied to files:
libs/ui-components/src/types/extraTypes.ts
📚 Learning: 2025-03-20T12:34:33.199Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/types/models/FileSpec.ts:8-8
Timestamp: 2025-03-20T12:34:33.199Z
Learning: Files in the types directory are autogenerated from the OpenAPI spec in the flightctl project and should not be directly modified.
Applied to files:
libs/ui-components/src/types/extraTypes.ts
📚 Learning: 2025-10-29T16:47:29.614Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
Applied to files:
libs/ui-components/src/utils/authProvider.tslibs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsxapps/standalone/src/app/components/Login/LoginPage.tsxproxy/bridge/testauth.go
📚 Learning: 2025-02-17T08:52:50.747Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 218
File: libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx:109-111
Timestamp: 2025-02-17T08:52:50.747Z
Learning: When wrapping a Button component (like `deleteAction`) inside another Button in React/PatternFly, use `component="a"` on the outer Button to avoid accessibility issues caused by button nesting. Include `tabIndex={0}` to ensure proper keyboard navigation.
Applied to files:
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx
📚 Learning: 2025-05-29T12:07:34.835Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 304
File: libs/ui-components/src/hooks/useThemePreferences.ts:57-65
Timestamp: 2025-05-29T12:07:34.835Z
Learning: When the application runs as a plugin inside the OCP console (indicated by the presence of `consoleTheme`), it should not call `updateThemeClass` to modify DOM theme classes. The OCP console handles theme application, and the plugin should only read the theme value for internal use like setting component themes.
Applied to files:
apps/standalone/src/app/components/Login/LoginPageLayout.tsx
📚 Learning: 2025-02-18T12:04:42.579Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
Applied to files:
apps/standalone/src/app/components/Login/LoginPage.tsx
🧬 Code graph analysis (7)
libs/ui-components/src/components/common/PageNavigation.tsx (4)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useNavigate.tsx (1)
useNavigate(49-72)libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)libs/ui-components/src/components/common/PermissionsContext.tsx (1)
usePermissionsContext(105-111)
libs/ui-components/src/types/extraTypes.ts (3)
libs/types/models/OIDCProviderSpec.ts (1)
OIDCProviderSpec(10-45)libs/types/models/OAuth2ProviderSpec.ts (1)
OAuth2ProviderSpec(10-57)libs/types/models/AuthProvider.ts (1)
AuthProvider(10-21)
libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetails.tsx (5)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(123-123)libs/ui-components/src/hooks/useFetchPeriodically.ts (1)
useFetchPeriodically(8-79)libs/ui-components/src/hooks/useNavigate.tsx (2)
useNavigate(49-72)Route(45-45)libs/ui-components/src/components/common/PermissionsContext.tsx (1)
usePermissionsContext(105-111)
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx (5)
libs/ui-components/src/components/Table/Table.tsx (1)
ApiSortTableColumn(16-24)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useNavigate.tsx (1)
useNavigate(49-72)libs/ui-components/src/components/common/PermissionsContext.tsx (1)
usePermissionsContext(105-111)libs/ui-components/src/components/AuthProvider/useAuthProviders.ts (1)
useAuthProviders(4-18)
apps/standalone/src/app/components/Login/LoginPageLayout.tsx (1)
libs/ui-components/src/hooks/useThemePreferences.ts (1)
updateThemeClass(33-39)
apps/standalone/src/app/components/Login/LoginPage.tsx (4)
libs/types/models/AuthProvider.ts (1)
AuthProvider(10-21)apps/standalone/src/app/utils/apiCalls.ts (1)
loginAPI(17-17)libs/ui-components/src/utils/authProvider.ts (1)
getProviderDisplayName(5-20)libs/types/models/AuthConfig.ts (1)
AuthConfig(6-23)
proxy/bridge/testauth.go (2)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (2)
FieldValidationResult(91-96)FieldValidation(85-89)proxy/log/log.go (1)
GetLogger(17-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Build ocp plugin
🔇 Additional comments (20)
libs/i18n/locales/en/translation.json (2)
40-180: Comprehensive authentication provider localization coverage.The new keys effectively support the multi-provider authentication flow: CRUD operations (lines 40–50), provider configuration fields including URLs and credentials (lines 52–87), claims and role/organization assignment strategies (lines 63–122), validation messages with specific guidance (lines 157–180), and connection testing results (lines 172–180). The inclusion of localized provider type names (OpenShift, Kubernetes, Ansible Automation Platform at lines 980–982) is appropriate for display purposes, consistent with the design that only OIDC and OAuth2 providers are editable via the form.
719-732: Kubernetes token authentication UI strings properly localized.The new keys for Kubernetes token-based login (lines 719–732) provide clear guidance on token format, security warnings, and authentication state. These align with the multi-provider strategy and cover the non-OIDC/OAuth2 provider flow.
libs/ui-components/src/components/common/PageNavigation.tsx (4)
1-23: LGTM! Imports are well-organized.All new imports (Button, Tooltip, CogIcon, ROUTE, RESOURCE, VERB, usePermissionsContext) are utilized in the implementation and support the new admin settings functionality.
65-69: Permission check correctly determines admin status.The implementation properly derives
isAdminfrom the AUTH_PROVIDER CREATE permission, which aligns with the approach that admin capabilities are determined by auth provider management permissions. The checkPermissions result is safely destructured, and any falsy value will correctly prevent the Settings button from rendering.
72-79: Excellent visibility logic and defensive programming.The optional chaining for
currentOrgDisplayName(line 72) handles missing organization data gracefully. TheshouldShowNavigationlogic (line 75) correctly shows the navigation bar when there's either organization selection or admin settings to display, with a clear explanatory comment. The early return pattern optimizes rendering performance.
98-111: Well-implemented Settings button with proper accessibility.The Settings button implementation follows best practices:
- Proper accessibility with
aria-label(line 103)- Descriptive tooltip explaining the action (line 100)
- Standard CogIcon for settings (line 105)
- Clean navigation to AUTH_PROVIDERS route (line 104)
- Internationalization support via translated text (line 107)
proxy/bridge/testauth.go (4)
96-240: LGTM! OIDC discovery and validation logic is comprehensive.The implementation correctly:
- Enforces issuer requirement for OIDC providers
- Performs SSRF validation on the issuer URL
- Fetches and parses the OIDC discovery document
- Validates required endpoints (authorization, token) and notes optional ones (userinfo)
- Tests endpoint reachability while safely skipping SSRF checks for discovered endpoints (they originate from the validated issuer)
- Provides detailed validation notes for each field
278-293: Verify whether userinfo URL should be required for OAuth2.The current implementation requires the userinfo URL for OAuth2 providers, but standard OAuth2 (RFC 6749) does not mandate a userinfo endpoint. While OpenID Connect defines a userinfo endpoint, it's typically optional. Requiring this field may be unnecessarily strict and could prevent valid OAuth2 configurations.
Please confirm whether this is intentional for your use case, or if the userinfo URL should be optional for OAuth2 providers (similar to how it's treated as optional in the OIDC discovery validation at line 193-195).
307-344: LGTM! SSRF protections are well-implemented.The implementation provides solid defense-in-depth by:
- Blocking localhost and *.localhost hostnames
- Rejecting loopback, link-local, and private IP ranges using Go's built-in checks
- Validating URLs before they are used in requests
The TOCTOU risk (DNS changing between validation and request) is acceptable given this is an admin-only feature.
Based on past review comments indicating SSRF concerns were addressed.
346-425: LGTM! Endpoint reachability testing is comprehensive and well-designed.The implementation:
- Applies SSRF validation for user-provided URLs while safely skipping it for discovered endpoints
- Validates URL schemes and structure before making requests
- Interprets HTTP status codes appropriately for different endpoint types (e.g., token endpoints expect POST, so 404 on GET is acceptable)
- Provides detailed, user-friendly notes for each validation result
- Logs failures at appropriate level for security monitoring
The nuanced status code handling (lines 390-418) correctly distinguishes between reachability issues and expected authentication/authorization responses.
apps/standalone/src/app/components/Login/LoginPage.tsx (2)
15-27: Redirect helper now validates responses and avoids bad URLs
redirectToProviderLoginnow checksresponse.okand the presence of a non‑emptyurlbefore redirecting, throwing errors otherwise so callers can surface a proper error state. This addresses the earlier risk of navigating to"undefined"or an invalid URL while keeping the flow simple.
57-59: Review comment is based on incorrect assumptions about i18next behaviori18next's default interpolation ignores whitespace inside {{ }} delimiters, so spaces like
{{ providerName }}versus{{providerName}}have no functional impact. Both will interpolate correctly.However, the trailing space outside the braces in the first key (
'Failed to initiate login with {{ providerName}} ') is a separate issue—it's literal text in the message that may be unintended.The review comment conflates two distinct problems:
- Spaces inside
{{ }}(harmless in i18next; no fix needed)- Trailing space outside the braces in the message string (could be cleaned up for code quality)
No code changes are required for interpolation to work correctly. The spaces do not prevent variable substitution.
Likely an incorrect or invalid review comment.
libs/ui-components/src/utils/authProvider.ts (1)
5-20: LGTM!The display name resolution logic is well-structured with appropriate fallbacks: checks for explicit
displayName, provides translated labels for built-in provider types (OpenShift, K8s, AAP), and falls back tometadata.name. The type assertion on line 19 is safe per established context.Based on learnings
libs/ui-components/src/types/extraTypes.ts (1)
82-95: LGTM!The
DynamicAuthProvidertypes andisDynamicAuthProvidertype guard correctly identify OIDC and OAuth2 providers as the dynamically manageable subset, which aligns with the CRUD form's design constraints. TheProviderTypeenum provides type-safe constants for all provider types across the system.Based on learnings
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx (3)
34-47: LGTM!The column definitions are straightforward and properly translated.
64-78: LGTM!The empty state correctly reuses
CreateAuthProviderButton, ensuring the action button respects RBAC permissions.
80-123: LGTM!The table implementation properly handles loading states, errors, and the delete flow. The
refetch()call on line 117 ensures the list updates after successful deletion. The type assertion on line 97 is safe per documented context.Based on learnings
libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetails.tsx (3)
20-23: LGTM!Permission definitions are correctly scoped to the actions available on this page (edit and delete).
25-99: LGTM!The details page properly handles data fetching, permission-gated actions (edit/delete), and navigation between details and YAML views. The delete flow correctly navigates back to the providers list on success. Type assertions are safe per documented context.
Based on learnings
101-109: LGTM!The permission wrapper correctly gates the entire details page with
VERB.GET, ensuring users can only view provider details if authorized.
celdrake
force-pushed
the
EDM-2373-support-multiple-auth-providers
branch
from
18bc90f to
a868316
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
libs/i18n/locales/en/translation.json (1)
16-16: Template variable spacing inconsistency persists with additional trailing space in key.Line 16 has two issues:
- The template variable
{{ providerName}}is missing a space before closing braces (should be{{ providerName }}).- The key itself has a trailing space:
"Failed to initiate login with {{ providerName}} "(space before the closing quote).This was flagged in a previous review. Additionally, compare with line 20 which uses a different variable name (
{{ provider }}).- "Failed to initiate login with {{ providerName}} ": "Failed to initiate login with {{ providerName}} ", + "Failed to initiate login with {{ providerName }}": "Failed to initiate login with {{ providerName }}",libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx (1)
49-76: Consider reusingCreateAuthProviderButtonin the toolbar for consistencyYou already have
CreateAuthProviderButtonwith an explicitVERB.CREATEcheck, but the toolbar renders a separate primary button that always shows when the page is visible. Even though the whole page is admin-gated, reusing the same component would avoid duplication and keep RBAC logic centralized:- <ToolbarItem> - <Button variant="primary" onClick={() => navigate(ROUTE.AUTH_PROVIDER_CREATE)}> - {t('Add authentication provider')} - </Button> - </ToolbarItem> + <ToolbarItem> + <CreateAuthProviderButton /> + </ToolbarItem>Not required for correctness, but it reduces duplication and keeps future permission changes in a single place.
Also applies to: 144-157
🧹 Nitpick comments (3)
proxy/bridge/testauth.go (2)
119-119: Normalize issuer URL to prevent malformed discovery URLs.Simple string concatenation can produce malformed URLs if the issuer has a trailing slash or path component. While most OIDC providers use root issuers, robust handling would improve reliability.
Consider normalizing the issuer URL:
+ // Ensure issuer doesn't have trailing slash + issuerURL := strings.TrimSuffix(req.Issuer, "/") - discoveryUrl := fmt.Sprintf("%s/.well-known/openid-configuration", req.Issuer) + discoveryUrl := fmt.Sprintf("%s/.well-known/openid-configuration", issuerURL)
346-425: Consider extracting status code interpretation logic for clarity.The status code handling (lines 390-418) is correct but involves deeply nested conditionals. Extracting this into a helper function would improve readability and testability.
Example refactor:
type endpointCheckResult struct { valid bool note string } func interpretStatusCode(statusCode int, fieldName string, isTokenEndpoint bool) endpointCheckResult { if statusCode >= 200 && statusCode < 500 { switch statusCode { case http.StatusNotFound: if isTokenEndpoint { return endpointCheckResult{true, fmt.Sprintf("%s is reachable (HTTP 404 for GET - endpoint likely accepts POST)", fieldName)} } return endpointCheckResult{false, fmt.Sprintf("%s returned HTTP 404 - endpoint not found", fieldName)} case http.StatusMethodNotAllowed: return endpointCheckResult{true, fmt.Sprintf("%s is reachable (HTTP 405 - endpoint exists, GET not supported)", fieldName)} case http.StatusUnauthorized, http.StatusForbidden: return endpointCheckResult{true, fmt.Sprintf("%s is reachable (HTTP %d - authentication required, as expected)", fieldName, statusCode)} case http.StatusBadRequest: return endpointCheckResult{true, fmt.Sprintf("%s is reachable (HTTP 400 - endpoint exists)", fieldName)} default: return endpointCheckResult{true, fmt.Sprintf("%s is reachable (HTTP %d)", fieldName, statusCode)} } } return endpointCheckResult{false, fmt.Sprintf("%s returned HTTP %d - server error", fieldName, statusCode)} }Then use it in
checkEndpointReachability:result := interpretStatusCode(resp.StatusCode, fieldName, isTokenEndpoint) notes = append(notes, result.note) valid = result.validlibs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (1)
49-61: Optional: clarifygetProviderTypeLabelscope and localize the fallbackGiven this module is explicitly scoped to OIDC/OAuth2 providers, handling only
ProviderType.OIDCandProviderType.OAuth2here is reasonable. Two small improvement options:
- If you expect this helper to ever be used for K8s/AAP/OpenShift, consider adding explicit cases for those types rather than returning
'N/A'.- For the default branch, you may want to use a translated label (e.g.,
t('N/A')or similar) instead of a bare string to keep everything localizable.Not strictly necessary today but may help future-proof the helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/standalone/src/app/components/Login/LoginPage.tsx(1 hunks)apps/standalone/src/app/components/Login/LoginPageLayout.tsx(1 hunks)libs/i18n/locales/en/translation.json(7 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetails.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProviderRow.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProvider.tsx(1 hunks)libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts(1 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(4 hunks)libs/ui-components/src/types/extraTypes.ts(2 hunks)libs/ui-components/src/utils/authProvider.ts(1 hunks)proxy/bridge/testauth.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/ui-components/src/components/common/PageNavigation.tsx
- apps/standalone/src/app/components/Login/LoginPageLayout.tsx
- libs/ui-components/src/components/AuthProvider/AuthProviderRow.tsx
- libs/ui-components/src/components/AuthProvider/AuthProviderDetails/AuthProviderDetails.tsx
- libs/ui-components/src/components/AuthProvider/CreateAuthProvider/CreateAuthProvider.tsx
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
📚 Learning: 2025-11-20T09:24:41.950Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts:288-326
Timestamp: 2025-11-20T09:24:41.950Z
Learning: In the flightctl-ui repository, the CreateAuthProvider form and its utilities (including getAuthProvider in libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts) are specifically designed for CRUD operations on OIDC and OAuth2 authentication providers only. Other provider types (K8s, AAP, OpenShift) are not supported by this form and should be rejected with an error if loading is attempted for editing.
Applied to files:
libs/ui-components/src/utils/authProvider.tslibs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsxapps/standalone/src/app/components/Login/LoginPage.tsxlibs/ui-components/src/types/extraTypes.tslibs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts
📚 Learning: 2025-11-20T07:33:34.656Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 378
File: libs/ui-components/src/utils/authProvider.ts:5-18
Timestamp: 2025-11-20T07:33:34.656Z
Learning: In the flightctl-ui repository, for AuthProvider objects, the metadata.name field is always present at runtime as it serves as the ID of the authprovider. The OpenAPI schema incorrectly defines it as optional (name?: string in ObjectMeta), but the type assertion `as string` is appropriate when accessing provider.metadata.name since it's guaranteed to exist.
Applied to files:
libs/ui-components/src/utils/authProvider.tslibs/ui-components/src/types/extraTypes.tslibs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts
📚 Learning: 2025-10-29T16:47:29.614Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
Applied to files:
libs/ui-components/src/utils/authProvider.tslibs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsxapps/standalone/src/app/components/Login/LoginPage.tsxproxy/bridge/testauth.golibs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts
📚 Learning: 2025-11-17T15:53:01.194Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 376
File: libs/ui-components/src/components/Repository/CreateRepository/CreateRepositoryForm.tsx:274-275
Timestamp: 2025-11-17T15:53:01.194Z
Learning: In PR #376 (flightctl/flightctl-ui), the permission system was updated to fetch all user permissions once at login via the Flight Control API. The `checkPermissions` function from `usePermissionsContext` is a pure calculation on cached permission arrays, not an async fetch, so loading state handling is generally not required at individual component call sites.
Applied to files:
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx
📚 Learning: 2025-02-18T12:04:42.579Z
Learnt from: rawagner
Repo: flightctl/flightctl-ui PR: 209
File: apps/standalone/src/app/utils/apiCalls.ts:14-16
Timestamp: 2025-02-18T12:04:42.579Z
Learning: In the flightctl-ui codebase, error handling for API calls follows a React pattern where the caller component (e.g., AppToolbar.tsx) implements try-catch blocks and manages error states, rather than handling errors within the API utility functions themselves.
Applied to files:
apps/standalone/src/app/components/Login/LoginPage.tsx
📚 Learning: 2025-02-17T08:41:57.993Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 0
File: :0-0
Timestamp: 2025-02-17T08:41:57.993Z
Learning: All files in `libs/types/models` are auto-generated using OpenAPI Generator from the schema defined in https://github.com/flightctl/flightctl/blob/main/api/v1alpha1/openapi.yaml. Changes to these files should be made in the schema instead of modifying the generated files directly.
Applied to files:
libs/ui-components/src/types/extraTypes.ts
📚 Learning: 2025-03-20T12:34:33.199Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 240
File: libs/types/models/FileSpec.ts:8-8
Timestamp: 2025-03-20T12:34:33.199Z
Learning: Files in the types directory are autogenerated from the OpenAPI spec in the flightctl project and should not be directly modified.
Applied to files:
libs/ui-components/src/types/extraTypes.ts
🧬 Code graph analysis (5)
libs/ui-components/src/components/AuthProvider/AuthProvidersPage.tsx (5)
libs/ui-components/src/components/Table/Table.tsx (1)
ApiSortTableColumn(16-24)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useNavigate.tsx (1)
useNavigate(49-72)libs/ui-components/src/components/common/PermissionsContext.tsx (1)
usePermissionsContext(105-111)libs/ui-components/src/components/AuthProvider/useAuthProviders.ts (1)
useAuthProviders(4-18)
apps/standalone/src/app/components/Login/LoginPage.tsx (4)
libs/types/models/AuthProvider.ts (1)
AuthProvider(10-21)apps/standalone/src/app/utils/apiCalls.ts (1)
loginAPI(17-17)libs/ui-components/src/utils/authProvider.ts (1)
getProviderDisplayName(5-20)libs/types/models/AuthConfig.ts (1)
AuthConfig(6-23)
libs/ui-components/src/types/extraTypes.ts (4)
libs/types/models/OIDCProviderSpec.ts (1)
OIDCProviderSpec(10-45)libs/types/models/OAuth2ProviderSpec.ts (1)
OAuth2ProviderSpec(10-57)proxy/auth/common.go (1)
AuthProvider(70-76)libs/types/models/AuthProvider.ts (1)
AuthProvider(10-21)
proxy/bridge/testauth.go (2)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (2)
FieldValidationResult(91-96)FieldValidation(85-89)proxy/log/log.go (1)
GetLogger(17-19)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (4)
libs/ui-components/src/components/AuthProvider/CreateAuthProvider/types.ts (8)
AuthProviderFormValues(56-82)DEFAULT_ROLE_SEPARATOR(23-23)isOrgAssignmentDynamic(35-37)isOrgAssignmentPerUser(39-41)isRoleAssignmentStatic(48-50)isRoleAssignmentDynamic(52-54)isOAuth2Provider(28-29)isOrgAssignmentStatic(31-33)libs/ui-components/src/types/extraTypes.ts (1)
DynamicAuthProviderSpec(83-83)libs/ui-components/src/utils/patch.ts (1)
appendJSONPatch(23-54)libs/ui-components/src/components/form/validations.ts (1)
validKubernetesDnsSubdomain(151-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build ocp plugin
- GitHub Check: Lint
- GitHub Check: Build
🔇 Additional comments (11)
libs/i18n/locales/en/translation.json (1)
980-982: Verify provider type entries align with form support constraints.Lines 980–982 add localization keys for "OpenShift", "Kubernetes", and "Ansible Automation Platform" provider types. Per learnings, the
CreateAuthProviderform supports only OIDC and OAuth2. Confirm these entries are used in read-only or system contexts (e.g., provider details view, provider type display) and not exposed in the CreateAuthProvider form where they would not be editable.proxy/bridge/testauth.go (6)
1-15: LGTM!Package declaration and imports are appropriate for the handler's functionality.
17-48: LGTM!Type definitions are well-structured with appropriate JSON tags and align with the frontend TypeScript types.
50-58: LGTM!Standard Go constructor pattern. The TLS config is properly stored for use in HTTP clients.
60-94: LGTM!The handler correctly validates the request method, decodes the request body, creates an appropriately configured HTTP client, and routes to the correct validator based on provider type.
242-305: LGTM!OAuth2 validation correctly enforces required fields (authorization, token, and userinfo URLs) and treats issuer as optional. SSRF protection is appropriately applied to all user-provided URLs.
307-344: <!-- [scratchpad_start]
[task verify_go_version 4/4 COMPLETE]
Verify Go 1.17+ support for net.IP.IsPrivate()[observations]
- go.mod specifies: go 1.24.0
- toolchain: go1.24.6
- Code uses net.IP.IsPrivate() on line 315
- IsPrivate() was added in Go 1.17
[analysis]
- Go 1.24.0 > Go 1.17.0
- Code is compatible with project's Go version requirement
- No compatibility issue exists
[done]
Verification complete: Go 1.24.0 supports net.IP.IsPrivate()
</scratchpad_end] -->No compatibility issues found.
The project's
go.modspecifiesgo 1.24.0, which is well above Go 1.17 wherenet.IP.IsPrivate()was introduced. The SSRF protection code is compatible.libs/ui-components/src/utils/authProvider.ts (1)
5-19: Display-name resolution logic looks solid and consistent
getProviderDisplayNamecorrectly prioritizesspec.displayName, provides translated labels for OpenShift/K8s/AAP, and safely falls back toprovider.metadata.name as string, which is guaranteed to exist forAuthProviderinstances in this codebase. Based on learnings.apps/standalone/src/app/components/Login/LoginPage.tsx (1)
15-27: Provider-driven login flow and redirect error handling look correctThe login page cleanly:
- Loads
AuthConfig, filters enabled providers, and prioritizes the default provider.- Distinguishes K8s (token form) from other providers (redirect via
redirectToProviderLogin).- Guards against multiple clicks with
isRedirecting.- Validates the redirect endpoint (
response.okand presence ofurl) so both manual selection and single-provider auto-redirect surface errors via state instead of navigating to invalid URLs.Overall, the control flow and state transitions for login look sound.
Also applies to: 65-105, 125-163
libs/ui-components/src/types/extraTypes.ts (1)
82-95: Dynamic auth-provider typing and type guard are well-structuredThe
DynamicAuthProviderSpec/DynamicAuthProviderpairing plusisDynamicAuthProvidercleanly narrow the generatedAuthProviderto just OIDC/OAuth2 specs, and theProviderTypeenum centralizes the string literals used across the UI. This should make the rest of the auth-provider code safer and easier to reason about.libs/ui-components/src/components/AuthProvider/CreateAuthProvider/utils.ts (1)
288-407: Patch generation and validation logic for auth providers looks coherentA few related pieces work together nicely here:
getAuthProviderbuilds a fullAuthProviderobject from form values, with a clear branch between OAuth2 (adds URLs) and OIDC (issuer-only), matching the dynamic-provider scope of this form.getAuthProviderPatchesnow correctly detects secret changes by comparingprevSpec.clientSecretvsnewSpec.clientSecret, falls back to a full/specreplace when both providerType and secret change, and otherwise:
- Replaces
/spec/providerTypeif needed,- Delegates common fields (incl. org/role assignments) to
patchAuthProviderFieldswith properskipSecretsbehavior,- Delegates OAuth2-only fields to
patchProviderTypeSpecificFields(adding/removing URLs as types change).authProviderSchemacomposes a base schema and then adds provider-type-specific and org-assignment-specific constraints, with sensible rules for scopes uniqueness, role assignment, and organization assignment.This structure should give you precise, minimal patches and robust validation for the OIDC/OAuth2 CRUD flows.
Also applies to: 410-469, 471-555
| "Failed to initiate login with {{ providerName}} ": "Failed to initiate login with {{ providerName}} ", | ||
| "Failed to initiate login": "Failed to initiate login", | ||
| "No authentication providers found. Please contact your administrator.": "No authentication providers found. Please contact your administrator.", | ||
| "Failed to load the authentication providers": "Failed to load the authentication providers", | ||
| "Redirecting to login for {{ provider }}...": "Redirecting to login for {{ provider }}...", |
There was a problem hiding this comment.
Inconsistent template variable naming across related keys.
Line 16 uses {{ providerName }} while line 20 uses {{ provider }} for what appear to be related error/redirect messages. Standardize the variable name across both keys for consistency.
- "Redirecting to login for {{ provider }}...": "Redirecting to login for {{ provider }}...",
+ "Redirecting to login for {{ providerName }}...": "Redirecting to login for {{ providerName }}...",📝 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.
| "Failed to initiate login with {{ providerName}} ": "Failed to initiate login with {{ providerName}} ", | |
| "Failed to initiate login": "Failed to initiate login", | |
| "No authentication providers found. Please contact your administrator.": "No authentication providers found. Please contact your administrator.", | |
| "Failed to load the authentication providers": "Failed to load the authentication providers", | |
| "Redirecting to login for {{ provider }}...": "Redirecting to login for {{ provider }}...", | |
| "Failed to initiate login with {{ providerName}} ": "Failed to initiate login with {{ providerName}} ", | |
| "Failed to initiate login": "Failed to initiate login", | |
| "No authentication providers found. Please contact your administrator.": "No authentication providers found. Please contact your administrator.", | |
| "Failed to load the authentication providers": "Failed to load the authentication providers", | |
| "Redirecting to login for {{ providerName }}...": "Redirecting to login for {{ providerName }}...", |
🤖 Prompt for AI Agents
In libs/i18n/locales/en/translation.json around lines 16 to 20, the template
variable names are inconsistent (line 16 uses {{ providerName }} while line 20
uses {{ provider }}); standardize them by choosing one variable name (e.g., {{
provider }}) and update the entries so all related keys use the same
placeholder, ensuring translation keys and any code that references these
templates are adjusted to match the chosen variable name.
| return; | ||
| } | ||
|
|
||
| setUserSelectedProvider(provider); |
There was a problem hiding this comment.
When user tries again, we should remove the previous error
| setUserSelectedProvider(provider); | |
| setError(undefined) | |
| setUserSelectedProvider(provider); |
There was a problem hiding this comment.
In general, the error handling did not work well if prefferedUsername was not present. Not a blocker though.
|
thanks @rawagner , I'll fix the issues in your CR in the follow-up. |
3 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.