From 24f9689f61bcff58a32571fb797aab0268873ef5 Mon Sep 17 00:00:00 2001 From: Rob Hudson Date: Fri, 2 Mar 2012 00:43:12 -0800 Subject: [PATCH] Updated permissions for contribution stats (bug 685382) --- apps/stats/tests/test_cron.py | 6 +- apps/stats/tests/test_views.py | 125 +++++++++++++++++--------- apps/stats/views.py | 27 ++++-- migrations/337-perms-contribstats.sql | 6 ++ 4 files changed, 109 insertions(+), 55 deletions(-) create mode 100644 migrations/337-perms-contribstats.sql diff --git a/apps/stats/tests/test_cron.py b/apps/stats/tests/test_cron.py index 4766c2c3b3..99f10be6d1 100644 --- a/apps/stats/tests/test_cron.py +++ b/apps/stats/tests/test_cron.py @@ -1,4 +1,4 @@ -from datetime import date, timedelta +import datetime from django.core.management import call_command @@ -105,12 +105,12 @@ class TestIndexLatest(amo.tests.ESTestCase): es = True def test_index_latest(self): - latest = date.today() - timedelta(days=5) + latest = datetime.date.today() - datetime.timedelta(days=5) UpdateCount.index({'date': latest}) self.refresh('update_counts') start = latest.strftime('%Y-%m-%d') - finish = date.today().strftime('%Y-%m-%d') + finish = datetime.date.today().strftime('%Y-%m-%d') with mock.patch('stats.cron.call_command') as call: cron.index_latest_stats() call.assert_called_with('index_stats', addons=None, diff --git a/apps/stats/tests/test_views.py b/apps/stats/tests/test_views.py index 08ccbcb6c5..a35803f4e6 100644 --- a/apps/stats/tests/test_views.py +++ b/apps/stats/tests/test_views.py @@ -1,7 +1,6 @@ # -*- coding: utf-8 -*- import csv import datetime -from datetime import date, timedelta from decimal import Decimal import json @@ -38,8 +37,11 @@ class StatsTest(amo.tests.ESTestCase): def get_view_response(self, view, **kwargs): view_args = self.url_args.copy() + head = kwargs.pop('head', False) view_args.update(kwargs) url = reverse(view, kwargs=view_args) + if head: + return self.client.head(url, follow=True) return self.client.get(url, follow=True) def views_gen(self, **kwargs): @@ -54,13 +56,13 @@ class StatsTest(amo.tests.ESTestCase): def public_views_gen(self, **kwargs): # all views are potentially public, except for contributions for view, args in self.views_gen(**kwargs): - if view.find('stats.contributions') != 0: + if not view.startswith('stats.contributions'): yield (view, args) def private_views_gen(self, **kwargs): # only contributions views are always private for view, args in self.views_gen(**kwargs): - if view.find('stats.contributions') == 0: + if view.startswith('stats.contributions'): yield (view, args) @@ -82,46 +84,80 @@ class ESStatsTest(StatsTest): class TestSeriesSecurity(ESStatsTest): """Tests to make sure all restricted data remains restricted.""" - def test_private_addon(self): - """Ensure 403 for all series of an addon with private stats.""" - # First as a logged in user with no special permissions - self.login_as_visitor() - for view, kwargs in self.views_gen(format='json'): - response = self.get_view_response(view, **kwargs) - eq_(response.status_code, 403, - 'unexpected http status for %s' % view) + def _check_it(self, views, status): + for view, kwargs in views: + response = self.get_view_response(view, head=True, **kwargs) + eq_(response.status_code, status, + 'unexpected http status for %s. got %s. expected %s' % ( + view, response.status_code, status)) - # Test view when user added to appropriate group. + def test_private_addon_no_groups(self): + # Logged in but no groups + self.login_as_visitor() + self._check_it(self.views_gen(format='json'), 403) + + def test_private_addon_stats_group(self): + # Logged in with stats group. user = UserProfile.objects.get(email='nobodyspecial@mozilla.com') group = Group.objects.create(name='Stats', rules='Stats:View') GroupUser.objects.create(user=user, group=group) - for view, kwargs in self.views_gen(format='json'): - response = self.get_view_response(view, **kwargs) - eq_(response.status_code, 200, - 'unexpected http status for %s' % view) - - # Again as an unauthenticated user - self.client.logout() - for view, kwargs in self.views_gen(format='json'): - response = self.get_view_response(view, **kwargs) - eq_(response.status_code, 403, - 'unexpected http status for %s' % view) - - def test_public_addon(self): - """Ensure 403 for sensitive series of an addon with public stats.""" - # First as a logged in user with no special permissions self.login_as_visitor() - for view, kwargs in self.private_views_gen(addon_id=5, format='json'): - response = self.get_view_response(view, **kwargs) - eq_(response.status_code, 403, - 'unexpected http status for %s' % view) - # Again as an unauthenticated user + self._check_it(self.public_views_gen(format='json'), 200) + self._check_it(self.private_views_gen(format='json'), 403) + + def test_private_addon_contrib_stats_group(self): + # Logged in with stats and contrib stats group. + user = UserProfile.objects.get(email='nobodyspecial@mozilla.com') + group1 = Group.objects.create(name='Stats', rules='Stats:View') + GroupUser.objects.create(user=user, group=group1) + group2 = Group.objects.create(name='Contribution Stats', + rules='ContributionStats:View') + GroupUser.objects.create(user=user, group=group2) + self.login_as_visitor() + + self._check_it(self.public_views_gen(format='json'), 200) + self._check_it(self.private_views_gen(format='json'), 200) + + def test_private_addon_anonymous(self): + # Not logged in self.client.logout() - for view, kwargs in self.private_views_gen(addon_id=5, format='json'): - response = self.get_view_response(view, **kwargs) - eq_(response.status_code, 403, - 'unexpected http status for %s' % view) + self._check_it(self.views_gen(format='json'), 403) + + def test_public_addon_no_groups(self): + # Logged in but no groups + self.login_as_visitor() + self._check_it(self.public_views_gen(addon_id=5, format='json'), 200) + self._check_it(self.private_views_gen(addon_id=5, format='json'), 403) + + def test_public_addon_stats_group(self): + # Logged in with stats group. + user = UserProfile.objects.get(email='nobodyspecial@mozilla.com') + group = Group.objects.create(name='Stats', rules='Stats:View') + GroupUser.objects.create(user=user, group=group) + self.login_as_visitor() + + self._check_it(self.public_views_gen(addon_id=5, format='json'), 200) + self._check_it(self.private_views_gen(addon_id=5, format='json'), 403) + + def test_public_addon_contrib_stats_group(self): + # Logged in with stats and contrib stats group. + user = UserProfile.objects.get(email='nobodyspecial@mozilla.com') + group1 = Group.objects.create(name='Stats', rules='Stats:View') + GroupUser.objects.create(user=user, group=group1) + group2 = Group.objects.create(name='Contribution Stats', + rules='ContributionStats:View') + GroupUser.objects.create(user=user, group=group2) + self.login_as_visitor() + + self._check_it(self.public_views_gen(addon_id=5, format='json'), 200) + self._check_it(self.private_views_gen(addon_id=5, format='json'), 200) + + def test_public_addon_anonymous(self): + # Not logged in + self.client.logout() + self._check_it(self.public_views_gen(addon_id=5, format='json'), 200) + self._check_it(self.private_views_gen(addon_id=5, format='json'), 403) class _TestCSVs(StatsTest, amo.tests.TestCase): @@ -253,12 +289,12 @@ class _TestCSVs(StatsTest, amo.tests.TestCase): def test_no_cache(self): """Test that the csv or json is not caching, due to lack of data.""" self.url_args = {'start': '20200101', 'end': '20200130', 'addon_id': 4} - response = self.get_view_response('stats.versions_series', + response = self.get_view_response('stats.versions_series', head=True, group='day', format='csv') eq_(response["cache-control"], 'max-age=0') self.url_args = {'start': '20200101', 'end': '20200130', 'addon_id': 4} - response = self.get_view_response('stats.versions_series', + response = self.get_view_response('stats.versions_series', head=True, group='day', format='json') eq_(response["cache-control"], 'max-age=0') @@ -278,7 +314,7 @@ class TestCacheControl(StatsTest, amo.tests.TestCase): """Tests we set cache control headers""" def _test_cache_control(self): - response = self.get_view_response('stats.downloads_series', + response = self.get_view_response('stats.downloads_series', head=True, group='month', format='json') assert response.get('cache-control', '').startswith('max-age='), ( 'Bad or no cache-control: %r' % response.get('cache-control', '')) @@ -402,7 +438,7 @@ class TestResponses(ESStatsTest): ]) def test_usage_by_os_csv(self): - r = self.get_view_response('stats.os_series', group='day', + r = self.get_view_response('stats.os_series', head=True, group='day', format='csv') eq_(r.status_code, 200) @@ -720,8 +756,8 @@ class TestSite(amo.tests.TestCase): def tests_period_day(self, _site_query): _site_query.return_value = ['.', '.'] - start = (date.today() - timedelta(days=3)) - end = date.today() + start = (datetime.date.today() - datetime.timedelta(days=3)) + end = datetime.date.today() self.client.get(reverse('stats.site.new', args=['day', start.strftime('%Y%m%d'), end.strftime('%Y%m%d'), 'json'])) @@ -742,5 +778,6 @@ class TestSite(amo.tests.TestCase): def tests_no_date(self, _site_query): _site_query.return_value = ['.', '.'] self.client.get(reverse('stats.site', args=['json', 'date'])) - eq_(_site_query.call_args[0][1], date.today() - timedelta(days=365)) - eq_(_site_query.call_args[0][2], date.today()) + eq_(_site_query.call_args[0][1], + datetime.date.today() - datetime.timedelta(days=365)) + eq_(_site_query.call_args[0][2], datetime.date.today()) diff --git a/apps/stats/views.py b/apps/stats/views.py index 4f3978fa31..1307af97fc 100644 --- a/apps/stats/views.py +++ b/apps/stats/views.py @@ -249,15 +249,26 @@ def check_stats_permission(request, addon, for_contributions=False): Raises PermissionDenied if user is not allowed. """ - if for_contributions or not addon.public_stats: - # only authenticated admins and authors - if (request.user.is_authenticated() and ( - acl.action_allowed(request, 'Stats', 'View') or - addon.has_author(request.amo_user))): - return - elif addon.public_stats: - # non-contributions, public: everybody can view + # If public, non-contributions: everybody can view. + if addon.public_stats and not for_contributions: return + + # Everything else requires an authenticated user. + if not request.user.is_authenticated(): + raise PermissionDenied + + if not for_contributions: + # Only authors and Stats Viewers allowed. + if (addon.has_author(request.amo_user) or + acl.action_allowed(request, 'Stats', 'View')): + return + + else: # For contribution stats. + # Only authors and Contribution Stats Viewers. + if (addon.has_author(request.amo_user) or + acl.action_allowed(request, 'ContributionStats', 'View')): + return + raise PermissionDenied diff --git a/migrations/337-perms-contribstats.sql b/migrations/337-perms-contribstats.sql new file mode 100644 index 0000000000..e7cf31af20 --- /dev/null +++ b/migrations/337-perms-contribstats.sql @@ -0,0 +1,6 @@ +UPDATE groups SET name=CONCAT(name, ' (OLD)') WHERE name='Contributions Stats Viewers'; +INSERT INTO groups (id, name, rules, notes, created, modified) VALUES + (50050, 'Contribution Stats Viewers', 'ContributionStats:View', '', NOW(), NOW()); +INSERT INTO groups_users ( + SELECT NULL, 50050, groups_users.user_id FROM groups, groups_users + WHERE groups.id=groups_users.group_id AND groups.name='Contributions Stats Viewers' AND groups.id < 50000);