From d9cc063be21604d28d11ee4c19d590fc6f40eb8b Mon Sep 17 00:00:00 2001 From: Arik Fraimovich Date: Wed, 4 May 2016 16:32:49 +0300 Subject: [PATCH] Fix: make sure we return dashboards only for current org --- redash/handlers/dashboards.py | 6 +-- redash/models.py | 15 ++++--- tests/test_models.py | 74 ++++++++++++++++++++++------------- 3 files changed, 58 insertions(+), 37 deletions(-) diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index fc58b2972..26ce573f0 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -11,11 +11,11 @@ from redash.handlers.base import BaseResource, get_object_or_404 class RecentDashboardsResource(BaseResource): @require_permission('list_dashboards') def get(self): - recent = [d.to_dict() for d in models.Dashboard.recent(self.current_user.groups, self.current_user.id, for_user=True)] + recent = [d.to_dict() for d in models.Dashboard.recent(self.current_org, self.current_user.groups, self.current_user.id, for_user=True)] global_recent = [] if len(recent) < 10: - global_recent = [d.to_dict() for d in models.Dashboard.recent(self.current_user.groups, self.current_user.id)] + global_recent = [d.to_dict() for d in models.Dashboard.recent(self.current_org, self.current_user.groups, self.current_user.id)] return take(20, distinct(chain(recent, global_recent), key=lambda d: d['id'])) @@ -23,7 +23,7 @@ class RecentDashboardsResource(BaseResource): class DashboardListResource(BaseResource): @require_permission('list_dashboards') def get(self): - dashboards = [d.to_dict() for d in models.Dashboard.all(self.current_user.groups, self.current_user.id)] + dashboards = [d.to_dict() for d in models.Dashboard.all(self.current_org, self.current_user.groups, self.current_user)] return dashboards diff --git a/redash/models.py b/redash/models.py index dbee6c4fe..3d30788ce 100644 --- a/redash/models.py +++ b/redash/models.py @@ -886,7 +886,7 @@ class Dashboard(ModelTimestampsMixin, BaseModel, BelongsToOrgMixin): } @classmethod - def all(cls, groups, user_id): + def all(cls, org, groups, user_id): query = cls.select().\ join(Widget, peewee.JOIN_LEFT_OUTER, on=(Dashboard.id == Widget.dashboard)). \ join(Visualization, peewee.JOIN_LEFT_OUTER, on=(Widget.visualization == Visualization.id)). \ @@ -896,15 +896,13 @@ class Dashboard(ModelTimestampsMixin, BaseModel, BelongsToOrgMixin): where((DataSourceGroup.group << groups) | (Dashboard.user == user_id) | (~(Widget.dashboard >> None) & (Widget.visualization >> None))). \ + where(Dashboard.org == org). \ group_by(Dashboard.id) + return query @classmethod - def get_by_slug_and_org(cls, slug, org): - return cls.get(cls.slug == slug, cls.org==org) - - @classmethod - def recent(cls, groups, user_id, for_user=False, limit=20): + def recent(cls, org, groups, user_id, for_user=False, limit=20): query = cls.select().where(Event.created_at > peewee.SQL("current_date - 7")). \ join(Event, peewee.JOIN_LEFT_OUTER, on=(Dashboard.id == Event.object_id.cast('integer'))). \ join(Widget, peewee.JOIN_LEFT_OUTER, on=(Dashboard.id == Widget.dashboard)). \ @@ -915,6 +913,7 @@ class Dashboard(ModelTimestampsMixin, BaseModel, BelongsToOrgMixin): where(~(Event.object_id >> None)). \ where(Event.object_type == 'dashboard'). \ where(Dashboard.is_archived == False). \ + where(Dashboard.org == org). \ where((DataSourceGroup.group << groups) | (Dashboard.user == user_id) | (~(Widget.dashboard >> None) & (Widget.visualization >> None))). \ @@ -928,6 +927,10 @@ class Dashboard(ModelTimestampsMixin, BaseModel, BelongsToOrgMixin): return query + @classmethod + def get_by_slug_and_org(cls, slug, org): + return cls.get(cls.slug == slug, cls.org==org) + def save(self, *args, **kwargs): if not self.slug: self.slug = utils.slugify(self.name) diff --git a/tests/test_models.py b/tests/test_models.py index 94a155df9..e4190c1d9 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -561,34 +561,42 @@ class TestDashboardAll(BaseTestCase): def test_requires_group_or_user_id(self): d1 = self.factory.create_dashboard() - self.assertNotIn(d1, models.Dashboard.all(d1.user.groups, None)) - self.assertIn(d1, models.Dashboard.all([0], d1.user.id)) + self.assertNotIn(d1, models.Dashboard.all(d1.user.org, d1.user.groups, None)) + self.assertIn(d1, models.Dashboard.all(d1.user.org, [0], d1.user.id)) def test_returns_dashboards_based_on_groups(self): - self.assertIn(self.w1.dashboard, models.Dashboard.all(self.u1.groups, None)) - self.assertIn(self.w2.dashboard, models.Dashboard.all(self.u2.groups, None)) - self.assertNotIn(self.w1.dashboard, models.Dashboard.all(self.u2.groups, None)) - self.assertNotIn(self.w2.dashboard, models.Dashboard.all(self.u1.groups, None)) + self.assertIn(self.w1.dashboard, models.Dashboard.all(self.u1.org, self.u1.groups, None)) + self.assertIn(self.w2.dashboard, models.Dashboard.all(self.u2.org, self.u2.groups, None)) + self.assertNotIn(self.w1.dashboard, models.Dashboard.all(self.u2.org, self.u2.groups, None)) + self.assertNotIn(self.w2.dashboard, models.Dashboard.all(self.u1.org, self.u1.groups, None)) def test_returns_each_dashboard_once(self): - dashboards = list(models.Dashboard.all(self.u2.groups, None)) + dashboards = list(models.Dashboard.all(self.u2.org, self.u2.groups, None)) self.assertEqual(len(dashboards), 2) def test_returns_dashboard_you_have_partial_access_to(self): - self.assertIn(self.w5.dashboard, models.Dashboard.all(self.u1.groups, None)) + self.assertIn(self.w5.dashboard, models.Dashboard.all(self.u1.org, self.u1.groups, None)) def test_returns_dashboards_created_by_user(self): d1 = self.factory.create_dashboard(user=self.u1) - self.assertIn(d1, models.Dashboard.all(self.u1.groups, self.u1.id)) - self.assertIn(d1, models.Dashboard.all([0], self.u1.id)) - self.assertNotIn(d1, models.Dashboard.all(self.u2.groups, self.u2.id)) + self.assertIn(d1, models.Dashboard.all(self.u1.org, self.u1.groups, self.u1.id)) + self.assertIn(d1, models.Dashboard.all(self.u1.org, [0], self.u1.id)) + self.assertNotIn(d1, models.Dashboard.all(self.u2.org, self.u2.groups, self.u2.id)) def test_returns_dashboards_with_text_widgets(self): w1 = self.factory.create_widget(visualization=None) - self.assertIn(w1.dashboard, models.Dashboard.all(self.u1.groups, None)) - self.assertIn(w1.dashboard, models.Dashboard.all(self.u2.groups, None)) + self.assertIn(w1.dashboard, models.Dashboard.all(self.u1.org, self.u1.groups, None)) + self.assertIn(w1.dashboard, models.Dashboard.all(self.u2.org, self.u2.groups, None)) + + def test_returns_dashboards_from_current_org_only(self): + w1 = self.factory.create_widget(visualization=None) + + user = self.factory.create_user(org=self.factory.create_org()) + + self.assertIn(w1.dashboard, models.Dashboard.all(self.u1.org, self.u1.groups, None)) + self.assertNotIn(w1.dashboard, models.Dashboard.all(user.org, user.groups, None)) class TestDashboardRecent(BaseTestCase): @@ -600,26 +608,26 @@ class TestDashboardRecent(BaseTestCase): models.Event.create(org=self.factory.org, user=self.u1, action="view", object_type="dashboard", object_id=self.w1.dashboard.id) - self.assertIn(self.w1.dashboard, models.Dashboard.recent(self.u1.groups, None)) - self.assertNotIn(self.w2.dashboard, models.Dashboard.recent(self.u1.groups, None)) - self.assertNotIn(self.w1.dashboard, models.Dashboard.recent(self.u2.groups, None)) + self.assertIn(self.w1.dashboard, models.Dashboard.recent(self.u1.org, self.u1.groups, None)) + self.assertNotIn(self.w2.dashboard, models.Dashboard.recent(self.u1.org, self.u1.groups, None)) + self.assertNotIn(self.w1.dashboard, models.Dashboard.recent(self.u1.org, self.u2.groups, None)) def test_returns_recent_dashboards_created_by_user(self): d1 = self.factory.create_dashboard(user=self.u1) models.Event.create(org=self.factory.org, user=self.u1, action="view", object_type="dashboard", object_id=d1.id) - self.assertIn(d1, models.Dashboard.recent([0], self.u1.id)) - self.assertNotIn(self.w2.dashboard, models.Dashboard.recent([0], self.u1.id)) - self.assertNotIn(d1, models.Dashboard.recent([0], self.u2.id)) + self.assertIn(d1, models.Dashboard.recent(self.u1.org, [0], self.u1.id)) + self.assertNotIn(self.w2.dashboard, models.Dashboard.recent(self.u1.org, [0], self.u1.id)) + self.assertNotIn(d1, models.Dashboard.recent(self.u2.org, [0], self.u2.id)) def test_returns_recent_dashboards_with_no_visualizations(self): w1 = self.factory.create_widget(visualization=None) models.Event.create(org=self.factory.org, user=self.u1, action="view", object_type="dashboard", object_id=w1.dashboard.id) - self.assertIn(w1.dashboard, models.Dashboard.recent([0], self.u1.id)) - self.assertNotIn(self.w2.dashboard, models.Dashboard.recent([0], self.u1.id)) + self.assertIn(w1.dashboard, models.Dashboard.recent(self.u1.org, [0], self.u1.id)) + self.assertNotIn(self.w2.dashboard, models.Dashboard.recent(self.u1.org, [0], self.u1.id)) def test_restricts_dashboards_for_user(self): models.Event.create(org=self.factory.org, user=self.u1, action="view", @@ -631,12 +639,12 @@ class TestDashboardRecent(BaseTestCase): models.Event.create(org=self.factory.org, user=self.u2, action="view", object_type="dashboard", object_id=self.w5.dashboard.id) - self.assertIn(self.w1.dashboard, models.Dashboard.recent(self.u1.groups, self.u1.id, for_user=True)) - self.assertIn(self.w2.dashboard, models.Dashboard.recent(self.u2.groups, self.u2.id, for_user=True)) - self.assertNotIn(self.w1.dashboard, models.Dashboard.recent(self.u2.groups, self.u2.id, for_user=True)) - self.assertNotIn(self.w2.dashboard, models.Dashboard.recent(self.u1.groups, self.u1.id, for_user=True)) - self.assertIn(self.w5.dashboard, models.Dashboard.recent(self.u1.groups, self.u1.id, for_user=True)) - self.assertIn(self.w5.dashboard, models.Dashboard.recent(self.u2.groups, self.u2.id, for_user=True)) + self.assertIn(self.w1.dashboard, models.Dashboard.recent(self.u1.org, self.u1.groups, self.u1.id, for_user=True)) + self.assertIn(self.w2.dashboard, models.Dashboard.recent(self.u2.org, self.u2.groups, self.u2.id, for_user=True)) + self.assertNotIn(self.w1.dashboard, models.Dashboard.recent(self.u2.org, self.u2.groups, self.u2.id, for_user=True)) + self.assertNotIn(self.w2.dashboard, models.Dashboard.recent(self.u1.org, self.u1.groups, self.u1.id, for_user=True)) + self.assertIn(self.w5.dashboard, models.Dashboard.recent(self.u1.org, self.u1.groups, self.u1.id, for_user=True)) + self.assertIn(self.w5.dashboard, models.Dashboard.recent(self.u2.org, self.u2.groups, self.u2.id, for_user=True)) def test_returns_each_dashboard_once(self): models.Event.create(org=self.factory.org, user=self.u1, action="view", @@ -644,5 +652,15 @@ class TestDashboardRecent(BaseTestCase): models.Event.create(org=self.factory.org, user=self.u1, action="view", object_type="dashboard", object_id=self.w1.dashboard.id) - dashboards = list(models.Dashboard.recent(self.u1.groups, None)) + dashboards = list(models.Dashboard.recent(self.u1.org, self.u1.groups, None)) self.assertEqual(len(dashboards), 1) + + def test_returns_dashboards_from_current_org_only(self): + w1 = self.factory.create_widget(visualization=None) + models.Event.create(org=self.factory.org, user=self.u1, action="view", + object_type="dashboard", object_id=w1.dashboard.id) + + user = self.factory.create_user(org=self.factory.create_org()) + + self.assertIn(w1.dashboard, models.Dashboard.recent(self.u1.org, self.u1.groups, None)) + self.assertNotIn(w1.dashboard, models.Dashboard.recent(user.org, user.groups, None))