EDM-1982: Add option to copy login command #357
Conversation
WalkthroughAdds AuthType enum and threads authType through backend and UI; implements one‑time session token endpoints and CLI login flows (modal + standalone page); exposes external API URL config; centralizes brandName utilities and translations; updates navigation, styles, copy UX, and related routing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Web UI (PageNavigation)
participant Proxy as Proxy /login
participant Auth as AuthProvider
participant Store as Session Store
rect rgb(245,248,255)
note over UI: Non‑K8S authType (OIDC/AAP)
User->>UI: Click "Get login command"
UI->>Proxy: POST /login/create-session-token
Proxy->>Auth: CreateSessionToken()
Auth->>Store: Store {sessionId -> apiToken}
Auth-->>Proxy: {sessionId}
Proxy-->>UI: {sessionId}
UI->>UI: Save redirectAfterLogin
UI-->>User: Open new tab to /login (forceReauth=true)
end
rect rgb(255,250,240)
note over Proxy,Auth: After OAuth callback and user follows link
User->>UI: Open /copy-login-command?sessionId=...
UI->>Proxy: GET /login/get-session-token?sessionId=...
Proxy->>Store: Single‑use fetch token
Store-->>Proxy: {token, serviceUrl}
Proxy-->>UI: {token, serviceUrl}
UI-->>User: Show copyable CLI login command
end
sequenceDiagram
autonumber
actor User
participant UI as Web UI (PageNavigation)
note over UI: K8S authType
User->>UI: Click "Get login command"
UI->>UI: Open CopyLoginCommandModal (uses config.externalApiUrl)
UI-->>User: Show copyable CLI login command in modal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 |
celdrake
force-pushed
the
EDM-1982-copy-login-command
branch
3 times, most recently
from
5724e35 to
d1158df
Compare
|
cc @pamparan PTAL at the videos. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
proxy/auth/auth.go (1)
38-225: Persist session-token state across replicas.
apiTokenMapkeeps the CLI token mapping in process memory. In a multi-replica deployment the POST/login/create-session-tokenmay land on pod A, but the subsequent GET/login/get-session-tokencan hit pod B where the map is empty, so the single-use token 404s. This breaks the flow whenever the requests are load-balanced to different instances. Please back this mapping with a shared store (or otherwise encode the token soGetSessionTokencan retrieve it without node-local state) before shipping.apps/standalone/src/app/context/AuthContext.ts (1)
47-85: ClearOAUTH_REDIRECT_AFTER_LOGIN_KEYon first read.If a user aborts the “Get login command” flow (closes the new tab or hits an IdP error), the redirect key stays in localStorage. The next normal login then hits this
/callbackpath, sees the stale key, and incorrectly follows the secondary-session branch—sending the user to/copy-login-commandinstead of restoring the UI session. Remove the key immediately after reading it so stale values can’t leak into primary logins:- const redirectAfterLogin = localStorage.getItem(OAUTH_REDIRECT_AFTER_LOGIN_KEY); + const redirectAfterLogin = localStorage.getItem(OAUTH_REDIRECT_AFTER_LOGIN_KEY); + localStorage.removeItem(OAUTH_REDIRECT_AFTER_LOGIN_KEY); const isPrimarySession = !secondarySessionRedirectPages.includes(redirectAfterLogin || ''); @@ - const newLoginResponse = (await resp.json()) as { sessionId: string }; - localStorage.removeItem(OAUTH_REDIRECT_AFTER_LOGIN_KEY); + const newLoginResponse = (await resp.json()) as { sessionId: string };(You can still guard the response for success, but the extra
removeItemcall becomes unnecessary.)
🧹 Nitpick comments (1)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.css (1)
7-10: Consider responsive width constraints.The fixed
40vwwidth may cause usability issues on very narrow mobile screens (where 40vw could be too small for command visibility) or very wide displays (where 40vw becomes unnecessarily large). Consider adding min/max constraints or using a responsive approach..fctl-login-command__copy-content { /* Keep a fixed width to prevent a change in size when the full command is displayed */ - width: 40vw; + width: clamp(300px, 40vw, 800px); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/ocp-plugin/src/components/common/WithPageLayout.tsx(1 hunks)apps/standalone/scripts/setup_env.sh(2 hunks)apps/standalone/src/app/components/AppLayout/AppLayout.tsx(2 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.css(1 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.tsx(2 hunks)apps/standalone/src/app/context/AuthContext.ts(7 hunks)apps/standalone/src/app/routes.tsx(3 hunks)libs/i18n/locales/en/translation.json(5 hunks)libs/ui-components/src/components/Masthead/CommandLineToolsPage.tsx(6 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.css(1 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx(1 hunks)libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx(1 hunks)libs/ui-components/src/components/common/CopyButton.tsx(1 hunks)libs/ui-components/src/components/common/CopyLoginCommandModal.tsx(1 hunks)libs/ui-components/src/components/common/PageNavigation.css(1 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(5 hunks)libs/ui-components/src/constants.ts(1 hunks)libs/ui-components/src/types/extraTypes.ts(1 hunks)libs/ui-components/src/utils/brand.ts(1 hunks)proxy/app.go(1 hunks)proxy/auth/aap.go(1 hunks)proxy/auth/auth.go(8 hunks)proxy/auth/common.go(1 hunks)proxy/auth/oauth.go(1 hunks)proxy/auth/oidc.go(1 hunks)proxy/config/config.go(1 hunks)proxy/config/ocp.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
proxy/config/ocp.go (1)
proxy/config/config.go (3)
RBACNs(22-22)FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
libs/ui-components/src/utils/brand.ts (1)
libs/ui-components/src/hooks/useAppContext.tsx (1)
AppContextProps(47-85)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (5)
proxy/auth/auth.go (1)
SessionTokenResponse(28-31)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
libs/ui-components/src/components/common/PageNavigation.tsx (4)
apps/ocp-plugin/src/hooks/useFetch.ts (1)
useFetch(40-99)libs/ui-components/src/hooks/useFetch.ts (1)
useFetch(3-9)libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)libs/ui-components/src/constants.ts (1)
OAUTH_REDIRECT_AFTER_LOGIN_KEY(5-5)
libs/ui-components/src/components/common/CopyLoginCommandModal.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
apps/standalone/src/app/context/AuthContext.ts (3)
libs/ui-components/src/constants.ts (1)
OAUTH_REDIRECT_AFTER_LOGIN_KEY(5-5)apps/standalone/src/app/utils/apiCalls.ts (1)
loginAPI(17-17)libs/ui-components/src/utils/organizationStorage.ts (1)
ORGANIZATION_STORAGE_KEY(1-1)
libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx (2)
libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
apps/standalone/src/app/components/AppLayout/AppToolbar.tsx (1)
apps/standalone/src/app/context/AuthContext.ts (1)
AuthContext(25-30)
proxy/auth/auth.go (2)
proxy/auth/common.go (3)
AuthProvider(27-34)TokenData(18-21)LoginParameters(23-25)proxy/config/config.go (2)
FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
apps/standalone/src/app/components/AppLayout/AppLayout.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)apps/standalone/src/app/context/AuthContext.ts (1)
AuthContext(25-30)
libs/ui-components/src/components/Masthead/CommandLineToolsPage.tsx (1)
libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
🔇 Additional comments (15)
apps/standalone/scripts/setup_env.sh (1)
32-32: LGTM!The new external URL configuration using nip.io DNS is appropriate for development environments and aligns with the external routing requirements.
proxy/app.go (1)
80-83: LGTM!The session token endpoints are well-documented and follow security best practices by implementing single-use tokens. The inline comments clearly explain the behavior.
libs/ui-components/src/constants.ts (1)
1-5: LGTM!The addition of
OAUTH_REDIRECT_AFTER_LOGIN_KEYis well-integrated with the new authentication flow, and the export style change improves readability.libs/ui-components/src/types/extraTypes.ts (1)
79-84: LGTM!The
AuthTypeenum is well-defined with clear, self-documenting values that support the new authentication flows across the application.apps/standalone/src/app/components/AppLayout/AppLayout.tsx (2)
30-30: LGTM!Moving
pageIdto the top-level scope eliminates redundancy and improves code organization.
37-37: LGTM!The
authTypeis properly extracted from context and threaded through toPageNavigation, enabling authentication-aware navigation behavior.Also applies to: 103-103
proxy/config/config.go (1)
11-11: LGTM!The
FctlApiUrlExternalvariable follows the established pattern for URL configuration and integrates cleanly with the external API URL requirements.proxy/config/ocp.go (1)
9-10: LGTM! Clean fallback logic for external API URL.The implementation correctly introduces
ExternalApiUrlwith a sensible fallback: useFctlApiUrlExternal(fromFLIGHTCTL_EXTERNAL_URLenv var) if available, otherwise default toFctlApiUrl. The struct field naming is consistent with existing conventions.Also applies to: 16-19, 22-23
proxy/auth/oidc.go (1)
197-203: LGTM! Auth interface updates are well-integrated.The changes correctly:
- Update
GetLoginRedirectURLto acceptforceReauthparameter, aligning with the updated interface and enabling reauthentication flows.- Add
GetAuthType()method returning"OIDC", providing consistent auth type identification for UI routing logic.Both changes integrate cleanly with the broader authentication flow enhancements.
apps/standalone/src/app/routes.tsx (1)
71-73: LGTM! New route is correctly isolated from app layout.The new
/copy-login-commandroute is properly:
- Lazy-loaded via
React.lazy, consistent with other pages.- Positioned outside
AppLayout(as noted in the comment), which is appropriate for a standalone authentication flow.- Wrapped in
TitledRoutefor consistent document title and suspense handling.- Excluded from
getAppRoutes, correctly indicating it's not part of the main navigation.Also applies to: 354-362
proxy/auth/oauth.go (1)
25-38: LGTM! Force reauthentication is correctly implemented.The implementation properly:
- Updates
loginRedirectsignature to acceptforceReauth boolparameter.- Conditionally adds the standard OIDC
prompt=loginparameter when reauthentication is requested.- Uses idiomatic Go URL query manipulation to safely modify the authorize URL.
This aligns with OAuth2/OIDC standards and enables the UI to explicitly request a fresh authentication flow.
libs/ui-components/src/components/common/PageNavigation.css (1)
1-16: LGTM! CSS correctly styles the new global actions masthead.The styling appropriately:
- Right-aligns sub-navigation toolbar items (
justify-content: flex-end).- Sets masthead height to match navigation menu items (3.5rem).
- Applies dark background using PatternFly CSS variables for theme consistency.
- Resets padding at the masthead level, then applies horizontal padding to the inner toolbar.
These changes support the new authType-aware navigation and login command UI without introducing accessibility or layout issues.
apps/standalone/src/app/components/AppLayout/AppToolbar.tsx (1)
23-23: LGTM! Clean migration from authEnabled to authType.The changes correctly:
- Import
AuthTypeenum for type-safe authentication state checks.- Replace
authEnabledboolean withauthTypeenum fromAuthContext.- Update rendering condition to
authType !== AuthType.DISABLED && username, maintaining equivalent behavior.This aligns with the broader auth flow refactoring and provides better type safety.
Also applies to: 73-73, 80-80
libs/ui-components/src/components/common/CopyButton.tsx (1)
16-16: LGTM! User feedback for copy action is well-implemented.The changes add useful UI feedback:
copiedstate tracks copy operation completion.- Tooltip dynamically shows "Copied!" for 1 second after copying.
useEffectproperly cleans up timeout on unmount or state changes.The implementation handles rapid clicks correctly: each copy action resets the timeout, ensuring "Copied!" shows for 1 second from the most recent copy.
Also applies to: 20-20, 23-33, 36-36
libs/ui-components/src/utils/brand.ts (1)
1-4: LGTM! Centralized branding utility promotes consistency.The
getBrandNameutility correctly:
- Accepts
settingsfromAppContextPropsfor type safety.- Returns appropriate brand name based on
isRHEMflag.- Provides a single source of truth for branding logic across components.
This simplifies brand-based translations and messaging throughout the UI.
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx
Outdated
Show resolved
Hide resolved
celdrake
force-pushed
the
EDM-1982-copy-login-command
branch
from
d1158df to
6b98d7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/ui-components/src/components/common/CopyButton.tsx (1)
41-46: Move click handler to Button (accessibility/keyboard support)onClick is on the icon, so keyboard activation of the button won’t copy. Attach onClick to Button and remove it from the icon.
Apply this diff:
return ( <Tooltip content={copied ? t('Copied!') : ariaLabel || t('Copy text')}> <Button variant={variant || 'plain'} isInline={variant === 'link'} + onClick={onCopy} icon={ <Icon size="sm"> - <CopyIcon onClick={onCopy} /> + <CopyIcon /> </Icon> } aria-label={ariaLabel || t('Copy text')} /> </Tooltip> );apps/standalone/src/app/context/AuthContext.ts (2)
64-85: Harden /callback login flow with error handling.If the POST to finalize login fails, the effect can hang without setting error/loading. Wrap in try/catch, check response.ok, and set loading/error appropriately.
- // In both cases, we trigger a new login flow - const resp = await fetch(loginEndpoint, { - headers: { - 'Content-Type': 'application/json', - }, - credentials: 'include', - method: 'POST', - body: JSON.stringify({ - code: code, - }), - }); - - if (isPrimarySession) { - const newLoginResponse = (await resp.json()) as { expiresIn: number }; - const now = nowInSeconds(); - localStorage.setItem(EXPIRATION, `${now + newLoginResponse.expiresIn}`); - lastRefresh = now; - } else { - const newLoginResponse = (await resp.json()) as { sessionId: string }; - localStorage.removeItem(OAUTH_REDIRECT_AFTER_LOGIN_KEY); - window.location.href = `/${redirectAfterLogin}?sessionId=${newLoginResponse.sessionId || ''}`; - } + // In both cases, we trigger a new login flow + try { + const resp = await fetch(loginEndpoint, { + headers: { 'Content-Type': 'application/json' }, + credentials: 'include', + method: 'POST', + body: JSON.stringify({ code }), + }); + if (!resp.ok) { + throw new Error('Failed to complete login'); + } + if (isPrimarySession) { + const newLoginResponse = (await resp.json()) as { expiresIn: number }; + const now = nowInSeconds(); + localStorage.setItem(EXPIRATION, `${now + newLoginResponse.expiresIn}`); + lastRefresh = now; + } else { + const newLoginResponse = (await resp.json()) as { sessionId: string }; + localStorage.removeItem(OAUTH_REDIRECT_AFTER_LOGIN_KEY); + window.location.href = `/${redirectAfterLogin}?sessionId=${newLoginResponse.sessionId || ''}`; + } + } catch (e) { + setError('Failed to complete login'); + setLoading(false); + return; + }
105-110: Set loading=false on non-200 user-info.Avoid indefinite spinner when /info returns unexpected status.
- if (resp.status !== 200) { - setError('Failed to get user info'); - return; - } + if (resp.status !== 200) { + setError('Failed to get user info'); + setLoading(false); + return; + }
♻️ Duplicate comments (1)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (1)
62-79: Fix inverted truncation logic and render toggle only when truncated.Current condition shows truncated text when expanded and full text when collapsed; also always shows the toggle. Invert the condition and guard the toggle on
isTruncated.- <CodeBlockCode className="fctl-login-command__codeblock"> - {/* When the full command is shown, hide the truncated command. This allows the full command to be copied using the keyboard */} - {isExpanded && isTruncated ? `${truncatedCommand}...` : loginCommand} - <ExpandableSection - toggleText={isExpanded ? t('Show less') : t('Show more')} - isExpanded={isExpanded} - isDetached - contentId="code-block-expand" - > - {loginCommand} - </ExpandableSection> - <ExpandableSectionToggle - isExpanded={isExpanded} - onToggle={(isExpanded) => setIsExpanded(isExpanded)} - contentId="code-block-expand" - direction={isExpanded ? 'up' : 'down'} - > - {isExpanded ? t('Show less') : t('Show more')} - </ExpandableSectionToggle> - </CodeBlockCode> + <CodeBlockCode className="fctl-login-command__codeblock"> + {/* When not expanded and the command is long, show a truncated preview with ellipsis */} + {(!isExpanded && isTruncated) ? `${truncatedCommand}...` : loginCommand} + {isTruncated && ( + <> + <ExpandableSection + toggleText={isExpanded ? t('Show less') : t('Show more')} + isExpanded={isExpanded} + isDetached + contentId="code-block-expand" + > + {loginCommand} + </ExpandableSection> + <ExpandableSectionToggle + isExpanded={isExpanded} + onToggle={(expanded) => setIsExpanded(expanded)} + contentId="code-block-expand" + direction={isExpanded ? 'up' : 'down'} + > + {isExpanded ? t('Show less') : t('Show more')} + </ExpandableSectionToggle> + </> + )} + </CodeBlockCode>
🧹 Nitpick comments (10)
proxy/config/ocp.go (1)
16-19: Minor: normalize and validate URL (optional)Optional: validate/normalize externalApiUrl (scheme/host) to prevent malformed values from config.
Also applies to: 22-24
proxy/auth/oauth.go (1)
25-38: Use Set instead of Add for query parameterUse query.Set("prompt","login") to avoid duplicate prompt entries across retries/redirects. Functionality unchanged otherwise.
- query := url.Query() - query.Add("prompt", "login") + query := url.Query() + query.Set("prompt", "login") url.RawQuery = query.Encode()Also confirm non-OIDC providers either ignore this param or do not use this code path.
proxy/auth/auth.go (5)
102-110: Set Content-Type for JSON redirect response (login GET)Add application/json header for consistency.
- response, err := json.Marshal(RedirectResponse{Url: loginUrl}) + w.Header().Set("Content-Type", "application/json") + response, err := json.Marshal(RedirectResponse{Url: loginUrl})
151-177: Guard nil resp and set Content-Type in GetUserInfoAvoid possible nil deref and set JSON content type.
- username, resp, err := a.provider.GetUserInfo(token) + username, resp, err := a.provider.GetUserInfo(token) if err != nil { ... } - if resp.StatusCode != http.StatusOK { + if resp == nil || resp.StatusCode != http.StatusOK { w.WriteHeader(resp.StatusCode) return } - userInfo := UserInfoResponse{Username: username, AuthType: a.provider.GetAuthType()} + w.Header().Set("Content-Type", "application/json") + userInfo := UserInfoResponse{Username: username, AuthType: a.provider.GetAuthType()}Also applies to: 169-175
268-285: Set Content-Type for JSON redirect response (CreateSessionToken GET)Mirror POST branch header setting.
- response, err := json.Marshal(RedirectResponse{Url: loginUrl}) + w.Header().Set("Content-Type", "application/json") + response, err := json.Marshal(RedirectResponse{Url: loginUrl})
295-306: Restrict GetSessionToken to GETTighten the surface area and align with semantics.
func (a *AuthHandler) GetSessionToken(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } if a.provider == nil { w.WriteHeader(http.StatusTeapot) return }
314-326: Consider including token expiry in response (optional)CLI may benefit from expiresIn to preemptively refresh. You already track it; consider adding it.
libs/ui-components/src/components/common/CopyButton.tsx (1)
23-33: Timeout typing and clipboard error handling
- Use ReturnType for env-agnostic typing.
- Optionally, set copied only on successful clipboard write to avoid false “Copied!”.
Apply this diff:
- React.useEffect(() => { - let timeout: NodeJS.Timeout; + React.useEffect(() => { + let timeout: ReturnType<typeof setTimeout> | undefined; if (copied) { timeout = setTimeout(() => { setCopied(false); }, 1000); } return () => { - clearTimeout(timeout); + if (timeout) clearTimeout(timeout); }; }, [copied]);And (optional) make copy robust:
- const onCopy = () => { - void navigator.clipboard.writeText(text); - setCopied(true); - }; + const onCopy = () => { + navigator.clipboard + .writeText(text) + .then(() => setCopied(true)) + .catch(() => { + // optionally surface a failure state; keeping silent here preserves current UX + }); + };apps/standalone/src/app/components/AppLayout/AppToolbar.tsx (1)
73-81: Auth gating by AuthType is correct; minor UX polish.Consider hiding the placeholder “User” until username is loaded (e.g., gate on a loading flag or show a skeleton) to avoid flicker when authType != DISABLED but username is temporarily empty.
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (1)
102-103: Use URLSearchParams to read sessionId.String split is brittle when multiple params exist or the order changes.
- const sessionId = window.location.search.split('sessionId=')[1]; + const sessionId = new URLSearchParams(window.location.search).get('sessionId') ?? '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/ocp-plugin/src/components/common/WithPageLayout.tsx(1 hunks)apps/standalone/scripts/setup_env.sh(2 hunks)apps/standalone/src/app/components/AppLayout/AppLayout.tsx(2 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.css(1 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.tsx(2 hunks)apps/standalone/src/app/context/AuthContext.ts(7 hunks)apps/standalone/src/app/routes.tsx(3 hunks)libs/i18n/locales/en/translation.json(5 hunks)libs/ui-components/src/components/Masthead/CommandLineToolsPage.tsx(6 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.css(1 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx(1 hunks)libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx(1 hunks)libs/ui-components/src/components/common/CopyButton.tsx(1 hunks)libs/ui-components/src/components/common/CopyLoginCommandModal.tsx(1 hunks)libs/ui-components/src/components/common/PageNavigation.css(1 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(5 hunks)libs/ui-components/src/constants.ts(1 hunks)libs/ui-components/src/types/extraTypes.ts(1 hunks)libs/ui-components/src/utils/brand.ts(1 hunks)proxy/app.go(1 hunks)proxy/auth/aap.go(1 hunks)proxy/auth/auth.go(8 hunks)proxy/auth/common.go(1 hunks)proxy/auth/oauth.go(1 hunks)proxy/auth/oidc.go(1 hunks)proxy/config/config.go(1 hunks)proxy/config/ocp.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- proxy/auth/common.go
- libs/ui-components/src/components/common/PageNavigation.css
- proxy/config/config.go
- proxy/auth/aap.go
- libs/ui-components/src/utils/brand.ts
- apps/standalone/scripts/setup_env.sh
- libs/ui-components/src/components/common/CopyLoginCommandModal.tsx
- libs/ui-components/src/components/Masthead/CopyLoginCommandPage.css
- proxy/app.go
- libs/ui-components/src/constants.ts
🧰 Additional context used
🧬 Code graph analysis (9)
libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx (2)
libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
proxy/config/ocp.go (1)
proxy/config/config.go (3)
RBACNs(22-22)FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
libs/ui-components/src/components/Masthead/CommandLineToolsPage.tsx (1)
libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
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/useFetch.ts (1)
useFetch(3-9)libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)libs/ui-components/src/constants.ts (1)
OAUTH_REDIRECT_AFTER_LOGIN_KEY(5-5)
apps/standalone/src/app/context/AuthContext.ts (3)
libs/ui-components/src/constants.ts (1)
OAUTH_REDIRECT_AFTER_LOGIN_KEY(5-5)apps/standalone/src/app/utils/apiCalls.ts (1)
loginAPI(17-17)libs/ui-components/src/utils/organizationStorage.ts (1)
ORGANIZATION_STORAGE_KEY(1-1)
apps/standalone/src/app/components/AppLayout/AppToolbar.tsx (1)
apps/standalone/src/app/context/AuthContext.ts (1)
AuthContext(25-30)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (5)
proxy/auth/auth.go (1)
SessionTokenResponse(28-31)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
apps/standalone/src/app/components/AppLayout/AppLayout.tsx (2)
libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)apps/standalone/src/app/context/AuthContext.ts (1)
AuthContext(25-30)
proxy/auth/auth.go (2)
proxy/auth/common.go (3)
AuthProvider(27-34)TokenData(18-21)LoginParameters(23-25)proxy/config/config.go (2)
FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
⏰ 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 (17)
libs/ui-components/src/types/extraTypes.ts (1)
79-84: Confirm enum strings match server valuesServer returns "OIDC" and "AAPGateway"; enum aligns. Please confirm whether "K8S" is ever returned by backend or is UI-only. If server adds new auth types, keep these strings in sync.
proxy/config/ocp.go (1)
9-11: External API URL exposure looks goodField and fallback logic are correct. Verify FLIGHTCTL_EXTERNAL_URL is set in OCP plugin env when needed.
proxy/auth/oidc.go (1)
197-203: LGTM: OIDC redirect + auth typeForwarding forceReauth and returning "OIDC" matches server expectations and UI needs.
apps/standalone/src/app/routes.tsx (2)
71-73: Lazy-load import looks goodThe new CopyLoginCommandPage is correctly lazy-loaded and scoped to the standalone app.
354-362: Standalone-only route placement is correctExposing /copy-login-command outside AppLayout cleanly avoids inheriting the app shell, as intended.
apps/standalone/src/app/components/AppLayout/AppLayout.tsx (3)
30-31: Single source of truth for main container idPromoting pageId avoids duplication and keeps SkipToContent and Page mainContainerId in sync.
37-37: Propagating authType from context to PageNavigationCorrectly threads authType to PageNavigation. Matches the updated API.
103-104: Navigation placementRendering PageNavigation above Outlet aligns with the new subnav UX.
apps/ocp-plugin/src/components/common/WithPageLayout.tsx (2)
9-15: K8S authType wiringAlways rendering PageNavigation with authType={AuthType.K8S} aligns the plugin with the K8S flow.
19-21: OrganizationGuard retainedKeeping the guard preserves context expectations in shared components while simplifying the plugin layout.
apps/standalone/src/app/components/AppLayout/AppToolbar.css (1)
1-3: Scope reduction looks fine; verify subnav alignment is intentional.Removing .fctl-subnav_toolbar overrides will left-align subnav items. Confirm this matches current design in both Standalone and OCP plugin.
libs/ui-components/src/components/common/PageNavigation.tsx (1)
68-71: All PageNavigation usages include the requiredauthType. Verified both instances in TSX files pass the prop.libs/ui-components/src/components/Masthead/CommandLineToolsPage.tsx (1)
24-175: LGTM! Clean refactoring from productName to brandName.The migration is thorough and consistent:
- Type definitions updated (
CommandLineToolsContentProps)- Import added for
getBrandNameutility- All template strings and interpolations updated
- Function parameters renamed appropriately
The logic remains sound, and the branding is now centralized via the
getBrandNameutility.libs/i18n/locales/en/translation.json (4)
37-37: LGTM! New login command translation keys are clear and well-structured.The new authentication flow strings (e.g., "Copy login command", "Get login command", "{{ brandName }} CLI authentication") provide clear user guidance. The use of
{{ brandName }}placeholders ensures consistent branding.Also applies to: 66-72, 89-90
589-593: LGTM! Consistent migration to brandName placeholders.All command-line tools and system restore messages now correctly use
{{ brandName }}instead of the previous{{ productName }}, maintaining consistency with the broader refactoring effort.Also applies to: 811-812
594-604: LGTM! Session token error and success messages are clear.The login success, error handling, and session token messages provide appropriate user feedback for the authentication flow. The strings are grammatically correct and use proper placeholders.
77-77: LGTM! Show more/less toggle strings are now consistent.Both "Show more" (line 596) and "Show less" (line 77) now use consistent lowercase styling, addressing the capitalization inconsistency flagged in the previous review.
Also applies to: 596-596
libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx
Show resolved
Hide resolved
celdrake
force-pushed
the
EDM-1982-copy-login-command
branch
from
6b98d7f to
b629a62
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (2)
105-105: Use URLSearchParams for robust query parsing.The current string split approach for extracting
sessionIdis fragile and will fail if there are multiple query parameters or if the parameter order changes.Apply this diff to use URLSearchParams:
- const sessionId = window.location.search.split('sessionId=')[1]; + const sessionId = new URLSearchParams(window.location.search).get('sessionId');
120-121: Remove unreachable placeholder fallback for API URL
The backend’sgetExternalServiceUrl()always returns a non-empty URL (defaulting tohttps://localhost:3443), sosessionToken.serviceUrlwill never be falsy. In CopyLoginCommandPage.tsx remove the|| DEFAULT_API_URLfallback and usesessionToken.serviceUrldirectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/i18n/locales/en/translation.json(5 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx(1 hunks)libs/ui-components/src/components/common/CopyLoginCommandModal.tsx(1 hunks)libs/ui-components/src/types/extraTypes.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
libs/ui-components/src/components/common/CopyLoginCommandModal.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (5)
proxy/auth/auth.go (1)
SessionTokenResponse(28-31)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
⏰ 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 (7)
libs/ui-components/src/types/extraTypes.ts (1)
79-84: LGTM!The AuthType enum is well-structured and provides clear type safety for authentication flows. The distinct value for AAP (
'AAPGateway') aligns with backend naming conventions.libs/ui-components/src/components/common/CopyLoginCommandModal.tsx (2)
25-26: Good fix for preventing placeholder copy.The loginCommand now correctly resolves to an empty string when
serviceUrlis not yet available, preventing users from copying a placeholder value. This addresses the previous review concern.
52-64: Confirm fallback behavior
On error fetching/config, the catch block setsserviceUrltoDEFAULT_API_URL('<API-URL>'), which enables the Copy button and lets users copy an invalid placeholder. Consider disabling the copy action on error (e.g. show an error state), using a real fallback URL, or explicitly documenting this behavior if intentional.libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (2)
37-96: LGTM! Truncation logic is correct.The component now properly checks if the command exceeds
TRUNCATED_COMMAND_LENGTHbefore adding ellipsis and the expand toggle. This correctly addresses the previous review concern about showing truncation UI when not needed.
154-205: LGTM! Page structure is well-organized.The page component properly handles back navigation to the opener window and renders the login command flow with appropriate layout and styling.
libs/i18n/locales/en/translation.json (2)
37-37: LGTM! Authentication translations are well-structured.The new translation keys for the copy login command flow are clear, consistent, and properly use the
{{ brandName }}placeholder for dynamic branding.Also applies to: 66-66, 68-72, 89-90, 594-604
589-593: Good branding consistency improvement.The migration from
{{ productName }}to{{ brandName }}creates consistent terminology across the application and aligns with the new brand utilities introduced in this PR.Also applies to: 811-812
celdrake
force-pushed
the
EDM-1982-copy-login-command
branch
from
b629a62 to
4458caf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
proxy/auth/auth.go (1)
228-237: Critical: Check token expiration on read.This still has the issue flagged in the previous review. The function doesn't validate whether the token has expired before returning it. Between cleanup intervals (up to 10 minutes), expired tokens can be retrieved and returned. Validate expiration on read to prevent using expired tokens.
Apply this diff to add expiration checks:
func (a *AuthHandler) getSingleUseApiTokenMapping(sessionId string) (*ApiToken, bool) { a.sessionMutex.Lock() defer a.sessionMutex.Unlock() session, exists := a.apiTokenMap[sessionId] - if exists { + if !exists { + return nil, false + } + + // Check if token has expired + now := time.Now() + if now.Sub(session.CreatedAt) > DefaultTokenMaxAge { + delete(a.apiTokenMap, sessionId) + return nil, false + } + if session.ExpiresIn != nil { + expiration := session.CreatedAt.Add(time.Duration(*session.ExpiresIn) * time.Second) + if now.After(expiration) { + delete(a.apiTokenMap, sessionId) + return nil, false + } + } + + // Token is valid, delete and return delete(a.apiTokenMap, sessionId) - } return session, exists }Based on past review comments.
🧹 Nitpick comments (3)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (2)
105-105: Consider using URLSearchParams for more robust query parameter parsing.The current approach using
split('sessionId=')[1]is fragile and could fail if:
- The sessionId is not the last query parameter
- There are multiple query parameters
- The sessionId value itself contains special characters
Apply this diff to use URLSearchParams:
- const sessionId = window.location.search.split('sessionId=')[1]; + const sessionId = new URLSearchParams(window.location.search).get('sessionId');Then update the condition on line 126:
- if (sessionId) { + if (sessionId) { void getSessionToken(); } else {
121-121: Clarify or handle missing serviceUrl more explicitly.When
sessionToken.serviceUrlis null/undefined, the fallback toDEFAULT_API_URL('') produces a command with a placeholder that users must manually replace. This could cause confusion.Consider one of these approaches:
Option 1: Show a warning alert when serviceUrl is missing:
const sessionToken = (await response.json()) as SessionTokenResponse; + if (!sessionToken.serviceUrl) { + setErrorMessage(t('Service URL is not available. Please contact your administrator.')); + return; + } - setLoginCommand(`flightctl login ${sessionToken.serviceUrl || DEFAULT_API_URL} --token=${sessionToken.token}`); + setLoginCommand(`flightctl login ${sessionToken.serviceUrl} --token=${sessionToken.token}`);Option 2: Add an inline alert explaining the placeholder:
If the placeholder is intentional, add a translation key and render an info alert when DEFAULT_API_URL is used, instructing users to replace it with their actual API URL.
proxy/auth/auth.go (1)
19-22: Well-defined token lifecycle constants.The cleanup interval and max age are reasonable for one-time session tokens. Consider making these configurable via environment variables if different deployments need different lifetimes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/i18n/locales/en/translation.json(5 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx(1 hunks)libs/ui-components/src/components/common/CopyLoginCommandModal.tsx(1 hunks)libs/ui-components/src/types/extraTypes.ts(1 hunks)proxy/auth/auth.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-components/src/types/extraTypes.ts
- libs/ui-components/src/components/common/CopyLoginCommandModal.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
proxy/auth/auth.go (3)
proxy/auth/common.go (3)
AuthProvider(27-34)TokenData(18-21)LoginParameters(23-25)proxy/log/log.go (1)
GetLogger(17-19)proxy/config/config.go (2)
FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (5)
proxy/auth/auth.go (1)
SessionTokenResponse(33-36)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
⏰ 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 (16)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (1)
160-164: Be aware of browser restrictions on window.opener and window.close().The current implementation assumes the window was opened via JavaScript (e.g.,
window.open()). If users navigate directly to this page or open it in a new tab manually:
window.openerwill be nullwindow.close()may be blocked by the browserWhile the optional chaining on line 162 handles null
window.openergracefully, you may want to verify the user experience whenwindow.close()fails. Consider adding fallback behavior such as navigating back to a safe page.For example:
const onSwitchBack = () => { (window.opener as Window)?.focus(); const closed = window.close(); // If close() returns undefined, the browser might block it // Consider redirecting to a safe page as fallback if (closed === undefined) { // window.location.href = '/'; // or another safe route } };Note:
window.close()typically returnsundefined, so this check may not be reliable. Test the actual behavior in your target browsers.libs/i18n/locales/en/translation.json (3)
589-593: LGTM! Branding migration to {{ brandName }} looks consistent.The updates from
productNametobrandNameand from{{brand}}to{{ brandName }}(with proper spacing) are consistent and align with the broader branding refactor described in the PR objectives.Also applies to: 811-812
594-604: New login command flow translations look complete.The new translation keys for the login command flow are comprehensive and cover success states, error states, and user guidance. The messaging is clear and user-friendly.
69-69: Verify if both translation keys are needed.Lines 69 and 595 are very similar but use different prepositions:
- Line 69: "authenticate with {{ brandName }}"
- Line 595: "authenticate to {{ brandName }}"
The code in CopyLoginCommandPage.tsx (line 50) uses the "authenticate to" variant. If line 69's "authenticate with" variant is unused or was intended to be the same key, consider consolidating them to avoid confusion.
Run the following script to check if the "authenticate with" variant is used elsewhere in the codebase:
Also applies to: 595-595
proxy/auth/auth.go (12)
29-29: LGTM: Type definitions support the session token feature.The new types and field additions appropriately support the CLI login command feature with proper metadata for token lifecycle management.
Also applies to: 33-36, 49-53
42-47: LGTM: Proper concurrency primitives added.The struct changes correctly introduce thread-safe state management with a mutex, in-memory token store, and shutdown channel for the cleanup goroutine.
199-205: LGTM: Cryptographically secure session ID generation.Correctly uses
crypto/randwith adequate entropy (128 bits) and proper error handling.
240-252: LGTM: Proper cleanup goroutine pattern.The cleanup routine correctly uses a ticker with graceful shutdown via the stop channel.
255-288: LGTM: Comprehensive cleanup logic.The dual expiration checks (age-based hard limit + token-specific expiration) ensure tokens never persist beyond
DefaultTokenMaxAgeregardless of their ownExpiresInvalue. Logging provides good observability without exposing sensitive data.
290-292: LGTM: Clean shutdown mechanism.Properly signals the cleanup goroutine to exit by closing the stop channel.
294-354: LGTM: Session token creation follows secure pattern.The endpoint correctly stores tokens server-side and returns only a session ID. Forcing fresh authentication (line 342) is appropriate for generating CLI tokens distinct from the UI session. However, this endpoint is subject to the unbounded map growth issue flagged earlier.
356-361: LGTM: Appropriate URL resolution for external access.Correctly provides external API URL for CLI clients while falling back to internal URL when external is not configured.
363-394: LGTM: Session token retrieval with proper one-time use.The endpoint correctly retrieves and invalidates the session token. The 404 response for missing sessions is appropriate, though it doesn't distinguish between "never existed", "expired", or "already used" - this ambiguity is acceptable for security.
87-87: LGTM: Method signatures updated for state management.Changing to pointer receivers is necessary for the new mutable state (
apiTokenMap). The addition of theforceparameter (line 115) andAuthTypepopulation (line 186) appropriately support the different authentication flows for UI vs CLI login.Also applies to: 115-116, 129-129, 164-164, 186-186, 396-396
56-59: Verify AuthHandler lifecycle to prevent goroutine leaks.If
NewAuthis called multiple times without properly stopping previous instances, cleanup goroutines from oldAuthHandlerinstances will continue running. Ensure that old instances haveStopTokenCleanup()called before creating new ones.Run the following script to verify if there are mechanisms to ensure proper cleanup:
Also applies to: 79-82
207-224: Add rate limiting or max map size to prevent DoS.The
apiTokenMaphas no size limit. A malicious actor could repeatedly hitCreateSessionTokento fill memory. Consider adding a maximum map size check or rate limiting at this layer or in middleware.Check if rate limiting exists at the HTTP handler level:
celdrake
force-pushed
the
EDM-1982-copy-login-command
branch
from
4458caf to
a09d4cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (1)
107-131: Enhance network and CORS error messaging
Within thecatchofgetSessionToken, add:
- if
!navigator.onLine, callsetErrorMessage(t('No internet connection'))- else if
error instanceof TypeError && error.message.includes('Failed to fetch'), callsetErrorMessage(t('Request blocked by CORS policy'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/i18n/locales/en/translation.json(5 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx(1 hunks)libs/ui-components/src/components/common/CopyButton.tsx(1 hunks)libs/ui-components/src/components/common/CopyLoginCommandModal.tsx(1 hunks)libs/ui-components/src/types/extraTypes.ts(1 hunks)proxy/auth/auth.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-components/src/components/common/CopyLoginCommandModal.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (5)
proxy/auth/auth.go (1)
SessionTokenResponse(33-36)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
proxy/auth/auth.go (3)
proxy/auth/common.go (3)
AuthProvider(27-34)TokenData(18-21)LoginParameters(23-25)proxy/log/log.go (1)
GetLogger(17-19)proxy/config/config.go (2)
FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
⏰ 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). (3)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (16)
proxy/auth/auth.go (8)
19-22: LGTM! Cleanup constants are well-balanced.The 10-minute cleanup interval and 15-minute max age provide a good balance between memory efficiency and operational flexibility for single-use session tokens.
42-53: LGTM! Thread-safe token storage design.The mutex-protected map with cleanup coordination channel follows Go concurrency best practices. The
ApiTokenstructure properly tracks both creation time and expiration for cleanup logic.
55-85: LGTM! Proper initialization with cleanup lifecycle.The initialization correctly sets up the token map and starts the cleanup goroutine only when authentication is enabled, preventing unnecessary background work.
87-127: LGTM! Force re-authentication support added.The
forcequery parameter properly enables fresh authentication flows when needed, and the pointer receiver correctly supports the handler's new stateful behavior.
198-224: LGTM! Cryptographically secure session ID generation.The use of
crypto/randwith 16 bytes (128 bits of entropy) provides strong security guarantees. Token storage is properly mutex-protected.
228-237: LGTM! Single-use retrieval pattern correctly implemented.The immediate deletion ensures tokens can only be retrieved once. The reliance on background cleanup (rather than TTL check on read) is an acceptable trade-off given the immediate-consumption use case.
239-288: LGTM! Comprehensive token cleanup implementation.The dual expiration checks (max age + token expiry) and activity logging provide robust memory management. This properly addresses the single-use token lifecycle concerns from previous reviews.
294-354: LGTM! Well-structured session token creation.The POST/GET split mirrors the
Loginendpoint for consistency. The forced re-authentication in the GET handler (Line 342) correctly ensures a fresh token distinct from the user's current session.libs/ui-components/src/types/extraTypes.ts (1)
78-84: LGTM!The
AuthTypeenum is well-defined with clear string literal values for each authentication provider type. The implementation aligns with the PR objective to support authentication type-based flows.libs/ui-components/src/components/common/CopyButton.tsx (1)
16-50: LGTM!The implementation of the copied state feedback is correct:
- State management with
useStateis appropriate- The
useEffectproperly cleans up the timeout on unmount or whencopiedchanges- The 1-second delay provides clear user feedback
- Tooltip content switches correctly based on the
copiedstatelibs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (4)
34-34: Verify the fallback scenario forDEFAULT_API_URL.The
DEFAULT_API_URLplaceholder ('<API-URL>') is used as a fallback whenserviceUrlis missing from the backend response (line 121). This may result in a confusing login command if the backend doesn't provide the service URL.Is it expected that the backend might not include
serviceUrlin theSessionTokenResponse? If this is an edge case, consider logging an error or displaying a more user-friendly message when the service URL is missing.Based on the backend snippet provided, the
SessionTokenResponsestruct includesServiceUrl, so it should always be present. If the backend guarantees this field, the fallback may be unnecessary defensive code.
37-96: LGTM!The
LoginCommandCopycomponent correctly implements the truncation logic. The expandable section and ellipsis are only shown when the command exceedsTRUNCATED_COMMAND_LENGTH, addressing the concern from the previous review.
160-164: Verify back-to-host behavior in different scenarios.The
onSwitchBackfunction attempts to focus the opener window and close the current window. However, there are potential edge cases:
Direct URL access: If the user navigates directly to this page (not opened via
window.open),window.openerwill benull, andwindow.close()might not work in some browsers for security reasons.Browser restrictions: Some browsers prevent scripts from closing windows that weren't opened by JavaScript.
Consider adding user feedback if the window fails to close, such as displaying a message to manually close the tab.
This is a minor UX concern. If the window doesn't close automatically, users can still close it manually. However, test this behavior across different browsers to ensure a consistent experience.
154-205: LGTM!The
CopyLoginCommandPagecomponent is well-structured:
- Correct use of PatternFly components for layout
- Brand name integration via
getBrandNameutility- Clear separation of concerns with the back-to-host action and body content
- Inline style for custom gap spacing is acceptable for this use case
libs/i18n/locales/en/translation.json (2)
37-37: LGTM!The new authentication and login command translation keys are well-structured:
- Clear and descriptive key names
- User-friendly messages with appropriate guidance
- Consistent use of
{{ brandName }}placeholder for branding- Comprehensive error messages for different scenarios
Also applies to: 66-72, 89-90, 594-604
589-593: LGTM!The branding migration from
productNametobrandNameis consistent and correct:
- All references updated to use
{{ brandName }}placeholder- Consistent spacing around the placeholder
- Aligns with the new
getBrandNameutility introduced in the PRAlso applies to: 811-812
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/standalone/scripts/setup_env.sh (1)
32-33: Document the nip.io dependency and its implications.The external URL relies on nip.io, a third-party wildcard DNS service. If nip.io experiences downtime or DNS resolution issues, the login command functionality will be affected. Consider:
- Adding a comment explaining why nip.io is used (likely for external routing/ingress in standalone mode)
- Documenting the difference between
FLIGHTCTL_SERVER(direct IP) andFLIGHTCTL_SERVER_EXTERNAL(nip.io subdomain)- Noting this dependency in the project's deployment documentation
Apply this diff to add inline documentation:
+# External server URL using nip.io for external DNS resolution +# This allows external clients to access the API via a resolvable hostname export FLIGHTCTL_SERVER_EXTERNAL="https://api.$EXTERNAL_IP.nip.io:3443"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/standalone/scripts/setup_env.sh(2 hunks)proxy/config/config.go(1 hunks)
⏰ 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). (2)
- GitHub Check: integration-tests
- GitHub Check: Lint
🔇 Additional comments (2)
apps/standalone/scripts/setup_env.sh (1)
59-59: LGTM!The echo statement properly logs the new environment variable, maintaining consistency with the existing logging pattern.
proxy/config/config.go (1)
11-11: No issues found; empty value handling is correct.The fallback logic in
proxy/config/ocp.go(lines 16-19) properly handles the empty default value with an explicit empty string check (if externalApiUrl == ""), falling back toFctlApiUrlwhen needed. This implementation is correct and safe.
celdrake
force-pushed
the
EDM-1982-copy-login-command
branch
from
67a1fe7 to
69bb2f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/standalone/src/app/context/AuthContext.ts (3)
105-109: Loading state not cleared on non-200 user-info.UI can hang in “loading” forever.
- if (resp.status !== 200) { - setError('Failed to get user info'); - return; - } + if (resp.status !== 200) { + setError('Failed to get user info'); + setLoading(false); + return; + }
113-117: Also clear loading on fetch error.} catch (err) { // eslint-disable-next-line console.log(err); setError('Failed to get user info'); + setLoading(false); }
64-85: Handle non-OK responses from login endpoint.If the POST fails, we still call resp.json() and proceed; guard with resp.ok and set an error.
- const resp = await fetch(loginEndpoint, { + const resp = await fetch(loginEndpoint, { headers: { 'Content-Type': 'application/json', }, credentials: 'include', method: 'POST', body: JSON.stringify({ code: code, }), }); + if (!resp.ok) { + setError('Failed to complete login flow'); + setLoading(false); + return; + }
♻️ Duplicate comments (3)
libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx (2)
19-20: Fix i18n interpolation placeholders (remove spaces inside {{ }}).The spaced interpolation placeholders
{{ brandName }}should be{{brandName}}without spaces. This issue was previously flagged but remains unresolved. Spaces inside interpolation keys can break i18next variable resolution.Apply this diff:
- '{{ brandName }} is waiting for the device to connect and report its status. It will report a ʼPending syncʼ status until it is able to reconnect. If it has configuration conflicts, it will report a ʼSuspendedʼ status and require manual action to resume.', + '{{brandName}} is waiting for the device to connect and report its status. It will report a ʼPending syncʼ status until it is able to reconnect. If it has configuration conflicts, it will report a ʼSuspendedʼ status and require manual action to resume.',
25-26: Fix i18n interpolation placeholders (remove spaces inside {{ }}).Same issue as line 19: use
{{brandName}}instead of{{ brandName }}.Apply this diff:
- '{{ brandName }} is waiting for devices to connect and report their status. Devices will report a ʼPending syncʼ status until they are able to connect. Devices with configuration conflicts will report a ʼSuspendedʼ status and require manual action to resume.', + '{{brandName}} is waiting for devices to connect and report their status. Devices will report a ʼPending syncʼ status until they are able to connect. Devices with configuration conflicts will report a ʼSuspendedʼ status and require manual action to resume.',libs/ui-components/src/components/common/PageNavigation.tsx (1)
83-94: Add error handling and loading state for session-token creation.The session-token creation flow (lines 89-92) lacks error handling and loading state. Network failures or non-200 responses will fail silently, and users can click the button multiple times while a request is in flight.
Apply this pattern:
const [showCopyLoginCommandModal, setShowCopyLoginCommandModal] = React.useState(false); + const [creatingSessionToken, setCreatingSessionToken] = React.useState(false); + const [sessionTokenError, setSessionTokenError] = React.useState<string>(); const onCopyLoginCommand = async () => { + setSessionTokenError(undefined); if (isAuthTypeWithModal) { setShowCopyLoginCommandModal(true); } else { - // A new tab will open, and it will require the user to login in again, so that a new login token is created. - // After login, the new tab will redirect the user to the copy login command page - const response = await proxyFetch('/login/create-session-token', { credentials: 'include' }); - const { url } = (await response.json()) as { url: string }; - localStorage.setItem(OAUTH_REDIRECT_AFTER_LOGIN_KEY, 'copy-login-command'); - window.open(url, '_blank', 'noopener,noreferrer'); + try { + setCreatingSessionToken(true); + const response = await proxyFetch('/login/create-session-token', { credentials: 'include' }); + if (!response.ok) { + throw new Error(`Failed to create session token: ${response.status}`); + } + const { url } = (await response.json()) as { url: string }; + localStorage.setItem(OAUTH_REDIRECT_AFTER_LOGIN_KEY, 'copy-login-command'); + window.open(url, '_blank', 'noopener,noreferrer'); + } catch (error) { + setSessionTokenError(t('Failed to start login flow. Please try again.')); + console.error('Session token creation failed:', error); + } finally { + setCreatingSessionToken(false); + } } };Then update the button and show errors:
<ToolbarItem> <WithTooltip showTooltip={!isAuthTypeWithModal} content={t('You will be directed to login in order to generate your login token command')} > <Button variant="link" icon={isAuthTypeWithModal ? undefined : <ExternalLinkAltIcon />} onClick={onCopyLoginCommand} + isDisabled={creatingSessionToken} + isLoading={creatingSessionToken} > - {' '} {t('Get login command')} </Button> </WithTooltip> </ToolbarItem> + {sessionTokenError && ( + <ToolbarItem> + <Alert isInline variant="danger" title={sessionTokenError} /> + </ToolbarItem> + )}
🧹 Nitpick comments (9)
proxy/auth/oidc.go (1)
197-203: LGTM! Consider using a constant for the auth type string.The implementation correctly implements the updated
AuthProviderinterface. TheforceReauthparameter is properly forwarded tologinRedirect.Consider defining auth type strings as package-level constants to avoid duplication and typos:
const ( AuthTypeOIDC = "OIDC" AuthTypeAAP = "AAP" AuthTypeK8S = "K8S" // etc. ) func (o *OIDCAuthHandler) GetAuthType() string { return AuthTypeOIDC }libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (1)
34-35: Avoid shipping a placeholder API URL.Backend returns serviceUrl; keep a sane fallback rather than "".
-const DEFAULT_API_URL = '<API-URL>'; +const DEFAULT_API_URL = window.location.origin; @@ - setLoginCommand(`flightctl login ${sessionToken.serviceUrl || DEFAULT_API_URL} --token=${sessionToken.token}`); + setLoginCommand( + `flightctl login ${sessionToken.serviceUrl || DEFAULT_API_URL} --token=${sessionToken.token}`, + );Also applies to: 121-121
apps/standalone/src/app/context/AuthContext.ts (1)
166-168: Minor: guard timeout cleanup.Avoid passing undefined to clearTimeout.
- return () => clearTimeout(refreshRef.current); + return () => { + if (refreshRef.current) clearTimeout(refreshRef.current); + };proxy/auth/auth.go (4)
148-162: Set Content-Type for JSON token responses.Clients rely on correct headers; add it once in respondWithToken to cover Login POST and Refresh.
func respondWithToken(w http.ResponseWriter, tokenData TokenData, expires *int64) { + w.Header().Set("Content-Type", "application/json") err := setCookie(w, tokenData)
115-126: Set Content-Type on Login redirect JSON.response, err := json.Marshal(RedirectResponse{Url: loginUrl}) if err != nil { ... } - if _, err := w.Write(response); err != nil { + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(response); err != nil { log.GetLogger().WithError(err).Warn("Failed to write response") }
186-195: Set Content-Type on GetUserInfo JSON.res, err := json.Marshal(userInfo) if err != nil { ... } - if _, err := w.Write(res); err != nil { + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(res); err != nil { log.GetLogger().WithError(err).Warn("Failed to write response") }
420-428: Set Content-Type on Logout JSON response.response, err := json.Marshal(redirectResp) if err != nil { ... } - if _, err := w.Write(response); err != nil { + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(response); err != nil { log.GetLogger().WithError(err).Warn("Failed to write response") }libs/i18n/locales/en/translation.json (2)
68-73: Unify near-duplicate login command strings.Keep one phrasing to avoid drift (“authenticate to” vs “authenticate with”). The page uses “to”; consider removing or aliasing the other.
Please confirm which you want, then we can:
- remove the unused key, or
- map both keys to the same value for consistency.
Also applies to: 594-599
77-79: Remove stray "more" entry (likely unused).Reduces noise and potential accidental uses.
"Show less": "Show less", - "more": "more", "Learn more": "Learn more",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/ocp-plugin/src/components/common/WithPageLayout.tsx(1 hunks)apps/standalone/scripts/setup_env.sh(2 hunks)apps/standalone/src/app/components/AppLayout/AppLayout.tsx(2 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.css(1 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.tsx(2 hunks)apps/standalone/src/app/context/AuthContext.ts(7 hunks)apps/standalone/src/app/routes.tsx(3 hunks)libs/i18n/locales/en/translation.json(5 hunks)libs/ui-components/src/components/Masthead/CommandLineToolsPage.tsx(6 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.css(1 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx(1 hunks)libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx(1 hunks)libs/ui-components/src/components/common/CopyButton.tsx(1 hunks)libs/ui-components/src/components/common/CopyLoginCommandModal.tsx(1 hunks)libs/ui-components/src/components/common/PageNavigation.css(1 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(5 hunks)libs/ui-components/src/constants.ts(1 hunks)libs/ui-components/src/types/extraTypes.ts(1 hunks)libs/ui-components/src/utils/brand.ts(1 hunks)proxy/app.go(1 hunks)proxy/auth/aap.go(1 hunks)proxy/auth/auth.go(7 hunks)proxy/auth/common.go(1 hunks)proxy/auth/oauth.go(1 hunks)proxy/auth/oidc.go(1 hunks)proxy/config/config.go(1 hunks)proxy/config/ocp.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/ui-components/src/components/common/CopyButton.tsx
- libs/ui-components/src/utils/brand.ts
- libs/ui-components/src/types/extraTypes.ts
- libs/ui-components/src/components/Masthead/CopyLoginCommandPage.css
- proxy/auth/aap.go
- proxy/app.go
🧰 Additional context used
🧬 Code graph analysis (10)
proxy/config/ocp.go (1)
proxy/config/config.go (3)
RBACNs(22-22)FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
libs/ui-components/src/components/SystemRestore/PendingSyncDevicesAlert.tsx (2)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
apps/standalone/src/app/components/AppLayout/AppToolbar.tsx (1)
apps/standalone/src/app/context/AuthContext.ts (1)
AuthContext(25-30)
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/useFetch.ts (1)
useFetch(3-9)libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)libs/ui-components/src/constants.ts (1)
OAUTH_REDIRECT_AFTER_LOGIN_KEY(5-5)
libs/ui-components/src/components/common/CopyLoginCommandModal.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
apps/standalone/src/app/context/AuthContext.ts (3)
libs/ui-components/src/constants.ts (1)
OAUTH_REDIRECT_AFTER_LOGIN_KEY(5-5)apps/standalone/src/app/utils/apiCalls.ts (1)
loginAPI(17-17)libs/ui-components/src/utils/organizationStorage.ts (1)
ORGANIZATION_STORAGE_KEY(1-1)
apps/standalone/src/app/components/AppLayout/AppLayout.tsx (2)
libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)apps/standalone/src/app/context/AuthContext.ts (1)
AuthContext(25-30)
proxy/auth/auth.go (3)
proxy/auth/common.go (3)
AuthProvider(27-34)TokenData(18-21)LoginParameters(23-25)proxy/log/log.go (1)
GetLogger(17-19)proxy/config/config.go (2)
FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
libs/ui-components/src/components/Masthead/CommandLineToolsPage.tsx (1)
libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (5)
proxy/auth/auth.go (1)
SessionTokenResponse(33-36)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
🔇 Additional comments (18)
apps/standalone/src/app/components/AppLayout/AppToolbar.css (1)
1-3: LGTM! CSS simplification aligns with toolbar refactoring.The removal of
.fctl-subnav_toolbarand global masthead styling is consistent with the refactored layout where these styles have been moved toPageNavigation.css. The simplified rule correctly maintains the right-alignment of the app toolbar.proxy/config/config.go (1)
11-11: LGTM! External API URL configuration added correctly.The new
FctlApiUrlExternalconfiguration variable follows the existing pattern and correctly usesgetEnvUrlVarto handle URL formatting (trailing slash removal). The empty string default is appropriate for an optional external URL.apps/standalone/src/app/routes.tsx (2)
71-73: LGTM! Lazy-loaded component follows established patterns.The lazy import for
CopyLoginCommandPageis consistent with other page components in the file and enables proper code splitting.
354-362: LGTM! Standalone route correctly configured.The route structure for
/copy-login-commandcorrectly:
- Exists outside the main AppLayout (as intended per the comment)
- Uses TitledRoute for consistent document title handling
- Follows the same pattern as the existing
/command-line-toolsstandalone routeproxy/config/ocp.go (2)
16-19: LGTM! Fallback logic correctly implemented.The external API URL resolution logic correctly prioritizes
FctlApiUrlExternalwhen available and falls back toFctlApiUrlwhen empty, ensuring the configuration is always populated with a valid URL.
10-10: LGTM! New field properly integrated.The
ExternalApiUrlfield is correctly added to the struct with proper JSON serialization tags and is populated in the response using the resolved external URL value.Also applies to: 22-23
proxy/auth/oauth.go (1)
25-38: LGTM! Force re-authentication correctly implemented.The
forceReauthparameter implementation:
- Correctly modifies the authorize URL to add
prompt=loginonly when requested- Uses proper URL query parameter manipulation
- Follows OAuth 2.0 standards for the
promptparameter- Maintains backward compatibility (when
forceReauthis false, URL is unchanged)libs/ui-components/src/components/common/PageNavigation.css (1)
1-16: LGTM! New CSS styling correctly implements global actions masthead.The styling correctly:
- Right-aligns the subnav toolbar items
- Sets the masthead height to match navigation menu items (3.5rem as documented)
- Uses PatternFly v5 CSS custom properties appropriately
- Applies consistent padding and dark background color
The CSS properly supports the new global actions UI (organization switcher, copy login command) introduced in this PR.
apps/ocp-plugin/src/components/common/WithPageLayout.tsx (2)
11-11: LGTM! PageNavigation correctly configured for OCP plugin.The
authType={AuthType.K8S}is the correct authentication type for the OCP plugin environment, which uses Kubernetes authentication. This aligns with the new authType-based behavior introduced across the PR.
9-14: LGTM! Layout refactoring correctly integrates OrganizationGuard.The updated layout structure:
- Properly wraps content with
OrganizationGuardto enforce organization selection- Maintains the
PageNavigationand children rendering within the guard boundary- Follows the established pattern for organization-aware layouts
Also applies to: 19-21
apps/standalone/scripts/setup_env.sh (1)
32-33: LGTM!The new
FLIGHTCTL_SERVER_EXTERNALvariable follows the existing pattern and properly uses nip.io for external DNS resolution.apps/standalone/src/app/components/AppLayout/AppToolbar.tsx (1)
73-80: LGTM!The migration from
authEnabledboolean toauthTypeenum is clean and maintains equivalent logic. The checkauthType !== AuthType.DISABLEDproperly replaces the previousauthEnabledcheck.apps/standalone/src/app/components/AppLayout/AppLayout.tsx (2)
30-30: Good refactor: pageId moved to module scope.Moving
pageIdout of the component body prevents redeclaration on each render.
37-37: LGTM!The
authTypeis properly extracted fromAuthContextand passed down toPageNavigation, enabling auth-type-aware navigation behavior.Also applies to: 103-103
libs/ui-components/src/components/common/PageNavigation.tsx (1)
68-68: LGTM!The
authTypeprop enables proper branching for different authentication flows.libs/ui-components/src/constants.ts (1)
1-5: LGTM!The export style change improves readability, and the new
OAUTH_REDIRECT_AFTER_LOGIN_KEYconstant is properly named and used by the login flow.proxy/auth/common.go (1)
32-33: All AuthProvider implementations are correctly updated.Verification confirms that both AuthProvider implementers in the codebase have been updated with the new interface methods:
- OIDCAuthHandler (oidc.go): All 6 interface methods implemented, including GetLoginRedirectURL (line 197) and GetAuthType (line 201)
- AAPAuthHandler (aap.go): All 6 interface methods implemented, including GetLoginRedirectURL (line 165) and GetAuthType (line 169)
Note: The codebase contains only OIDC and AAP as AuthProvider implementations. The oauth.go file contains only helper functions for token exchange, not an AuthProvider struct. No K8S provider exists.
libs/ui-components/src/components/Masthead/CommandLineToolsPage.tsx (1)
138-176: LGTM: branding refactor and error handling are consistent.No functional issues found.
| React.useEffect(() => { | ||
| const loadServiceUrl = async () => { | ||
| try { | ||
| const configResp = await proxyFetch(`/config`, { credentials: 'include' }); | ||
| const config = (await configResp.json()) as { externalApiUrl: string }; | ||
| setServiceUrl(config.externalApiUrl); | ||
| } catch (error) { | ||
| // Shouldn't happen, but use the default API URL instead | ||
| setServiceUrl(DEFAULT_API_URL); | ||
| } | ||
| }; | ||
|
|
||
| void loadServiceUrl(); | ||
| }, [proxyFetch]); |
There was a problem hiding this comment.
Re-examine error handling: placeholder can still be copied.
Although the past review comment was marked as addressed, the error path on line 36 sets serviceUrl to DEFAULT_API_URL ('<API-URL>'), which means users can still copy the placeholder value if the fetch fails. The comment on line 35 says "Shouldn't happen," but errors should be handled gracefully.
Consider these improvements:
const [serviceUrl, setServiceUrl] = React.useState('');
+ const [loadError, setLoadError] = React.useState(false);
const loginCommand = serviceUrl ? String.raw`flightctl login ${serviceUrl} --token=$(oc whoami -t)` : '';
React.useEffect(() => {
const loadServiceUrl = async () => {
try {
const configResp = await proxyFetch(`/config`, { credentials: 'include' });
const config = (await configResp.json()) as { externalApiUrl: string };
setServiceUrl(config.externalApiUrl);
} catch (error) {
- // Shouldn't happen, but use the default API URL instead
- setServiceUrl(DEFAULT_API_URL);
+ setLoadError(true);
}
};
void loadServiceUrl();
}, [proxyFetch]);Then in the UI, show an error state instead of allowing the placeholder to be copied:
<StackItem>
+ {loadError ? (
+ <Alert variant="danger" title={t('Failed to load API URL')} />
+ ) : (
<CodeBlock
actions={
loginCommand
? [
<CodeBlockAction key="copy-command">
<CopyButton text={loginCommand} ariaLabel={t('Copy login command')} />
</CodeBlockAction>,
]
: undefined
}
>
<CodeBlockCode>{loginCommand || t('Loading...')}</CodeBlockCode>
</CodeBlock>
+ )}
</StackItem>🤖 Prompt for AI Agents
In libs/ui-components/src/components/common/CopyLoginCommandModal.tsx around
lines 28 to 41, the catch block currently assigns the placeholder
DEFAULT_API_URL which allows users to copy an invalid URL; instead, on fetch
failure set serviceUrl to null/undefined (or a dedicated error flag), log the
error, and surface an error state in the component (e.g., show an error message
and disable or hide the copy button) so the placeholder cannot be copied; ensure
proxyFetch errors are caught and the UI branches for loading / success / error
appropriately.
There was a problem hiding this comment.
It's intentional to show the "degraded" command rather than an error.
There was a problem hiding this comment.
@celdrake, understood! I'll note that this is an intentional design choice. The degraded command with the placeholder provides a fallback rather than blocking the user entirely.
celdrake
force-pushed
the
EDM-1982-copy-login-command
branch
from
69bb2f9 to
8bc50f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/ui-components/src/components/common/CopyLoginCommandModal.tsx (1)
34-36: Error path still allows copying the placeholder URL.The catch block sets
serviceUrltoDEFAULT_API_URL, which enables the copy button with the<API-URL>placeholder. This issue was flagged in a previous review but remains unresolved. Users should not be able to copy an invalid command.Apply this diff to properly handle the error state:
const [serviceUrl, setServiceUrl] = React.useState(''); + const [loadError, setLoadError] = React.useState(false); const loginCommand = serviceUrl ? String.raw`flightctl login ${serviceUrl} --token=$(oc whoami -t)` : ''; React.useEffect(() => { const loadServiceUrl = async () => { try { const configResp = await proxyFetch(`/config`, { credentials: 'include' }); const config = (await configResp.json()) as { externalApiUrl: string }; setServiceUrl(config.externalApiUrl); } catch (error) { - // Shouldn't happen, but use the default API URL instead - setServiceUrl(DEFAULT_API_URL); + setLoadError(true); } }; void loadServiceUrl(); }, [proxyFetch]);Then update the UI to show an error state:
<StackItem> + {loadError ? ( + <Alert variant="danger" title={t('Failed to load API URL')} isInline /> + ) : ( <CodeBlock actions={ loginCommand ? [ <CodeBlockAction key="copy-command"> <CopyButton text={loginCommand} ariaLabel={t('Copy login command')} /> </CodeBlockAction>, ] : undefined } > <CodeBlockCode>{loginCommand || t('Loading...')}</CodeBlockCode> </CodeBlock> + )} </StackItem>libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (1)
110-112: Remove leading slash and URL-encode query parameter.The endpoint path includes a leading slash and the sessionId is not URL-encoded. Per a previous review,
fetchUiProxyconstructs URLs as${serviceUrl}/api/${endpoint}, so the leading slash creates a malformed path (/api//login/...). Additionally, sessionId should be URL-encoded for safety.Apply this diff:
- const response = await proxyFetch(`/login/get-session-token?sessionId=${sessionId}`, { + const response = await proxyFetch( + `login/get-session-token?sessionId=${encodeURIComponent(sessionId || '')}`, + { credentials: 'include', - }); + } + );Note: The same issue exists in
libs/ui-components/src/components/common/PageNavigation.tsxat line 89 with/login/create-session-token(mentioned in the past review).
🧹 Nitpick comments (1)
proxy/auth/auth.go (1)
290-292: Make StopTokenCleanup idempotent to prevent panics.Closing an already-closed channel will panic. If
StopTokenCleanupis called multiple times (e.g., in deferred shutdown handlers), the application will crash.Apply this diff to use
sync.Oncefor idempotent shutdown:type AuthHandler struct { provider AuthProvider sessionMutex sync.Mutex apiTokenMap map[string]*ApiToken stopTokenCleanup chan struct{} + stopOnce sync.Once }Then update the method:
func (a *AuthHandler) StopTokenCleanup() { - close(a.stopTokenCleanup) + a.stopOnce.Do(func() { + close(a.stopTokenCleanup) + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/i18n/locales/en/translation.json(5 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx(1 hunks)libs/ui-components/src/components/common/CopyButton.tsx(1 hunks)libs/ui-components/src/components/common/CopyLoginCommandModal.tsx(1 hunks)libs/ui-components/src/types/extraTypes.ts(1 hunks)proxy/auth/auth.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-components/src/components/common/CopyButton.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
proxy/auth/auth.go (3)
proxy/auth/common.go (3)
AuthProvider(27-34)TokenData(18-21)LoginParameters(23-25)proxy/log/log.go (1)
GetLogger(17-19)proxy/config/config.go (2)
FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
libs/ui-components/src/components/common/CopyLoginCommandModal.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (5)
proxy/auth/auth.go (1)
SessionTokenResponse(33-36)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
🔇 Additional comments (13)
libs/ui-components/src/types/extraTypes.ts (1)
79-84: LGTM!The AuthType enum is well-defined with clear, distinct values for each authentication provider. The implementation is straightforward and aligns with the PR's goal of supporting different authentication flows.
libs/i18n/locales/en/translation.json (1)
37-37: LGTM!The translation updates systematically replace
productNamewithbrandNameand introduce comprehensive login command messaging. The consistent spacing in template variables ({{ brandName }}) and clear, user-friendly error messages enhance the UX.Also applies to: 66-72, 89-90, 589-593, 594-603, 810-811
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (2)
37-96: LGTM!The truncation logic properly checks if the command exceeds
TRUNCATED_COMMAND_LENGTHbefore showing the expand/collapse toggle. This addresses the previous review feedback about only showing "..." when content is actually truncated.
160-164: LGTM!The back-button handler correctly uses optional chaining to safely focus the opener window before closing the current tab, providing a smooth UX for the popup authentication flow.
proxy/auth/auth.go (9)
1-22: LGTM! Imports and constants are well-structured.The cryptographically secure random generation imports, synchronization primitives, and reasonable token lifecycle constants (10-minute cleanup interval, 15-minute max age) are appropriate for the session-token feature.
24-53: LGTM! Type definitions support the session-token flow.The new types (
SessionTokenResponse,ApiToken) and updated structures (UserInfoResponsewithAuthType,AuthHandlerwith session management fields) properly model the one-time token retrieval feature.
55-85: LGTM! Constructor properly initializes session management.The initialization of
apiTokenMapandstopTokenCleanupchannel, along with the conditional start of the cleanup goroutine, correctly establishes the token lifecycle infrastructure.
87-127: LGTM! Login method correctly supports forced re-authentication.The addition of the
forcequery parameter and its threading through toGetLoginRedirectURLenables the UI to trigger fresh authentication flows when needed for the CLI login command feature.
164-196: LGTM! AuthType now included in user info response.Populating
AuthTypefrom the provider enables the frontend to make authentication-method-specific UI decisions (e.g., showing/hiding the CLI login command option).
198-205: LGTM! Cryptographically secure session ID generation.The use of
crypto/randwith 16 bytes (128 bits of entropy) provides adequate security for session identifiers.
294-354: LGTM! CreateSessionToken correctly implements the token-exchange flow.The two-step flow (GET for login redirect, POST for token exchange and mapping) properly supports the CLI login command feature. Forcing fresh authentication ensures a distinct token is generated.
356-361: LGTM! External URL resolution is straightforward.The fallback to internal URL when external is not configured is appropriate for the CLI login command feature.
363-394: LGTM! GetSessionToken correctly implements single-use retrieval.The endpoint properly retrieves the token via the single-use mapping, returns the service URL and token, and relies on the deletion in
getSingleUseApiTokenMappingto enforce one-time use.
| func (a *AuthHandler) getSingleUseApiTokenMapping(sessionId string) (*ApiToken, bool) { | ||
| a.sessionMutex.Lock() | ||
| defer a.sessionMutex.Unlock() | ||
|
|
||
| session, exists := a.apiTokenMap[sessionId] | ||
| if exists { | ||
| delete(a.apiTokenMap, sessionId) | ||
| } | ||
| return session, exists | ||
| } |
There was a problem hiding this comment.
Missing TTL enforcement in token retrieval.
The past review thread indicates that TTL checking was added to getSingleUseApiTokenMapping to prevent retrieval of stale tokens, but the current implementation only deletes the mapping without verifying age or expiration. Between cleanup cycles (10-minute intervals), stale tokens can still be retrieved.
Apply this diff to add TTL enforcement on retrieval:
func (a *AuthHandler) getSingleUseApiTokenMapping(sessionId string) (*ApiToken, bool) {
a.sessionMutex.Lock()
defer a.sessionMutex.Unlock()
session, exists := a.apiTokenMap[sessionId]
- if exists {
- delete(a.apiTokenMap, sessionId)
+ if !exists {
+ return nil, false
+ }
+
+ // Expire stale sessions
+ if time.Since(session.CreatedAt) > DefaultTokenMaxAge {
+ delete(a.apiTokenMap, sessionId)
+ return nil, false
+ }
+
+ // Check ExpiresIn if set
+ if session.ExpiresIn != nil {
+ expiration := session.CreatedAt.Add(time.Duration(*session.ExpiresIn) * time.Second)
+ if time.Now().After(expiration) {
+ delete(a.apiTokenMap, sessionId)
+ return nil, false
+ }
}
+
+ delete(a.apiTokenMap, sessionId)
return session, exists
}🤖 Prompt for AI Agents
In proxy/auth/auth.go around lines 228 to 237, the method
getSingleUseApiTokenMapping deletes the mapping but does not enforce TTL on
retrieval; modify it to check the token's creation/time field against the
configured TTL (e.g., a.tokenTTL or a.singleUseTTL) while holding sessionMutex,
and if the token is older than TTL delete the mapping and return nil,false; only
return the token if it exists and is within TTL. Ensure you use time.Now() to
compute age, keep the existing mutex lock/unlock, and preserve deletion behavior
for expired tokens to prevent reuse.
There was a problem hiding this comment.
The purpose of the clean-up cycles is to free memory from tokens were never retrieved in the first place.
The current implementation almost guarantees that the token should be retrieved immediately, so it's not a problem if the token is requested between cycles (since anyway it can only be consumed once)
There was a problem hiding this comment.
@celdrake, thank you for clarifying the design intent. Your explanation makes sense—the single-use consumption pattern is the primary security control, and the cleanup cycles serve as a memory management mechanism for tokens that are never retrieved (edge cases like abandoned flows or errors).
Since the token is deleted immediately on first retrieval, the immediate-retrieval assumption is reasonable for the typical flow. The concern about stale tokens between cleanup cycles is mitigated by the single-use guarantee.
If you'd like to help future reviewers understand this design choice, you could consider adding a brief comment above getSingleUseApiTokenMapping or startTokenCleanup explaining that cleanup is for memory management of unretrieved tokens rather than TTL enforcement on retrieval. But that's optional—the current implementation is sound given your design goals.
✏️ Learnings added
Learnt from: celdrake
PR: flightctl/flightctl-ui#357
File: proxy/auth/auth.go:228-237
Timestamp: 2025-10-21T13:53:29.911Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Added a clarifying comment to "startTokenCleanup"
There was a problem hiding this comment.
@celdrake, thank you for adding that clarifying comment! 🎉 That will help future reviewers and maintainers understand the design intent behind the cleanup mechanism—distinguishing memory management from security controls is an important clarification.
🧠 Learnings used
Learnt from: celdrake
PR: flightctl/flightctl-ui#357
File: proxy/auth/auth.go:228-237
Timestamp: 2025-10-21T13:53:29.911Z
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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
celdrake
force-pushed
the
EDM-1982-copy-login-command
branch
from
8bc50f7 to
75cdd4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
proxy/auth/auth.go (1)
79-83: Ensure cleanup goroutine is stopped on shutdown
Wire AuthHandler.StopTokenCleanup() into the app’s graceful shutdown path. This avoids leaking a goroutine.libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (1)
110-112: Drop leading slash and URL‑encode sessionId in proxyFetch.Prevents double slashes and ensures a safe query param.
- const response = await proxyFetch(`/login/get-session-token?sessionId=${sessionId}`, { + const response = await proxyFetch( + `login/get-session-token?sessionId=${encodeURIComponent(sessionId ?? '')}`, + { credentials: 'include', - }); + } + );
🧹 Nitpick comments (13)
proxy/auth/auth.go (5)
117-125: Set Content-Type: application/json on all JSON responsesSeveral JSON responses omit the header; add it for consistency and correct clients.
@@ - if _, err := w.Write(response); err != nil { + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(response); err != nil { log.GetLogger().WithError(err).Warn("Failed to write response") } @@ - exp, err := json.Marshal(ExpiresInResp{ExpiresIn: expires}) + w.Header().Set("Content-Type", "application/json") + exp, err := json.Marshal(ExpiresInResp{ExpiresIn: expires}) @@ - if _, err := w.Write(res); err != nil { + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(res); err != nil { log.GetLogger().WithError(err).Warn("Failed to write response") } @@ - if _, err := w.Write(response); err != nil { + w.Header().Set("Content-Type", "application/json") + if _, err := w.Write(response); err != nil { log.GetLogger().WithError(err).Warn("Failed to write response") } @@ - response, err := json.Marshal(redirectResp) + w.Header().Set("Content-Type", "application/json") + response, err := json.Marshal(redirectResp)Also applies to: 148-162, 186-196, 344-352, 420-428
42-47: Make StopTokenCleanup idempotent to avoid double-close panicsClose on a channel twice panics. Guard with sync.Once.
type AuthHandler struct { provider AuthProvider sessionMutex sync.Mutex apiTokenMap map[string]*ApiToken stopTokenCleanup chan struct{} + stopOnce sync.Once } @@ func (a *AuthHandler) StopTokenCleanup() { - close(a.stopTokenCleanup) + a.stopOnce.Do(func() { close(a.stopTokenCleanup) }) }Also applies to: 290-292
254-256: Fix comment typo for constant nameUse DefaultTokenMaxAge to match the identifier.
-// Removes expired tokens and tokens older than DefaultMaxTokenAge from the apiTokenMap +// Removes expired tokens and tokens older than DefaultTokenMaxAge from the apiTokenMap
226-237: Document “single-use, no TTL-on-read” intent at retrieval pointAdd a brief comment here to reflect the design (cleanup is for unretrieved tokens; retrieval deletes regardless of age). Helps future reviewers.
// Gets the auth token associated to a given sessionId. // Deletes the mapping immediately after retrieval so it can only be used once. +// Note: TTL is not enforced on retrieval by design; tokens are expected to be fetched immediately. +// Cleanup routines remove unretrieved/old entries to bound memory usage. (See startTokenCleanup/cleanupExpiredTokens.) func (a *AuthHandler) getSingleUseApiTokenMapping(sessionId string) (*ApiToken, bool) {Confirm this mirrors your clarifying comment in startTokenCleanup. Based on learnings.
219-221: Prefer defer for unlock for future-proofingUsing defer reduces risk if code grows between lock/unlock.
- a.sessionMutex.Lock() - a.apiTokenMap[sessionId] = session - a.sessionMutex.Unlock() + a.sessionMutex.Lock() + defer a.sessionMutex.Unlock() + a.apiTokenMap[sessionId] = sessionlibs/ui-components/src/types/extraTypes.ts (1)
79-84: Use string-literal union (and confirm external value for AAP).Enums add runtime code; a string-literal union (with a frozen object) keeps types without payload. Also, please confirm the server/API expects "AAPGateway" for the AAP case.
-export enum AuthType { - DISABLED = 'DISABLED', - OIDC = 'OIDC', - K8S = 'K8S', - AAP = 'AAPGateway', -} +export const AuthType = { + DISABLED: 'DISABLED', + OIDC: 'OIDC', + K8S: 'K8S', + AAP: 'AAPGateway', +} as const; +export type AuthType = typeof AuthType[keyof typeof AuthType];libs/ui-components/src/components/common/CopyLoginCommandModal.tsx (2)
31-31: Remove leading slash in proxyFetch endpoint.Avoid double slashes in the proxy URL and stay consistent with other calls.
- const configResp = await proxyFetch(`/config`, { credentials: 'include' }); + const configResp = await proxyFetch(`config`, { credentials: 'include' });
35-37: If degraded fallback is used, make it explicit to the user.Since copying '' is intentional on fetch failure, add an inline warning so users know to replace it.
- } catch (error) { - // Shouldn't happen, but use the default API URL instead - setServiceUrl(DEFAULT_API_URL); - } + } catch (error) { + // Degraded: use placeholder and warn the user to replace it. + setServiceUrl(DEFAULT_API_URL); + }<StackItem> <Alert variant="info" title={t('Next steps')}> {t( "After running this command, you'll be authenticated and can use the {{ brandName }} CLI to manage your edge devices from your terminal.", { brandName }, )} </Alert> </StackItem> + {serviceUrl === DEFAULT_API_URL && ( + <StackItem> + <Alert + variant="warning" + isInline + title={t('API URL unavailable')} + > + {t('Replace <API-URL> with your service URL before running the command.')} + </Alert> + </StackItem> + )}Also applies to: 66-73
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (3)
114-117: Avoid shadowing state with a local const.Rename the local variable for clarity.
- const tokenNotFound = response.status === 404; - if (tokenNotFound) { - setTokenNotFound(tokenNotFound); + const isNotFound = response.status === 404; + if (isNotFound) { + setTokenNotFound(true); }
147-149: Add an accessible label to the loading spinner.Improves screen reader experience.
- } else if (!loginCommand) { - return <Spinner size="xl" />; - } + } else if (!loginCommand) { + return <Spinner size="xl" aria-label={t('Loading...')} />; + }
47-49: “Login successful!” may be misleading here.This page prepares a command; consider “Login command ready” to avoid implying the CLI auth already happened.
- <Alert variant="success" isInline title={t('Login successful!')} /> + <Alert variant="success" isInline title={t('Login command ready')} />Note: Add/adjust the translation key accordingly (see translation.json comment).
libs/i18n/locales/en/translation.json (2)
594-596: Align copy with actual flow (“command ready” vs “login successful”).Avoid implying the user is already authenticated.
- "Login successful!": "Login successful!", + "Login command ready": "Login command ready",And update the component to use this key.
89-91: Minor grammar tweak for clarity.- "You will be directed to login in order to generate your login token command": "You will be directed to login in order to generate your login token command", + "You will be directed to log in to generate your login token command": "You will be directed to log in to generate your login token command",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/i18n/locales/en/translation.json(5 hunks)libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx(1 hunks)libs/ui-components/src/components/common/CopyButton.tsx(1 hunks)libs/ui-components/src/components/common/CopyLoginCommandModal.tsx(1 hunks)libs/ui-components/src/types/extraTypes.ts(1 hunks)proxy/auth/auth.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-components/src/components/common/CopyButton.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T13:53:29.911Z
Learnt from: celdrake
PR: flightctl/flightctl-ui#357
File: proxy/auth/auth.go:228-237
Timestamp: 2025-10-21T13:53:29.911Z
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/auth.go
🧬 Code graph analysis (3)
proxy/auth/auth.go (2)
proxy/auth/common.go (3)
AuthProvider(27-34)TokenData(18-21)LoginParameters(23-25)proxy/config/config.go (2)
FctlApiUrlExternal(11-11)FctlApiUrl(10-10)
libs/ui-components/src/components/common/CopyLoginCommandModal.tsx (3)
libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
libs/ui-components/src/components/Masthead/CopyLoginCommandPage.tsx (5)
proxy/auth/auth.go (1)
SessionTokenResponse(33-36)libs/ui-components/src/hooks/useTranslation.ts (1)
useTranslation(5-8)libs/ui-components/src/utils/error.ts (1)
getErrorMessage(6-17)libs/ui-components/src/hooks/useAppContext.tsx (1)
useAppContext(122-122)libs/ui-components/src/utils/brand.ts (1)
getBrandName(3-4)
⏰ 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: Build
- GitHub Check: Lint
🔇 Additional comments (3)
proxy/auth/auth.go (3)
198-205: Secure, collision-resistant session IDs — LGTM
186-186: Surfacing AuthType in GetUserInfo — LGTMKeeps UI logic simple for provider-specific flows.
356-361: External service URL resolution — LGTMClear precedence: external override fallback to internal.
|
Marked as Draft until I know whether this could impact the Auth providers feature. |
1 participant
Adds a new "Get login command" option to the Subnav.
Requires backend changes from PR
Depending on the authentication provider:
In OCP plugin, new token cannot be generaetd
https://github.com/user-attachments/assets/1d69d54a-bbbf-4166-bdf5-14918c5405b1
In Standalone, new token can be generated.
https://github.com/user-attachments/assets/8dc2f9c8-9d78-45b4-bc54-64f9560c32d5
Summary by CodeRabbit
New Features
Improvements
Chores