From d985ce888f5d8e97fa2f27b5b030321e49812c3f Mon Sep 17 00:00:00 2001 From: "Bermudez, Jaime" Date: Fri, 10 Mar 2017 16:23:55 -0500 Subject: [PATCH 1/4] Update monkey-patching of reverse method to account for Django 1.10 moving that method into the django.urls package. --- .gitignore | 2 + django_auth_lti/patch_reverse.py | 12 +++- .../tests/test_lti_auth_middleware.py | 4 +- run_tests.py | 24 +++---- setup.py | 4 +- tests/__init__.py | 0 tests/test_lti_auth_middleware.py | 67 +++++++++++++++++++ tests/test_settings.py | 31 +++++++++ tests/test_verification.py | 36 ++++++++++ tests/urls.py | 5 ++ tox.ini | 10 +++ 11 files changed, 177 insertions(+), 18 deletions(-) create mode 100644 tests/__init__.py create mode 100644 tests/test_lti_auth_middleware.py create mode 100644 tests/test_settings.py create mode 100755 tests/test_verification.py create mode 100644 tests/urls.py create mode 100644 tox.ini diff --git a/.gitignore b/.gitignore index 7790279..875d642 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,5 @@ dist *.egg-info *.egg .idea +.tox/ +.eggs/ diff --git a/django_auth_lti/patch_reverse.py b/django_auth_lti/patch_reverse.py index 86cd429..16427c8 100644 --- a/django_auth_lti/patch_reverse.py +++ b/django_auth_lti/patch_reverse.py @@ -1,13 +1,19 @@ """ Monkey-patch django.core.urlresolvers.reverse to add resource_link_id to all URLs """ +import logging from urlparse import urlparse, urlunparse, parse_qs from urllib import urlencode -from django.core import urlresolvers +# Django 1.10 moved urlresolvers into django.urls package +try: + from django import urls as urlresolvers +except ImportError: + from django.core import urlresolvers from .thread_local import get_current_request +logger = logging.getLogger(__name__) django_reverse = None @@ -19,6 +25,7 @@ def reverse(*args, **kwargs): :param kwargs['exclude_resource_link_id']: Do not add the resource link id as a query parameter :returns Django named url """ + logger.debug("inside custom reverse function!") request = get_current_request() # Check for custom exclude_resource_link_id kwarg and remove it before passing kwargs to django reverse @@ -39,10 +46,11 @@ def reverse(*args, **kwargs): def patch_reverse(): """ - Monkey-patches the django.core.urlresolvers.reverse function. Will not patch twice. + Monkey-patches the reverse function. Will not patch twice. """ global django_reverse if urlresolvers.reverse is not reverse: + logger.debug("inside patch_reverse where urlresolvers.reverse != reverse") django_reverse = urlresolvers.reverse urlresolvers.reverse = reverse diff --git a/django_auth_lti/tests/test_lti_auth_middleware.py b/django_auth_lti/tests/test_lti_auth_middleware.py index b5baf30..617190a 100644 --- a/django_auth_lti/tests/test_lti_auth_middleware.py +++ b/django_auth_lti/tests/test_lti_auth_middleware.py @@ -3,7 +3,7 @@ from mock import patch from django.test import RequestFactory from django.contrib.auth import models -from django_auth_lti.middleware import LTIAuthMiddleware +from django_auth_lti.middleware_patched import MultiLTILaunchAuthMiddleware @patch('django_auth_lti.middleware.logger') @@ -11,7 +11,7 @@ class TestLTIAuthMiddleware(unittest.TestCase): longMessage = True def setUp(self): - self.mw = LTIAuthMiddleware() + self.mw = MultiLTILaunchAuthMiddleware() def build_lti_launch_request(self, post_data): """ diff --git a/run_tests.py b/run_tests.py index 9454b23..ca8149f 100644 --- a/run_tests.py +++ b/run_tests.py @@ -1,15 +1,15 @@ #!/usr/bin/env python +import os +import sys +import django from django.conf import settings - - -def runtests(): - settings.configure( - # App-specific setttings here - ) - # settings must be configured for this import to work - from django.test.runner import DiscoverRunner - DiscoverRunner(interactive=False, failfast=False).run_tests(['django_auth_lti']) - -if __name__ == '__main__': - runtests() +from django.test.utils import get_runner + +if __name__ == "__main__": + os.environ['DJANGO_SETTINGS_MODULE'] = 'tests.test_settings' + django.setup() + TestRunner = get_runner(settings) + test_runner = TestRunner() + failures = test_runner.run_tests(["tests"]) + sys.exit(bool(failures)) diff --git a/setup.py b/setup.py index b518336..d571d94 100644 --- a/setup.py +++ b/setup.py @@ -8,7 +8,7 @@ setup( name='django-auth-lti', - version='1.2.6', + version='1.2.7', packages=['django_auth_lti'], include_package_data=True, license='TBD License', # example license @@ -33,7 +33,7 @@ "Django>=1.6", "ims-lti-py==0.6", "django-braces==1.3.1", - "oauth2==1.9.0.post1", # to catch errors uncaught by ims-lti-py + "oauth2==1.9.0.post1", # to catch errors uncaught by ims-lti-py ], tests_require=[ 'mock', diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/test_lti_auth_middleware.py b/tests/test_lti_auth_middleware.py new file mode 100644 index 0000000..b5baf30 --- /dev/null +++ b/tests/test_lti_auth_middleware.py @@ -0,0 +1,67 @@ +import unittest +import mock +from mock import patch +from django.test import RequestFactory +from django.contrib.auth import models +from django_auth_lti.middleware import LTIAuthMiddleware + + +@patch('django_auth_lti.middleware.logger') +class TestLTIAuthMiddleware(unittest.TestCase): + longMessage = True + + def setUp(self): + self.mw = LTIAuthMiddleware() + + def build_lti_launch_request(self, post_data): + """ + Utility method that builds a fake lti launch request with custom data. + """ + # Add message type to post data + post_data.update(lti_message_type='basic-lti-launch-request') + # Add resource_link_id to post data + post_data.update(resource_link_id='d202fb112a14f27107149ed874bf630aa8e029a5') + + request = RequestFactory().post('/fake/lti/launch', post_data) + request.user = mock.Mock(name='User', spec=models.User) + request.session = {} + return request + + @patch('django_auth_lti.middleware.auth') + def test_roles_merged_with_custom_roles(self, mock_auth, mock_logger): + """ + Assert that 'roles' list in session contains merged set of roles when custom role key is + defined and values have been passed in. + """ + request = self.build_lti_launch_request({ + 'roles': 'RoleOne,RoleTwo', + 'test_custom_role_key': 'My,Custom,Roles', + }) + with patch('django_auth_lti.middleware.settings', LTI_CUSTOM_ROLE_KEY='test_custom_role_key'): + self.mw.process_request(request) + self.assertEqual(request.LTI.get('roles'), ['RoleOne', 'RoleTwo', 'My', 'Custom', 'Roles']) + + @patch('django_auth_lti.middleware.auth') + def test_roles_merge_with_empty_custom_roles(self, mock_auth, mock_logger): + """ + Assert that 'roles' list in session contains original set when custom role key is defined with empty data. + """ + request = self.build_lti_launch_request({ + 'roles': 'RoleOne,RoleTwo', + 'test_custom_role_key': '', + }) + with patch('django_auth_lti.middleware.settings', LTI_CUSTOM_ROLE_KEY='test_custom_role_key'): + self.mw.process_request(request) + self.assertEqual(request.LTI.get('roles'), ['RoleOne', 'RoleTwo']) + + @patch('django_auth_lti.middleware.auth') + def test_roles_not_merged_with_no_role_key(self, mock_auth, mock_logger): + """ + Assert that 'roles' list in session contains original set when no custom role key is defined. + """ + request = self.build_lti_launch_request({ + 'roles': 'RoleOne,RoleTwo', + 'test_custom_role_key': 'My,Custom,Roles', + }) + self.mw.process_request(request) + self.assertEqual(request.LTI.get('roles'), ['RoleOne', 'RoleTwo']) diff --git a/tests/test_settings.py b/tests/test_settings.py new file mode 100644 index 0000000..bec6c3e --- /dev/null +++ b/tests/test_settings.py @@ -0,0 +1,31 @@ +import os + +# Build paths inside the project like this: os.path.join(BASE_DIR, ...) +BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + +SECRET_KEY = 'fake-key' + +INSTALLED_APPS = [ + 'django.contrib.auth', + 'django.contrib.contenttypes', + 'django.contrib.sessions', + 'django.contrib.messages', + 'django.contrib.staticfiles', + 'django_auth_lti', +] + +ROOT_URLCONF = 'tests.urls' + +TEMPLATES = [ + { + 'BACKEND': 'django.template.backends.django.DjangoTemplates', + 'APP_DIRS': True, + }, +] + +DATABASES = { + 'default': { + 'ENGINE': 'django.db.backends.sqlite3', + 'NAME': os.path.join(BASE_DIR, 'db.sqlite3'), + } +} \ No newline at end of file diff --git a/tests/test_verification.py b/tests/test_verification.py new file mode 100755 index 0000000..febcae7 --- /dev/null +++ b/tests/test_verification.py @@ -0,0 +1,36 @@ +from unittest import TestCase +from mock import MagicMock +from django_auth_lti.verification import is_allowed +from django.core.exceptions import ImproperlyConfigured, PermissionDenied + +class TestVerification(TestCase): + + def test_is_allowed_success(self): + request = MagicMock(LTI={"roles": ["admin"]}) + allowed_roles = ["admin", "student"] + user_is_allowed = is_allowed(request, allowed_roles, False) + self.assertTrue(user_is_allowed) + + def test_is_allowed_success_one_role(self): + request = MagicMock(LTI={"roles": ["admin"]}) + allowed_roles = "admin" + user_is_allowed = is_allowed(request, allowed_roles, False) + self.assertTrue(user_is_allowed) + + def test_is_allowed_failure(self): + request = MagicMock(LTI={"roles":[]}) + allowed_roles = ["admin", "student"] + user_is_allowed = is_allowed(request, allowed_roles, False) + self.assertFalse(user_is_allowed) + + def test_is_allowed_failure_one_role(self): + request = MagicMock(LTI={"roles":[]}) + allowed_roles = "admin" + user_is_allowed = is_allowed(request, allowed_roles, False) + self.assertFalse(user_is_allowed) + + def test_is_allowed_exception(self): + request = MagicMock(LTI={"roles":["TF"]}) + allowed_roles = ["admin", "student"] + self.assertRaises(PermissionDenied, is_allowed, + request, allowed_roles, True) diff --git a/tests/urls.py b/tests/urls.py new file mode 100644 index 0000000..eccbde3 --- /dev/null +++ b/tests/urls.py @@ -0,0 +1,5 @@ +# from django.conf.urls import url + +urlpatterns = [ + # url(r'^admin/', admin.site.urls), +] diff --git a/tox.ini b/tox.ini new file mode 100644 index 0000000..c6ab233 --- /dev/null +++ b/tox.ini @@ -0,0 +1,10 @@ +[tox] +envlist = + py27-django{18,19,110} +[testenv] +deps = + py27: mock + django18: Django >= 1.8, < 1.9 + django19: Django >= 1.9, < 1.10 + django110: Django >= 1.10, < 1.11 +commands = python run_tests.py From fe954e59da8f0614e0398e714ea129591aa61e1a Mon Sep 17 00:00:00 2001 From: "Bermudez, Jaime" Date: Wed, 15 Mar 2017 17:18:24 -0400 Subject: [PATCH 2/4] Fix monkey patching of reverse method to work with Django 1.10+ --- django_auth_lti/patch_reverse.py | 40 +++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/django_auth_lti/patch_reverse.py b/django_auth_lti/patch_reverse.py index 16427c8..a172bfb 100644 --- a/django_auth_lti/patch_reverse.py +++ b/django_auth_lti/patch_reverse.py @@ -1,15 +1,10 @@ """ -Monkey-patch django.core.urlresolvers.reverse to add resource_link_id to all URLs +Monkey-patch django's reverse function to add resource_link_id to all URLs. """ -import logging from urlparse import urlparse, urlunparse, parse_qs from urllib import urlencode -# Django 1.10 moved urlresolvers into django.urls package -try: - from django import urls as urlresolvers -except ImportError: - from django.core import urlresolvers +from django.core import urlresolvers from .thread_local import get_current_request @@ -20,26 +15,30 @@ def reverse(*args, **kwargs): """ - Call django's reverse function and append the current resource_link_id as a query parameter + Call django's reverse function and append the current resource_link_id as a + query parameter - :param kwargs['exclude_resource_link_id']: Do not add the resource link id as a query parameter + :param kwargs['exclude_resource_link_id']: Do not add the resource link id + as a query parameter :returns Django named url """ - logger.debug("inside custom reverse function!") request = get_current_request() - # Check for custom exclude_resource_link_id kwarg and remove it before passing kwargs to django reverse + # Check for custom exclude_resource_link_id kwarg and remove it before + # passing kwargs to django reverse exclude_resource_link_id = kwargs.pop('exclude_resource_link_id', False) url = django_reverse(*args, **kwargs) if not exclude_resource_link_id: - # Append resource_link_id query param if exclude_resource_link_id kwarg was not passed or is False + # Append resource_link_id query param if exclude_resource_link_id kwarg + # was not passed or is False parsed = urlparse(url) query = parse_qs(parsed.query) if 'resource_link_id' not in query.keys(): query['resource_link_id'] = request.LTI.get('resource_link_id') url = urlunparse( - (parsed.scheme, parsed.netloc, parsed.path, parsed.params, urlencode(query), parsed.fragment) + (parsed.scheme, parsed.netloc, parsed.path, parsed.params, + urlencode(query), parsed.fragment) ) return url @@ -50,9 +49,22 @@ def patch_reverse(): """ global django_reverse if urlresolvers.reverse is not reverse: - logger.debug("inside patch_reverse where urlresolvers.reverse != reverse") django_reverse = urlresolvers.reverse urlresolvers.reverse = reverse + # Django 1.10 moves url helper functions like `reverse` into a new urls + # module, so we need to patch it as well. In addition, the + # django.shortcuts module now includes `reverse` directly, and the + # module appears to be loaded before middleware so we need to + # retroactively patch that `reverse` reference as well. + try: + import django + from django import urls + + urls.reverse = reverse + django.shortcuts.reverse = reverse + except ImportError: + pass + patch_reverse() From 18b06a88668b33a9957d104c6f75ab60bbe2dcdc Mon Sep 17 00:00:00 2001 From: "Bermudez, Jaime" Date: Wed, 15 Mar 2017 17:21:25 -0400 Subject: [PATCH 3/4] Remove orphaned logger instance. --- django_auth_lti/patch_reverse.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/django_auth_lti/patch_reverse.py b/django_auth_lti/patch_reverse.py index a172bfb..b1bf507 100644 --- a/django_auth_lti/patch_reverse.py +++ b/django_auth_lti/patch_reverse.py @@ -8,8 +8,6 @@ from .thread_local import get_current_request -logger = logging.getLogger(__name__) - django_reverse = None From be632fe41cebbdcf4ef314c9f00baa008b93e4c9 Mon Sep 17 00:00:00 2001 From: "Bermudez, Jaime" Date: Thu, 16 Mar 2017 12:54:48 -0400 Subject: [PATCH 4/4] Remove unused urls module and `ROOT_URLCONF` setting from test_settings.py file. --- tests/test_settings.py | 4 +--- tests/urls.py | 5 ----- 2 files changed, 1 insertion(+), 8 deletions(-) delete mode 100644 tests/urls.py diff --git a/tests/test_settings.py b/tests/test_settings.py index bec6c3e..fb9a2ae 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -14,8 +14,6 @@ 'django_auth_lti', ] -ROOT_URLCONF = 'tests.urls' - TEMPLATES = [ { 'BACKEND': 'django.template.backends.django.DjangoTemplates', @@ -28,4 +26,4 @@ 'ENGINE': 'django.db.backends.sqlite3', 'NAME': os.path.join(BASE_DIR, 'db.sqlite3'), } -} \ No newline at end of file +} diff --git a/tests/urls.py b/tests/urls.py deleted file mode 100644 index eccbde3..0000000 --- a/tests/urls.py +++ /dev/null @@ -1,5 +0,0 @@ -# from django.conf.urls import url - -urlpatterns = [ - # url(r'^admin/', admin.site.urls), -]