diff --git a/cms/djangoapps/appsembler/tests/test_multi_tenant_with_login.py b/cms/djangoapps/appsembler/tests/test_multi_tenant_with_login.py index 741580587bcc..9ed17e3d11a2 100644 --- a/cms/djangoapps/appsembler/tests/test_multi_tenant_with_login.py +++ b/cms/djangoapps/appsembler/tests/test_multi_tenant_with_login.py @@ -1,25 +1,52 @@ """ Tests for APPSEMBLER_MULTI_TENANT_EMAILS in Studio login. + +Special note: + +This test module needs to patch `cms.urls.urlpatterns` to include urlpatterns +from `cms.djangoapps.appsembler.urls`. This works by overriding the +`doango.conf.settings.ROOT_URLCONF` with `django.test.utils.override_settings` +at the TestCase class level with the `urlpatterns` list declared in the module +containing the TestCase class. + +For this test module, we've added a `urlpatterns` module level variable and +assigned it the value of `cms.urls.urlpatterns` then appended the conditionally +included urlpatterns we need to run the tests. + +Then we add `@override_settings(ROOT_URLCONF=__name__)` to the TestClass + +There are other ways to do this. However, this is simple and does not require +our code to explicitly hack `sys.modules` reloading """ import ddt import pytest -from unittest import skipIf from django.core.exceptions import MultipleObjectsReturned +from django.conf.urls import include, url +from django.urls import reverse +from bs4 import BeautifulSoup as soup from mock import patch -from django.conf import settings from django.test import TestCase -from django.urls import reverse +from django.test.utils import override_settings from rest_framework import status from student.roles import CourseAccessRole, CourseCreatorRole, CourseInstructorRole, CourseStaffRole - from student.tests.factories import UserFactory +import cms.urls +from cms.djangoapps.appsembler.views import LoginView + + +# Set the urlpatterns we want to use for our tests in this module only +urlpatterns = cms.urls.urlpatterns + [ + url(r'', include('cms.djangoapps.appsembler.urls')) +] + @ddt.ddt -@skipIf(settings.TAHOE_TEMP_MONKEYPATCHING_JUNIPER_TESTS, 'RED-1571: Refactor') +@override_settings(ROOT_URLCONF=__name__) # the module that contains `urlpatterns` @patch.dict('django.conf.settings.FEATURES', {'APPSEMBLER_MULTI_TENANT_EMAILS': True}) +@patch.dict('django.conf.settings.FEATURES', {'TAHOE_STUDIO_LOCAL_LOGIN': True}) class MultiTenantStudioLoginTestCase(TestCase): """ Testing the APPSEMBLER_MULTI_TENANT_EMAILS feature when enabled in Studio. @@ -35,20 +62,12 @@ class MultiTenantStudioLoginTestCase(TestCase): def setUp(self): super(MultiTenantStudioLoginTestCase, self).setUp() - self.url = reverse('login_post') # CMS login endpoint. + self.url = reverse('login') self.customer = UserFactory.create(email=self.EMAIL, password=self.PASSWORD) - def test_login_with_course_creator_role(self): - """ - Test the APPSEMBLER_MULTI_TENANT_EMAILS feature when enabled in Studio for CourseCreatorRole. - """ - CourseAccessRole.objects.create(user=self.customer, role=CourseCreatorRole.ROLE) - response = self.client.post(self.url, { - 'email': self.EMAIL, - 'password': self.PASSWORD, - }) - assert response.status_code == status.HTTP_200_OK, response.content - assert response.json()['success'], response.content + def get_error_message_text(self, response): + return soup(response.content, + 'html.parser').find(id='login_error').p.get_text() def test_login_no_course_creator(self): """ @@ -59,23 +78,33 @@ def test_login_no_course_creator(self): 'password': self.PASSWORD, }) assert response.status_code == status.HTTP_200_OK, response.content - assert not response.json()['success'], response.content - assert response.json()['value'] == self.FAILURE_MESSAGE, response.content + # Assert we do NOT have a logged-in session (authorized user) + self.assertNotIn('_auth_user_id', self.client.session) + assert response['Content-Type'] == 'text/html; charset=utf-8' + error_message = self.get_error_message_text(response) + assert error_message == LoginView.error_messages['invalid_login'] - @ddt.data(CourseStaffRole.ROLE, CourseInstructorRole.ROLE) - def test_login_for_course_staff(self, course_role_name): + @ddt.data(CourseStaffRole.ROLE, CourseInstructorRole.ROLE, CourseCreatorRole.ROLE) + def test_login_for_course_access_role(self, course_role_name): """ - Test the APPSEMBLER_MULTI_TENANT_EMAILS feature when enabled in Studio for Course{Instructor,Staff}Role's. + Test the APPSEMBLER_MULTI_TENANT_EMAILS feature when enabled in Studio + for Course{Instructor,Staff}Role's. """ CourseAccessRole.objects.create(user=self.customer, role=course_role_name) response = self.client.post(self.url, { 'email': self.EMAIL, 'password': self.PASSWORD, }) - assert response.status_code == status.HTTP_200_OK, response.content - assert response.json()['success'], response.content - - @patch('student.views.login.log') + assert response.status_code == status.HTTP_302_FOUND, response.content + assert not response.content + new_url = response.url + response = self.client.get(new_url) + assert response.status_code == status.HTTP_200_OK + # Assert we DO have a logged-in session (authorized user) + self.assertIn('_auth_user_id', self.client.session) + assert response['Content-Type'] == 'text/html; charset=utf-8' + + @patch('cms.djangoapps.appsembler.views.logger') def test_error_on_two_emails_found(self, mock_log): """ Test that two users with CourseCreatorRole if found 500 shows up. @@ -97,35 +126,48 @@ def test_error_on_two_emails_found(self, mock_log): }) assert mock_log.exception.called, 'Should be called to log our custom message' - @ddt.data(CourseStaffRole.ROLE, CourseInstructorRole.ROLE) - def test_login_for_course_staff(self, course_role_name): + @pytest.mark.skip(reason="For now, we mandate Studio users have a unique email address") + def test_login_for_course_staff_but_learner_on_another_site_original(self): """ - Test the APPSEMBLER_MULTI_TENANT_EMAILS feature when enabled in Studio for Course{Instructor,Staff}Role's. + Test the login for a learner in a site but a staff in another. + + When APPSEMBLER_MULTI_TENANT_EMAILS feature when enabled in Studio """ - CourseAccessRole.objects.create(user=self.customer, role=course_role_name) + # Add a learner with the same email. + UserFactory.create(email=self.EMAIL, password='another_password') + + CourseAccessRole.objects.create(user=self.customer, role=CourseStaffRole.ROLE) response = self.client.post(self.url, { 'email': self.EMAIL, 'password': self.PASSWORD, }) - assert response.status_code == status.HTTP_200_OK, response.content - assert response.json()['success'], response.content - - def test_login_for_course_staff_but_learner_on_another_site(self): + assert response.status_code == status.HTTP_302_FOUND, response.content + assert not response.content + new_url = response.url + response = self.client.get(new_url) + assert response.status_code == status.HTTP_200_OK + # Assert we DO have a logged-in session (authorized user) + self.assertIn('_auth_user_id', self.client.session) + assert response['Content-Type'] == 'text/html; charset=utf-8' + + @patch('cms.djangoapps.appsembler.views.logger') + def test_login_for_course_staff_but_learner_on_another_site(self, mock_log): """ Test the login for a learner in a site but a staff in another. When APPSEMBLER_MULTI_TENANT_EMAILS feature when enabled in Studio """ # Add a learner with the same email. - _learner_2 = UserFactory.create(email=self.EMAIL, password='another_password') + UserFactory.create(email=self.EMAIL, password='another_password') CourseAccessRole.objects.create(user=self.customer, role=CourseStaffRole.ROLE) - response = self.client.post(self.url, { - 'email': self.EMAIL, - 'password': self.PASSWORD, - }) - assert response.status_code == status.HTTP_200_OK, response.content - assert response.json()['success'], response.content + assert not mock_log.exception.called, 'Not to be called yet' + with pytest.raises(MultipleObjectsReturned): + self.client.post(self.url, { + 'email': self.EMAIL, + 'password': self.PASSWORD, + }) + assert mock_log.exception.called, 'Should be called to log our custom message' def test_login_for_course_staff_in_two_courses(self): """ @@ -140,8 +182,14 @@ def test_login_for_course_staff_in_two_courses(self): 'email': self.EMAIL, 'password': self.PASSWORD, }) - assert response.status_code == status.HTTP_200_OK, response.content - assert response.json()['success'], response.content + assert response.status_code == status.HTTP_302_FOUND, response.content + assert not response.content + new_url = response.url + response = self.client.get(new_url) + assert response.status_code == status.HTTP_200_OK + # Assert we DO have a logged-in session (authorized user) + self.assertIn('_auth_user_id', self.client.session) + assert response['Content-Type'] == 'text/html; charset=utf-8' def test_failed_login(self): """ @@ -153,5 +201,8 @@ def test_failed_login(self): 'password': 'wrong_password', }) assert response.status_code == status.HTTP_200_OK, response.content - assert not response.json()['success'], response.content - assert response.json()['value'] == self.FAILURE_MESSAGE + # Assert we do NOT have a logged-in session (authorized user) + self.assertNotIn('_auth_user_id', self.client.session) + assert response['Content-Type'] == 'text/html; charset=utf-8' + error_message = self.get_error_message_text(response) + assert error_message == LoginView.error_messages['invalid_login'] diff --git a/cms/djangoapps/appsembler/views.py b/cms/djangoapps/appsembler/views.py index 394a037d0d34..a0e3c1b2bcf7 100644 --- a/cms/djangoapps/appsembler/views.py +++ b/cms/djangoapps/appsembler/views.py @@ -1,36 +1,46 @@ """Appsembler custom views for Studio -Views here provide Studio local login/logout +This module contains LoginView and support functions to enable local +login from Studio in MTE mode + +See the LoginView class docstring for details on this class """ +import logging from django.conf import settings -from django.contrib.auth import authenticate, login +from django.contrib.auth import authenticate, get_user_model, login +from django.http import HttpResponseServerError from django.shortcuts import redirect -from django.views import View -from django.views.decorators.clickjacking import xframe_options_deny -from django.views.decorators.csrf import ensure_csrf_cookie from django.urls import reverse from django.utils.decorators import method_decorator +from django.utils.translation import ugettext as _ +from django.views import View +from django.views.decorators.clickjacking import xframe_options_deny +from django.views.decorators.csrf import csrf_protect from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -from openedx.core.djangoapps.user_authn.views.login import _get_user_by_email - +from student.models import CourseAccessRole +from student.roles import CourseCreatorRole, CourseInstructorRole, CourseStaffRole from edxmako.shortcuts import render_to_response +logger = logging.getLogger(__name__) + + def forgot_password_link(): return "//{base}/login#forgot-password-modal".format(base=settings.LMS_BASE) def platform_name(): - return configuration_helpers.get_value('platform_name', settings.PLATFORM_NAME) + return configuration_helpers.get_value('platform_name', + settings.PLATFORM_NAME) -def render_login_page(show_login_error_message=False): - """Convenience function to put the login page +def render_login_page(login_error_message=None): + """Convenience function to render the login page Arguments: - show_login_error_message (bool): flag to show if a login attempt failed + login_error_message (str): error message to show. Doesn't show if None Returns: django.http.response.HttpResponse object with the login page content @@ -38,32 +48,122 @@ def render_login_page(show_login_error_message=False): return render_to_response( 'login_page.html', { - 'show_login_error_message': show_login_error_message, + 'login_error_message': login_error_message, 'forgot_password_link': forgot_password_link(), 'platform_name': platform_name(), } ) +def has_course_access_role(user): + """Checks for account authorization to use Studio + + Arguments: user record for the account to check + + Returns: + True if the account has a Studio authorized course access role + False if the account does not have a Studio authorized course access + role + """ + return CourseAccessRole.objects.filter( + user_id=user.id, + role__in=[ + CourseCreatorRole.ROLE, + CourseInstructorRole.ROLE, + CourseStaffRole.ROLE, + ]).exists() + + +def is_global_admin(user): + """Checks for global authorization (is_staff or is_superuser) + """ + return user.is_staff or user.is_superuser + + class LoginView(View): - """Basic login view to allow for Studio local logins + """Basic login view class to allow for Studio local logins + + Allows a user account to log in to Studio using an email address and + password under the following conditions: + + 1. The email address is associated with only one user account + 2. The user account has a Studio authorized course access group role OR + The user account has global staff or superuser privileges + + ## Tech Debt Note: + + Refactor this into a FormView class so we can shift the validation into a + Form based class. Ideally we may be able to extend Django's `LoginView` + and/or `AuthenticationForm` classes where we are replacing username with + email for the form and using our authorization code + + By layering custom authorization on top of existing Django class based login + code, we should be able to reduce the size of this class (less of our code) + and rely on the platform more for security. """ - @method_decorator(ensure_csrf_cookie) + error_messages = { + 'invalid_login': _( + 'Email or password is incorrect. ' + 'Please ensure that you are a course staff in order to use Studio.' + ), + } + + @method_decorator(csrf_protect) @method_decorator(xframe_options_deny) def get(self, request): return render_login_page() - @method_decorator(ensure_csrf_cookie) - def post(self, request, *args, **kwargs): - user = _get_user_by_email(request) - password = request.POST['password'] - - if user: - user = authenticate(request, username=user.username, password=password) - - if not user: - return render_login_page(show_login_error_message=True) - - login(request, user) - return redirect(reverse('home')) + @method_decorator(csrf_protect) + def post(self, request): + + if 'email' not in request.POST or 'password' not in request.POST: + # Expected fields in the post are missing + logger.exception('Missing form data from Studio login form page') + return HttpResponseServerError() + + user_model = get_user_model() + try: + user = user_model.objects.get(email=self.request.POST['email']) + password = self.request.POST['password'] + + user = authenticate(self.request, + username=user.username, + password=password) + if not user: + return self.render_login_page_with_error('invalid_login') + # So we actually have a user at this point who has authenticated + # Now see if the user has authorization + if not (is_global_admin(user) or has_course_access_role(user)): + return self.render_login_page_with_error('invalid_login') + + login(request, user) + return redirect(reverse('home')) + + # Copy/paste/reformat from Tahoe Hawthorn common/student/views/login.py + except user_model.MultipleObjectsReturned: + self.log_multiple_objects_returned() + # Raise the exception again. + # Not very friendly but allows us to identify properly if enough + # issues were reported instead of a silent error + raise + + except user_model.DoesNotExist: + return self.render_login_page_with_error('invalid_login') + + def log_multiple_objects_returned(self): + if settings.FEATURES.get('SQUELCH_PII_IN_LOGS'): + email = '' + else: + email = self.request.POST['email'] + + logger.exception( + 'Studio Multi-Tenant Emails error: More than one user were ' + 'found with the same email. ' + 'Please change to a different email on either one of the ' + 'accounts: {email}'.format(email=email) + ) + + def render_login_page_with_error(self, error_code): + return render_login_page( + login_error_message=self.error_messages[error_code]) diff --git a/cms/templates/login_page.html b/cms/templates/login_page.html index b22ac096063c..817499308ea6 100644 --- a/cms/templates/login_page.html +++ b/cms/templates/login_page.html @@ -19,11 +19,11 @@

${_("Sign In to {studio_name}").format(studio_name=set
- %if show_login_error_message: + %if login_error_message:
- ${_("Email or password is incorrect. Please ensure that you are a course staff in order to use Studio.")} +

${_(login_error_message)}

%endif