msyavuz commented on code in PR #44653:
URL: https://github.com/apache/superset/pull/44653#discussion_r4133113881
##########
superset/db_engine_specs/gsheets.py:
##########
@@ -250,9 +250,13 @@ def impersonate_user(
engine_kwargs: dict[str, Any],
) -> tuple[URL, dict[str, Any]]:
if username is not None:
- user = security_manager.find_user(username=username)
- if user and user.email:
- url = url.update_query_dict({"subject": user.email})
+ # Resolved from the database rather than from ``username``: with
+ # ``IMPERSONATE_WITH_EMAIL_PREFIX`` enabled the caller has already
+ # substituted the email prefix into ``username``, so looking it up
+ # here as if it were still the login finds nothing whenever the two
+ # differ, silently leaving the subject unset.
+ if email := database.get_impersonation_email():
Review Comment:
Called with no argument, this reads the user from database.url_object, while
_get_sqla_engine passes sqlalchemy_url. get_effective_user only looks at the
URL when there's no request user (alerts, reports, Celery), so this rarely
matters. Still, passing the URL through would keep both paths identical. Same
for snowflake.
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -353,10 +353,8 @@ def impersonate_user(
# leaving the default/service-account username paired
# with this user's OAuth token. Use it as given.
url = url.set(username=username)
- else:
- user = security_manager.find_user(username=username)
- if user and user.email:
- url = url.set(username=user.email)
+ elif email := database.get_impersonation_email():
Review Comment:
Same as the gsheets comment: this reads the user from url_object, while core
uses sqlalchemy_url.
##########
superset/utils/rls.py:
##########
@@ -328,7 +329,15 @@ def collect_rls_predicates_for_sql(
)
}
)
- except Exception:
+ except Exception as ex:
+ # The block above is not only SQL parsing: `get_predicates_for_table`
+ # queries `db.session` and `get_default_catalog()` builds an engine, so
+ # a caught DB error can leave db.session in "pending rollback" state,
+ # which would poison unrelated queries later in this request. A parse
+ # failure touches no session, so only roll back for a DB error.
+ if isinstance(ex, SQLAlchemyError):
Review Comment:
The inner handler in helpers.py rolls back unconditionally, and its comment
explains why: DB work can poison the session and then a different, non-DB error
can surface. The same can happen here. get_default_catalog() builds an engine,
and that path can now raise SupersetErrorException from
find_user_for_impersonation, which isn't a SQLAlchemyError, so this check would
skip the rollback. Should we use the same rule in both places?
--
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]
