fix(api): Scope item and user lists and issue linking to an organization

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:

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

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:

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)

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

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)

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) => {

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');
 

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) => {

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 () => {

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);
+  });
 });

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 () => {