(superset) 05/16: fix(security manager): Users should not have access to all draft dashboards (#27015)

[mi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

michaelsmolina pushed a commit to branch 3.1
in repository https://gitbox.apache.org/repos/asf/superset.git

commit 5a6109b2dee046764babcfef4a5a67c32f2256ea
Author: Vitor Avila <96...@users.noreply.github.com>
AuthorDate: Wed Feb 7 13:20:41 2024 -0300

    fix(security manager): Users should not have access to all draft dashboards (#27015)
    
    (cherry picked from commit 01e2f8ace31950ca337a6a8d7348d37c59cf8126)
---
 superset/security/manager.py                       | 31 ++++++++++++----------
 .../dashboards/security/security_rbac_tests.py     | 24 ++++++++++-------
 2 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/superset/security/manager.py b/superset/security/manager.py
index 28046d0fef..e11ab68e74 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -2018,25 +2018,28 @@ class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
             if self.is_admin() or self.is_owner(dashboard):
                 return
 
-            # RBAC and legacy (datasource inferred) access controls.
+            # TODO: Once a better sharing flow is in place, we should move the
+            # dashboard.published check here so that it's applied to both
+            # regular RBAC and DASHBOARD_RBAC
+
+            # DASHBOARD_RBAC logic - Manage dashboard access through roles.
+            # Only applicable in case the dashboard has roles set.
             if is_feature_enabled("DASHBOARD_RBAC") and dashboard.roles:
                 if dashboard.published and {role.id for role in dashboard.roles} & {
                     role.id for role in self.get_user_roles()
                 }:
                     return
-            elif (
-                # To understand why we rely on status and give access to draft dashboards
-                # without roles take a look at:
-                #
-                #   - https://github.com/apache/superset/pull/24350#discussion_r1225061550
-                #   - https://github.com/apache/superset/pull/17511#issuecomment-975870169
-                #
-                not dashboard.published
-                or not dashboard.datasources
-                or any(
-                    self.can_access_datasource(datasource)
-                    for datasource in dashboard.datasources
-                )
+
+            # REGULAR RBAC logic
+            # User can only acess the dashboard in case:
+            #    It doesn't have any datasets; OR
+            #    They have access to at least one dataset used.
+            # We currently don't check if the dashboard is published,
+            # to allow creators to share a WIP dashboard with a viewer
+            # to collect feedback.
+            elif not dashboard.datasources or any(
+                self.can_access_datasource(datasource)
+                for datasource in dashboard.datasources
             ):
                 return
 
diff --git a/tests/integration_tests/dashboards/security/security_rbac_tests.py b/tests/integration_tests/dashboards/security/security_rbac_tests.py
index 792c9d1716..4f1cc5b221 100644
--- a/tests/integration_tests/dashboards/security/security_rbac_tests.py
+++ b/tests/integration_tests/dashboards/security/security_rbac_tests.py
@@ -404,22 +404,28 @@ class TestDashboardRoleBasedSecurity(BaseTestDashboardSecurity):
         for dash in published_dashboards + draft_dashboards:
             revoke_access_to_dashboard(dash, "Public")
 
-    def test_get_draft_dashboard_without_roles_by_uuid(self):
+    def test_cannot_get_draft_dashboard_without_roles_by_uuid(self):
         """
         Dashboard API: Test get draft dashboard without roles by uuid
         """
         admin = self.get_user("admin")
-        dashboard = self.insert_dashboard("title", "slug1", [admin.id])
-        assert not dashboard.published
-        assert dashboard.roles == []
+
+        database = create_database_to_db(name="test_db_rbac")
+        table = create_datasource_table_to_db(
+            name="test_datasource_rbac", db_id=database.id, owners=[admin]
+        )
+        dashboard_to_access = create_dashboard_to_db(
+            dashboard_title="test_dashboard_rbac",
+            owners=[admin],
+            slices=[create_slice_to_db(datasource_id=table.id)],
+        )
+        assert not dashboard_to_access.published
+        assert dashboard_to_access.roles == []
 
         self.login(username="gamma")
-        uri = f"api/v1/dashboard/{dashboard.uuid}"
+        uri = f"api/v1/dashboard/{dashboard_to_access.uuid}"
         rv = self.client.get(uri)
-        assert rv.status_code == 200
-        # rollback changes
-        db.session.delete(dashboard)
-        db.session.commit()
+        assert rv.status_code == 403
 
     def test_cannot_get_draft_dashboard_with_roles_by_uuid(self):
         """