diff --git a/backend-py/src/api/endpoints/items.py b/backend-py/src/api/endpoints/items.py index ae305f6..63a8e27 100644 --- a/backend-py/src/api/endpoints/items.py +++ b/backend-py/src/api/endpoints/items.py @@ -2,7 +2,7 @@ from flask import jsonify, request, Response from flask.views import MethodView -from werkzeug.exceptions import NotFound +from werkzeug.exceptions import BadRequest, NotFound from src.api.endpoints.base import register_api from src.api.serializers import serialize @@ -40,24 +40,27 @@ def _get_item_or_404(self, item_id: int) -> Item: return item def index(self) -> Response: + # Always scope items to a single organization so one tenant can never + # list another tenant's items. organization_slug = request.args.get("organization") - user_id = request.args.get("user") - - query = Item.query + if not organization_slug: + raise BadRequest("The 'organization' query parameter is required.") + organization = Organization.query.filter( + Organization.slug == organization_slug + ).first() + if not organization: + raise NotFound - if organization_slug: - organization_option = Organization.query.filter( - Organization.slug == organization_slug - ).first() - if organization_option: - query = query.filter(Item.organization_id == organization_option.id) - linked_query = query.filter(Item.sentry_id is not None) - if linked_query.count() > 0: - return add_sentry_api_data(organization_option, query) + query = Item.query.filter(Item.organization_id == organization.id) + user_id = request.args.get("user") if user_id is not None: query = query.filter(Item.assignee_id == user_id) + linked_query = query.filter(Item.sentry_id is not None) + if linked_query.count() > 0: + return add_sentry_api_data(organization, query) + return jsonify(serialize(query.all())) def get(self, item_id: int) -> Response: diff --git a/backend-py/src/api/endpoints/sentry/issue_link.py b/backend-py/src/api/endpoints/sentry/issue_link.py index 3853dbe..ecbf05a 100644 --- a/backend-py/src/api/endpoints/sentry/issue_link.py +++ b/backend-py/src/api/endpoints/sentry/issue_link.py @@ -88,7 +88,14 @@ def link_issue_link() -> Response: user_link_data: IssueLinkSettings = request.json.get("fields") # Associate the Sentry Issue with the item from our application the user selected. - item = Item.query.filter(Item.id == user_link_data.get("itemId")).first() + # Only look up items in this installation's organization, so an installation + # can't link to another organization's items. + item = Item.query.filter( + Item.id == user_link_data.get("itemId"), + Item.organization_id == organization.id, + ).first() + if not item: + return Response("", 404) item.sentry_id = request.json.get("issueId") db_session.commit() app.logger.info("Linked item through Sentry Issue Link UI Component") diff --git a/backend-py/src/api/endpoints/users.py b/backend-py/src/api/endpoints/users.py index 80f1f5c..9457b79 100644 --- a/backend-py/src/api/endpoints/users.py +++ b/backend-py/src/api/endpoints/users.py @@ -2,7 +2,7 @@ from flask import jsonify, request, Response from flask.views import MethodView -from werkzeug.exceptions import NotFound +from werkzeug.exceptions import BadRequest, NotFound from src.api.endpoints.base import register_api from src.api.serializers import serialize @@ -19,17 +19,18 @@ def _get_user_or_404(self, user_id: int) -> User: return user def index(self) -> Response: + # Always scope users to a single organization so one tenant can never + # list another tenant's users. organization_slug = request.args.get("organization") + if not organization_slug: + raise BadRequest("The 'organization' query parameter is required.") + organization = Organization.query.filter( + Organization.slug == organization_slug + ).first() + if not organization: + raise NotFound - query = User.query - - if organization_slug is not None: - organization_option = Organization.query.filter( - Organization.slug == organization_slug - ).first() - if organization_option: - query = query.filter(User.organization_id == organization_option.id) - + query = User.query.filter(User.organization_id == organization.id) return jsonify(serialize(query.all())) def get(self, user_id: int) -> Response: diff --git a/backend-py/tests/api/items/test_index.py b/backend-py/tests/api/items/test_index.py index 40975b6..6ff59ad 100644 --- a/backend-py/tests/api/items/test_index.py +++ b/backend-py/tests/api/items/test_index.py @@ -16,18 +16,28 @@ def setUp(self): super().setUp() other_organization = self.create_organization("other") - other_user = self.create_user(other_organization, "other user") - self.create_item(other_organization, other_user) + self.other_user = self.create_user(other_organization, "other user") + self.create_item(other_organization, self.other_user) - def test_index(self): - response = self.get_success_response() - assert len(response.json) == 2 + def test_index_requires_organization(self): + self.get_error_response(status_code=400) + + def test_index_unknown_organization(self): + self.get_error_response(organization="does-not-exist", status_code=404) def test_index_user(self): - response = self.get_success_response(user=self.user.id) + self.create_item(self.organization) + response = self.get_success_response( + organization=self.organization.slug, user=self.user.id + ) - assert len(response.json) == 1 - assert response.json[0]["assigneeId"] == self.user.id + assert [item["id"] for item in response.json] == [self.item.id] + + def test_index_user_from_other_organization(self): + response = self.get_success_response( + organization=self.organization.slug, user=self.other_user.id + ) + assert response.json == [] def test_index_organization(self): response = self.get_success_response(organization=self.organization.slug) diff --git a/backend-py/tests/api/sentry/test_issue_link.py b/backend-py/tests/api/sentry/test_issue_link.py index a8f894b..88ef5eb 100644 --- a/backend-py/tests/api/sentry/test_issue_link.py +++ b/backend-py/tests/api/sentry/test_issue_link.py @@ -57,3 +57,15 @@ def test_link_issue_link(self): assert response.json.get("webUrl") is not None assert response.json.get("project") == "ACME" assert response.json.get("identifier") == str(self.item.id) + + def test_link_issue_link_other_organization_item(self): + other_organization = self.create_organization("other") + other_item = self.create_item(other_organization) + db_session.commit() + payload = { + **self.payload, + "fields": {**self.payload["fields"], "itemId": other_item.id}, + } + + self.get_error_response(data=payload, status_code=404) + assert other_item.sentry_id is None diff --git a/backend-py/tests/api/users/test_index.py b/backend-py/tests/api/users/test_index.py index 0094dac..0dffb45 100644 --- a/backend-py/tests/api/users/test_index.py +++ b/backend-py/tests/api/users/test_index.py @@ -8,9 +8,11 @@ def setUp(self): other_organization = self.create_organization("other") self.create_user(other_organization, "other user") - def test_index(self): - response = self.get_success_response() - assert len(response.json) == 2 + def test_index_requires_organization(self): + self.get_error_response(status_code=400) + + def test_index_unknown_organization(self): + self.get_error_response(organization="does-not-exist", status_code=404) def test_index_organization(self): response = self.get_success_response(organization=self.organization.slug) diff --git a/backend-ts/src/api/items.ts b/backend-ts/src/api/items.ts index 16b5273..a67d735 100644 --- a/backend-ts/src/api/items.ts +++ b/backend-ts/src/api/items.ts @@ -29,22 +29,26 @@ async function addSentryAPIData( } router.get('/', async (request, response) => { + // Always scope items to a single organization so one tenant can never + // list another tenant's items. const {organization: slug} = request.query; - if (slug) { - const organization = await Organization.findOne({ - include: Item, - where: {slug}, - }); - if (organization) { - return response.send( - organization.items.some(item => !!item.sentryId) - ? await addSentryAPIData(organization) - : organization.items - ); - } + if (!slug) { + return response + .status(400) + .send({message: "The 'organization' query parameter is required"}); } - const items = await Item.findAll(); - return response.send(items); + const organization = await Organization.findOne({ + include: Item, + where: {slug}, + }); + if (!organization) { + return response.sendStatus(404); + } + return response.send( + organization.items.some(item => !!item.sentryId) + ? await addSentryAPIData(organization) + : organization.items + ); }); router.post('/', async (request, response) => { diff --git a/backend-ts/src/api/sentry/issueLink.ts b/backend-ts/src/api/sentry/issueLink.ts index 3206d1f..8320183 100644 --- a/backend-ts/src/api/sentry/issueLink.ts +++ b/backend-ts/src/api/sentry/issueLink.ts @@ -57,7 +57,17 @@ router.post('/link', async (request, response) => { const issueLinkData = request.body as IssueLinkRequestData; // Associate the Sentry Issue with the item from our application the user selected. - const item = await Item.findByPk(issueLinkData.fields.itemId); + // Only look up items in this installation's organization, so an installation + // can't link to another organization's items. + const item = await Item.findOne({ + where: { + id: issueLinkData.fields.itemId, + organizationId: response.locals.organization.id, + }, + }); + if (!item) { + return response.sendStatus(404); + } await item.update({sentryId: issueLinkData.issueId}); console.info('Linked item through Sentry Issue Link UI Component'); diff --git a/backend-ts/src/api/users.ts b/backend-ts/src/api/users.ts index 5cffb6e..2ce600f 100644 --- a/backend-ts/src/api/users.ts +++ b/backend-ts/src/api/users.ts @@ -6,18 +6,22 @@ import User from '../models/User.model'; const router = express.Router(); router.get('/', async (request, response) => { + // Always scope users to a single organization so one tenant can never + // list another tenant's users. const {organization: slug} = request.query; - if (slug) { - const organization = await Organization.findOne({ - include: User, - where: {slug}, - }); - if (organization) { - return response.send(organization.users); - } + if (!slug) { + return response + .status(400) + .send({message: "The 'organization' query parameter is required"}); } - const users = await User.findAll(); - return response.send(users); + const organization = await Organization.findOne({ + include: User, + where: {slug}, + }); + if (!organization) { + return response.sendStatus(404); + } + return response.send(organization.users); }); router.post('/', async (request, response) => { diff --git a/backend-ts/tests/api/items.spec.ts b/backend-ts/tests/api/items.spec.ts index 62ebfdb..c3a37ea 100644 --- a/backend-ts/tests/api/items.spec.ts +++ b/backend-ts/tests/api/items.spec.ts @@ -26,10 +26,16 @@ describe(path, () => { afterAll(async () => await closeTestServer()); - it('handles GET all', async () => { + it('rejects GET without an organization', async () => { const response = await request(server).get(path); - assert.equal(response.body.length, itemTitles.length); - assert.equal(response.statusCode, 200); + assert.equal(response.statusCode, 400); + }); + + it('returns 404 for GET with an unknown organization', async () => { + const response = await request(server) + .get(path) + .query({organization: 'does-not-exist'}); + assert.equal(response.statusCode, 404); }); it('handles GET by organization', async () => { diff --git a/backend-ts/tests/api/sentry/issueLink.spec.ts b/backend-ts/tests/api/sentry/issueLink.spec.ts index 983d33b..01b66a5 100644 --- a/backend-ts/tests/api/sentry/issueLink.spec.ts +++ b/backend-ts/tests/api/sentry/issueLink.spec.ts @@ -55,4 +55,16 @@ describe(`GET ${path}`, () => { assert.equal(response.body.project, 'ACME'); assert.equal(response.body.identifier, item.id); }); + + it("doesn't link to another organization's items", async () => { + const otherItem = await createItem(); + const payload = { + ...MOCK_ISSUE_LINK, + fields: {...MOCK_ISSUE_LINK.fields, itemId: otherItem.id}, + }; + const response = await request(server).post(`${path}/link`).send(payload); + assert.equal(response.statusCode, 404); + await otherItem.reload(); + assert.equal(otherItem.sentryId, null); + }); }); diff --git a/backend-ts/tests/api/users.spec.ts b/backend-ts/tests/api/users.spec.ts index a21afe3..a0ea298 100644 --- a/backend-ts/tests/api/users.spec.ts +++ b/backend-ts/tests/api/users.spec.ts @@ -21,10 +21,16 @@ describe(path, () => { afterAll(async () => await closeTestServer()); - it('handles GET', async () => { + it('rejects GET without an organization', async () => { const response = await request(server).get(path); - assert.equal(response.body.length, userNames.length); - assert.equal(response.statusCode, 200); + assert.equal(response.statusCode, 400); + }); + + it('returns 404 for GET with an unknown organization', async () => { + const response = await request(server) + .get(path) + .query({organization: 'does-not-exist'}); + assert.equal(response.statusCode, 404); }); it('handles GET by organization', async () => {