fix: Redirect to login on unauthorised access for Dashboard.#23280
fix: Redirect to login on unauthorised access for Dashboard.#23280dheeraj281 wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
My testing, applying this patch to 2.1.0rc1 and with DASHBOARD_RBAC enabled:
Talking with @dheeraj281 in Slack it sounds like the AccessDeniedError exception in the case with the Public role configured is getting raised elsewhere. Anyone have an idea how to get this behavior for that case, too? If that should be addressed separately I can open a different issue for that. |
Codecov Report
@@ Coverage Diff @@
## master #23280 +/- ##
==========================================
+ Coverage 67.51% 67.53% +0.01%
==========================================
Files 1900 1907 +7
Lines 73318 73468 +150
Branches 7935 7976 +41
==========================================
+ Hits 49498 49613 +115
- Misses 21787 21803 +16
- Partials 2033 2052 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@sfirke the reason for that is when using a edit: nvm, looks like this code isn't as general as I thought it was |
| def on_security_exception(self: Any, ex: Exception) -> Response: | ||
| def on_security_exception(self: Any, ex: Exception) -> Union[Response, werkz_Response]: | ||
| if not g.user or not utils.get_user_id(): | ||
| return redirect(appbuilder.get_url_for_login) |
There was a problem hiding this comment.
Should probably just return the following as the caller could be expecting json response and that redirect will end up returning html. Not sure if making this change will actually still fix the issue though
| return redirect(appbuilder.get_url_for_login) | |
| return self.response(401, **{"message": utils.error_msg_from_exception(ex)}) |
There was a problem hiding this comment.
Actually, you might be right here, looks like this is only ever used in one place
There was a problem hiding this comment.
hmm, looks like the only place check_dashboard_access is actually used, this default arg is overridden with a lambda that pretty much does the exact same thing 🤔 . Now I'm confused how this is working.
superset/superset/views/core.py
Line 1822 in ed11123
There was a problem hiding this comment.
Not sure how but when I tested it is calling on_security_exception, the default callable in case of DashboardAccessDeniedError occurred. Do you think instead of making changes here in on_security_exception we should look into why lambda is not working? 🤔
Committed suggested change to handle public role redirect. Co-authored-by: ʈᵃᵢ <tai@apache.org>
|
Hi! So if we can support, please give us a hands up! |
|
@DerLinne thanks for the offer of support! I am remain eager for this feature too. Can you try testing it thoroughly on your deployment and sharing any feedback, for starters? |
|
@sadpandajoe might be able to help in testing here too? @dheeraj281 this needs a bit of a rebase to resolve conflicts, if you don't mind. |
|
ping @dheeraj281 @dheeraj-jaiswal-lowes - are you still interested in working on this fix? The project would still benefit from it! |
|
@sfirke Sure, I will take a look. |
|
I think I have a solution that works for me. Starting with the current master branch and none of the changes in this PR, I edited where the redirect goes after access check fails when user is anonymous. I replaced https://github.com/apache/superset/blob/master/superset/views/core.py#L794-L799 with: except SupersetSecurityException as ex:
# anonymous users should get the login screen, others should go to dashboard list
redirect_url = f"{appbuilder.get_url_for_login}?next={request.url}" if g.user is None or g.user.is_anonymous else "/dashboard/list/"
warn_msg = "This dashboard does not allow public access." if g.user is None or g.user.is_anonymous else utils.error_msg_from_exception(ex)
return redirect_with_flash(
url=redirect_url,
message=warn_msg,
category="danger",
)Thoughts from folks in this thread? I have DASHBOARD_RBAC enabled and a Public role enabled, I'd want to know that it works for people not using those features. |
|
Hello @sfirke ! Please, is it possible to implement it on the superset kubernetes helm chart ? Thanks in advance! Gabriel. |
|
Another PR to accomplish this was merged yesterday, #30380. Please feel free to test it out on master branch now. To tidy up the repo, I'm going to close this one as no longer needed. |
6 participants
SUMMARY
As per Feature request #22190 , When a user is trying to access the dashboard link it says you don't have access to this Dashboard instead of redirecting user to the login page. This was happening due to issue with dashboard rbac feature.
As part of fix, when dashboard rbac feature is enabled. and DashboardAccessDeniedError is raised after all the checks, it will check if user is logged in or not in the callback function and it will redirect user to login accordingly.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION