From 0fb95026787422209393f60ee0d64710bb38adb9 Mon Sep 17 00:00:00 2001 From: Michael Krotscheck Date: Tue, 28 Oct 2014 13:32:57 -0700 Subject: [PATCH] Updated & Expanded Subscription API. This patch adds broad functional test coverage to the subscriptions API, including a fix for a bug introduced when we started to treat enum searches as a collection. The changes are as follows: - Added test for regular user and superuser actions. - Added current user permission checks for various actions. - Added data validity checks for create/destroy subscriptions. - Added supported subscription types to DB api. - Added mock data. - Properly updated target_type API parameter for searching. - Fixed deferred worker use of enum types. Change-Id: Icc80aa87222863f7c1c620cd1a78b965571daa33 --- storyboard/api/v1/subscriptions.py | 68 +++++- storyboard/db/api/subscriptions.py | 14 ++ storyboard/db/models.py | 2 + .../notifications/subscriptions_handler.py | 12 +- storyboard/tests/api/test_subscriptions.py | 223 ++++++++++++++++++ storyboard/tests/mock_data.py | 28 +++ 6 files changed, 328 insertions(+), 19 deletions(-) create mode 100644 storyboard/tests/api/test_subscriptions.py diff --git a/storyboard/api/v1/subscriptions.py b/storyboard/api/v1/subscriptions.py index 8c5dc7c6..c29e3035 100644 --- a/storyboard/api/v1/subscriptions.py +++ b/storyboard/api/v1/subscriptions.py @@ -14,17 +14,18 @@ # limitations under the License. from oslo.config import cfg +from pecan import abort from pecan import request from pecan import response from pecan import rest from pecan.secure import secure -from wsme.exc import ClientSideError from wsme import types as wtypes import wsmeext.pecan as wsme_pecan from storyboard.api.auth import authorization_checks as checks from storyboard.api.v1 import base from storyboard.db.api import subscriptions as subscription_api +from storyboard.db.api import users as user_api CONF = cfg.CONF @@ -60,7 +61,7 @@ class SubscriptionsController(rest.RestController): Provides Create, Delete, and search methods for resource subscriptions. """ - @secure(checks.guest) + @secure(checks.authenticated) @wsme_pecan.wsexpose(Subscription, int) def get_one(self, subscription_id): """Retrieve a specific subscription record. @@ -68,13 +69,18 @@ class SubscriptionsController(rest.RestController): :param subscription_id: The unique id of this subscription. """ - return Subscription.from_db_model( - subscription_api.subscription_get(subscription_id) - ) + subscription = subscription_api.subscription_get(subscription_id) + current_user = user_api.user_get(request.current_user_id) - @secure(checks.guest) - @wsme_pecan.wsexpose([Subscription], int, int, unicode, int, int, unicode, - unicode) + if subscription.user_id != request.current_user_id \ + and not current_user.is_superuser: + abort(403, "You do not have access to this record.") + + return Subscription.from_db_model(subscription) + + @secure(checks.authenticated) + @wsme_pecan.wsexpose([Subscription], int, int, [unicode], int, int, + unicode, unicode) def get(self, marker=None, limit=None, target_type=None, target_id=None, user_id=None, sort_field='id', sort_dir='asc'): """Retrieve a list of subscriptions. @@ -93,6 +99,12 @@ class SubscriptionsController(rest.RestController): limit = CONF.page_size_default limit = min(CONF.page_size_maximum, max(1, limit)) + # Sanity check on user_id + current_user = user_api.user_get(request.current_user_id) + if user_id != request.current_user_id \ + and not current_user.is_superuser: + user_id = request.current_user_id + # Resolve the marker record. marker_sub = subscription_api.subscription_get(marker) @@ -125,8 +137,35 @@ class SubscriptionsController(rest.RestController): :param subscription: A subscription within the request body. """ - # Override the user id. - subscription.user_id = request.current_user_id + # Data sanity check - are all fields set? + if not subscription.target_type or not subscription.target_id: + abort(400, 'You are missing either the target_type or the' + ' target_id') + + # Sanity check on user_id + current_user = user_api.user_get(request.current_user_id) + if not subscription.user_id: + subscription.user_id = request.current_user_id + elif subscription.user_id != request.current_user_id \ + and not current_user.is_superuser: + abort(403, "You can only subscribe to resources on your own.") + + # Data sanity check: The resource must exist. + resource = subscription_api.subscription_get_resource( + target_type=subscription.target_type, + target_id=subscription.target_id) + if not resource: + abort(400, 'You cannot subscribe to a nonexistent resource.') + + # Data sanity check: The subscription cannot be duplicated for this + # user. + existing = subscription_api.subscription_get_all( + target_type=[subscription.target_type, ], + target_id=subscription.target_id, + user_id=subscription.user_id) + + if existing: + abort(409, 'You are already subscribed to this resource.') result = subscription_api.subscription_create(subscription.as_dict()) return Subscription.from_db_model(result) @@ -138,10 +177,13 @@ class SubscriptionsController(rest.RestController): :param subscription_id: The unique id of the subscription to delete. """ - subscription = subscription_api.subscription_get(subscription_id) - if subscription.user_id != request.current_user_id: - raise ClientSideError(status_code=403) + + # Sanity check on user_id + current_user = user_api.user_get(request.current_user_id) + if subscription.user_id != request.current_user_id \ + and not current_user.is_superuser: + abort(403, "You can only remove your own subscriptions.") subscription_api.subscription_delete(subscription_id) diff --git a/storyboard/db/api/subscriptions.py b/storyboard/db/api/subscriptions.py index 6658a52e..443925a4 100644 --- a/storyboard/db/api/subscriptions.py +++ b/storyboard/db/api/subscriptions.py @@ -16,6 +16,13 @@ from storyboard.db.api import base as api_base from storyboard.db import models +SUPPORTED_TYPES = { + 'project': models.Project, + 'project_group': models.ProjectGroup, + 'story': models.Story, + 'task': models.Task +} + def subscription_get(subscription_id): return api_base.entity_get(models.Subscription, subscription_id) @@ -32,6 +39,13 @@ def subscription_get_all_by_target(target_type, target_id): target_id=target_id) +def subscription_get_resource(target_type, target_id): + if target_type not in SUPPORTED_TYPES: + return None + + return api_base.entity_get(SUPPORTED_TYPES[target_type], target_id) + + def subscription_get_count(**kwargs): return api_base.entity_get_count(models.Subscription, **kwargs) diff --git a/storyboard/db/models.py b/storyboard/db/models.py index bfd920d4..3c97236c 100644 --- a/storyboard/db/models.py +++ b/storyboard/db/models.py @@ -377,6 +377,8 @@ class Subscription(ModelBuilder, Base): # Cant use foreign key here as it depends on the type target_id = Column(Integer) + _public_fields = ["id", "target_type", "target_id", "user_id"] + class SubscriptionEvents(ModelBuilder, Base): __tablename__ = 'subscription_events' diff --git a/storyboard/notifications/subscriptions_handler.py b/storyboard/notifications/subscriptions_handler.py index f49bd6ca..5aa5f78c 100644 --- a/storyboard/notifications/subscriptions_handler.py +++ b/storyboard/notifications/subscriptions_handler.py @@ -34,12 +34,12 @@ def handle_timeline_events(event, author_id): # Handling tasks targeted. target_sub = subscriptions_api.subscription_get_all_by_target( - 'task', task_id) + ['task'], task_id) target_subs.extend(target_sub) # Handling stories targeted. target_sub = subscriptions_api.subscription_get_all_by_target( - 'story', story_id) + ['story'], story_id) target_subs.extend(target_sub) # Handling projects, project groups targeted for stories without tasks. @@ -51,14 +51,14 @@ def handle_timeline_events(event, author_id): # Handling projects targeted. target_sub = subscriptions_api.subscription_get_all_by_target( - 'project', project_id) + ['project'], project_id) target_subs.extend(target_sub) # Handling project groups targeted. pgs = project_groups_api.project_group_get_all(project_id=project_id) for pg in pgs: target_sub = subscriptions_api.subscription_get_all_by_target( - 'project_group', pg.id) + ['project_group'], pg.id) target_subs.extend(target_sub) for sub in target_subs: @@ -88,7 +88,7 @@ def handle_resources(method, resource_id, sub_resource_id, author_id): # Handling project addition/deletion to/from project_group. target_sub = subscriptions_api.subscription_get_all_by_target( - 'project', sub_resource_id) + ['project'], sub_resource_id) target_subs.extend(target_sub) for sub in target_subs: @@ -117,7 +117,7 @@ def handle_resources(method, resource_id, sub_resource_id, author_id): if method == 'DELETE': # Handling project_group targeted. target_sub = subscriptions_api.subscription_get_all_by_target( - 'project_group', resource_id) + ['project_group'], resource_id) target_subs.extend(target_sub) for sub in target_subs: diff --git a/storyboard/tests/api/test_subscriptions.py b/storyboard/tests/api/test_subscriptions.py new file mode 100644 index 00000000..60702b29 --- /dev/null +++ b/storyboard/tests/api/test_subscriptions.py @@ -0,0 +1,223 @@ +# Copyright (c) 2014 Hewlett-Packard Development Company, L.P. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import json + +from storyboard.tests import base + + +class TestSubscriptionsAsNobody(base.FunctionalTest): + def setUp(self): + super(TestSubscriptionsAsNobody, self).setUp() + self.resource = '/subscriptions' + + def test_get_subscriptions(self): + response = self.get_json(self.resource, expect_errors=True) + self.assertEqual(401, response.status_code) + + +class TestSubscriptionsAsUser(base.FunctionalTest): + def setUp(self): + super(TestSubscriptionsAsUser, self).setUp() + self.resource = '/subscriptions' + self.default_headers['Authorization'] = 'Bearer valid_user_token' + + def test_get_subscriptions(self): + """Assert that a base user has no subscriptions.""" + response = self.get_json(self.resource) + self.assertEqual(0, len(response)) + + def test_create_subscriptions(self): + """Assert that we can create subscriptions.""" + response = self.post_json(self.resource, { + 'target_id': 1, + 'target_type': 'project' + }) + subscription = json.loads(response.body) + + self.assertEquals('project', subscription['target_type']) + self.assertEquals(1, subscription['target_id']) + self.assertEquals(2, subscription['user_id']) + self.assertIsNotNone(subscription['id']) + + response2 = self.post_json(self.resource, { + 'user_id': 2, + 'target_id': 2, + 'target_type': 'project' + }) + subscription2 = json.loads(response2.body) + + self.assertEquals('project', subscription2['target_type']) + self.assertEquals(2, subscription2['target_id']) + self.assertEquals(2, subscription2['user_id']) + self.assertIsNotNone(subscription2['id']) + + def test_delete_subscriptions(self): + """Assert that we can delete subscriptions.""" + response = self.post_json(self.resource, { + 'target_id': 1, + 'target_type': 'project' + }) + subscription = json.loads(response.body) + + search_response_1 = self.get_json(self.resource) + self.assertEqual(1, len(search_response_1)) + + response2 = self.delete(self.resource + '/' + str(subscription['id']), + expect_errors=True) + self.assertEqual(204, response2.status_code) + + search_response_2 = self.get_json(self.resource) + self.assertEqual(0, len(search_response_2)) + + def test_create_subscription_not_self(self): + """Assert that a regular user cannot create a subscription for other + people. + """ + response = self.post_json(self.resource, { + 'target_id': 1, + 'target_type': 'project', + 'user_id': 1 + }, expect_errors=True) + self.assertEqual(403, response.status_code) + + def test_get_subscription(self): + """Assert that we can read subscriptions. Note that the mock data has + two subscriptions for the mock superuser, these should not show up + here. + """ + self.post_json(self.resource, { + 'target_id': 1, + 'target_type': 'project' + }) + + response = self.get_json(self.resource) + self.assertEquals(1, len(response)) + + response = self.get_json(self.resource + + '?target_type=project') + self.assertEqual(1, len(response)) + + response = self.get_json(self.resource + + '?target_type=project&target_id=1') + self.assertEqual(1, len(response)) + + response = self.get_json(self.resource + + '?target_type=story&target_id=1') + self.assertEqual(0, len(response)) + + def test_create_subscription_for_invalid_resource(self): + """Assert that subscribing to something that doesn't exist is + invalid. + """ + response = self.post_json(self.resource, { + 'target_id': 100, + 'target_type': 'project' + }, expect_errors=True) + self.assertEquals(400, response.status_code) + + response = self.post_json(self.resource, { + 'target_id': 100, + 'target_type': 'story' + }, expect_errors=True) + self.assertEquals(400, response.status_code) + + response = self.post_json(self.resource, { + 'target_id': 100, + 'target_type': 'task' + }, expect_errors=True) + self.assertEquals(400, response.status_code) + + response = self.post_json(self.resource, { + 'target_id': 100, + 'target_type': 'notarealresource' + }, expect_errors=True) + self.assertEquals(400, response.status_code) + + def test_cannot_create_duplicates(self): + """Assert that we cannot create duplicate subscriptions.""" + response1 = self.post_json(self.resource, { + 'target_id': 1, + 'target_type': 'project' + }) + self.assertEquals(200, response1.status_code) + response2 = self.post_json(self.resource, { + 'target_id': 1, + 'target_type': 'project' + }, expect_errors=True) + self.assertEqual(409, response2.status_code) + + def test_cannot_search_for_not_self(self): + """Assert that we cannot search other people's subscriptions. Note + that there are subscriptions in the mock data. + """ + response = self.get_json(self.resource + '?user_id=1') + self.assertEqual(0, len(response)) + + +class TestSubscriptionsAsSuperuser(base.FunctionalTest): + def setUp(self): + super(TestSubscriptionsAsSuperuser, self).setUp() + self.resource = '/subscriptions' + self.default_headers['Authorization'] = 'Bearer valid_superuser_token' + + def test_get_subscriptions(self): + """Assert that a base user has no subscriptions.""" + response = self.get_json(self.resource) + self.assertEqual(3, len(response)) + + def test_create_subscription_not_self(self): + """Assert that we can create subscriptions for others.""" + response = self.post_json(self.resource, { + 'user_id': 3, + 'target_id': 1, + 'target_type': 'project' + }) + subscription = json.loads(response.body) + + self.assertEquals('project', subscription['target_type']) + self.assertEquals(1, subscription['target_id']) + self.assertEquals(3, subscription['user_id']) + self.assertIsNotNone(subscription['id']) + + def test_get_subscription(self): + """Assert that we can read subscriptions for others. + """ + response = self.get_json(self.resource + '/3') + self.assertEquals(3, response['id']) + self.assertEquals(3, response['user_id']) + + def test_can_search_for_not_self(self): + """Assert that we can search other people's subscriptions.""" + response = self.get_json(self.resource + '?user_id=3') + self.assertEqual(1, len(response)) + + def test_delete_subscription_other(self): + """Assert that we can delete subscriptions.""" + response = self.post_json(self.resource, { + 'user_id': 3, + 'target_id': 1, + 'target_type': 'project' + }) + subscription = json.loads(response.body) + + search_response_1 = self.get_json(self.resource + '?user_id=3') + self.assertEqual(2, len(search_response_1)) + + response2 = self.delete(self.resource + '/' + str(subscription['id']), + expect_errors=True) + self.assertEqual(204, response2.status_code) + + search_response_2 = self.get_json(self.resource + '?user_id=3') + self.assertEqual(1, len(search_response_2)) diff --git a/storyboard/tests/mock_data.py b/storyboard/tests/mock_data.py index 312536ef..343cf3c5 100644 --- a/storyboard/tests/mock_data.py +++ b/storyboard/tests/mock_data.py @@ -19,6 +19,7 @@ from storyboard.db.models import AccessToken from storyboard.db.models import Project from storyboard.db.models import ProjectGroup from storyboard.db.models import Story +from storyboard.db.models import Subscription from storyboard.db.models import Task from storyboard.db.models import User @@ -42,6 +43,11 @@ def load(): username='regularuser', email='regularuser@example.com', full_name='Regular User', + is_superuser=False), + User(id=3, + username='otheruser', + email='otheruser@example.com', + full_name='Other User', is_superuser=False) ]) @@ -185,6 +191,28 @@ def load(): ) ]) + # Load some subscriptions. + load_data([ + Subscription( + id=1, + user_id=1, + target_type='project', + target_id=1 + ), + Subscription( + id=2, + user_id=1, + target_type='project', + target_id=3 + ), + Subscription( + id=3, + user_id=3, + target_type='story', + target_id=1 + ), + ]) + def load_data(data): """Pre load test data into the database.