From 38e88e3011c51a103fb1d3353e10d8a317adc8a0 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Fri, 10 Oct 2014 15:56:03 -0400 Subject: [PATCH 1/3] Check for empty URL and api key before old analytics api call. Encode course key as a URL parameter. --- lms/djangoapps/instructor/views/api.py | 7 +++++-- lms/djangoapps/instructor/views/legacy.py | 3 ++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 3d5bb7a566..8403942204 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -25,6 +25,7 @@ from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbid from django.utils.html import strip_tags import string # pylint: disable=W0402 import random +import urllib from util.json_request import JsonResponse from instructor.views.instructor_task_helpers import extract_email_features, extract_task_features @@ -1791,13 +1792,15 @@ def proxy_legacy_analytics(request, course_id): analytics_name = request.GET.get('aname') # abort if misconfigured - if not (hasattr(settings, 'ANALYTICS_SERVER_URL') and hasattr(settings, 'ANALYTICS_API_KEY')): + if not (hasattr(settings, 'ANALYTICS_SERVER_URL') and + hasattr(settings, 'ANALYTICS_API_KEY') and + settings.ANALYTICS_SERVER_URL and settings.ANALYTICS_API_KEY): return HttpResponse("Analytics service not configured.", status=501) url = "{}get?aname={}&course_id={}&apikey={}".format( settings.ANALYTICS_SERVER_URL, analytics_name, - course_id.to_deprecated_string(), + urllib.quote(course_id.to_deprecated_string()), settings.ANALYTICS_API_KEY, ) diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index 1e9b11acac..728b04777a 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -11,6 +11,7 @@ import logging import os import re import requests +import urllib from collections import defaultdict, OrderedDict from markupsafe import escape @@ -910,7 +911,7 @@ def instructor_dashboard(request, course_id): """ url = settings.ANALYTICS_SERVER_URL + \ u"get?aname={}&course_id={}&apikey={}".format( - analytics_name, course_key.to_deprecated_string(), settings.ANALYTICS_API_KEY + analytics_name, urllib.quote(unicode(course_key)), settings.ANALYTICS_API_KEY ) try: res = requests.get(url) From 831dd593c9285356bad15e85d0c5ecb4f0a5c54b Mon Sep 17 00:00:00 2001 From: John Eskew Date: Tue, 14 Oct 2014 17:43:18 -0400 Subject: [PATCH 2/3] Add tests for empty analytics server info and url-encoded params. --- lms/djangoapps/instructor/tests/test_api.py | 82 ++++++++++++++++++--- lms/djangoapps/instructor/views/api.py | 4 +- 2 files changed, 74 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 3c529ec077..a76d5ecff6 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -29,6 +29,8 @@ from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from courseware.tests.helpers import LoginEnrollmentTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore from student.tests.factories import UserFactory from courseware.tests.factories import StaffFactory, InstructorFactory, BetaTesterFactory from student.roles import CourseBetaTesterRole @@ -2563,6 +2565,7 @@ class TestInstructorEmailContentList(ModuleStoreTestCase, LoginEnrollmentTestCas self.assertDictEqual(expected_info, returned_info) +@ddt.ddt @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @override_settings(ANALYTICS_SERVER_URL="http://robotanalyticsserver.netbot:900/") @override_settings(ANALYTICS_API_KEY="robot_api_key") @@ -2590,25 +2593,84 @@ class TestInstructorAPIAnalyticsProxy(ModuleStoreTestCase, LoginEnrollmentTestCa self.instructor = InstructorFactory(course_key=self.course.id) self.client.login(username=self.instructor.username, password='test') + @ddt.data((ModuleStoreEnum.Type.mongo, False), (ModuleStoreEnum.Type.split, True)) + @ddt.unpack @patch.object(instructor.views.api.requests, 'get') - def test_analytics_proxy_url(self, act): + def test_analytics_proxy_url(self, store_type, assert_wo_encoding, act): """ Test legacy analytics proxy url generation. """ + with modulestore().default_store(store_type): + course = CourseFactory.create() + instructor_local = InstructorFactory(course_key=course.id) + self.client.login(username=instructor_local.username, password='test') + + act.return_value = self.FakeProxyResponse() + + url = reverse('proxy_legacy_analytics', kwargs={'course_id': course.id.to_deprecated_string()}) + response = self.client.get(url, { + 'aname': 'ProblemGradeDistribution' + }) + self.assertEqual(response.status_code, 200) + + # Make request URL pattern - everything but course id. + url_pattern = "{url}get?aname={aname}&course_id={course_id}&apikey={api_key}".format( + url="http://robotanalyticsserver.netbot:900/", + aname="ProblemGradeDistribution", + course_id="{course_id!s}", + api_key="robot_api_key", + ) + + if assert_wo_encoding: + # Format url with no URL-encoding of parameters. + assert_url = url_pattern.format(course_id=course.id.to_deprecated_string()) + with self.assertRaises(AssertionError): + act.assert_called_once_with(assert_url) + + # Format url *with* URL-encoding of parameters. + expected_url = url_pattern.format(course_id=quote(course.id.to_deprecated_string())) + act.assert_called_once_with(expected_url) + + @override_settings(ANALYTICS_SERVER_URL="") + @patch.object(instructor.views.api.requests, 'get') + def test_analytics_proxy_server_url(self, act): + """ + Test legacy analytics when empty server url. + """ act.return_value = self.FakeProxyResponse() url = reverse('proxy_legacy_analytics', kwargs={'course_id': self.course.id.to_deprecated_string()}) response = self.client.get(url, { 'aname': 'ProblemGradeDistribution' }) - self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 501) - # check request url - expected_url = "{url}get?aname={aname}&course_id={course_id!s}&apikey={api_key}".format( - url="http://robotanalyticsserver.netbot:900/", - aname="ProblemGradeDistribution", - course_id=self.course.id.to_deprecated_string(), - api_key="robot_api_key", - ) - act.assert_called_once_with(expected_url) + @override_settings(ANALYTICS_API_KEY="") + @patch.object(instructor.views.api.requests, 'get') + def test_analytics_proxy_api_key(self, act): + """ + Test legacy analytics when empty server API key. + """ + act.return_value = self.FakeProxyResponse() + + url = reverse('proxy_legacy_analytics', kwargs={'course_id': self.course.id.to_deprecated_string()}) + response = self.client.get(url, { + 'aname': 'ProblemGradeDistribution' + }) + self.assertEqual(response.status_code, 501) + + @override_settings(ANALYTICS_SERVER_URL="") + @override_settings(ANALYTICS_API_KEY="") + @patch.object(instructor.views.api.requests, 'get') + def test_analytics_proxy_empty_url_and_api_key(self, act): + """ + Test legacy analytics when empty server url & API key. + """ + act.return_value = self.FakeProxyResponse() + + url = reverse('proxy_legacy_analytics', kwargs={'course_id': self.course.id.to_deprecated_string()}) + response = self.client.get(url, { + 'aname': 'ProblemGradeDistribution' + }) + self.assertEqual(response.status_code, 501) @patch.object(instructor.views.api.requests, 'get') def test_analytics_proxy(self, act): diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 8403942204..901dab3533 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1697,7 +1697,7 @@ def send_email(request, course_id): course_id, request.user, send_to, - subject,message, + subject, message, template_name=template_name, from_addr=from_addr ) @@ -1800,7 +1800,7 @@ def proxy_legacy_analytics(request, course_id): url = "{}get?aname={}&course_id={}&apikey={}".format( settings.ANALYTICS_SERVER_URL, analytics_name, - urllib.quote(course_id.to_deprecated_string()), + urllib.quote(unicode(course_id)), settings.ANALYTICS_API_KEY, ) From 5e103a918bd06fda8c7ac2c22627586b3e665718 Mon Sep 17 00:00:00 2001 From: John Eskew Date: Wed, 15 Oct 2014 22:02:49 -0400 Subject: [PATCH 3/3] Fix pylint errors to get below limit. --- .../xmodule/xmodule/tests/test_progress.py | 60 +++++++++---------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/common/lib/xmodule/xmodule/tests/test_progress.py b/common/lib/xmodule/xmodule/tests/test_progress.py index 33d129663e..74835ffc97 100644 --- a/common/lib/xmodule/xmodule/tests/test_progress.py +++ b/common/lib/xmodule/xmodule/tests/test_progress.py @@ -23,12 +23,12 @@ class ProgressTest(unittest.TestCase): def test_create_object(self): # These should work: - p = Progress(0, 2) - p = Progress(1, 2) - p = Progress(2, 2) + prg1 = Progress(0, 2) # pylint: disable=W0612 + prg2 = Progress(1, 2) # pylint: disable=W0612 + prg3 = Progress(2, 2) # pylint: disable=W0612 - p = Progress(2.5, 5.0) - p = Progress(3.7, 12.3333) + prg4 = Progress(2.5, 5.0) # pylint: disable=W0612 + prg5 = Progress(3.7, 12.3333) # pylint: disable=W0612 # These shouldn't self.assertRaises(ValueError, Progress, 0, 0) @@ -44,10 +44,10 @@ class ProgressTest(unittest.TestCase): self.assertEqual((0, 2), Progress(-2, 2).frac()) def test_frac(self): - p = Progress(1, 2) - (a, b) = p.frac() - self.assertEqual(a, 1) - self.assertEqual(b, 2) + prg = Progress(1, 2) + (a_mem, b_mem) = prg.frac() + self.assertEqual(a_mem, 1) + self.assertEqual(b_mem, 2) def test_percent(self): self.assertEqual(self.not_started.percent(), 0) @@ -98,38 +98,38 @@ class ProgressTest(unittest.TestCase): def test_to_js_detail_str(self): '''Test the Progress.to_js_detail_str() method''' f = Progress.to_js_detail_str - for p in (self.not_started, self.half_done, self.done): - self.assertEqual(f(p), str(p)) + for prg in (self.not_started, self.half_done, self.done): + self.assertEqual(f(prg), str(prg)) # But None should be encoded as 0 self.assertEqual(f(None), "0") def test_add(self): '''Test the Progress.add_counts() method''' - p = Progress(0, 2) - p2 = Progress(1, 3) - p3 = Progress(2, 5) - pNone = None + prg1 = Progress(0, 2) + prg2 = Progress(1, 3) + prg3 = Progress(2, 5) + prg_none = None add = lambda a, b: Progress.add_counts(a, b).frac() - self.assertEqual(add(p, p), (0, 4)) - self.assertEqual(add(p, p2), (1, 5)) - self.assertEqual(add(p2, p3), (3, 8)) + self.assertEqual(add(prg1, prg1), (0, 4)) + self.assertEqual(add(prg1, prg2), (1, 5)) + self.assertEqual(add(prg2, prg3), (3, 8)) - self.assertEqual(add(p2, pNone), p2.frac()) - self.assertEqual(add(pNone, p2), p2.frac()) + self.assertEqual(add(prg2, prg_none), prg2.frac()) + self.assertEqual(add(prg_none, prg2), prg2.frac()) def test_equality(self): '''Test that comparing Progress objects for equality works correctly.''' - p = Progress(1, 2) - p2 = Progress(2, 4) - p3 = Progress(1, 2) - self.assertTrue(p == p3) - self.assertFalse(p == p2) + prg1 = Progress(1, 2) + prg2 = Progress(2, 4) + prg3 = Progress(1, 2) + self.assertTrue(prg1 == prg3) + self.assertFalse(prg1 == prg2) # Check != while we're at it - self.assertTrue(p != p2) - self.assertFalse(p != p3) + self.assertTrue(prg1 != prg2) + self.assertFalse(prg1 != prg3) class ModuleProgressTest(unittest.TestCase): @@ -137,6 +137,6 @@ class ModuleProgressTest(unittest.TestCase): ''' def test_xmodule_default(self): '''Make sure default get_progress exists, returns None''' - xm = x_module.XModule(Mock(), get_test_system(), DictFieldData({'location': 'a://b/c/d/e'}), Mock()) - p = xm.get_progress() - self.assertEqual(p, None) + xmod = x_module.XModule(Mock(), get_test_system(), DictFieldData({'location': 'a://b/c/d/e'}), Mock()) + prg = xmod.get_progress() + self.assertEqual(prg, None)