Skip to content

feature: Oauth authentication#41

Open
Cragsmann wants to merge 6 commits into
functions-dev:masterfrom
Cragsmann:SRVOCF-856--oauth-authentication
Open

feature: Oauth authentication#41
Cragsmann wants to merge 6 commits into
functions-dev:masterfrom
Cragsmann:SRVOCF-856--oauth-authentication

Conversation

@Cragsmann

@Cragsmann Cragsmann commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Screen.Recording.2026-06-15.at.13.32.53.mov
  • Add full GitHub OAuth authentication with popup-based authorization code flow + PKCE
  • Keep PAT (Personal Access Token) as fallback authentication method
  • Add disconnect functionality via dropdown menu with confirmation modal
  • Split UserAvatar into smaller, testable components

Changes

Backend (backend/main.go)

  • GET /api/oauth/config: returns { client_id, enabled } from env vars
  • POST /api/oauth/callback: proxies authorization code + PKCE verifier to GitHub's token endpoint, returns access token

Frontend

  • OAuthService (src/common/services/oauth/): popup-based PKCE flow with state/CSRF validation, popup-close detection, and sessionStorage cleanup
  • OAuthCallbackPage (src/pages/oauth-callback/): minimal popup callback page that postMessages the authorization code back to the opener window
  • PatModal (src/common/components/PatModal.tsx): extracted from UserAvatar, OAuth button conditionally enabled based on backend config, "or" divider, PAT input
  • DisconnectConfirmModal (src/common/components/DisconnectConfirmModal.tsx): extracted confirmation dialog for disconnect action
  • UserAvatar (src/common/components/UserAvatar.tsx): logged-in state shows dropdown with disconnect option, not-logged-in state shows connect button + PatModal
  • Storage keys: unified TOKEN_KEY replaces hardcoded 'func-console-pat' across codebase, AUTH_METHOD_KEY tracks 'pat' | 'oauth'

Helm chart

  • values.yaml: plugin.oauth.enabled, githubClientId, githubSecretName, githubSecretKey
  • deployment.yaml: conditionally injects GITHUB_CLIENT_ID and GITHUB_CLIENT_SECRET (from K8s Secret) as env vars

Test plan

  • 147 tests passing (35 new tests for PatModal, DisconnectConfirmModal, and updated UserAvatar)
  • OAuth flow tested end-to-end: backend config, popup opens to GitHub, user authorizes, popup closes, user authenticated
  • PAT flow still works as fallback
  • Without OAuth env vars, button shows disabled with "Coming soon" tooltip
  • Disconnect clears token, user, and auth method from sessionStorage

Relates to SRVOCF-856

@Cragsmann Cragsmann requested review from matejvasek and twoGiants and removed request for twoGiants June 15, 2026 11:39

@matejvasek matejvasek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

via Claude:

Review

The architecture is solid: PKCE with server-side token exchange keeps the client secret off the frontend, the popup flow avoids full-page redirects in the console plugin context, and the component decomposition (PatModal, DisconnectConfirmModal) is a clear improvement. Good test coverage at 35 new tests.

Issues

1. Security: client_id exposed when OAuth is disabled

handleOAuthConfig returns client_id even when enabled is false. If a cluster has GITHUB_CLIENT_ID set but not GITHUB_CLIENT_SECRET, the client ID leaks to unauthenticated callers. Consider returning an empty client_id when enabled is false:

func handleOAuthConfig(w http.ResponseWriter, _ *http.Request) {
    enabled := githubClientID != "" && githubClientSecret != ""
    cid := ""
    if enabled {
        cid = githubClientID
    }
    // ...
}

2. Security: GitHub token response forwarded verbatim to the client

handleOAuthCallback does w.Write(body), forwarding the entire GitHub response body. GitHub's token response may include fields beyond access_token (e.g., scope, token_type, and potentially future fields). It would be safer to extract only access_token and return that:

token, _ := ghResp["access_token"].(string)
if token == "" {
    jsonError(w, "no access_token in GitHub response", http.StatusBadGateway)
    return
}
json.NewEncoder(w).Encode(map[string]string{"access_token": token})

3. Bug: PAT_KEY alias creates a migration trap

In types.ts:

export const TOKEN_KEY = 'func-console-token';
export const PAT_KEY = TOKEN_KEY;

