rusackas commented on PR #37234:
URL: https://github.com/apache/superset/pull/37234#issuecomment-3994040820
Hey @tahvane1, thanks for the ping! I took a closer look at all the review
comments.
A few of the bot comments were false positives or not really a big deal, so
I've resolved them.
For the sake of the PR here, some items would still need to be addressed:
1. Pre-commit — Please make sure `pre-commit run --all-files` passes
cleanly.
2. The feature flag is not documented, and has no default state, in
`config.py` and thus will not be listed on the generated [feature flags docs
page](https://superset.apache.org/admin-docs/configuration/feature-flags).
However, there's a bigger issue to wrangle about the need for a feature flag
at all. There's some trepidation (generally) about adding new feature flags.
Often these involve proposing a SIP (or at least a Lazy Consensus thread on the
Dev mailing list).
One suggestion to get around that would be to simplify the approach
entirely. The current implementation creates a 79-line file that duplicates
FAB's AuthOAuthView.login just to add a 3-line single-provider check, plus a
feature flag and conditional view registration. I don't think we need the
feature flag at all, really. If someone configures exactly one OAuth provider,
there's nothing to "select," so skipping is just good UX.
This could be simplified to ~10 lines added to the existing `auth.py`,
with no new file or feature flag:
```
class SupersetOAuthView(BaseSupersetView, AuthOAuthView):
route_base = "/login"
@expose("/")
@expose("/<provider>")
@no_cache
def login(self, provider: Optional[str] = None) -> WerkzeugResponse:
if provider is None:
providers = list(self.appbuilder.sm.oauth_remotes.keys())
if len(providers) == 1:
provider = providers[0]
return super().login(provider)
```
Then in `security/manager.py`, just always register SupersetOAuthView when
`auth_type == AUTH_OAUTH` instead of conditionally swapping views based on a
feature flag. This avoids duplicating FAB internals (which can break on
upgrades), eliminates the feature flag, and keeps the change minimal.
The remaining ask would be test coverage — a few unit tests for the
single-provider redirect behavior would be good to have.
Let me/us know your thoughts on the above. Thanks!
--
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]
