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.

Sign up for GitHub

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

Jump to bottom

fix: Dashboard access when DASHBOARD_RBAC is disabled #17511

Merged
merged 6 commits into from
Nov 23, 2021
Merged

fix: Dashboard access when DASHBOARD_RBAC is disabled #17511

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
678a739
fix: Dashboard access when RBAC is disabled
michael-s-molina Nov 22, 2021
35f4a36
Sends 403 when forbidden
michael-s-molina Nov 22, 2021
cb8bce1
Fixes issort
michael-s-molina Nov 22, 2021
81e1630
Changes assertion
michael-s-molina Nov 22, 2021
893595e
Allow access to unpublished dashboards that don't have roles
michael-s-molina Nov 22, 2021
e993221
Fixes the test_get_dashboard_changed_on test
michael-s-molina Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions superset/dashboards/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from superset.dashboards.commands.create import CreateDashboardCommand
from superset.dashboards.commands.delete import DeleteDashboardCommand
from superset.dashboards.commands.exceptions import (
DashboardAccessDeniedError,
DashboardBulkDeleteFailedError,
DashboardCreateFailedError,
DashboardDeleteFailedError,
Expand Down Expand Up @@ -267,6 +268,8 @@ def get(self, id_or_slug: str) -> Response:
$ref: '#/components/responses/400'
401:
$ref: '#/components/responses/401'
403:
$ref: '#/components/responses/403'
404:
$ref: '#/components/responses/404'
"""
Expand All @@ -275,6 +278,8 @@ def get(self, id_or_slug: str) -> Response:
dash = DashboardDAO.get_by_id_or_slug(id_or_slug)
result = self.dashboard_get_response_schema.dump(dash)
return self.response(200, result=result)
except DashboardAccessDeniedError:
return self.response_403()
except DashboardNotFoundError:
return self.response_404()

Expand Down Expand Up @@ -327,6 +332,8 @@ def get_datasets(self, id_or_slug: str) -> Response:
$ref: '#/components/responses/400'
401:
$ref: '#/components/responses/401'
403:
$ref: '#/components/responses/403'
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with returning 403, although it's more coherent with the current setup that we return 404, note that the dashboard filter is serving has security so we only "show" resources that are available to the user. Also, exposing less detailed info about why a resource is not available for access to a user the better.

RFC is not 100% clear regarding this:
https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.3
https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.4
it's acceptable to return 404 if we don't want to disclose that the dashboard exists but it's access is forbidden to the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dpgaspar We have other endpoints also returning 403. I'll bring this discussion to our meeting today and if we decide to return 404 I'll open another PR fixing all endpoints.

404:
$ref: '#/components/responses/404'
"""
Expand All @@ -336,6 +343,8 @@ def get_datasets(self, id_or_slug: str) -> Response:
self.dashboard_dataset_schema.dump(dataset) for dataset in datasets
]
return self.response(200, result=result)
except DashboardAccessDeniedError:
return self.response_403()
except DashboardNotFoundError:
return self.response_404()

Expand Down Expand Up @@ -386,6 +395,8 @@ def get_charts(self, id_or_slug: str) -> Response:
$ref: '#/components/responses/400'
401:
$ref: '#/components/responses/401'
403:
$ref: '#/components/responses/403'
404:
$ref: '#/components/responses/404'
"""
Expand All @@ -401,6 +412,8 @@ def get_charts(self, id_or_slug: str) -> Response:
form_data.pop("label_colors", None)

return self.response(200, result=result)
except DashboardAccessDeniedError:
return self.response_403()
except DashboardNotFoundError:
return self.response_404()

Expand Down
18 changes: 11 additions & 7 deletions superset/security/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1174,19 +1174,23 @@ def raise_for_dashboard_access(dashboard: "Dashboard") -> None:
from superset.views.base import get_user_roles, is_user_admin
from superset.views.utils import is_owner

has_rbac_access = True

if is_feature_enabled("DASHBOARD_RBAC"):
has_rbac_access = any(
dashboard_role.id in [user_role.id for user_role in get_user_roles()]
for dashboard_role in dashboard.roles
)
can_access = (
is_user_admin()
or is_owner(dashboard, g.user)
or (dashboard.published and has_rbac_access)
)

if not can_access:
raise DashboardAccessDeniedError()
can_access = (
is_user_admin()
or is_owner(dashboard, g.user)
or (dashboard.published and has_rbac_access)
or (not dashboard.published and not dashboard.roles)
)

if not can_access:
raise DashboardAccessDeniedError()

@staticmethod
def can_access_based_on_dashboard(datasource: "BaseDatasource") -> bool:
Expand Down
2 changes: 1 addition & 1 deletion tests/integration_tests/dashboards/api_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ def test_get_dashboard_no_data_access(self):
self.login(username="gamma")
uri = f"api/v1/dashboard/{dashboard.id}"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 200)
assert rv.status_code == 200
# rollback changes
db.session.delete(dashboard)
db.session.commit()
Expand Down