opendev/storyboard
Code Issues Proposed changes

Updated & Expanded Subscription API.

Browse Source 
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
This commit is contained in:
Michael Krotscheck 2014-10-28 13:32:57 -07:00
parent 35bc126876
commit 0fb9502678
6 changed files with 328 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

68
storyboard/api/v1/subscriptions.py
View File

@ -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)

14
storyboard/db/api/subscriptions.py
View File

@ -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)

2
storyboard/db/models.py
View File

@ -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'

12
storyboard/notifications/subscriptions_handler.py
View File

@ -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:

223
storyboard/tests/api/test_subscriptions.py Normal file
View File

@ -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))

28
storyboard/tests/mock_data.py
View File

@ -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.