Use HTTP header for comments service auth

Previously, authentication was done using a URL parameter, which would
appear in various logs. Now, authentication is done more appropriately
with an HTTP header. Note that this requires cs_comments_service commit
cf39aabdd160176ebf206ca19d3ee030161a0b47 or later.

lms/djangoapps/django_comment_client/base/tests.py

@@ -87,14 +87,19 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase):
                                                'course_id': self.course_id})
         response = self.client.post(url, data=thread)
         assert_true(mock_request.called)
-        mock_request.assert_called_with('post',
-                                        'http://localhost:4567/api/v1/i4x-MITx-999-course-Robot_Super_Course/threads',
-                                        data={'body': u'this is a post',
-                                        'anonymous_to_peers': False, 'user_id': 1,
-                                        'title': u'Hello',
-                                        'commentable_id': u'i4x-MITx-999-course-Robot_Super_Course',
-                                        'anonymous': False, 'course_id': u'MITx/999/Robot_Super_Course',
-                                        'api_key': 'PUT_YOUR_API_KEY_HERE'}, timeout=5)
+        mock_request.assert_called_with(
+            'post',
+            'http://localhost:4567/api/v1/i4x-MITx-999-course-Robot_Super_Course/threads',
+            data={
+                'body': u'this is a post',
+                'anonymous_to_peers': False, 'user_id': 1,
+                'title': u'Hello',
+                'commentable_id': u'i4x-MITx-999-course-Robot_Super_Course',
+                'anonymous': False, 'course_id': u'MITx/999/Robot_Super_Course',
+            },
+            headers={'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+            timeout=5
+        )
         assert_equal(response.status_code, 200)
 
     def test_flag_thread(self, mock_request):
@@ -123,9 +128,32 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase):
         response = self.client.post(url)
         assert_true(mock_request.called)
 
