feat: On window focus, redirect to login if the user has been logged out #18773

suddjian · 2022-02-17T01:00:36Z

SUMMARY

If you have Superset open in the background and your session expires, the frontend will still display the logged-in state until the next request happens to fire, jarringly sending you all of a sudden to the login page.

Until now.

This code listens for the document's visibilitychange event. When the tab is focused, a request is made to the new /api/v1/me/ endpoint, which returns 401 if the user is not logged in. The SupersetClient detects the 401 and sends the user to the login page.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-02-16.at.4.58.09.PM.mov

TESTING INSTRUCTIONS

Open Superset in two tabs. Log out of one tab. Switch to the other tab. It will immediately redirect to the login page.

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

suddjian · 2022-02-17T01:02:35Z

superset/users/api.py

+    openapi_spec_component_schemas = (UserResponseSchema,)
+
+    @expose("/", methods=["GET"])
+    @safe


I didn't want to add a permission for this API, because it should be callable by any user. @dpgaspar this is safe, yeah?

I think this getMe API is great, as it may have plenty o' uses (e.g. when we revisit the Profile design). But just to say it "out loud" we could also implement a GET /time API or something as a lightweight, security-irrelevant API, if performance or security ever become a factor here.

suddjian · 2022-02-17T01:03:11Z

superset/views/utils.py

