Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(auth): when user is not logged in, failure to access a dashboard should redirect to login screen #30380

Merged
merged 3 commits into from
Sep 25, 2024

Conversation

sfirke
Copy link
Member

@sfirke sfirke commented Sep 24, 2024

SUMMARY

If a viewer is not logged in, or they are the public/anonymous user, and they click a link that takes them to a Superset dashboard that is not public, they are currently told "You don't have access" and sent to the list of dashboards, where they will see only public ones listed. This is often misleading: in most cases, the problem is not that they don't have access -- they just need to log in.

After the PR, such cases are routed to the login screen, and after a successful login the users are sent back to the dashboard they were trying to access.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

current_redirect.mp4

AFTER

redirect_new.mp4

note I've changed the error message since recording this

TESTING INSTRUCTIONS

Create a dashboard with restricted access, try to view it while not logged in.

ADDITIONAL INFORMATION

Implements this feature request: #22190
Replaces this stale PR: #23280

I have the DASHBOARD_RBAC flag enabled as well as the Public role in use. My code alterations are minimal so I don't think it will negatively affect deployments that differ from mine, but it would be good to have someone check.

I'm not sure how to write tests for this but am open to it if someone can advise.

not sure if changes to manager.py are necessary or an artifact of experimenting
@github-actions github-actions bot added the api Related to the REST API label Sep 24, 2024
@dosubot dosubot bot added the authentication Related to authentication label Sep 24, 2024
@sfirke sfirke changed the title feat(auth): when user is not logged in, failure to access a dashboard will redirect to login screen feat(auth): when user is not logged in, failure to access a dashboard should redirect to login screen Sep 24, 2024
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.90%. Comparing base (76d897e) to head (d8ad102).
Report is 774 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #30380       +/-   ##
===========================================
+ Coverage   60.48%   83.90%   +23.41%     
===========================================
  Files        1931      533     -1398     
  Lines       76236    38524    -37712     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32322    -13792     
+ Misses      28017     6202    -21815     
+ Partials     2105        0     -2105     
Flag Coverage Δ
hive 48.99% <0.00%> (-0.18%) ⬇️
javascript ?
mysql 76.88% <100.00%> (?)
postgres 76.95% <100.00%> (?)
presto 53.49% <0.00%> (-0.32%) ⬇️
python 83.90% <100.00%> (+20.41%) ⬆️
sqlite 76.40% <100.00%> (?)
unit 60.64% <0.00%> (+3.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a non-blocking question on the next url param.

superset/views/core.py Show resolved Hide resolved
@sfirke sfirke merged commit 69d5f76 into apache:master Sep 25, 2024
45 checks passed
@sfirke sfirke deleted the redirect-public-to-login branch September 25, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the REST API authentication Related to authentication size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants