rusackas commented on PR #35044:
URL: https://github.com/apache/superset/pull/35044#issuecomment-3549050763
Thanks for the contribution! I think even if this isn't perfect, it's
probably good, so long as we add some "this is just an example" kind of
disclaimer - i.e. "We offer this as a starting point, and provide no
warranties. Your deployment may have different requirements or security
concerns to contend with."
That said, I ran this through AI, and got some feedback. Some of it sounds
pretty critical, but I'd love your feedback on whether or not we should take
these issues into consideration.
- Syntax errors in the example superset_config snippet
- You used colons instead of equals for Python assignments (e.g.
SECRET_KEY: '...' and OIDC_OPENID_REALM: '...'). Change these to =.
- Incorrect / fragile calls to AppBuilder / Flask-OIDC APIs
- In OIDCSecurityManager.init you do
OpenIDConnect(self.appbuilder.get_app). Usually you must call the getter:
self.appbuilder.get_app() (or pass the Flask app instance). Verify AppBuilder
API in your Superset/FAB version.
- Check calls like self.appbuilder.get_url_for_index and
self.appbuilder.get_url_for_login — verify whether these are properties or
methods in your FAB version (you may need parentheses).
- Inconsistency in role names
- In the login handler you create a user with sm.find_role("Gamma") but in
the config you set AUTH_USER_REGISTRATION_ROLE = 'Public'. Make these
consistent or explain why they differ.
- OIDC client_secret.json structure and Flask-OIDC expectations
- The example client_secret.json structure in the doc may not match what
Flask-OIDC expects by default (Flask-OIDC/OAuth2 libraries commonly use a "web"
or "client" key layout). Add a note to verify the file format against
Flask-OIDC docs and Keycloak client export format.
- Logout flow and redirect
- The logout implementation is fragile: calling oidc.logout(), calling
super().logout(), then building a redirect using
oidc.client_secrets.get("issuer") may not work reliably. client_secrets shape
and key lookup may differ; also oidc.client_secrets may not exist in the form
you expect. Test and pin the exact behavior against the Flask-OIDC release you
intend to support.
- Security hardening & production guidance
- OIDC_ID_TOKEN_COOKIE_SECURE = False is unsafe for production; set True
if using HTTPS in production (and document that).
- Don’t hard-code SECRET_KEY or client secrets in repo — recommend
environment variables or a vault, and warn not to commit client_secret.json to
source control.
- Recommend setting appropriate cookie, session, and token lifetimes and
validating issuer/audience claims.
- Package maintenance / recommendation
- Flask-OIDC has not been actively maintained compared to newer libraries
(e.g. Authlib). Consider recommending Authlib or documenting that Flask-OIDC
may be legacy, and note which versions of Flask and Flask-OIDC you tested
against. You already mention 2.2.0 which is good; also call out Flask version
compatibility.
- User identity mapping & uniqueness
- You use email (or preferred_username) to create users. Document
assumptions about which attribute you use as the unique identifier and what to
do if Keycloak doesn't provide email or if emails aren't unique.
- Example readability and usability
- Small improvements: use consistent quoting, show exact file placement
(e.g. path of keycloak_security_manager.py relative to superset_config.py),
show how to enable/disable self-registration clearly, and provide an example
Keycloak client configuration (redirect URIs, access type, service accounts,
etc.).
--
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]