The storage key changed from 'func-console-pat' to 'func-console-token', but there is no migration. Any user who authenticated before this PR will have their session silently dropped (their func-console-pat entry is orphaned). If that is intentional since this is a PoC, the PAT_KEY alias is misleading and should just be removed. If it is not intentional, a migration read of the old key would be needed.

4. Bug: ForgeConnectionProvider not notified on disconnect

disconnect() in useUserAvatar clears sessionStorage and local state, but never calls connectToForge (or an equivalent disconnect callback) on ForgeConnectionContext. This means ForgeConnectionContext.isActive remains true and connectionId is stale after disconnect, so downstream consumers (e.g., data-fetching hooks) won't know the user logged out until a full page refresh.

5. Singleton OAuthService caches config forever

OAuthService.#configCache is set once and never invalidated. If a cluster admin enables OAuth after the plugin is loaded, the user must do a full page refresh. This might be acceptable for a PoC, but worth noting.

6. Missing redirect_uri in token exchange

The handleOAuthCallback handler sends client_id, client_secret, code, and code_verifier to GitHub's token endpoint, but omits redirect_uri. GitHub's OAuth docs state that redirect_uri is required if it was included in the authorization request (and the frontend does include it). This may cause token exchange failures in some configurations. Worth verifying empirically, since GitHub may not strictly enforce it in all cases.

7. Missing error status code check for GitHub response

In handleOAuthCallback, the code reads the body and checks for a JSON error field, but never checks resp.StatusCode. If GitHub returns a non-200 with a non-JSON body, the json.Unmarshal will fail with a generic "invalid response from GitHub" instead of surfacing the actual HTTP status.

Nits

  • Inline style in PatModal: style={{ marginBottom: '1rem' }} on the Alert. Should use a PF utility class like pf-v6-u-mb-md for consistency with the rest of the PR (which already uses utility classes like pf-v6-u-my-md).
  • onOpenChange parameter naming: onOpenChange={(setOpen) => !setOpen && closeDropdown()} -- the parameter name setOpen is misleading since it is actually isOpen. Consider (open) => !open && closeDropdown().
  • Unused AuthMethod type: export type AuthMethod = 'pat' | 'oauth' is exported but never imported anywhere. The literal 'pat' | 'oauth' is used inline in finishLogin.
  • No tests for OAuthService or OAuthCallbackPage: The core PKCE/popup logic and the callback page rendering/postMessage behavior are untested.

Summary

The main items to address before merge are #3 (silent session drop / dead alias) and #4 (disconnect not propagated to context). Items #1, #2, and #6 are security/correctness fixes that are straightforward. The rest are minor.

@Cragsmann

Cragsmann commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the thorough review! Pushed fixes in f86e356. Here's the rundown:

Issues

# Item Status
1 Security: client_id exposed when OAuth is disabled Fixed. handleOAuthConfig now returns an empty client_id when enabled is false.
2 Security: GitHub token response forwarded verbatim Fixed. handleOAuthCallback now extracts only access_token and returns that.
3 Bug: PAT_KEY alias / migration trap Fixed. Removed the PAT_KEY alias and the unused AuthMethod type. All imports updated to use TOKEN_KEY directly. No migration needed since this is pre-release/PoC.
4 Bug: Disconnect not propagated to ForgeConnectionContext Fixed. Added disconnectFromForge to the context interface and provider, and UserAvatar.disconnect() now calls it to clear isActive and user.
5 Singleton OAuthService caches config forever Acknowledged, acceptable for PoC.
6 Missing redirect_uri in token exchange Verified empirically: GitHub does not enforce redirect_uri on the token exchange when code_verifier (PKCE) is present. Leaving as-is for now, can add if we hit issues.
7 Missing error status code check for GitHub response Fixed. Added resp.StatusCode != http.StatusOK check before reading the body.

Nits

  • Inline style in PatModal: Fixed, replaced with pf-v6-u-mb-md utility class.
  • onOpenChange parameter naming: Fixed, renamed setOpen to open.

Comment thread backend/main.go Outdated
}
}

var (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put this new functionality into a new file e.g. oauth_handler.go

@Cragsmann Cragsmann marked this pull request as ready for review June 16, 2026 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants