fix: Refactor SQL engine username logic #19914

john-bodley · 2022-05-01T22:58:40Z

SUMMARY

This PR fixes a number of issues (some cosmetic) with the SQL username. Specifically:

The username was referred to as both username and user_name. The inconsistency added undue complexity.
It a number of places the g.user.username was passed as the username argument of the get_sqla_engine function (or similar) even when it was the default fallback. This added unnecessary complexity, i.e., from reading the code it wasn't apparent when/why the username would be overridden. In reality it mostly isn't, i.e., it should always be the logged in user unless the query is being executed from Celery—where there is no user logged in.
The logic introduced in fix: [alert] should run alert query from report account #17499 wasn't suffice, in actuality it likely added more complexity, as although it tried to ensure that the get_df for the alert was executed by the passed in user, if the underlying query used Jinja templating for fetching the the latest partition or similar those queries were executed by the user defined in the SQLAlchemy URI (if defined; falling back to the system user)—leveraging the SQLAlchemy inspector—per here.

The fix for (1) was to rename user_name to username where possible. For (2) and (3) the combined fix was to actually remove overriding the username in functions given the level of nesting, etc. and rely simply on using g.user.username, i.e., the logged in user, as the user (or effective user) for all queries. Given we're using globals via flask.g—Flask's versions of globals which persist for the duration of the request—why not update it rather than passing around another username everywhere. For instances where there was no user logged in: alerting, async SQL Lab queries, etc. we simply temporarily override the g.user using the provided username. Hopefully this approach simplified the code and brought more transparency.

The one doubt I had was with how the app context works with Celery and whether flask.g is thread safe, though per here it seems we create a new app context for every task.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI. Updated existing and added new unit tests.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

john-bodley · 2022-05-01T23:00:56Z

superset/databases/commands/test_connection.py

-                if not alive:
-                    raise DBAPIError(None, None, None)
+
+            with override_user(self._actor):


Same logic as previously, though indented due to the context manager.

john-bodley · 2022-05-01T23:01:05Z

superset/databases/commands/validate.py

-            errors = database.db_engine_spec.extract_errors(ex, context)
-            raise DatabaseTestConnectionFailedError(errors) from ex
+
+        with override_user(self._actor):


Same logic as previously, though indented due to the context manager.

john-bodley · 2022-05-01T23:01:24Z

superset/db_engine_specs/base.py

-        return database.get_sqla_engine(
-            schema=schema, nullpool=True, user_name=user_name, source=source
-        )
+        return database.get_sqla_engine(schema=schema, source=source)


nullpool=True is the default.

john-bodley · 2022-05-01T23:44:50Z

superset/reports/commands/alert.py

