From 4b1edf322e8f129bcc63af64c80624021d1d2430 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 29 Jan 2020 14:33:01 +0000 Subject: [PATCH 1/4] [query] deprecate can access owned queries permission --- superset/security/manager.py | 15 ++--- superset/views/core.py | 2 +- superset/views/sql_lab.py | 6 +- tests/sqllab_tests.py | 118 +++++++++++++++++++++-------------- 4 files changed, 81 insertions(+), 60 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 8b436209e59ed..58a2db1d1fac3 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -117,6 +117,7 @@ class SupersetSecurityManager(SecurityManager): "can_override_role_permissions", "can_approve", "can_update_role", + "can_access_all_queries", } READ_ONLY_PERMISSION = {"can_show", "can_list"} @@ -132,7 +133,6 @@ class SupersetSecurityManager(SecurityManager): "schema_access", "datasource_access", "metric_access", - "can_only_access_owned_queries", } ACCESSIBLE_PERMS = {"can_userinfo"} @@ -177,15 +177,13 @@ def can_access(self, permission_name: str, view_name: str) -> bool: return self.is_item_public(permission_name, view_name) return self._has_view_access(user, permission_name, view_name) - def can_only_access_owned_queries(self) -> bool: + def can_access_all_queries(self) -> bool: """ - Return True if the user can only access owned queries, False otherwise. + Return True if the user can access all queries, False otherwise. - :returns: Whether the use can only access owned queries + :returns: Whether the use can access all queries """ - return self.can_access( - "can_only_access_owned_queries", "can_only_access_owned_queries" - ) + return self.can_access("can_access_all_queries", "can_access_all_queries") def all_datasource_access(self) -> bool: """ @@ -538,11 +536,10 @@ def create_custom_permissions(self) -> None: """ Create custom FAB permissions. """ - self.add_permission_view_menu("all_datasource_access", "all_datasource_access") self.add_permission_view_menu("all_database_access", "all_database_access") self.add_permission_view_menu( - "can_only_access_owned_queries", "can_only_access_owned_queries" + "can_access_all_queries", "can_access_all_queries" ) def create_missing_perms(self) -> None: diff --git a/superset/views/core.py b/superset/views/core.py index d96d67c7960fb..27d54da588145 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2597,7 +2597,7 @@ def search_queries(self) -> Response: :returns: Response with list of sql query dicts """ query = db.session.query(Query) - if security_manager.can_only_access_owned_queries(): + if not security_manager.can_access_all_queries(): search_user_id = g.user.get_user_id() else: search_user_id = request.args.get("user_id") diff --git a/superset/views/sql_lab.py b/superset/views/sql_lab.py index e531418688800..4bbe38531d8a6 100644 --- a/superset/views/sql_lab.py +++ b/superset/views/sql_lab.py @@ -40,12 +40,12 @@ class QueryFilter(BaseFilter): # pylint: disable=too-few-public-methods def apply(self, query: BaseQuery, value: Callable) -> BaseQuery: """ - Filter queries to only those owned by current user if - can_only_access_owned_queries permission is set. + Filter queries to only those owned by current user. If + can_access_all_queries permission is set a user can list all queries :returns: query """ - if security_manager.can_only_access_owned_queries(): + if not security_manager.can_access_all_queries(): query = query.filter(Query.user_id == g.user.get_user_id()) return query diff --git a/tests/sqllab_tests.py b/tests/sqllab_tests.py index 05566d7a81dcc..0cce56efa7171 100644 --- a/tests/sqllab_tests.py +++ b/tests/sqllab_tests.py @@ -223,41 +223,56 @@ def test_search_query_on_time(self): data = json.loads(resp) self.assertEqual(2, len(data)) - def test_search_query_with_owner_only_perms(self) -> None: + def test_search_query_only_owned(self) -> None: """ - Test a search query with can_only_access_owned_queries perm added to - Admin and make sure only Admin queries show up. + Test a search query with a user that does not have can_access_all_queries. """ - session = db.session - - # Add can_only_access_owned_queries perm to Admin user - owned_queries_view = security_manager.find_permission_view_menu( - "can_only_access_owned_queries", "can_only_access_owned_queries" - ) - security_manager.add_permission_role( - security_manager.find_role("Admin"), owned_queries_view - ) - session.commit() - - # Test search_queries for Admin user + # Test search_queries for Alpha user self.run_some_queries() - self.login("admin") + self.login("gamma_sqllab") - user_id = security_manager.find_user("admin").id + user_id = security_manager.find_user("gamma_sqllab").id data = self.get_json_resp("/superset/search_queries") - self.assertEqual(2, len(data)) + + self.assertEqual(1, len(data)) user_ids = {k["userId"] for k in data} self.assertEqual(set([user_id]), user_ids) - # Remove can_only_access_owned_queries from Admin - owned_queries_view = security_manager.find_permission_view_menu( - "can_only_access_owned_queries", "can_only_access_owned_queries" - ) - security_manager.del_permission_role( - security_manager.find_role("Admin"), owned_queries_view - ) - - session.commit() + # def test_search_query_with_owner_only_perms(self) -> None: + # """ + # Test a search query with can_only_access_owned_queries perm added to + # Admin and make sure only Admin queries show up. + # """ + # session = db.session + # + # # Add can_only_access_owned_queries perm to Admin user + # owned_queries_view = security_manager.find_permission_view_menu( + # "can_only_access_owned_queries", "can_only_access_owned_queries" + # ) + # security_manager.add_permission_role( + # security_manager.find_role("Admin"), owned_queries_view + # ) + # session.commit() + # + # # Test search_queries for Admin user + # self.run_some_queries() + # self.login("admin") + # + # user_id = security_manager.find_user("admin").id + # data = self.get_json_resp("/superset/search_queries") + # self.assertEqual(2, len(data)) + # user_ids = {k["userId"] for k in data} + # self.assertEqual(set([user_id]), user_ids) + # + # # Remove can_only_access_owned_queries from Admin + # owned_queries_view = security_manager.find_permission_view_menu( + # "can_only_access_owned_queries", "can_only_access_owned_queries" + # ) + # security_manager.del_permission_role( + # security_manager.find_role("Admin"), owned_queries_view + # ) + # + # session.commit() def test_alias_duplicate(self): self.run_sql( @@ -356,45 +371,54 @@ def test_queryview_filter(self) -> None: assert admin.username in user_queries assert gamma_sqllab.username in user_queries - def test_queryview_filter_owner_only(self) -> None: + def test_queryview_can_access_all_queries(self) -> None: """ - Test queryview api with can_only_access_owned_queries perm added to - Admin and make sure only Admin queries show up. + Test queryview api with can_access_all_queries perm added to + gamma and make sure all queries show up. """ session = db.session - # Add can_only_access_owned_queries perm to Admin user - owned_queries_view = security_manager.find_permission_view_menu( - "can_only_access_owned_queries", "can_only_access_owned_queries" + # Add can_access_all_queries perm to Gamma user + all_queries_view = security_manager.find_permission_view_menu( + "can_access_all_queries", "can_access_all_queries" ) + security_manager.add_permission_role( - security_manager.find_role("Admin"), owned_queries_view + security_manager.find_role("gamma_sqllab"), all_queries_view ) session.commit() # Test search_queries for Admin user self.run_some_queries() - self.login("admin") - + self.login("gamma_sqllab") url = "/queryview/api/read" data = self.get_json_resp(url) - admin = security_manager.find_user("admin") - self.assertEqual(2, len(data["result"])) - all_admin_user_queries = all( - [result.get("username") == admin.username for result in data["result"]] - ) - assert all_admin_user_queries is True + self.assertEqual(3, len(data["result"])) - # Remove can_only_access_owned_queries from Admin - owned_queries_view = security_manager.find_permission_view_menu( - "can_only_access_owned_queries", "can_only_access_owned_queries" + # Remove can_access_all_queries from gamma sqllab + all_queries_view = security_manager.find_permission_view_menu( + "can_access_all_queries", "can_access_all_queries" ) security_manager.del_permission_role( - security_manager.find_role("Admin"), owned_queries_view + security_manager.find_role("gamma_sqllab"), all_queries_view ) session.commit() + def test_queryview_admin_can_access_all_queries(self) -> None: + """ + Test queryview api with can_access_all_queries perm added to + Admin and make sure only Admin queries show up. This is the default + """ + # Test search_queries for Admin user + self.run_some_queries() + self.login("admin") + + url = "/queryview/api/read" + data = self.get_json_resp(url) + admin = security_manager.find_user("admin") + self.assertEqual(3, len(data["result"])) + def test_api_database(self): self.login("admin") self.create_fake_db() @@ -407,7 +431,7 @@ def test_api_database(self): "page": 0, "page_size": -1, } - url = "api/v1/database/?{}={}".format("q", prison.dumps(arguments)) + url = f"api/v1/database/?q={prison.dumps(arguments)}" self.assertEqual( {"examples", "fake_db_100"}, {r.get("database_name") for r in self.get_json_resp(url)["result"]}, From 4fa6167ae706fb9ec2f088e068518616ba55ec1e Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Wed, 29 Jan 2020 14:37:58 +0000 Subject: [PATCH 2/4] [query] remove commented code --- tests/sqllab_tests.py | 36 ------------------------------------ 1 file changed, 36 deletions(-) diff --git a/tests/sqllab_tests.py b/tests/sqllab_tests.py index 0cce56efa7171..9c65ba56ae973 100644 --- a/tests/sqllab_tests.py +++ b/tests/sqllab_tests.py @@ -238,42 +238,6 @@ def test_search_query_only_owned(self) -> None: user_ids = {k["userId"] for k in data} self.assertEqual(set([user_id]), user_ids) - # def test_search_query_with_owner_only_perms(self) -> None: - # """ - # Test a search query with can_only_access_owned_queries perm added to - # Admin and make sure only Admin queries show up. - # """ - # session = db.session - # - # # Add can_only_access_owned_queries perm to Admin user - # owned_queries_view = security_manager.find_permission_view_menu( - # "can_only_access_owned_queries", "can_only_access_owned_queries" - # ) - # security_manager.add_permission_role( - # security_manager.find_role("Admin"), owned_queries_view - # ) - # session.commit() - # - # # Test search_queries for Admin user - # self.run_some_queries() - # self.login("admin") - # - # user_id = security_manager.find_user("admin").id - # data = self.get_json_resp("/superset/search_queries") - # self.assertEqual(2, len(data)) - # user_ids = {k["userId"] for k in data} - # self.assertEqual(set([user_id]), user_ids) - # - # # Remove can_only_access_owned_queries from Admin - # owned_queries_view = security_manager.find_permission_view_menu( - # "can_only_access_owned_queries", "can_only_access_owned_queries" - # ) - # security_manager.del_permission_role( - # security_manager.find_role("Admin"), owned_queries_view - # ) - # - # session.commit() - def test_alias_duplicate(self): self.run_sql( "SELECT name as col, gender as col FROM birth_names LIMIT 10", From 07022896f34be16b97d5c5e5cfc9a3be50ee81a2 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 30 Jan 2020 10:31:55 +0000 Subject: [PATCH 3/4] [csv upload] address comments --- superset/security/manager.py | 10 ++++------ superset/views/core.py | 11 ++++++++--- tests/sqllab_tests.py | 10 +++++----- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 58a2db1d1fac3..e46aa693241fa 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -117,7 +117,7 @@ class SupersetSecurityManager(SecurityManager): "can_override_role_permissions", "can_approve", "can_update_role", - "can_access_all_queries", + "all_query_access", } READ_ONLY_PERMISSION = {"can_show", "can_list"} @@ -181,9 +181,9 @@ def can_access_all_queries(self) -> bool: """ Return True if the user can access all queries, False otherwise. - :returns: Whether the use can access all queries + :returns: Whether the user can access all queries """ - return self.can_access("can_access_all_queries", "can_access_all_queries") + return self.can_access("all_query_access", "all_query_access") def all_datasource_access(self) -> bool: """ @@ -538,9 +538,7 @@ def create_custom_permissions(self) -> None: """ self.add_permission_view_menu("all_datasource_access", "all_datasource_access") self.add_permission_view_menu("all_database_access", "all_database_access") - self.add_permission_view_menu( - "can_access_all_queries", "can_access_all_queries" - ) + self.add_permission_view_menu("all_query_access", "all_query_access") def create_missing_perms(self) -> None: """ diff --git a/superset/views/core.py b/superset/views/core.py index 27d54da588145..cbc690129701c 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -2597,10 +2597,15 @@ def search_queries(self) -> Response: :returns: Response with list of sql query dicts """ query = db.session.query(Query) - if not security_manager.can_access_all_queries(): - search_user_id = g.user.get_user_id() - else: + if security_manager.can_access_all_queries(): search_user_id = request.args.get("user_id") + elif ( + request.args.get("user_id") is not None + and request.args.get("user_id") != g.user.get_user_id() + ): + return Response(status=403, mimetype="application/json") + else: + search_user_id = g.user.get_user_id() database_id = request.args.get("database_id") search_text = request.args.get("search_text") status = request.args.get("status") diff --git a/tests/sqllab_tests.py b/tests/sqllab_tests.py index 9c65ba56ae973..be452c558781a 100644 --- a/tests/sqllab_tests.py +++ b/tests/sqllab_tests.py @@ -342,9 +342,9 @@ def test_queryview_can_access_all_queries(self) -> None: """ session = db.session - # Add can_access_all_queries perm to Gamma user + # Add all_query_access perm to Gamma user all_queries_view = security_manager.find_permission_view_menu( - "can_access_all_queries", "can_access_all_queries" + "all_query_access", "all_query_access" ) security_manager.add_permission_role( @@ -359,9 +359,9 @@ def test_queryview_can_access_all_queries(self) -> None: data = self.get_json_resp(url) self.assertEqual(3, len(data["result"])) - # Remove can_access_all_queries from gamma sqllab + # Remove all_query_access from gamma sqllab all_queries_view = security_manager.find_permission_view_menu( - "can_access_all_queries", "can_access_all_queries" + "all_query_access", "all_query_access" ) security_manager.del_permission_role( security_manager.find_role("gamma_sqllab"), all_queries_view @@ -371,7 +371,7 @@ def test_queryview_can_access_all_queries(self) -> None: def test_queryview_admin_can_access_all_queries(self) -> None: """ - Test queryview api with can_access_all_queries perm added to + Test queryview api with all_query_access perm added to Admin and make sure only Admin queries show up. This is the default """ # Test search_queries for Admin user From 4e8f617b7843854850ea19b66a03469a3d5275b2 Mon Sep 17 00:00:00 2001 From: Daniel Gaspar Date: Thu, 30 Jan 2020 13:52:10 +0000 Subject: [PATCH 4/4] Updated UPDATING --- UPDATING.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/UPDATING.md b/UPDATING.md index 533414fcea29a..a01e1b8a163bd 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -22,6 +22,11 @@ This file documents any backwards-incompatible changes in Superset and assists people when migrating to a new version. ## Next +* [9046](https://github.com/apache/incubator-superset/pull/9046): Replaces `can_only_access_owned_queries` by +`all_query_access` favoring a white list approach. Since a new permission is introduced use `superset init` +to create and associate it by default to the `Admin` role. Note that, by default, all non `Admin` users will +not be able to access queries they do not own. + * [8901](https://github.com/apache/incubator-superset/pull/8901): The datasource's update timestamp has been added to the query object's cache key to ensure updates to datasources are always reflected in associated query results. As a consequence all