Merge pull request #3609 from /issues/3607/1

Fix #3607 - Make modify_labels and edit_issue expect tuple instead of Request

tests/unit/test_api_urls.py

@@ -49,6 +49,12 @@ def mock_api_response(response_config={}):
     return api_response
 
 
+def get_user_mock():
+    user = MagicMock()
+    user.user_id = 1
+    return user
+
+
 class TestAPIURLs(unittest.TestCase):
     """Tests for all API URLs."""
 
@@ -137,12 +143,6 @@ def test_api_comments_link_header_auth(self):
             self.assertEqual(
                 rv.content_type, 'text/html')
 
-    def test_api_set_labels_without_auth(self):
-        """API setting labels without auth returns JSON 403 error code."""
-        rv = self.app.post('/api/issues/1/labels',
-                           environ_base=headers, data='[]')
-        self.assertEqual(rv.status_code, 403)
-
     def test_api_user_activity_without_auth(self):
         """API access to user activity without auth returns JSON 401."""
         rv = self.app.get('/api/issues/miketaylr/creator',
@@ -160,37 +160,89 @@ def test_api_search_wrong_parameter(self):
         self.assertEqual(rv.content_type, 'application/json')
         self.assertEqual(json_body['status'], 404)
 
-    def test_api_patch_issue_state_status(self):
+    @patch('webcompat.db.User.query')
+    def test_api_patch_issue_state_status(self, db_user_mock):
         """Patching the issue - Incompatible state and status."""
         with webcompat.app.app_context():
+
+            # mock authenticated user
+            db_user_mock.return_value.get.return_value = get_user_mock()
+            with self.app.session_transaction() as sess:
+                sess['user_id'] = 1
+
             webcompat.app.config.update(STATUSES=STATUSES)
             data = {'state': 'closed', 'milestone': 2}
             patch_data = json.dumps(data)
             rv = self.app.patch('/api/issues/1/edit', data=patch_data,
                                 environ_base=headers)
             self.assertEqual(rv.status_code, 403)
 
-    def test_api_patch_issue_too_many_json_elements(self):
+    @patch('webcompat.db.User.query')
+    def test_api_patch_issue_too_many_json_elements(self, db_user_mock):
         """Patching the issue - Too many elements in the JSON."""
         with webcompat.app.app_context():
+
+            # mock authenticated user
+            db_user_mock.return_value.get.return_value = get_user_mock()
+            with self.app.session_transaction() as sess:
+                sess['user_id'] = 1
+
             data = {'state': 'open', 'milestone': 2, 'foobar': 'z'}
             patch_data = json.dumps(data)
             rv = self.app.patch('/api/issues/1/edit', data=patch_data,
                                 environ_base=headers)
             self.assertEqual(rv.status_code, 403)
 
+    def test_api_patch_issue_without_auth(self):
+        """Patching the issue - Request without auth."""
+        with webcompat.app.app_context():
+            data = {'state': 'open', 'milestone': 2}
+            patch_data = json.dumps(data)
+            rv = self.app.patch('/api/issues/1/edit', data=patch_data,
+                                environ_base=headers)
+            self.assertEqual(rv.status_code, 403)
+
+    @patch('webcompat.db.User.query')
     @patch('webcompat.api.endpoints.api_request')
-    def test_api_patch_issue_valid_request(self, github_data):
+    def test_api_patch_issue_valid_request(self, github_data, db_user_mock):
         """Patching the issue - Valid request."""
         with webcompat.app.app_context():
-            github_data.return_value = mock_api_response(
-                {'status_code': 200, 'content': '[]'})
+
+            # mock authenticated user
+            db_user_mock.return_value.get.return_value = get_user_mock()
+            with self.app.session_transaction() as sess:
+                sess['user_id'] = 1
+
+            github_data.return_value = ('[]', 200, {})
             data = {'state': 'open', 'milestone': 2}
             patch_data = json.dumps(data)
             rv = self.app.patch('/api/issues/1/edit', data=patch_data,
                                 environ_base=headers)
             self.assertEqual(rv.status_code, 200)
 
+    @patch('webcompat.db.User.query')
+    @patch('webcompat.api.endpoints.api_request')
+    def test_api_set_labels_valid_request(self, github_data, db_user_mock):
+        """Setting labels - Valid request."""
+        with webcompat.app.app_context():
+
+            # mock authenticated user
+            db_user_mock.return_value.get.return_value = get_user_mock()
+            with self.app.session_transaction() as sess:
+                sess['user_id'] = 1
+
+            github_data.return_value = ('[]', 200, {})
+            rv = self.app.post('/api/issues/1/labels',
+                               environ_base=headers,
+                               data='["engine-gecko","priority-important"]')
+            self.assertEqual(rv.status_code, 200)
+
+    def test_api_set_labels_without_auth(self):
+        """API setting labels without auth returns JSON 403 error code."""
+        rv = self.app.post('/api/issues/1/labels',
+                           environ_base=headers, data='[]')
+        self.assertEqual(rv.status_code, 403)
+
 
 if __name__ == '__main__':
     unittest.main()

webcompat/api/endpoints.py

@@ -52,10 +52,13 @@ def edit_issue(number):
     """XHR endpoint to push back edits to GitHub for a single issue.
 
     - It only allows change of state and change of milestones.
-    - It is always proxied to allow any logged in user
-      to be able to edit issues.
+    - It's not proxied, so only users with write access are
+      able to edit issues.
       Format: {'milestone': 2, 'state': 'open'}
     """
+    if not g.user:
+        abort(403)
+
     path = 'repos/{0}/{1}'.format(ISSUES_PATH, number)
     patch_data = json.loads(request.data)
     # Create a list of associated milestones id with their mandatory state.
@@ -65,8 +68,9 @@ def edit_issue(number):
     data_check = (patch_data['milestone'], patch_data['state'])
     # The PATCH data can only be of length: 2
     if data_check in valid_statuses and len(patch_data) == 2:
-        edit = api_request('patch', path, data=request.data)
-        return (edit.content, edit.status_code,
+        (content, status_code, headers) = api_request('patch',
+                                                      path, data=request.data)
+        return (content, status_code,
                 {'content-type': JSON_MIME_HTML})
     # Default will be 403 for this route
     abort(403)
@@ -201,14 +205,12 @@ def modify_labels(number):
     """XHR endpoint to modify issue labels.
 
     Sending in an empty array removes them all as well.
-    This method is always proxied because non-repo collabs
-    can't normally edit labels for an issue.
+    This method is not proxied, so only users with write access
+    will be able to edit labels.
     """
     if g.user:
         path = 'repos/{0}/{1}/labels'.format(ISSUES_PATH, number)
-        labels = api_request('put', path, data=request.data)
-        return (labels.content, labels.status_code,
-                get_response_headers(labels))
+        return api_request('put', path, data=request.data)
     else:
         abort(403)