From 30bc1e2ff6fe2fc588564cc1e32c83720a8c59d0 Mon Sep 17 00:00:00 2001 From: Arik Fraimovich Date: Mon, 30 Dec 2019 10:07:20 +0200 Subject: [PATCH] Refine permissions usage in Redash to allow for guest users (#4492) * Allow executing query with either view_query or execute_query permissions. * Render AuthHeader according to permissions. * Don't return dashboards where you only have access to textbox widget. Closes #4099. --- .../app/components/app-header/AppHeader.jsx | 58 ++++++++++--------- client/app/services/auth.js | 6 ++ redash/handlers/query_results.py | 7 ++- redash/models/__init__.py | 1 - redash/permissions.py | 12 +++- tests/test_models.py | 28 ++++++--- 6 files changed, 73 insertions(+), 39 deletions(-) diff --git a/client/app/components/app-header/AppHeader.jsx b/client/app/components/app-header/AppHeader.jsx index 451328666f..76a9c9d602 100644 --- a/client/app/components/app-header/AppHeader.jsx +++ b/client/app/components/app-header/AppHeader.jsx @@ -51,29 +51,33 @@ function DesktopNavbar() { )} - - {currentUser.hasPermission("create_query") && ( - - New Query - - )} - {currentUser.hasPermission("create_dashboard") && ( - - CreateDashboardDialog.showModal()}>New Dashboard - - )} - - New Alert - - - }> - - + {currentUser.canCreate() && ( + + {currentUser.hasPermission("create_query") && ( + + New Query + + )} + {currentUser.hasPermission("create_dashboard") && ( + + CreateDashboardDialog.showModal()}>New Dashboard + + )} + {currentUser.hasPermission("list_alerts") && ( + + New Alert + + )} + + }> + + + )}
@@ -126,9 +130,11 @@ function DesktopNavbar() { Users )} - - Query Snippets - + {currentUser.hasPermission("create_query") && ( + + Query Snippets + + )} {currentUser.hasPermission("list_users") && ( Alert Destinations diff --git a/client/app/services/auth.js b/client/app/services/auth.js index 80cec85d57..063f68e949 100644 --- a/client/app/services/auth.js +++ b/client/app/services/auth.js @@ -10,6 +10,12 @@ export const currentUser = { return this.hasPermission("admin") || (userId && userId === this.id); }, + canCreate() { + return ( + this.hasPermission("create_query") || this.hasPermission("create_dashboard") || this.hasPermission("list_alerts") + ); + }, + hasPermission(permission) { return includes(this.permissions, permission); }, diff --git a/redash/handlers/query_results.py b/redash/handlers/query_results.py index 7caeb0b140..9a045f1982 100644 --- a/redash/handlers/query_results.py +++ b/redash/handlers/query_results.py @@ -11,6 +11,7 @@ not_view_only, require_access, require_permission, + require_any_of_permission, view_only, ) from redash.tasks import QueryTask @@ -220,7 +221,7 @@ def add_cors_headers(headers): settings.ACCESS_CONTROL_ALLOW_CREDENTIALS ).lower() - @require_permission("view_query") + @require_any_of_permission(("view_query", "execute_query")) def options(self, query_id=None, query_result_id=None, filetype="json"): headers = {} self.add_cors_headers(headers) @@ -237,7 +238,7 @@ def options(self, query_id=None, query_result_id=None, filetype="json"): return make_response("", 200, headers) - @require_permission("view_query") + @require_any_of_permission(("view_query", "execute_query")) def post(self, query_id): """ Execute a saved query. @@ -283,7 +284,7 @@ def post(self, query_id): else: return error_messages["no_permission"] - @require_permission("view_query") + @require_any_of_permission(("view_query", "execute_query")) def get(self, query_id=None, query_result_id=None, filetype="json"): """ Retrieve query results. diff --git a/redash/models/__init__.py b/redash/models/__init__.py index 4baef0864f..3022b98e0a 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -1073,7 +1073,6 @@ def all(cls, org, group_ids, user_id): ( DataSourceGroup.group_id.in_(group_ids) | (Dashboard.user_id == user_id) - | ((Widget.dashboard != None) & (Widget.visualization == None)) ), Dashboard.org == org, ) diff --git a/redash/permissions.py b/redash/permissions.py index eefc5132d1..92c34dc157 100644 --- a/redash/permissions.py +++ b/redash/permissions.py @@ -55,13 +55,17 @@ def require_access(obj, user, need_view_only): class require_permissions(object): - def __init__(self, permissions): + def __init__(self, permissions, allow_one=False): self.permissions = permissions + self.allow_one = allow_one def __call__(self, fn): @functools.wraps(fn) def decorated(*args, **kwargs): - has_permissions = current_user.has_permissions(self.permissions) + if self.allow_one: + has_permissions = any([current_user.has_permission(permission) for permission in self.permissions]) + else: + has_permissions = current_user.has_permissions(self.permissions) if has_permissions: return fn(*args, **kwargs) @@ -75,6 +79,10 @@ def require_permission(permission): return require_permissions((permission,)) +def require_any_of_permission(permissions): + return require_permissions(permissions, True) + + def require_admin(fn): return require_permission("admin")(fn) diff --git a/tests/test_models.py b/tests/test_models.py index d6c35d3700..8eae2e5cb3 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -736,24 +736,38 @@ def test_returns_dashboards_created_by_user(self): d1, list(models.Dashboard.all(self.u2.org, self.u2.group_ids, self.u2.id)) ) - def test_returns_dashboards_with_text_widgets(self): + def test_returns_dashboards_with_text_widgets_to_creator(self): w1 = self.factory.create_widget(visualization=None) + self.assertEqual(w1.dashboard.user, self.factory.user) self.assertIn( - w1.dashboard, models.Dashboard.all(self.u1.org, self.u1.group_ids, None) + w1.dashboard, + list( + models.Dashboard.all( + self.factory.user.org, + self.factory.user.group_ids, + self.factory.user.id, + ) + ), ) - self.assertIn( - w1.dashboard, models.Dashboard.all(self.u2.org, self.u2.group_ids, None) + self.assertNotIn( + w1.dashboard, + list(models.Dashboard.all(self.u1.org, self.u1.group_ids, self.u1.id)), ) def test_returns_dashboards_from_current_org_only(self): - w1 = self.factory.create_widget(visualization=None) + w1 = self.factory.create_widget() user = self.factory.create_user(org=self.factory.create_org()) self.assertIn( - w1.dashboard, models.Dashboard.all(self.u1.org, self.u1.group_ids, None) + w1.dashboard, + list( + models.Dashboard.all( + self.factory.user.org, self.factory.user.group_ids, None + ) + ), ) self.assertNotIn( - w1.dashboard, models.Dashboard.all(user.org, user.group_ids, None) + w1.dashboard, list(models.Dashboard.all(user.org, user.group_ids, user.id)) )