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

fix: Dashboard access when DASHBOARD_RBAC is disabled #17511

Merged

Conversation

michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented Nov 22, 2021

SUMMARY

This PR fixes the security manager to also check for dashboard access when DASHBOARD_RBAC is disabled. Previously, if DASHBOARD_RBAC was disabled, this piece of code was skipped:

can_access = (
    is_user_admin()
    or is_owner(dashboard, g.user)
    or (dashboard.published and has_rbac_access)
)

I also changed some endpoints to emit a 403 when an access error occurs.

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

@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 22, 2021
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for fixing!

tests/integration_tests/dashboards/api_tests.py Outdated Show resolved Hide resolved
@michael-s-molina
Copy link
Member Author

@villebro @amitmiran137 these tests in dashboard/api_tests.py are checking for specific conditions:

@pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
def test_get_draft_dashboard_datasets(self):
    """
    All users should have access to dashboards without roles
    """
    self.login(username="gamma")
    uri = "api/v1/dashboard/world_health/datasets"
    response = self.get_assert_metric(uri, "get_datasets")
    self.assertEqual(response.status_code, 200)

@pytest.mark.usefixtures("create_dashboards")
def test_get_draft_dashboard_charts(self):
    """
    All users should have access to draft dashboards without roles
    """
    self.login(username="gamma")
    dashboard = self.dashboards[0]
    uri = f"api/v1/dashboard/{dashboard.id}/charts"
    response = self.get_assert_metric(uri, "get_charts")
    assert response.status_code == 200

I added the following line giving access to any user if the dashboard is not published and has no roles.

or (not dashboard.published and not dashboard.roles)

Can you confirm that this is the expected behavior?

Copy link
Member

@amitmiran137 amitmiran137 left a comment

Choose a reason for hiding this comment

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

The original intention here was to keep the old behaviour before DASHBOARD_RBAC era where there was no enforcement to dashboard access at all

But I agree that access should be enforced.

There was another idea by @dpgaspar
That We might want to consider switching to 404 this would prevent malicious API calls from finding out which dashboard IDs exist and which are not.

Altough personaly I prefer 403

@villebro
Copy link
Member

The original intention here was to keep the old behaviour before DASHBOARD_RBAC era where there was no enforcement to dashboard access at all

But I agree that access should be enforced.

There was another idea by @dpgaspar That We might want to consider switching to 404 this would prevent malicious API calls from finding out which dashboard IDs exist and which are not.

Altough personaly I prefer 403

Right, I remember we were pretty prudent during review in making sure the behavior was unchanged when the feature flag was disabled. Regarding 403/404 , I'm also more a 403 type of guy for this sort of stuff, but I'm ok with 404 if that's the security pattern we've chosen.

@villebro
Copy link
Member

I tagged @john-bodley and @etr2460 for reviews - can you think of a reason why dashboard access should not be enforced? The raise_for_dashboard_access method was added in #12875 - previously this didn't exist at all. To me it seems this access should be enforced.

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

Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

Looks good, personally I think that this is getting hard to follow, security filters set here: https://github.com/apache/superset/blob/master/superset/dashboards/filters.py#L72 should probably use the same code path for raise_for_dashboard_access

@@ -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.

@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #17511 (e993221) into master (1f8eff7) will decrease coverage by 0.21%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17511      +/-   ##
==========================================
- Coverage   76.99%   76.77%   -0.22%     
==========================================
  Files        1046     1047       +1     
  Lines       56491    56505      +14     
  Branches     7798     7798              
==========================================
- Hits        43494    43384     -110     
- Misses      12741    12865     +124     
  Partials      256      256              
Flag Coverage Δ
hive ?
mysql 81.99% <70.00%> (-0.01%) ⬇️
postgres 82.00% <70.00%> (-0.01%) ⬇️
presto ?
python 82.08% <70.00%> (-0.42%) ⬇️
sqlite 81.68% <70.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset/dashboards/api.py 92.15% <50.00%> (-0.89%) ⬇️
superset/security/manager.py 91.86% <100.00%> (+0.02%) ⬆️
superset/db_engines/hive.py 0.00% <0.00%> (-85.19%) ⬇️
superset/db_engine_specs/hive.py 70.27% <0.00%> (-16.22%) ⬇️
superset/db_engine_specs/presto.py 83.50% <0.00%> (-6.89%) ⬇️
superset/commands/utils.py 97.05% <0.00%> (-2.95%) ⬇️
superset/views/database/mixins.py 81.03% <0.00%> (-1.73%) ⬇️
superset/connectors/sqla/models.py 86.79% <0.00%> (-1.59%) ⬇️
superset/models/core.py 89.26% <0.00%> (-0.74%) ⬇️
superset/db_engine_specs/base.py 88.20% <0.00%> (-0.39%) ⬇️
... 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 1f8eff7...e993221. Read the comment docs.

@michael-s-molina michael-s-molina merged commit 7602431 into apache:master Nov 23, 2021
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
* fix: Dashboard access when RBAC is disabled

* Sends 403 when forbidden

* Fixes issort

* Changes assertion

* Allow access to unpublished dashboards that don't have roles

* Fixes the test_get_dashboard_changed_on test
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants