feature: Oauth authentication#41
Conversation
Cragsmann
requested review from
matejvasek and
twoGiants
and removed request for
twoGiants
There was a problem hiding this comment.
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 likepf-v6-u-mb-mdfor consistency with the rest of the PR (which already uses utility classes likepf-v6-u-my-md). onOpenChangeparameter naming:onOpenChange={(setOpen) => !setOpen && closeDropdown()}-- the parameter namesetOpenis misleading since it is actuallyisOpen. Consider(open) => !open && closeDropdown().- Unused
AuthMethodtype:export type AuthMethod = 'pat' | 'oauth'is exported but never imported anywhere. The literal'pat' | 'oauth'is used inline infinishLogin. - No tests for
OAuthServiceorOAuthCallbackPage: 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.
|
Thanks for the thorough review! Pushed fixes in f86e356. Here's the rundown: Issues
Nits
|
|
Hey @Cragsmann @matejvasek ! I see you did some substantial work here, thank you for that. I just skimmed it so far. I don't want to invest the time for a full review as we have no confirmation yet if we'll need this feature. I propose to close it but keep the PR link in Jira for reference. If it turns out we'll need this we reopen and I'll review. Thoughts? @Cragsmann @matejvasek |
| protocol: TCP | ||
| args: | ||
| - "--https-port={{ .Values.plugin.port }}" | ||
| {{- if .Values.plugin.oauth.enabled }} |
There was a problem hiding this comment.
Is this all hard-wired during installation? Could we make it configurable via some ConfigMap? So admin can enable it post-installation by modifying some configmap and then doing deployment roll-out?
There was a problem hiding this comment.
diff --git a/charts/openshift-console-plugin/templates/configmap-oauth.yaml b/charts/openshift-console-plugin/templates/configmap-oauth.yaml
new file mode 100644
index 0000000..ceb4a4b
--- /dev/null
+++ b/charts/openshift-console-plugin/templates/configmap-oauth.yaml
@@ -0,0 +1,9 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: {{ template "openshift-console-plugin.name" . }}-oauth
+ namespace: {{ .Release.Namespace }}
+ labels:
+ {{- include "openshift-console-plugin.labels" . | nindent 4 }}
+data:
+ GITHUB_CLIENT_ID: {{ .Values.plugin.oauth.githubClientId | quote }}
diff --git a/charts/openshift-console-plugin/templates/deployment.yaml b/charts/openshift-console-plugin/templates/deployment.yaml
index 37a79fe..8eaa70b 100644
--- a/charts/openshift-console-plugin/templates/deployment.yaml
+++ b/charts/openshift-console-plugin/templates/deployment.yaml
@@ -13,6 +13,8 @@ spec:
{{- include "openshift-console-plugin.selectorLabels" . | nindent 6 }}
template:
metadata:
+ annotations:
+ checksum/oauth-config: {{ include (print $.Template.BasePath "/configmap-oauth.yaml") . | sha256sum }}
labels:
{{- include "openshift-console-plugin.labels" . | nindent 8 }}
spec:
@@ -28,16 +30,19 @@ spec:
protocol: TCP
args:
- "--https-port={{ .Values.plugin.port }}"
- {{- if .Values.plugin.oauth.enabled }}
env:
- name: GITHUB_CLIENT_ID
- value: {{ .Values.plugin.oauth.githubClientId | quote }}
+ valueFrom:
+ configMapKeyRef:
+ name: {{ template "openshift-console-plugin.name" . }}-oauth
+ key: GITHUB_CLIENT_ID
+ optional: true
- name: GITHUB_CLIENT_SECRET
valueFrom:
secretKeyRef:
name: {{ .Values.plugin.oauth.githubSecretName }}
key: {{ .Values.plugin.oauth.githubSecretKey }}
- {{- end }}
+ optional: true
imagePullPolicy: {{ .Values.plugin.imagePullPolicy }}
{{- if and (.Values.plugin.securityContext.enabled) (.Values.plugin.containerSecurityContext) }}
securityContext: {{ tpl (toYaml (omit .Values.plugin.containerSecurityContext "enabled")) $ | nindent 12 }}
diff --git a/charts/openshift-console-plugin/values.yaml b/charts/openshift-console-plugin/values.yaml
index 229bf23..e0e1c57 100644
--- a/charts/openshift-console-plugin/values.yaml
+++ b/charts/openshift-console-plugin/values.yaml
@@ -26,9 +26,8 @@ plugin:
memory: 50Mi
basePath: /
oauth:
- enabled: false
githubClientId: ""
- githubSecretName: ""
+ githubSecretName: "github-oauth"
githubSecretKey: "client-secret"
certificateSecretName: ""
serviceAccount:
There was a problem hiding this comment.
Enabling GitHub OAuth (post-installation)
OAuth is disabled by default. The plugin determines whether OAuth is active
based on the presence of a non-empty GITHUB_CLIENT_ID and
GITHUB_CLIENT_SECRET. To enable it after installation, follow the steps
below.
Replace console-functions-plugin with the namespace where the plugin is installed and
console-functions-plugin with the Helm release name (e.g. console-functions-plugin).
1. Create a GitHub OAuth App
- Go to GitHub > Settings > Developer settings > OAuth Apps > New OAuth App
(direct link: https://github.com/settings/applications/new). - Fill in the form:
- Application name -- any descriptive name (e.g. "OpenShift Functions Console").
- Homepage URL -- the URL of your OpenShift Console
(e.g.https://console-openshift-console.apps.<cluster-domain>). - Authorization callback URL -- the console URL with the plugin's
callback path appended, e.g.
https://console-openshift-console.apps.<cluster-domain>/faas/oauth/callback.
- Click Register application.
- On the next page, copy the Client ID.
- Click Generate a new client secret, then copy the generated value.
Store it securely; GitHub will not show it again.
2. Create the Secret
oc create secret generic github-oauth \
--from-literal=client-secret=<YOUR_CLIENT_SECRET> \
-n console-functions-pluginIf you used a custom secret name or key during installation
(plugin.oauth.githubSecretName / plugin.oauth.githubSecretKey),
use those values instead of github-oauth and client-secret.
3. Set the Client ID in the ConfigMap
oc patch configmap console-functions-plugin-oauth \
-n console-functions-plugin \
--type merge \
-p '{"data":{"GITHUB_CLIENT_ID":"<YOUR_CLIENT_ID>"}}'4. Restart the pods
The deployment has a checksum annotation that triggers a rollout
automatically on helm upgrade. After a manual ConfigMap edit you need
to restart the pods yourself:
oc rollout restart deployment/console-functions-plugin -n console-functions-plugin5. Verify
oc rollout status deployment/console-functions-plugin -n console-functions-pluginCheck the plugin UI. The "Sign in with GitHub" button should now be active.
Disabling OAuth
To disable OAuth again, clear the Client ID:
oc patch configmap console-functions-plugin-oauth \
-n console-functions-plugin \
--type merge \
-p '{"data":{"GITHUB_CLIENT_ID":""}}'
oc rollout restart deployment/console-functions-plugin -n console-functions-pluginYou can optionally delete the Secret as well:
oc delete secret github-oauth -n console-functions-plugin
|
While testing I had to do: diff --git a/Dockerfile b/Dockerfile
index 6b9c593..f6f8f35 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -27,6 +27,7 @@ RUN CGO_ENABLED=0 GOOS=$TARGETOS GOARCH=$TARGETARCH go build -ldflags="-s -w" -o
FROM registry.access.redhat.com/ubi9-micro:latest
+COPY --from=go-build /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
COPY --from=go-build /opt/app-root/src/backend/plugin-backend /usr/bin/plugin-backend
USER 1001
Otherwise backend would not trust However this change should already be in |
|
When trying to create a function I got an error: and the repository is in half-created state, see: https://github.com/matejvasek/fn-testing-abc Perhaps some permissions are missing. But it also might be some race. |
Findings:
|
|
Disconnect does not refresh/clean the functions list. |
3 participants
Summary
Screen.Recording.2026-06-15.at.13.32.53.mov
Changes
Backend (
backend/main.go)GET /api/oauth/config: returns{ client_id, enabled }from env varsPOST /api/oauth/callback: proxies authorization code + PKCE verifier to GitHub's token endpoint, returns access tokenFrontend
src/common/services/oauth/): popup-based PKCE flow with state/CSRF validation, popup-close detection, and sessionStorage cleanupsrc/pages/oauth-callback/): minimal popup callback page thatpostMessages the authorization code back to the opener windowsrc/common/components/PatModal.tsx): extracted from UserAvatar, OAuth button conditionally enabled based on backend config, "or" divider, PAT inputsrc/common/components/DisconnectConfirmModal.tsx): extracted confirmation dialog for disconnect actionsrc/common/components/UserAvatar.tsx): logged-in state shows dropdown with disconnect option, not-logged-in state shows connect button + PatModalTOKEN_KEYreplaces hardcoded'func-console-pat'across codebase,AUTH_METHOD_KEYtracks'pat' | 'oauth'Helm chart
values.yaml:plugin.oauth.enabled,githubClientId,githubSecretName,githubSecretKeydeployment.yaml: conditionally injectsGITHUB_CLIENT_IDandGITHUB_CLIENT_SECRET(from K8s Secret) as env varsTest plan
Relates to SRVOCF-856