-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
fix(dashboard-rbac): use normal rbac when no roles chosen #23586
Changes from all commits
ec6389f
5ea6461
1ebf751
efef1d4
6022e5d
6eead1c
3eb4e07
24ce746
777196d
f6a459c
cacc2c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,6 +67,7 @@ | |
TableColumn, | ||
) | ||
from superset.constants import QUERY_EARLY_CANCEL_KEY | ||
from superset.dashboards.commands.exceptions import DashboardAccessDeniedError | ||
from superset.dashboards.commands.importers.v0 import ImportDashboardsCommand | ||
from superset.dashboards.dao import DashboardDAO | ||
from superset.dashboards.permalink.commands.get import GetDashboardPermalinkCommand | ||
|
@@ -167,6 +168,7 @@ | |
get_form_data, | ||
get_viz, | ||
loads_request_json, | ||
redirect_with_flash, | ||
sanitize_datasource_data, | ||
) | ||
from superset.viz import BaseViz | ||
|
@@ -192,6 +194,7 @@ | |
"disable_data_preview", | ||
] | ||
|
||
DASHBOARD_LIST_URL = "/dashboard/list/" | ||
DATASOURCE_MISSING_ERR = __("The data source seems to have been deleted") | ||
USER_MISSING_ERR = __("The user seems to have been deleted") | ||
PARAMETER_MISSING_ERR = __( | ||
|
@@ -1826,9 +1829,7 @@ def favstar( # pylint: disable=no-self-use | |
@expose("/dashboard/<dashboard_id_or_slug>/") | ||
@event_logger.log_this_with_extra_payload | ||
@check_dashboard_access( | ||
on_error=lambda self, ex: Response( | ||
utils.error_msg_from_exception(ex), status=403 | ||
) | ||
on_error=lambda msg: redirect_with_flash(DASHBOARD_LIST_URL, msg, "danger") | ||
) | ||
def dashboard( | ||
self, | ||
|
@@ -1846,25 +1847,33 @@ def dashboard( | |
if not dashboard: | ||
abort(404) | ||
|
||
if config["ENABLE_ACCESS_REQUEST"]: | ||
for datasource in dashboard.datasources: | ||
datasource = DatasourceDAO.get_datasource( | ||
datasource_type=DatasourceType(datasource.type), | ||
datasource_id=datasource.id, | ||
session=db.session(), | ||
has_access_ = False | ||
for datasource in dashboard.datasources: | ||
datasource = DatasourceDAO.get_datasource( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are the datasets reloaded? |
||
datasource_type=DatasourceType(datasource.type), | ||
datasource_id=datasource.id, | ||
session=db.session(), | ||
) | ||
if datasource and security_manager.can_access_datasource( | ||
datasource=datasource, | ||
): | ||
has_access_ = True | ||
|
||
if has_access_ is False and config["ENABLE_ACCESS_REQUEST"]: | ||
flash( | ||
__(security_manager.get_datasource_access_error_msg(datasource)), | ||
"danger", | ||
) | ||
if datasource and not security_manager.can_access_datasource( | ||
datasource=datasource | ||
): | ||
flash( | ||
__( | ||
security_manager.get_datasource_access_error_msg(datasource) | ||
), | ||
"danger", | ||
) | ||
return redirect( | ||
f"/superset/request_access/?dashboard_id={dashboard.id}" | ||
) | ||
return redirect( | ||
f"/superset/request_access/?dashboard_id={dashboard.id}" | ||
) | ||
|
||
if has_access_: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we break after confirming that they have access to at least one as opposed to all datasets? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answering both this and the previous question here:
|
||
break | ||
|
||
if dashboard.datasources and not has_access_: | ||
flash(DashboardAccessDeniedError.message, "danger") | ||
return redirect(DASHBOARD_LIST_URL) | ||
|
||
dash_edit_perm = security_manager.is_owner( | ||
dashboard | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@villebro can all this logic reside in the security manager? The issue is having security logic outside of the security manager adds complexity if you have custom logic in your security manager which grants everyone access to view the dashboard but block at the chart level if you don't have access to the underlying dataset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it would probably make sense to have this in the security manager. However, in the scope of this PR I decided not to change the existing logic.