codeant-ai-for-open-source[bot] commented on code in PR #38092:
URL: https://github.com/apache/superset/pull/38092#discussion_r3550391271


##########
docs/admin_docs/security/oauth.mdx:
##########
@@ -0,0 +1,183 @@
+---
+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 group-to-role mapping, single logout with Keycloak, and PKCE support.
+
+First, create a Secret containing the following values:
+
+* `CLIENT_ID`
+* `CLIENT_SECRET`
+* `OIDC_ISSUER`
+
+The Keycloak client must be configured to support the PKCE flow.
+
+To enable PKCE, set **Proof Key for Code Exchange Code Challenge Method** to 
**S256** under:
+
+`Clients -> my_superset_clientid -> Advanced -> Advanced settings`
+
+This implementation correctly handles the case where a user successfully 
authenticates with Keycloak but does not belong to any of the configured groups.
+
+The following snippet defines the mapping between Keycloak groups and Superset 
roles:
+
+```python
+AUTH_ROLES_MAPPING = {
+    f'{CLIENT_ID}_admin': ['Admin'],
+    f'{CLIENT_ID}_alpha': ['Alpha'],
+    f'{CLIENT_ID}_gamma': ['Gamma'],
+    f'{CLIENT_ID}_public': ['Public'],
+}
+```
+
+If a user does not belong to any of the mapped groups, they will be redirected 
back to the login page.
+
+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`
+
+## Helm chart configuration
+```yaml
+extraSecretEnv:
+  CLIENT_ID: my_superset_clientid
+  CLIENT_SECRET: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+  OIDC_ISSUER: https://mykeycloak.youpi.fr/auth/realms/MYREALM
+
+configOverrides:
+  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'
+    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',

Review Comment:
   **Suggestion:** The PKCE option is configured under `client_kwargs`, but 
Flask-AppBuilder/Authlib expects `code_challenge_method` at the `remote_app` 
level; in this location it can be ignored, so the example may run without PKCE 
even though the docs say PKCE is enabled. [api mismatch]
   
   <details>
   <summary><b>Severity Level:</b> Critical 🚨</summary>
   
   ```mdx
   ❌ PKCE not enabled despite documentation, weakening OAuth security.
   ⚠️ Operators misled about Keycloak client PKCE configuration.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Follow the Keycloak Helm example in 
`docs/admin_docs/security/oauth.mdx:71-100`,
   copying `OAUTH_PROVIDERS` and related config into your `superset_config.py` 
or Helm
   `configOverrides` as documented.
   
   2. Note that in this example the PKCE option is set under `client_kwargs` at 
lines 93-96
   (`code_challenge_method': 'S256'`), whereas the PKCE docs in
   `docs/admin_docs/configuration/configuring-superset.mdx:377-386` configure
   `code_challenge_method` directly inside the `remote_app` dict.
   
   3. Start Superset with this configuration and initiate a Keycloak login; the 
OAuth client
   used by Flask-AppBuilder is built from `OAUTH_PROVIDERS` (see
   
`docs/admin_docs_versioned_docs/version-6.1.0/configuration/configuring-superset.mdx:282-289`
   which describes this wiring).
   
   4. Inspect the authorization request sent to Keycloak (for example via 
browser developer
   tools); because `code_challenge_method` is not at the expected level in the 
config, the
   request lacks PKCE parameters (`code_challenge` and 
`code_challenge_method`), so the flow
   runs without PKCE even though the documentation claims it is enabled.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=254e4bb3973640f8929f1cc7947d0813&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=254e4bb3973640f8929f1cc7947d0813&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:** 93:96
   **Comment:**
        *Api Mismatch: The PKCE option is configured under `client_kwargs`, but 
Flask-AppBuilder/Authlib expects `code_challenge_method` at the `remote_app` 
level; in this location it can be ignored, so the example may run without PKCE 
even though the docs say PKCE is enabled.
   
   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=55c98b1c9c2da49419b64d50d1e538d1fd5b094e8ae50c36c4ed65db0959d770&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38092&comment_hash=55c98b1c9c2da49419b64d50d1e538d1fd5b094e8ae50c36c4ed65db0959d770&reaction=dislike'>👎</a>



##########
docs/admin_docs/security/oauth.mdx:
##########
@@ -0,0 +1,183 @@
+---
+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 group-to-role mapping, single logout with Keycloak, and PKCE support.
+
+First, create a Secret containing the following values:
+
+* `CLIENT_ID`
+* `CLIENT_SECRET`
+* `OIDC_ISSUER`
+
+The Keycloak client must be configured to support the PKCE flow.
+
+To enable PKCE, set **Proof Key for Code Exchange Code Challenge Method** to 
**S256** under:
+
+`Clients -> my_superset_clientid -> Advanced -> Advanced settings`
+
+This implementation correctly handles the case where a user successfully 
authenticates with Keycloak but does not belong to any of the configured groups.
+
+The following snippet defines the mapping between Keycloak groups and Superset 
roles:
+
+```python
+AUTH_ROLES_MAPPING = {
+    f'{CLIENT_ID}_admin': ['Admin'],
+    f'{CLIENT_ID}_alpha': ['Alpha'],
+    f'{CLIENT_ID}_gamma': ['Gamma'],
+    f'{CLIENT_ID}_public': ['Public'],
+}
+```
+
+If a user does not belong to any of the mapped groups, they will be redirected 
back to the login page.
+
+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`
+
+## Helm chart configuration
+```yaml
+extraSecretEnv:
+  CLIENT_ID: my_superset_clientid
+  CLIENT_SECRET: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
+  OIDC_ISSUER: https://mykeycloak.youpi.fr/auth/realms/MYREALM
+
+configOverrides:
+  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'
+    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',
+                },
+            },
+        }
+    ]
+
+    # Make sure you create these groups on Keycloak
+    AUTH_ROLES_MAPPING = {
+        f'{CLIENT_ID}_admin': ['Admin'],
+        f'{CLIENT_ID}_alpha': ['Alpha'],
+        f'{CLIENT_ID}_gamma': ['Gamma'],
+        f'{CLIENT_ID}_public': ['Public'],
+    }
+
+
+    class CustomOAuthView(AuthOAuthView):
+        # Override the logout method to also log out from the OIDC provider
+        @expose('/logout/', methods=['GET', 'POST'])
+        def logout(self):
+            super().logout()
+            session.clear()
+            return redirect(
+                
f'{OIDC_ISSUER}/protocol/openid-connect/logout?post_logout_redirect_uri={request.url_root.strip("/")}&client_id={CLIENT_ID}'
+            )
+
+        # 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)

Review Comment:
   **Suggestion:** The userinfo fetch path can raise exceptions 
(`raise_for_status` or JSON/HTTP failures), but this call is outside the 
existing `try` block, so transient IdP errors will produce a 500 instead of a 
controlled redirect/error flow. [possible bug]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   ❌ OAuth callback can 500 on IdP userinfo errors.
   ⚠️ Users see generic crash instead of graceful login failure.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Use the Keycloak configuration example from 
`docs/admin_docs/security/oauth.mdx`,
   including `CustomOAuthView.oauth_authorized` (lines 122-154) and
   `CustomSsoSecurityManager.get_oauth_user_info` (lines 162-178) in your 
Superset
   configuration.
   
   2. A user completes authentication at Keycloak and is redirected to the 
callback endpoint
   `/oauth-authorized/keycloak`, which is exposed by 
`CustomOAuthView.oauth_authorized` at
   line 123.
   
   3. Simulate a transient failure on the userinfo endpoint configured as 
`OIDC_USERINFO_URL`
   (line 79), for example by stopping Keycloak or forcing it to return a 500 
error; the call
   `oauth_remotes[provider].get(OIDC_USERINFO_URL)` or `me.raise_for_status()` 
at lines
   164-165 throws an exception.
   
   4. Because the only try/except block in `oauth_authorized` (lines 125-129) 
wraps the
   access-token exchange but not the userinfo call at line 132, the exception 
propagates out
   of the view and results in an HTTP 500 response instead of the intended 
redirect
   `self.appbuilder.get_url_for_index`, as confirmed by repository search which 
finds no
   additional wrapper around this method.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b58ab9dfe22e4a43bc118803f92a94cc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b58ab9dfe22e4a43bc118803f92a94cc&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:** 132:132
   **Comment:**
        *Possible Bug: The userinfo fetch path can raise exceptions 
(`raise_for_status` or JSON/HTTP failures), but this call is outside the 
existing `try` block, so transient IdP errors will produce a 500 instead of a 
controlled redirect/error flow.
   
   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=8326b9034cee0e8db0ef61f382c51328caef4cbc7c3a64276648d366c4694880&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38092&comment_hash=8326b9034cee0e8db0ef61f382c51328caef4cbc7c3a64276648d366c4694880&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]

Reply via email to