-        call_list = [(('get', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d'), {'params': {'mark_as_read': True, 'api_key': 'PUT_YOUR_API_KEY_HERE'}, 'timeout': 5}),
-                     (('put', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d/abuse_flag'), {'data': {'api_key': 'PUT_YOUR_API_KEY_HERE', 'user_id': '1'}, 'timeout': 5}),
-                     (('get', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d'), {'params': {'mark_as_read': True, 'api_key': 'PUT_YOUR_API_KEY_HERE'}, 'timeout': 5})]
+        call_list = [
+            (
+                ('get', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d'),
+                {
+                    'params': {'mark_as_read': True},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            ),
+            (
+                ('put', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d/abuse_flag'),
+                {
+                    'data': {'user_id': '1'},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            ),
+            (
+                ('get', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d'),
+                {
+                    'params': {'mark_as_read': True},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            )
+        ]
 
         assert_equal(call_list, mock_request.call_args_list)
 
@@ -157,9 +185,32 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase):
         response = self.client.post(url)
         assert_true(mock_request.called)
 
-        call_list = [(('get', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d'), {'params': {'mark_as_read': True, 'api_key': 'PUT_YOUR_API_KEY_HERE'}, 'timeout': 5}),
-                     (('put', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d/abuse_unflag'), {'data': {'api_key': 'PUT_YOUR_API_KEY_HERE', 'user_id': '1'}, 'timeout': 5}),
-                     (('get', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d'), {'params': {'mark_as_read': True, 'api_key': 'PUT_YOUR_API_KEY_HERE'}, 'timeout': 5})]
+        call_list = [
+            (
+                ('get', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d'),
+                {
+                    'params': {'mark_as_read': True},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            ),
+            (
+                ('put', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d/abuse_unflag'),
+                {
+                    'data': {'user_id': '1'},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            ),
+            (
+                ('get', 'http://localhost:4567/api/v1/threads/518d4237b023791dca00000d'),
+                {
+                    'params': {'mark_as_read': True},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            )
+        ]
 
         assert_equal(call_list, mock_request.call_args_list)
 
@@ -187,9 +238,32 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase):
         response = self.client.post(url)
         assert_true(mock_request.called)
 
-        call_list = [(('get', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d'), {'params': {'api_key': 'PUT_YOUR_API_KEY_HERE'}, 'timeout': 5}),
-                     (('put', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d/abuse_flag'), {'data': {'api_key': 'PUT_YOUR_API_KEY_HERE', 'user_id': '1'}, 'timeout': 5}),
-                     (('get', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d'), {'params': {'api_key': 'PUT_YOUR_API_KEY_HERE'}, 'timeout': 5})]
+        call_list = [
+            (
+                ('get', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d'),
+                {
+                    'params': {},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            ),
+            (
+                ('put', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d/abuse_flag'),
+                {
+                    'data': {'user_id': '1'},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            ),
+            (
+                ('get', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d'),
+                {
+                    'params': {},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            )
+        ]
 
         assert_equal(call_list, mock_request.call_args_list)
 
@@ -217,9 +291,32 @@ class ViewsTestCase(UrlResetMixin, ModuleStoreTestCase):
         response = self.client.post(url)
         assert_true(mock_request.called)
 
-        call_list = [(('get', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d'), {'params': {'api_key': 'PUT_YOUR_API_KEY_HERE'}, 'timeout': 5}),
-                     (('put', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d/abuse_unflag'), {'data': {'api_key': 'PUT_YOUR_API_KEY_HERE', 'user_id': '1'}, 'timeout': 5}),
-                     (('get', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d'), {'params': {'api_key': 'PUT_YOUR_API_KEY_HERE'}, 'timeout': 5})]
+        call_list = [
+            (
+                ('get', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d'),
+                {
+                    'params': {},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            ),
+            (
+                ('put', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d/abuse_unflag'),
+                {
+                    'data': {'user_id': '1'},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            ),
+            (
+                ('get', 'http://localhost:4567/api/v1/comments/518d4237b023791dca00000d'),
+                {
+                    'params': {},
+                    'headers': {'X-Edx-Api-Key': 'PUT_YOUR_API_KEY_HERE'},
+                    'timeout': 5
+                }
+            )
+        ]
 
         assert_equal(call_list, mock_request.call_args_list)
 

lms/djangoapps/django_comment_client/tests/mock_cs_server/mock_cs_server.py

@@ -27,7 +27,7 @@ class MockCommentServiceRequestHandler(BaseHTTPRequestHandler):
                     (json.dumps(post_dict), self.path))
 
         # Every good post has at least an API key
-        if 'api_key' in post_dict:
+        if 'X-Edx-Api-Key' in self.headers:
             response = self.server._response_str
             # Log the response
             logger.debug("Comment Service: sending response %s" % json.dumps(response))
@@ -62,7 +62,7 @@ class MockCommentServiceRequestHandler(BaseHTTPRequestHandler):
                     (json.dumps(post_dict), self.path))
 
         # Every good post has at least an API key
-        if 'api_key' in post_dict:
+        if 'X-Edx-Api-Key' in self.headers:
             response = self.server._response_str
             # Log the response
             logger.debug("Comment Service: sending response %s" % json.dumps(response))

lms/djangoapps/django_comment_client/tests/mock_cs_server/test_mock_cs_server.py

@@ -43,10 +43,10 @@ class MockCommentServiceServerTest(unittest.TestCase):
         of how you would create a new user
         """
         # Send a request
-        values = {'username': u'user100', 'api_key': 'TEST_API_KEY',
+        values = {'username': u'user100',
                   'external_id': '4', 'email': u'user100@edx.org'}
         data = json.dumps(values)
-        headers = {'Content-Type': 'application/json', 'Content-Length': len(data)}
+        headers = {'Content-Type': 'application/json', 'Content-Length': len(data), 'X-Edx-Api-Key': 'TEST_API_KEY'}
         req = urllib2.Request(self.server_url + '/api/v1/users/4', data, headers)
 
         # Send the request to the mock cs server

lms/lib/comment_client/utils.py

@@ -31,18 +31,14 @@ def merge_dict(dic1, dic2):
 def perform_request(method, url, data_or_params=None, *args, **kwargs):
     if data_or_params is None:
         data_or_params = {}
-    data_or_params['api_key'] = settings.API_KEY
+    headers = {'X-Edx-Api-Key': settings.API_KEY}
     try:
         with dog_stats_api.timer('comment_client.request.time'):
             if method in ['post', 'put', 'patch']:
-                response = requests.request(method, url, data=data_or_params, timeout=5)
+                response = requests.request(method, url, data=data_or_params, headers=headers, timeout=5)
             else:
-                response = requests.request(method, url, params=data_or_params, timeout=5)
+                response = requests.request(method, url, params=data_or_params, headers=headers, timeout=5)
     except Exception as err:
-        # remove API key if it is in the params
-        if 'api_key' in data_or_params:
-            log.info('Deleting API key from params')
-            del data_or_params['api_key']
         log.exception("Trying to call {method} on {url} with params {params}".format(
             method=method, url=url, params=data_or_params))
         # Reraise with a single exception type