From 6b8f570bf4f583211761d9ab0dadbfdbcbff9e95 Mon Sep 17 00:00:00 2001 From: David Markey Date: Sun, 1 Sep 2024 23:13:50 +0100 Subject: [PATCH 01/13] feat(embedded) Add hook to allow superset admins to validate guest token parameters --- superset/config.py | 9 +++++++++ superset/security/api.py | 12 +++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/superset/config.py b/superset/config.py index 670c518931078..3de93e0cb2bd5 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1721,6 +1721,15 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument # Guest token audience for the embedded superset, either string or callable GUEST_TOKEN_JWT_AUDIENCE: Callable[[], str] | str | None = None +# A callable that can be supplied to do extra validation of guest token configuration +# for example certain RLS parameters: +# lambda x: len(x['rls']) == 1 and "tenent_id=" in x['rls'][0]['clause'] +# +# Takes the GuestTokenUser dict as a n argument +# Return False from the callable to return a HTTP 400 to the user. + +GUEST_TOKEN_VALIDATOR_HOOK = None + # A SQL dataset health check. Note if enabled it is strongly advised that the callable # be memoized to aid with performance, i.e., # diff --git a/superset/security/api.py b/superset/security/api.py index 61fd68e6f0e7c..50a346de93759 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -27,6 +27,8 @@ from superset.commands.dashboard.embedded.exceptions import ( EmbeddedDashboardNotFoundError, ) +from superset.config import GUEST_TOKEN_VALIDATOR_HOOK +from superset.exceptions import SupersetGenericErrorException from superset.extensions import event_logger from superset.security.guest_token import GuestTokenResourceType from superset.views.base_api import BaseSupersetApi, statsd_metrics @@ -148,7 +150,15 @@ def guest_token(self) -> Response: try: body = guest_token_create_schema.load(request.json) self.appbuilder.sm.validate_guest_token_resources(body["resources"]) - + # Run validator to ensure the token parameters are OK. + if GUEST_TOKEN_VALIDATOR_HOOK is not None: + if callable(GUEST_TOKEN_VALIDATOR_HOOK): + if not GUEST_TOKEN_VALIDATOR_HOOK(body): + raise ValidationError(message="Guest token validation failed") + else: + raise SupersetGenericErrorException( + message="Guest token validator hook not callable" + ) # todo validate stuff: # make sure username doesn't reference an existing user # check rls rules for validity? From 7f66b9e771aa939a43759038abecfa544d1493df Mon Sep 17 00:00:00 2001 From: David Markey Date: Mon, 2 Sep 2024 23:41:10 +0100 Subject: [PATCH 02/13] Add tests for GUEST_TOKEN_VALIDATOR_HOOK Some cleanup and refactoring --- superset/config.py | 4 +- superset/security/api.py | 12 +- tests/integration_tests/security/api_tests.py | 143 ++++++++++++++++++ 3 files changed, 152 insertions(+), 7 deletions(-) diff --git a/superset/config.py b/superset/config.py index 3de93e0cb2bd5..f41093588da29 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1723,9 +1723,9 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument # A callable that can be supplied to do extra validation of guest token configuration # for example certain RLS parameters: -# lambda x: len(x['rls']) == 1 and "tenent_id=" in x['rls'][0]['clause'] +# lambda x: len(x['rls']) == 1 and "tenant_id=" in x['rls'][0]['clause'] # -# Takes the GuestTokenUser dict as a n argument +# Takes the GuestTokenUser dict as an argument # Return False from the callable to return a HTTP 400 to the user. GUEST_TOKEN_VALIDATOR_HOOK = None diff --git a/superset/security/api.py b/superset/security/api.py index 50a346de93759..c13b72d6f0741 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -17,7 +17,7 @@ import logging from typing import Any -from flask import request, Response +from flask import current_app, request, Response from flask_appbuilder import expose from flask_appbuilder.api import safe from flask_appbuilder.security.decorators import permission_name, protect @@ -27,7 +27,6 @@ from superset.commands.dashboard.embedded.exceptions import ( EmbeddedDashboardNotFoundError, ) -from superset.config import GUEST_TOKEN_VALIDATOR_HOOK from superset.exceptions import SupersetGenericErrorException from superset.extensions import event_logger from superset.security.guest_token import GuestTokenResourceType @@ -150,10 +149,13 @@ def guest_token(self) -> Response: try: body = guest_token_create_schema.load(request.json) self.appbuilder.sm.validate_guest_token_resources(body["resources"]) + guest_token_validator_hook = current_app.config.get( + "GUEST_TOKEN_VALIDATOR_HOOK" + ) # Run validator to ensure the token parameters are OK. - if GUEST_TOKEN_VALIDATOR_HOOK is not None: - if callable(GUEST_TOKEN_VALIDATOR_HOOK): - if not GUEST_TOKEN_VALIDATOR_HOOK(body): + if guest_token_validator_hook is not None: + if callable(guest_token_validator_hook): + if not guest_token_validator_hook(body): raise ValidationError(message="Guest token validation failed") else: raise SupersetGenericErrorException( diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index c2d9b130cdc58..73aa3d0ed3d62 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -137,6 +137,149 @@ def test_post_guest_token_bad_resources(self): self.assert400(response) + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": None}) + def test_guest_token_validator_hook_unconfigured(self): + """ + Security API: Test the API works normally with GUEST_TOKEN_VALIDATOR_HOOK + configured to None. + """ + self.dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) + self.login(ADMIN_USERNAME) + user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} + resource = {"type": "dashboard", "id": str(self.embedded.uuid)} + rls_rule = {"dataset": 1, "clause": "1=1"} + params = {"user": user, "resources": [resource], "rls": [rls_rule]} + + response = self.client.post( + self.uri, data=json.dumps(params), content_type="application/json" + ) + + self.assert200(response) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: False}) + def test_guest_token_validator_hook_negative(self): + """ + Security API: Test the API returns a 400 with a hook that returns False. + """ + self.dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) + self.login(ADMIN_USERNAME) + user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} + resource = {"type": "dashboard", "id": str(self.embedded.uuid)} + rls_rule = {"dataset": 1, "clause": "1=1"} + params = {"user": user, "resources": [resource], "rls": [rls_rule]} + + response = self.client.post( + self.uri, data=json.dumps(params), content_type="application/json" + ) + + self.assert400(response) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: True}) + def test_guest_token_validator_hook_positive(self): + """ + Security API: Test the API returns a 400 with a hook that returns False. + """ + self.dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) + self.login(ADMIN_USERNAME) + user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} + resource = {"type": "dashboard", "id": str(self.embedded.uuid)} + rls_rule = {"dataset": 1, "clause": "1=1"} + params = {"user": user, "resources": [resource], "rls": [rls_rule]} + + response = self.client.post( + self.uri, data=json.dumps(params), content_type="application/json" + ) + + self.assert200(response) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": 123}) + def test_guest_token_validator_not_callable(self): + """ + Security API: Test the API blows up if hook not configured with a callable + """ + self.dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) + self.login(ADMIN_USERNAME) + user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} + resource = {"type": "dashboard", "id": str(self.embedded.uuid)} + rls_rule = {"dataset": 1, "clause": "1=1"} + params = {"user": user, "resources": [resource], "rls": [rls_rule]} + + response = self.client.post( + self.uri, data=json.dumps(params), content_type="application/json" + ) + + self.assert500(response) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: [][0]}) + def test_guest_token_validator_raises_exception(self): + """ + Security API: Test the API blows up if validator blows up. + """ + self.dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) + self.login(ADMIN_USERNAME) + user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} + resource = {"type": "dashboard", "id": str(self.embedded.uuid)} + rls_rule = {"dataset": 1, "clause": "1=1"} + params = {"user": user, "resources": [resource], "rls": [rls_rule]} + + response = self.client.post( + self.uri, data=json.dumps(params), content_type="application/json" + ) + + self.assert500(response) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @with_config( + { + "GUEST_TOKEN_VALIDATOR_HOOK": lambda x: len(x["rls"]) == 1 + and "tenant_id=" in x["rls"][0]["clause"] + } + ) + def test_guest_token_validator_real_world(self): + """ + Security API: Test the API with a real world example + """ + self.dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) + self.login(ADMIN_USERNAME) + user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} + resource = {"type": "dashboard", "id": str(self.embedded.uuid)} + rls_rule = {"dataset": 1, "clause": "1=1"} + params = {"user": user, "resources": [resource], "rls": [rls_rule]} + + response = self.client.post( + self.uri, data=json.dumps(params), content_type="application/json" + ) + + self.assert400(response) + + rls_rule = {"dataset": 1, "clause": "tenant_id=1234"} + params = {"user": user, "resources": [resource], "rls": [rls_rule]} + + response = self.client.post( + self.uri, data=json.dumps(params), content_type="application/json" + ) + + self.assert200(response) + + params = {"user": user, "resources": [resource], "rls": []} + + response = self.client.post( + self.uri, data=json.dumps(params), content_type="application/json" + ) + + self.assert400(response) + class TestSecurityRolesApi(SupersetTestCase): uri = "api/v1/security/roles/" # noqa: F541 From 01fd1d5e23c3c84606946acc314a0ee73004e802 Mon Sep 17 00:00:00 2001 From: David Markey Date: Mon, 2 Sep 2024 23:58:25 +0100 Subject: [PATCH 03/13] Fix test description, --- tests/integration_tests/security/api_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index 73aa3d0ed3d62..e284dacd8900d 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -182,7 +182,7 @@ def test_guest_token_validator_hook_negative(self): @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: True}) def test_guest_token_validator_hook_positive(self): """ - Security API: Test the API returns a 400 with a hook that returns False. + Security API: Test the API returns a 200 with a hook that returns True. """ self.dash = db.session.query(Dashboard).filter_by(slug="births").first() self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) From 65d97d9161910d40c76a7cbcfd674e45e6b4b732 Mon Sep 17 00:00:00 2001 From: David Markey Date: Tue, 3 Sep 2024 09:36:46 +0100 Subject: [PATCH 04/13] Update comment here to make it clear some generic validation is needed. --- superset/security/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/api.py b/superset/security/api.py index c13b72d6f0741..02bf6b7101ea8 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -161,7 +161,7 @@ def guest_token(self) -> Response: raise SupersetGenericErrorException( message="Guest token validator hook not callable" ) - # todo validate stuff: + # TODO: Add generic validation: # make sure username doesn't reference an existing user # check rls rules for validity? token = self.appbuilder.sm.create_guest_access_token( From 2111888b04d1a4a8df79832a3b6933952344808b Mon Sep 17 00:00:00 2001 From: David Markey Date: Thu, 5 Sep 2024 00:03:14 +0100 Subject: [PATCH 05/13] Refactor tests to be more succint --- tests/integration_tests/security/api_tests.py | 115 ++++-------------- 1 file changed, 23 insertions(+), 92 deletions(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index e284dacd8900d..13a6c745d6719 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -20,6 +20,7 @@ import jwt import pytest +from flask import current_app from flask_wtf.csrf import generate_csrf from superset import db from superset.daos.dashboard import EmbeddedDashboardDAO @@ -138,147 +139,77 @@ def test_post_guest_token_bad_resources(self): self.assert400(response) @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": None}) - def test_guest_token_validator_hook_unconfigured(self): + def test_guest_token_validator_hook(self): """ Security API: Test the API works normally with GUEST_TOKEN_VALIDATOR_HOOK configured to None. """ - self.dash = db.session.query(Dashboard).filter_by(slug="births").first() - self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) - self.login(ADMIN_USERNAME) - user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} - resource = {"type": "dashboard", "id": str(self.embedded.uuid)} - rls_rule = {"dataset": 1, "clause": "1=1"} - params = {"user": user, "resources": [resource], "rls": [rls_rule]} - - response = self.client.post( - self.uri, data=json.dumps(params), content_type="application/json" - ) - self.assert200(response) - - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: False}) - def test_guest_token_validator_hook_negative(self): - """ - Security API: Test the API returns a 400 with a hook that returns False. - """ self.dash = db.session.query(Dashboard).filter_by(slug="births").first() self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) self.login(ADMIN_USERNAME) user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} resource = {"type": "dashboard", "id": str(self.embedded.uuid)} - rls_rule = {"dataset": 1, "clause": "1=1"} + rls_rule = {"dataset": 1, "clause": "tenant_id=123"} params = {"user": user, "resources": [resource], "rls": [rls_rule]} + # Test False case from validator - should raise 400 + current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: False response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) self.assert400(response) - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: True}) - def test_guest_token_validator_hook_positive(self): - """ - Security API: Test the API returns a 200 with a hook that returns True. - """ - self.dash = db.session.query(Dashboard).filter_by(slug="births").first() - self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) - self.login(ADMIN_USERNAME) - user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} - resource = {"type": "dashboard", "id": str(self.embedded.uuid)} - rls_rule = {"dataset": 1, "clause": "1=1"} - params = {"user": user, "resources": [resource], "rls": [rls_rule]} - + # Test False case from validator - should be 200 + current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: True response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) self.assert200(response) - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": 123}) - def test_guest_token_validator_not_callable(self): - """ - Security API: Test the API blows up if hook not configured with a callable - """ - self.dash = db.session.query(Dashboard).filter_by(slug="births").first() - self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) - self.login(ADMIN_USERNAME) - user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} - resource = {"type": "dashboard", "id": str(self.embedded.uuid)} - rls_rule = {"dataset": 1, "clause": "1=1"} - params = {"user": user, "resources": [resource], "rls": [rls_rule]} + # Test validator is not callable - should be 500 + current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = 123 response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) - self.assert500(response) - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: [][0]}) - def test_guest_token_validator_raises_exception(self): - """ - Security API: Test the API blows up if validator blows up. - """ - self.dash = db.session.query(Dashboard).filter_by(slug="births").first() - self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) - self.login(ADMIN_USERNAME) - user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} - resource = {"type": "dashboard", "id": str(self.embedded.uuid)} - rls_rule = {"dataset": 1, "clause": "1=1"} - params = {"user": user, "resources": [resource], "rls": [rls_rule]} + # Test validator throws exception - should be 500 + current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: [][0] response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) - self.assert500(response) - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - @with_config( - { - "GUEST_TOKEN_VALIDATOR_HOOK": lambda x: len(x["rls"]) == 1 - and "tenant_id=" in x["rls"][0]["clause"] - } - ) - def test_guest_token_validator_real_world(self): - """ - Security API: Test the API with a real world example - """ - self.dash = db.session.query(Dashboard).filter_by(slug="births").first() - self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) - self.login(ADMIN_USERNAME) - user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} - resource = {"type": "dashboard", "id": str(self.embedded.uuid)} - rls_rule = {"dataset": 1, "clause": "1=1"} - params = {"user": user, "resources": [resource], "rls": [rls_rule]} + # Test validator real world example, check tenant_id is in clause + # Should be 200. + current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = ( + lambda x: len(x["rls"]) == 1 and "tenant_id=" in x["rls"][0]["clause"] + ) response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) + self.assert200(response) - self.assert400(response) - - rls_rule = {"dataset": 1, "clause": "tenant_id=1234"} - params = {"user": user, "resources": [resource], "rls": [rls_rule]} - + # Test validator real world example, check tenant_id is in clause + # Should be 400 as there is no RLS clause + params["rls"] = [] response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) + self.assert400(response) - self.assert200(response) - - params = {"user": user, "resources": [resource], "rls": []} + current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = None response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) - - self.assert400(response) + self.assert200(response) class TestSecurityRolesApi(SupersetTestCase): From da8aa11dde12f95b83fb4e0c12e2595ee3d90747 Mon Sep 17 00:00:00 2001 From: David Markey Date: Thu, 5 Sep 2024 00:15:59 +0100 Subject: [PATCH 06/13] Fix test comment --- tests/integration_tests/security/api_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index 13a6c745d6719..0cfc1f7c2aa2b 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -161,7 +161,7 @@ def test_guest_token_validator_hook(self): self.assert400(response) - # Test False case from validator - should be 200 + # Test True case from validator - should be 200 current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: True response = self.client.post( self.uri, data=json.dumps(params), content_type="application/json" From 661acec6ce08ab8fe7f36d8b1f78c2390d4633cf Mon Sep 17 00:00:00 2001 From: David Markey Date: Thu, 5 Sep 2024 00:17:25 +0100 Subject: [PATCH 07/13] Fix another test comment --- tests/integration_tests/security/api_tests.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index 0cfc1f7c2aa2b..77ea2d4e43662 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -141,8 +141,7 @@ def test_post_guest_token_bad_resources(self): @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_guest_token_validator_hook(self): """ - Security API: Test the API works normally with GUEST_TOKEN_VALIDATOR_HOOK - configured to None. + Security API: Test various scenarios for the GUEST_TOKEN_VALIDATOR_HOOK """ self.dash = db.session.query(Dashboard).filter_by(slug="births").first() From 6827ef4b8a61fa243e361eb41ce636173bb6916a Mon Sep 17 00:00:00 2001 From: David Markey Date: Thu, 5 Sep 2024 00:34:19 +0100 Subject: [PATCH 08/13] Add extra comment about default configuration --- tests/integration_tests/security/api_tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index 77ea2d4e43662..460fc7af3c005 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -203,6 +203,7 @@ def test_guest_token_validator_hook(self): ) self.assert400(response) + # Revert/Test that the default configuration has no side effects - 200 current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = None response = self.client.post( From 0777f64d5661b02515a0d62e59837ec7c81fbe3d Mon Sep 17 00:00:00 2001 From: David Markey Date: Mon, 9 Sep 2024 23:13:34 +0100 Subject: [PATCH 09/13] Break out Guest Token Validator tests and DRY --- tests/integration_tests/security/api_tests.py | 100 +++++++++--------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index 460fc7af3c005..4e8041c3882cb 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -138,78 +138,78 @@ def test_post_guest_token_bad_resources(self): self.assert400(response) - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") - def test_guest_token_validator_hook(self): - """ - Security API: Test various scenarios for the GUEST_TOKEN_VALIDATOR_HOOK - """ - self.dash = db.session.query(Dashboard).filter_by(slug="births").first() - self.embedded = EmbeddedDashboardDAO.upsert(self.dash, []) +@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices", scope="class") +class TestSecurityGuestTokenApiTokenValidator(SupersetTestCase): + uri = "api/v1/security/guest_token/" # noqa: F541 + + def _get_guest_token_with_rls(self, rls_rule): + dash = db.session.query(Dashboard).filter_by(slug="births").first() + self.embedded = EmbeddedDashboardDAO.upsert(dash, []) self.login(ADMIN_USERNAME) user = {"username": "bob", "first_name": "Bob", "last_name": "Also Bob"} resource = {"type": "dashboard", "id": str(self.embedded.uuid)} - rls_rule = {"dataset": 1, "clause": "tenant_id=123"} params = {"user": user, "resources": [resource], "rls": [rls_rule]} - - # Test False case from validator - should raise 400 - current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: False - response = self.client.post( + return self.client.post( self.uri, data=json.dumps(params), content_type="application/json" ) - self.assert400(response) + def test_guest_token_validator_hook_denied(self): + """ + Security API: Test False case from validator - should be 400 + """ + current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: False + rls_rule = {"dataset": 1, "clause": "tenant_id=123"} + self.assert400(self._get_guest_token_with_rls(rls_rule)) - # Test True case from validator - should be 200 + def test_guest_token_validator_hook_denied_allowed(self): + """ + Security API: Test True case from validator - should be 200 + """ current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: True - response = self.client.post( - self.uri, data=json.dumps(params), content_type="application/json" - ) - - self.assert200(response) + rls_rule = {"dataset": 1, "clause": "tenant_id=123"} + self.assert200(self._get_guest_token_with_rls(rls_rule)) - # Test validator is not callable - should be 500 + def test_guest_validator_hook_not_callable(self): + """ + Security API: Test validator throws exception when function throws + should be 500 + """ + rls_rule = {"dataset": 1, "clause": "tenant_id=123"} current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = 123 + self.assert500(self._get_guest_token_with_rls(rls_rule)) - response = self.client.post( - self.uri, data=json.dumps(params), content_type="application/json" - ) - self.assert500(response) - - # Test validator throws exception - should be 500 + def test_guest_validator_hook_throws_exception(self): + """ + Security API: Test validator throws exception - should be 500 + """ + rls_rule = {"dataset": 1, "clause": "tenant_id=123"} current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: [][0] + self.assert500(self._get_guest_token_with_rls(rls_rule)) - response = self.client.post( - self.uri, data=json.dumps(params), content_type="application/json" - ) - self.assert500(response) - + def test_guest_validator_hook_real_world_example_positive(self): + """ + Security API:Test validator real world example, check tenant_id is in clause + Positive case + """ # Test validator real world example, check tenant_id is in clause # Should be 200. current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = ( lambda x: len(x["rls"]) == 1 and "tenant_id=" in x["rls"][0]["clause"] ) + rls_rule = {"dataset": 1, "clause": "tenant_id=123"} + self.assert200(self._get_guest_token_with_rls(rls_rule)) - response = self.client.post( - self.uri, data=json.dumps(params), content_type="application/json" - ) - self.assert200(response) - - # Test validator real world example, check tenant_id is in clause - # Should be 400 as there is no RLS clause - params["rls"] = [] - response = self.client.post( - self.uri, data=json.dumps(params), content_type="application/json" - ) - self.assert400(response) - - # Revert/Test that the default configuration has no side effects - 200 - current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = None - - response = self.client.post( - self.uri, data=json.dumps(params), content_type="application/json" + def test_guest_validator_hook_real_world_example_negative(self): + """ + Security API:Test validator real world example, check tenant_id is in clause + Negative case + """ + current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = ( + lambda x: len(x["rls"]) == 1 and "tenant_id=" in x["rls"][0]["clause"] ) - self.assert200(response) + rls_rule = {} + self.assert400(self._get_guest_token_with_rls(rls_rule)) class TestSecurityRolesApi(SupersetTestCase): From 49c37b39805f5a009df72c8bf7c34c600c43d060 Mon Sep 17 00:00:00 2001 From: David Markey Date: Mon, 9 Sep 2024 23:17:05 +0100 Subject: [PATCH 10/13] Fix test comment --- tests/integration_tests/security/api_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index 4e8041c3882cb..b874e69da9540 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -172,7 +172,7 @@ def test_guest_token_validator_hook_denied_allowed(self): def test_guest_validator_hook_not_callable(self): """ - Security API: Test validator throws exception when function throws + Security API: Test validator throws exception when isn't callable should be 500 """ rls_rule = {"dataset": 1, "clause": "tenant_id=123"} From ad2c543ab67716b942013d2d593c044f46540ee7 Mon Sep 17 00:00:00 2001 From: David Markey Date: Mon, 9 Sep 2024 23:18:15 +0100 Subject: [PATCH 11/13] Fix comment spacing --- tests/integration_tests/security/api_tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index b874e69da9540..8c26439dc2002 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -189,7 +189,7 @@ def test_guest_validator_hook_throws_exception(self): def test_guest_validator_hook_real_world_example_positive(self): """ - Security API:Test validator real world example, check tenant_id is in clause + Security API: Test validator real world example, check tenant_id is in clause Positive case """ # Test validator real world example, check tenant_id is in clause @@ -202,7 +202,7 @@ def test_guest_validator_hook_real_world_example_positive(self): def test_guest_validator_hook_real_world_example_negative(self): """ - Security API:Test validator real world example, check tenant_id is in clause + Security API: Test validator real world example, check tenant_id is in clause Negative case """ current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = ( From 8b1da410b9202ecc0b574cd49bb96a374bfa9a22 Mon Sep 17 00:00:00 2001 From: David Markey Date: Mon, 9 Sep 2024 23:25:58 +0100 Subject: [PATCH 12/13] Move back to use the with_config decorator to stop pollution --- tests/integration_tests/security/api_tests.py | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index 8c26439dc2002..f54f2c645e7d3 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -20,7 +20,6 @@ import jwt import pytest -from flask import current_app from flask_wtf.csrf import generate_csrf from superset import db from superset.daos.dashboard import EmbeddedDashboardDAO @@ -154,39 +153,45 @@ def _get_guest_token_with_rls(self, rls_rule): self.uri, data=json.dumps(params), content_type="application/json" ) + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: False}) def test_guest_token_validator_hook_denied(self): """ Security API: Test False case from validator - should be 400 """ - current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: False rls_rule = {"dataset": 1, "clause": "tenant_id=123"} self.assert400(self._get_guest_token_with_rls(rls_rule)) + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: True}) def test_guest_token_validator_hook_denied_allowed(self): """ Security API: Test True case from validator - should be 200 """ - current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: True rls_rule = {"dataset": 1, "clause": "tenant_id=123"} self.assert200(self._get_guest_token_with_rls(rls_rule)) + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": 123}) def test_guest_validator_hook_not_callable(self): """ Security API: Test validator throws exception when isn't callable should be 500 """ rls_rule = {"dataset": 1, "clause": "tenant_id=123"} - current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = 123 self.assert500(self._get_guest_token_with_rls(rls_rule)) + @with_config({"GUEST_TOKEN_VALIDATOR_HOOK": lambda x: [][0]}) def test_guest_validator_hook_throws_exception(self): """ Security API: Test validator throws exception - should be 500 """ rls_rule = {"dataset": 1, "clause": "tenant_id=123"} - current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = lambda x: [][0] self.assert500(self._get_guest_token_with_rls(rls_rule)) + @with_config( + { + "GUEST_TOKEN_VALIDATOR_HOOK": lambda x: len(x["rls"]) == 1 + and "tenant_id=" in x["rls"][0]["clause"] + } + ) def test_guest_validator_hook_real_world_example_positive(self): """ Security API: Test validator real world example, check tenant_id is in clause @@ -194,20 +199,20 @@ def test_guest_validator_hook_real_world_example_positive(self): """ # Test validator real world example, check tenant_id is in clause # Should be 200. - current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = ( - lambda x: len(x["rls"]) == 1 and "tenant_id=" in x["rls"][0]["clause"] - ) rls_rule = {"dataset": 1, "clause": "tenant_id=123"} self.assert200(self._get_guest_token_with_rls(rls_rule)) + @with_config( + { + "GUEST_TOKEN_VALIDATOR_HOOK": lambda x: len(x["rls"]) == 1 + and "tenant_id=" in x["rls"][0]["clause"] + } + ) def test_guest_validator_hook_real_world_example_negative(self): """ Security API: Test validator real world example, check tenant_id is in clause Negative case """ - current_app.config["GUEST_TOKEN_VALIDATOR_HOOK"] = ( - lambda x: len(x["rls"]) == 1 and "tenant_id=" in x["rls"][0]["clause"] - ) rls_rule = {} self.assert400(self._get_guest_token_with_rls(rls_rule)) From 341e2e19d26cba28943c622b4354be2768d55544 Mon Sep 17 00:00:00 2001 From: David Markey Date: Tue, 10 Sep 2024 16:12:52 +0100 Subject: [PATCH 13/13] Take out unneeded # noqa: F541 --- tests/integration_tests/security/api_tests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/security/api_tests.py b/tests/integration_tests/security/api_tests.py index f54f2c645e7d3..67aecd73b0912 100644 --- a/tests/integration_tests/security/api_tests.py +++ b/tests/integration_tests/security/api_tests.py @@ -81,7 +81,7 @@ def test_login(self): class TestSecurityGuestTokenApi(SupersetTestCase): - uri = "api/v1/security/guest_token/" # noqa: F541 + uri = "api/v1/security/guest_token/" def test_post_guest_token_unauthenticated(self): """ @@ -140,7 +140,7 @@ def test_post_guest_token_bad_resources(self): @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices", scope="class") class TestSecurityGuestTokenApiTokenValidator(SupersetTestCase): - uri = "api/v1/security/guest_token/" # noqa: F541 + uri = "api/v1/security/guest_token/" def _get_guest_token_with_rls(self, rls_rule): dash = db.session.query(Dashboard).filter_by(slug="births").first()