codeant-ai-for-open-source[bot] commented on code in PR #38092: URL: https://github.com/apache/superset/pull/38092#discussion_r3671440421
########## docs/admin_docs/security/oauth.mdx: ########## @@ -0,0 +1,269 @@ +--- +title: OAuth Configuration +sidebar_position: 4 +--- + +# Keycloak + +## Complete example: Keycloak with PKCE and group mapping + +This example demonstrates a complete Keycloak integration with Superset, including: + +* **PKCE** (Proof Key for Code Exchange) on top of a confidential client +* **Group-to-role mapping**: Keycloak groups are mapped to Superset roles at every login (`AUTH_ROLES_SYNC_AT_LOGIN`) +* **Access control**: users who authenticate successfully but do not belong to any mapped group are rejected with an explicit message instead of being silently logged in +* **Single logout** (RP-Initiated Logout): logging out of Superset also terminates the Keycloak SSO session, using `id_token_hint` + +## Prerequisites: Keycloak client configuration + +### 1. Client settings + +Create a confidential OIDC client (e.g. `my_superset_clientid`) with: + +* **Valid redirect URIs**: `https://<superset-host>/oauth-authorized/keycloak` +* **Valid post logout redirect URIs**: `https://<superset-host>` — required for the single-logout redirect to be accepted by Keycloak + +### 2. Enable PKCE + +Set **Proof Key for Code Exchange Code Challenge Method** to **S256** under: + +`Clients -> my_superset_clientid -> Advanced -> Advanced settings` + +### 3. Expose group membership in the userinfo response + +:::warning +Keycloak does **not** expose a `groups` claim by default. Without this mapper, every user will be rejected with "no valid groups" even if they are correctly assigned in Keycloak. +::: + +Add a **Group Membership** mapper to the client: + +`Clients -> my_superset_clientid -> Client scopes -> my_superset_clientid-dedicated -> Add mapper -> By configuration -> Group Membership` + +* **Token Claim Name**: `groups` +* **Full group path**: **OFF** — otherwise groups are returned as `/my_superset_clientid_admin` (with a leading slash) and the role mapping will silently fail +* **Add to userinfo**: **ON** + +### 4. Create the groups + +Make sure the following groups exist in Keycloak and that users are assigned to at least one of them: + +* `<my_superset_clientid>_admin` +* `<my_superset_clientid>_alpha` +* `<my_superset_clientid>_gamma` +* `<my_superset_clientid>_public` + +The mapping between Keycloak groups and Superset roles is defined by `AUTH_ROLES_MAPPING` in the configuration below. Users who do not belong to any of the mapped groups are shown an explicit access-denied page, without being logged in. + +:::note +This example deliberately renders the access-denied page directly from the OAuth callback instead of using Flask `flash()` messages. Starting with the React-based login page (Superset 6.x lineage), flash messages are no longer displayed anywhere in the UI, so a `flash()` + redirect would fail silently and the user would bounce between Superset and Keycloak (whose SSO session is still active) with no explanation. A directly rendered response works on every Superset version. The `AuthOAuthView` subclassing pattern itself still works on 6.x because the `/oauth-authorized/<provider>` callback route is still served by Flask-AppBuilder — but verify against your target version, as login-flow extension points are being reworked. +::: + +:::note +The issuer URL format depends on your Keycloak version. Legacy (Wildfly-based, < 17) distributions use `https://<keycloak-host>/auth/realms/<REALM>`, while modern (Quarkus-based) distributions use `https://<keycloak-host>/realms/<REALM>` — without the `/auth` prefix. +::: + +## Kubernetes Secret + +Create a Secret containing the OIDC credentials, so they never appear in your Helm values (and therefore never end up in Git): + +```bash +kubectl create secret generic superset-oidc \ + --namespace superset \ + --from-literal=CLIENT_ID='my_superset_clientid' \ + --from-literal=CLIENT_SECRET='xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx' \ + --from-literal=OIDC_ISSUER='https://mykeycloak.youpi.fr/realms/MYREALM' +``` + +In a GitOps workflow, prefer provisioning this Secret through your secret management tooling (e.g. External Secrets Operator, Sealed Secrets) rather than creating it imperatively. + +## Helm chart configuration + +Reference the Secret with `envFromSecret` instead of putting values in `extraSecretEnv`: + +```yaml +envFromSecret: superset-oidc Review Comment: **Suggestion:** Setting `envFromSecret` replaces the chart's default environment Secret rather than adding another Secret. The documented `superset-oidc` Secret contains only OIDC variables, so the generated `DB_*` and `REDIS_*` variables are no longer injected into the pods, causing the Superset web, worker, and initialization containers to fail when they attempt to connect to the database or cache. Use `envFromSecrets` for the additional OIDC Secret, or include all required chart environment keys in the referenced Secret. [api mismatch] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Helm initialization cannot locate database settings. - ❌ Web and worker pods lose database connectivity. - ❌ Celery components lose Redis connection settings. - ⚠️ Documented Keycloak deployment fails before OAuth login. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create the documented `superset-oidc` Secret from `docs/admin_docs/security/oauth.mdx:65-75`; it contains only `CLIENT_ID`, `CLIENT_SECRET`, and `OIDC_ISSUER`. 2. Deploy the chart with `envFromSecret: superset-oidc` as documented at `docs/admin_docs/security/oauth.mdx:81-84`. 3. The chart injects only the selected Secret through `helm/superset/templates/deployment.yaml:120-125`, `helm/superset/templates/deployment-worker.yaml:114-119`, and `helm/superset/templates/init-job.yaml:88-93`; the default environment Secret is no longer referenced. 4. The chart-generated default Secret normally supplies `DB_HOST`, `DB_PORT`, `DB_USER`, `DB_PASS`, `DB_NAME`, and Redis variables in `helm/superset/templates/secret-env.yaml:21-35`. The init container then reads `DB_HOST` and `DB_PORT` at `helm/superset/values.yaml:445-450`, while other startup checks read database and Redis variables at `helm/superset/values.yaml:582-583`. 5. Observe that initialization and the web or worker pods fail or cannot connect because the OIDC-only Secret does not provide the required database and Redis environment variables. Configure `envFromSecrets: [superset-oidc]` instead, or include the chart environment keys in the referenced Secret. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=de1093b5a68b49d3a3b952c7868f7dd7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=de1093b5a68b49d3a3b952c7868f7dd7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/admin_docs/security/oauth.mdx **Line:** 84:84 **Comment:** *Api Mismatch: Setting `envFromSecret` replaces the chart's default environment Secret rather than adding another Secret. The documented `superset-oidc` Secret contains only OIDC variables, so the generated `DB_*` and `REDIS_*` variables are no longer injected into the pods, causing the Superset web, worker, and initialization containers to fail when they attempt to connect to the database or cache. Use `envFromSecrets` for the additional OIDC Secret, or include all required chart environment keys in the referenced Secret. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38092&comment_hash=e7c22e396da9b828d5f16dc4f501660b0d0f778850b66e27a61d5a97c1695baa&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38092&comment_hash=e7c22e396da9b828d5f16dc4f501660b0d0f778850b66e27a61d5a97c1695baa&reaction=dislike'>👎</a> ########## docs/admin_docs/security/oauth.mdx: ########## @@ -0,0 +1,269 @@ +--- +title: OAuth Configuration +sidebar_position: 4 +--- + +# Keycloak + +## Complete example: Keycloak with PKCE and group mapping + +This example demonstrates a complete Keycloak integration with Superset, including: + +* **PKCE** (Proof Key for Code Exchange) on top of a confidential client +* **Group-to-role mapping**: Keycloak groups are mapped to Superset roles at every login (`AUTH_ROLES_SYNC_AT_LOGIN`) +* **Access control**: users who authenticate successfully but do not belong to any mapped group are rejected with an explicit message instead of being silently logged in +* **Single logout** (RP-Initiated Logout): logging out of Superset also terminates the Keycloak SSO session, using `id_token_hint` + +## Prerequisites: Keycloak client configuration + +### 1. Client settings + +Create a confidential OIDC client (e.g. `my_superset_clientid`) with: + +* **Valid redirect URIs**: `https://<superset-host>/oauth-authorized/keycloak` +* **Valid post logout redirect URIs**: `https://<superset-host>` — required for the single-logout redirect to be accepted by Keycloak + +### 2. Enable PKCE + +Set **Proof Key for Code Exchange Code Challenge Method** to **S256** under: + +`Clients -> my_superset_clientid -> Advanced -> Advanced settings` + +### 3. Expose group membership in the userinfo response + +:::warning +Keycloak does **not** expose a `groups` claim by default. Without this mapper, every user will be rejected with "no valid groups" even if they are correctly assigned in Keycloak. +::: + +Add a **Group Membership** mapper to the client: + +`Clients -> my_superset_clientid -> Client scopes -> my_superset_clientid-dedicated -> Add mapper -> By configuration -> Group Membership` + +* **Token Claim Name**: `groups` +* **Full group path**: **OFF** — otherwise groups are returned as `/my_superset_clientid_admin` (with a leading slash) and the role mapping will silently fail +* **Add to userinfo**: **ON** + +### 4. Create the groups + +Make sure the following groups exist in Keycloak and that users are assigned to at least one of them: + +* `<my_superset_clientid>_admin` +* `<my_superset_clientid>_alpha` +* `<my_superset_clientid>_gamma` +* `<my_superset_clientid>_public` + +The mapping between Keycloak groups and Superset roles is defined by `AUTH_ROLES_MAPPING` in the configuration below. Users who do not belong to any of the mapped groups are shown an explicit access-denied page, without being logged in. + +:::note +This example deliberately renders the access-denied page directly from the OAuth callback instead of using Flask `flash()` messages. Starting with the React-based login page (Superset 6.x lineage), flash messages are no longer displayed anywhere in the UI, so a `flash()` + redirect would fail silently and the user would bounce between Superset and Keycloak (whose SSO session is still active) with no explanation. A directly rendered response works on every Superset version. The `AuthOAuthView` subclassing pattern itself still works on 6.x because the `/oauth-authorized/<provider>` callback route is still served by Flask-AppBuilder — but verify against your target version, as login-flow extension points are being reworked. +::: + +:::note +The issuer URL format depends on your Keycloak version. Legacy (Wildfly-based, < 17) distributions use `https://<keycloak-host>/auth/realms/<REALM>`, while modern (Quarkus-based) distributions use `https://<keycloak-host>/realms/<REALM>` — without the `/auth` prefix. +::: + +## Kubernetes Secret + +Create a Secret containing the OIDC credentials, so they never appear in your Helm values (and therefore never end up in Git): + +```bash +kubectl create secret generic superset-oidc \ + --namespace superset \ + --from-literal=CLIENT_ID='my_superset_clientid' \ + --from-literal=CLIENT_SECRET='xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx' \ + --from-literal=OIDC_ISSUER='https://mykeycloak.youpi.fr/realms/MYREALM' +``` + +In a GitOps workflow, prefer provisioning this Secret through your secret management tooling (e.g. External Secrets Operator, Sealed Secrets) rather than creating it imperatively. + +## Helm chart configuration + +Reference the Secret with `envFromSecret` instead of putting values in `extraSecretEnv`: + +```yaml +envFromSecret: superset-oidc + +configOverrides: + enable_proxy_fix: | + # Required when Superset runs behind an ingress / reverse proxy terminating TLS. + # Without it, redirect_uri and the post-logout redirect are generated with + # http:// and Keycloak rejects the callback. + ENABLE_PROXY_FIX = True + + enable_oauth: | + from flask import redirect, request, session + from flask_appbuilder import expose + from flask_appbuilder.security.manager import AUTH_OAUTH + from flask_appbuilder.security.views import AuthOAuthView + from flask_login import login_user + from superset.security import SupersetSecurityManager + import logging + import os + + log = logging.getLogger(__name__) + + AUTH_TYPE = AUTH_OAUTH + AUTH_USER_REGISTRATION = True + AUTH_ROLES_SYNC_AT_LOGIN = True + AUTH_USER_REGISTRATION_ROLE = None + + PROVIDER_NAME = 'keycloak' + CLIENT_ID = os.getenv('CLIENT_ID') + CLIENT_SECRET = os.getenv('CLIENT_SECRET') + OIDC_ISSUER = os.getenv('OIDC_ISSUER') + OIDC_BASE_URL = f'{OIDC_ISSUER}/protocol/openid-connect' + OIDC_AUTH_URL = f'{OIDC_BASE_URL}/auth' + OIDC_METADATA_URL = f'{OIDC_ISSUER}/.well-known/openid-configuration' + OIDC_TOKEN_URL = f'{OIDC_BASE_URL}/token' + OIDC_USERINFO_URL = f'{OIDC_BASE_URL}/userinfo' + OIDC_LOGOUT_URL = f'{OIDC_BASE_URL}/logout' + + OAUTH_PROVIDERS = [ + { + 'name': PROVIDER_NAME, + 'token_key': 'access_token', + 'icon': 'fa-circle-o', + 'remote_app': { + 'api_base_url': OIDC_BASE_URL, + 'access_token_url': OIDC_TOKEN_URL, + 'authorize_url': OIDC_AUTH_URL, + 'request_token_url': None, + 'server_metadata_url': OIDC_METADATA_URL, + 'client_id': CLIENT_ID, + 'client_secret': CLIENT_SECRET, + 'client_kwargs': { + 'scope': 'openid email profile', + 'code_challenge_method': 'S256', + 'response_type': 'code', + }, + }, + } + ] + + # Keycloak group -> Superset role mapping. + # Make sure these groups exist in Keycloak (see Prerequisites). + AUTH_ROLES_MAPPING = { + f'{CLIENT_ID}_admin': ['Admin'], + f'{CLIENT_ID}_alpha': ['Alpha'], + f'{CLIENT_ID}_gamma': ['Gamma'], + f'{CLIENT_ID}_public': ['Public'], + } + + + def access_denied_response(message, login_url): + # Rendered directly instead of flash(): the React-based login page + # (Superset 6.x) does not display Flask flash messages, so a + # flash + redirect fails silently. This works on every version. + return ( + '<html><body style="font-family: sans-serif; text-align: center; margin-top: 10%">' + '<h2>Access denied</h2>' + f'<p>{message}</p>' + f'<p>Contact your administrator, then <a href="{login_url}">try again</a>.</p>' + '</body></html>', + 403, + ) + + + class CustomOAuthView(AuthOAuthView): + # Override the logout method to also terminate the Keycloak SSO session + # (OIDC RP-Initiated Logout). The id_token stored at login time is passed + # as id_token_hint so Keycloak can identify the session without prompting. + @expose('/logout/', methods=['GET', 'POST']) + def logout(self): + id_token = session.get('oidc_id_token') + super().logout() + session.clear() + post_logout_redirect_uri = request.url_root.strip('/') + logout_url = ( + f'{OIDC_LOGOUT_URL}' + f'?post_logout_redirect_uri={post_logout_redirect_uri}' + f'&client_id={CLIENT_ID}' + ) + if id_token: + logout_url += f'&id_token_hint={id_token}' + return redirect(logout_url) + + # Override the authorized method to handle the OIDC callback and map groups to roles + @expose('/oauth-authorized/<provider>') + def oauth_authorized(self, provider): + # Step 1: exchange the authorization code for an access token + try: + resp = self.appbuilder.sm.oauth_remotes[provider].authorize_access_token() + except Exception as e: + log.exception(f'Error fetching access token: {e}') + return redirect(self.appbuilder.get_url_for_index) + + # Step 2: fetch user info from the OIDC provider + userinfo = self.appbuilder.sm.get_oauth_user_info(provider, resp) + role_keys = userinfo.get('role_keys', []) + known_groups = [g for g in role_keys if g in AUTH_ROLES_MAPPING] Review Comment: **Suggestion:** The custom callback only handles exceptions from `authorize_access_token()`. Failures from the userinfo request, HTTP status validation, JSON parsing, or user provisioning in the following code are uncaught and produce a server error instead of the controlled OAuth failure response described by this example. Wrap the entire callback processing flow in the error handling path or explicitly handle these failures. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Keycloak outages produce OAuth callback 500 errors. - ❌ Malformed userinfo responses bypass access-denied handling. - ⚠️ Users receive server errors instead of login recovery. - ⚠️ Provisioning failures expose an uncontrolled authentication path. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure the documented Keycloak provider, then begin the real OAuth flow through the Flask-AppBuilder callback route `/oauth-authorized/keycloak` defined at `docs/admin_docs/security/oauth.mdx:186-188`. 2. Complete authorization successfully so `authorize_access_token()` returns at `docs/admin_docs/security/oauth.mdx:191`, allowing execution to leave the try block at lines 190-194. 3. Make the subsequent Keycloak userinfo request fail, return a non-success status, or return invalid JSON. The custom security manager performs that request at `docs/admin_docs/security/oauth.mdx:236-240`, calls `raise_for_status()` at line 239, and calls `json()` at line 240 without handling exceptions. 4. The callback invokes `get_oauth_user_info()` at `docs/admin_docs/security/oauth.mdx:197`; exceptions from the userinfo request or parsing therefore propagate past the only callback handler instead of producing the controlled redirect used for token-exchange failures. 5. A separate provisioning failure can occur when `auth_user_oauth(userinfo)` is called at `docs/admin_docs/security/oauth.mdx:214`; only a returned `None` is handled at lines 215-222, while database or role-synchronization exceptions remain uncaught and result in a server error. Wrap the complete callback flow or explicitly handle these failure classes. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fa0f23316f4d442a845a61e88476d315&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fa0f23316f4d442a845a61e88476d315&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/admin_docs/security/oauth.mdx **Line:** 197:199 **Comment:** *Possible Bug: The custom callback only handles exceptions from `authorize_access_token()`. Failures from the userinfo request, HTTP status validation, JSON parsing, or user provisioning in the following code are uncaught and produce a server error instead of the controlled OAuth failure response described by this example. Wrap the entire callback processing flow in the error handling path or explicitly handle these failures. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38092&comment_hash=5cfdbc8fa2484cfd75f284a277ead976feb8942fe9935d2c633d77e137a5b89c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38092&comment_hash=5cfdbc8fa2484cfd75f284a277ead976feb8942fe9935d2c633d77e137a5b89c&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