-            )
-            return df
+
+            with override_user(


Same logic as previously, though indented due to the context manager.

john-bodley · 2022-05-01T23:45:20Z

superset/sql_lab.py

-            stats_logger.incr("error_sqllab_unhandled")
-            query = get_query(query_id, session)
-            return handle_query_error(ex, query, session)
+        with override_user(current_app.app_builder.sm.find_user(username=username)):


Same logic as previously, though indented due to the context manager.

john-bodley · 2022-05-01T23:49:03Z

superset/sql_lab.py

@@ -149,37 +156,35 @@ def get_sql_results(  # pylint: disable=too-many-arguments
    rendered_query: str,
    return_results: bool = True,
    store_results: bool = False,
-    user_name: Optional[str] = None,
+    username: Optional[str] = None,


We still need to pass in the username to the async Celery task and then override.

john-bodley · 2022-05-02T23:40:11Z

tests/integration_tests/base_tests.py

            self.logout()
-            self.login(username=(user_name or "admin"))
+            self.login(username=username)


Given that on like #372 username is truthy there's no need for the username or "admin" logic.

codecov · 2022-05-03T00:06:24Z

Codecov Report

Merging #19914 (fff9ad0) into master (fff9ad0) will not change coverage.
The diff coverage is n/a.

❗ Current head fff9ad0 differs from pull request most recent head ea901ba. Consider uploading reports for the commit ea901ba to get more accurate results

@@           Coverage Diff           @@
##           master   #19914   +/-   ##
=======================================
  Coverage   66.36%   66.36%           
=======================================
  Files        1712     1712           
  Lines       64088    64088           
  Branches     6744     6744           
=======================================
  Hits        42529    42529           
  Misses      19846    19846           
  Partials     1713     1713

Flag	Coverage Δ
hive	`53.65% <0.00%> (ø)`
mysql	`82.04% <0.00%> (ø)`
postgres	`82.10% <0.00%> (ø)`
presto	`53.51% <0.00%> (ø)`
python	`82.53% <0.00%> (ø)`
sqlite	`81.83% <0.00%> (ø)`
unit	`49.01% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fff9ad0...ea901ba. Read the comment docs.

dpgaspar · 2022-05-10T12:02:53Z

superset/utils/core.py

@@ -1400,6 +1401,30 @@ def get_username() -> Optional[str]:
        return None


+@contextmanager
+def override_user(user: Optional[User]) -> Iterator[Any]:


should it be def override_user(user: Optional[User]) -> Iterator[None]: ?

dpgaspar · 2022-05-10T12:20:27Z

superset/utils/core.py

+    else:
+        g.user = user
+        yield
+        delattr(g, "user")


g is thread safe, it's created here: https://github.com/pallets/flask/blob/bd56d19b167822a9a23e2e9e2a07ccccc36baa8d/src/flask/globals.py#L59

Uses werkzeug Local: https://werkzeug.palletsprojects.com/en/1.0.x/local/#module-werkzeug.local

villebro

LGTM. I've long been annoyed by the user_?name = g.user.username if g.user and hasattr(g.user, "username") else None snippet that is littered around the codebase. Super happy to see it removed and centralized behind a single codepath.

Co-authored-by: John Bodley <john.bodley@airbnb.com>

pull-request-size bot added the size/XL label May 1, 2022

john-bodley changed the title ~~fix: Refactor SQL username logic~~ fix: Refactor SQL engine username logic May 1, 2022

john-bodley commented May 1, 2022

View reviewed changes

john-bodley force-pushed the john-bodley0--fix-username branch from 405a7d1 to c3f77b9 Compare May 2, 2022 00:01

john-bodley requested review from betodealmeida, villebro and dpgaspar and removed request for betodealmeida May 2, 2022 01:19

john-bodley force-pushed the john-bodley0--fix-username branch from c3f77b9 to 7fc9f54 Compare May 2, 2022 01:20

john-bodley marked this pull request as ready for review May 2, 2022 02:34

john-bodley force-pushed the john-bodley0--fix-username branch 7 times, most recently from 6e69f87 to 0fd917d Compare May 2, 2022 23:37

john-bodley commented May 2, 2022

View reviewed changes

john-bodley force-pushed the john-bodley0--fix-username branch from 0fd917d to 77d90ff Compare May 2, 2022 23:59

john-bodley force-pushed the john-bodley0--fix-username branch 2 times, most recently from 00c93e0 to 31c405e Compare May 4, 2022 03:39

john-bodley requested review from etr2460 and ktmud May 6, 2022 17:18

dpgaspar approved these changes May 10, 2022

View reviewed changes

dpgaspar mentioned this pull request May 10, 2022

feat: RLS for SQL Lab #19999

Merged

9 tasks

villebro approved these changes May 11, 2022

View reviewed changes

john-bodley force-pushed the john-bodley0--fix-username branch 2 times, most recently from 1792826 to 8d6dfb6 Compare May 12, 2022 23:52

fix: Refactor SQL username logic

7431fb6

john-bodley force-pushed the john-bodley0--fix-username branch from 8d6dfb6 to 7431fb6 Compare May 13, 2022 03:17

john-bodley merged commit 449d08b into apache:master May 13, 2022

john-bodley deleted the john-bodley0--fix-username branch May 13, 2022 04:03

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022

fix: Refactor SQL username logic (apache#19914)

ef1403d

Co-authored-by: John Bodley <john.bodley@airbnb.com>

john-bodley mentioned this pull request Jun 10, 2022

chore(rls): Remove passing global username #20344

Merged

9 tasks

john-bodley mentioned this pull request Jul 5, 2022

refactor: Deprecate ensure_user_is_set in favor of override_user #20502

Merged

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Refactor SQL engine username logic #19914

fix: Refactor SQL engine username logic #19914

john-bodley commented May 1, 2022 •

edited

Loading

john-bodley May 1, 2022

john-bodley May 1, 2022

john-bodley May 1, 2022

john-bodley May 1, 2022

john-bodley May 1, 2022

john-bodley May 1, 2022

john-bodley May 2, 2022

codecov bot commented May 3, 2022 •

edited

Loading

dpgaspar May 10, 2022

dpgaspar May 10, 2022

villebro left a comment

fix: Refactor SQL engine username logic #19914

fix: Refactor SQL engine username logic #19914

Conversation

john-bodley commented May 1, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 3, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

john-bodley commented May 1, 2022 •

edited

Loading

codecov bot commented May 3, 2022 •

edited

Loading