aminghadersohi commented on PR #43309: URL: https://github.com/apache/superset/pull/43309#issuecomment-5359345961
Closing this in favour of a SIP. @Vitor-Avila you convinced me. The core problem isn't in `validate_parameters` at all — it's that validation authenticates as one identity and queries authenticate as another, and every fix that stays inside the validator either papers over that split or trades a false negative for a false positive. Both of my attempts here died on it. Deprecating SA + domain-wide delegation the way you described — checkbox unwired from SA auth, queries always SA-only, per-user impersonation served by native OAuth2 — resolves the split at the design level rather than patching around it, so that's the direction we're taking. I'll put up the SIP. When I do, I'll carry over the findings from this PR so they don't get lost: - The modal hardcodes `impersonate_user = true` for GSheets on save ([`DatabaseModal/index.tsx#L1051-L1053`](https://github.com/apache/superset/blob/master/superset-frontend/src/features/databases/DatabaseModal/index.tsx#L1051-L1053)), overriding the backend's `impersonate_user = Column(Boolean, default=False)` (`superset/models/core.py#L212`). That single line is what makes delegation mandatory for every UI-created connection. - The impersonate control isn't reachable during create: the Advanced tab is gated on `useTabLayout = isEditMode || useSqlAlchemyForm` (`#L763`), and the only other `ExtraOptions` render is inside `renderFinishState()` (`#L1999-L2002`). - Today's user-visible symptom is a service account without domain-wide delegation being rejected at Connect with *"The URL could not be identified. Please check for typos and make sure that 'Type of Google Sheets allowed' selection matches the input"* — when the real cause is an `invalid_grant` from the delegation lookup. Worth fixing that message regardless of how the SIP lands, since it's equally misleading for a permission error or a network failure. One alternative I'll raise in the SIP for completeness rather than relitigate here: your original checkbox-gating idea may survive if the hardcode is removed *and* the control is exposed during create, since existing connections keep their stored `impersonate_user = true` and would be unaffected. That's a smaller, non-breaking change. You may still prefer the full deprecation as the cleaner long-term model — that's a reasonable call and the SIP is the right venue to weigh both. Thanks for the careful review here; you caught a wrong premise in my PR description before it turned into a fix that would have broken query execution for the people it claimed to help. -- 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]
