sadpandajoe commented on code in PR #44653:
URL: https://github.com/apache/superset/pull/44653#discussion_r4134280288
##########
superset/utils/database.py:
##########
@@ -123,6 +127,64 @@ def warm_and_release_connection(instance: Any,
*relationships: str) -> None:
session.expire_on_commit = True
+def find_user_for_impersonation(username: str) -> User | None:
+ """
+ Resolve the login backing an impersonated database session.
+
+ ``find_user`` is a metadata-DB read, so it inherits any failed transaction
+ left behind earlier in the request and reports ``PendingRollbackError``
+ instead of the original fault — blaming this lookup for an unrelated
+ failure. Roll back and retry once so a poisoned session doesn't cost us the
+ lookup.
+
+ Only ``PendingRollbackError`` is handled. Any other ``SQLAlchemyError``
+ describes this lookup's own failure and propagates untouched: rolling back
+ on, say, an ``IntegrityError`` raised by autoflushing the caller's pending
+ writes would silently discard work this function knows nothing about.
+
+ The resolved value becomes the identity the analytic database connects as,
+ so a lookup that still fails must not degrade to the un-resolved login:
+ that would silently query as a different principal than the one being
+ impersonated. Raise instead.
+
+ :param username: the Superset login to resolve
+ :return: the matching user, or ``None`` if no such login exists
+ :raises SupersetErrorException: if the session is still unusable after a
+ rollback and retry
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset import db
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetErrorException
+ from superset.extensions import security_manager
+
+ try:
+ return security_manager.find_user(username=username)
+ except PendingRollbackError:
+ logger.warning(
+ "Impersonation lookup for %s failed on a broken transaction; "
+ "rolling back and retrying once.",
+ username,
+ exc_info=True,
+ )
+ db.session.rollback() # pylint: disable=consider-using-transaction
Review Comment:
This rollback can silently drop an in-flight write rather than just repair a
stale read. `find_user_for_impersonation()` is also reached from
`Database.get_default_catalog()` (e.g. BigQuery's implementation falls back to
`get_sqla_engine()` when the URI has no host/database), and
`UpdateDatabaseCommand.run()` calls `get_default_catalog()` a second time right
after `DatabaseDAO.update()` has already mutated `self._model` in place, before
that change is flushed. If the metadata-DB connection is unhealthy at exactly
that point, this rollback expires the pending, uncommitted model changes; the
retry then succeeds quietly, `SyncPermissionsCommand` runs, and
`@transaction()` commits normally on the next line — the API reports success
but the database connection update was never persisted. Before this PR the same
failure propagated out of `get_default_catalog()` uncaught, and
`@transaction()`'s own except-branch correctly rolled back and reported the
failure via `on_error`. Could this
check `g.in_transaction` (as `superset/utils/decorators.py:265` does) and
re-raise instead of retrying when it's running inside a write command?
--
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]
