driver impersonation fix#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Starburst driver’s Trino session-user impersonation logic to rely solely on the JDBC clientInfo flag (impersonate:true) and removes the previous PAT-/database-specific override behavior.
Changes:
- Simplifies
impersonate-userto set TrinosessionUseronly whenclientInfoincludesimpersonate:true, using the current Metabase user’s email. - Simplifies
remove-impersonationto clear TrinosessionUseronly whenclientInfoincludesimpersonate:true. - Removes the prior special-casing for PAT auth / specific database IDs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (if | ||
| (clojure.string/includes? (.getProperty (.getClientInfo conn) "ClientInfo" "") "impersonate:true") | ||
| (let [email (get (deref api/*current-user*) :email)] | ||
| (.setSessionUser (.unwrap conn TrinoConnection) email)) | ||
| nil)) |
There was a problem hiding this comment.
This change removes the previous PAT-specific fallback (and the ability to set a non-email session user) and now always derives the Trino session user from api/*current-user*. If *current-user* is unset/empty in PAT or other non-request contexts, email will be nil and setSessionUser will receive null, which can break query execution. Consider restoring a safe fallback (e.g., use the DB connection user, a dedicated service user, or skip impersonation with a clear error) when :email is missing, and clarify the intended behavior for PAT flows.
| nil))) | ||
| (if | ||
| (clojure.string/includes? (.getProperty (.getClientInfo conn) "ClientInfo" "") "impersonate:true") | ||
| (let [email (get (deref api/*current-user*) :email)] |
There was a problem hiding this comment.
With the removal of the previous force-pat-user? path, there’s no test coverage ensuring Starburst queries behave correctly when executed via PAT / non-interactive auth (where api/*current-user* may differ or be absent). Add a regression test covering impersonation/session-user behavior for PAT (or explicitly assert it’s unsupported) to prevent breaking token-based access.
| (let [email (get (deref api/*current-user*) :email)] | |
| (when-let [email (some-> api/*current-user* deref :email not-empty)] |
No description provided.