@@ -81,6 +81,7 @@ def bootstrap_user_data(user: User, include_perms: bool = False) -> Dict[str, An
            "lastName": user.last_name,
            "userId": user.id,
            "isActive": user.is_active,
+            "isAnonymous": user.is_anonymous,


Added is_anonymous here for consistency

ad-m · 2022-02-17T02:32:37Z

superset-frontend/src/preamble.ts

+if (bootstrapData.user?.isActive) {
+  document.addEventListener('visibilitychange', () => {
+    // we only care about the tab becoming visible, not vice versa
+    if (document.visibilityState !== 'visible') return;


Do we want to do this every time the user changes a tab (even if there are two Superset tabs), or maybe not more often than every x seconds?

It's a pretty cheap endpoint to call, and most people probably aren't changing tabs often enough to be an issue. I think this is fine.

Agree with @suddjian , but good callout — I always appreciate a performance-oriented mindset

codecov · 2022-02-17T22:18:17Z

Codecov Report

Merging #18773 (978864f) into master (3cccc63) will increase coverage by 0.01%.
The diff coverage is 83.01%.

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

@@            Coverage Diff             @@
##           master   #18773      +/-   ##
==========================================
+ Coverage   66.31%   66.33%   +0.01%     
==========================================
  Files        1620     1622       +2     
  Lines       63080    63114      +34     
  Branches     6370     6372       +2     
==========================================
+ Hits        41833    41865      +32     
- Misses      19591    19592       +1     
- Partials     1656     1657       +1

Flag	Coverage Δ
hive	`52.25% <87.09%> (+0.03%)`	⬆️
mysql	`81.44% <100.00%> (+0.02%)`	⬆️
postgres	`81.49% <100.00%> (+0.02%)`	⬆️
presto	`52.09% <87.09%> (+0.03%)`	⬆️
python	`81.92% <100.00%> (+0.02%)`	⬆️
sqlite	`81.18% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...frontend/src/SqlLab/components/ResultSet/index.tsx	`50.73% <ø> (ø)`
superset-frontend/src/preamble.ts	`0.00% <0.00%> (ø)`
...erset-frontend/src/profile/components/fixtures.tsx	`100.00% <ø> (ø)`
superset-frontend/src/views/App.tsx	`0.00% <0.00%> (ø)`
superset/views/utils.py	`82.20% <ø> (ø)`
...lLab/components/ExploreCtasResultsButton/index.tsx	`8.33% <33.33%> (ø)`
...perset-frontend/src/views/components/MenuRight.tsx	`80.00% <81.81%> (-0.49%)`	⬇️
...rset-frontend/src/components/DeleteModal/index.tsx	`84.21% <100.00%> (ø)`
superset-frontend/src/views/components/Menu.tsx	`56.41% <100.00%> (+1.14%)`	⬆️
superset/initialization/__init__.py	`90.72% <100.00%> (+0.51%)`	⬆️
... and 6 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 3cccc63...e17686b. Read the comment docs.

rusackas · 2022-02-17T23:48:40Z

superset-frontend/src/preamble.ts

-export let bootstrapData: any;
+export let bootstrapData: {
+  user?: User | undefined;
+  common?: any;


Le sigh... not the concern of this PR by any means, but maybe we should make backlog ticket(s) to add types for common/config.

rusackas · 2022-02-17T23:56:52Z

/testenv up

github-actions · 2022-02-17T23:58:37Z

@rusackas Ephemeral environment spinning up at http://18.236.157.164:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

rusackas

Curious what feedback @dpgaspar would have on the backend/security issue, but the changes look good to me, and it sure seems like it does the trick in testing!

geido · 2022-02-18T14:31:29Z

superset/users/api.py

+            401:
+              $ref: '#/components/responses/401'
+        """
+        if g.user is None or g.user.is_anonymous:


I am wondering if returning 401 for anonymous users won't effectively make read-only access impossible for them?

Oh ok I see what you are doing in the frontend to avoid this being requested for anon users bootstrapData.user?.isActive. I am thinking if this might confuse someone else when using this endpoint in some other places and if we might end up in that problem, but I might just be overthinking it right now.

This endpoint is only called if the user isActive, which is synonymous with !isAnonymous

github-actions · 2022-02-24T18:10:09Z

Ephemeral environment shutdown and build artifacts deleted.

suddjian requested review from rusackas and geido February 17, 2022 01:00

superset-github-bot bot added the preset-io label Feb 17, 2022

pull-request-size bot added the size/L label Feb 17, 2022

suddjian added 3 commits February 16, 2022 17:00

/me api

fe68c12

test it

0ff9e56

watch for window activation and check auth

d99a5c0

suddjian force-pushed the check-auth-on-focus branch from b02c381 to d99a5c0 Compare February 17, 2022 01:01

suddjian commented Feb 17, 2022

View reviewed changes

simplify

40c40ed

suddjian changed the title ~~Check auth on focus~~ feat: On window focus, redirect to login if the user has been logged out Feb 17, 2022

more comment

f86de51

ad-m reviewed Feb 17, 2022

View reviewed changes

suddjian added 2 commits February 17, 2022 13:38

making ci happy

1be3e0c

mypy should ignore tests

e17686b

rusackas reviewed Feb 17, 2022

View reviewed changes

rusackas reviewed Feb 18, 2022

View reviewed changes

geido reviewed Feb 18, 2022

View reviewed changes

rusackas approved these changes Feb 24, 2022

View reviewed changes

suddjian merged commit da3bc48 into apache:master Feb 24, 2022

rusackas deleted the check-auth-on-focus branch February 24, 2022 18:09

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: On window focus, redirect to login if the user has been logged out #18773

feat: On window focus, redirect to login if the user has been logged out #18773

suddjian commented Feb 17, 2022

suddjian Feb 17, 2022

rusackas Feb 17, 2022

suddjian Feb 17, 2022

ad-m Feb 17, 2022

suddjian Feb 17, 2022

rusackas Feb 17, 2022

codecov bot commented Feb 17, 2022 •

edited

rusackas Feb 17, 2022

rusackas commented Feb 17, 2022

github-actions bot commented Feb 17, 2022

rusackas left a comment

geido Feb 18, 2022

geido Feb 18, 2022 •

edited

suddjian Feb 18, 2022

github-actions bot commented Feb 24, 2022

feat: On window focus, redirect to login if the user has been logged out #18773

feat: On window focus, redirect to login if the user has been logged out #18773

Conversation

suddjian commented Feb 17, 2022

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

codecov bot commented Feb 17, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

rusackas commented Feb 17, 2022

github-actions bot commented Feb 17, 2022

rusackas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geido Feb 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2022

codecov bot commented Feb 17, 2022 •

edited

geido Feb 18, 2022 •

edited