diff --git a/CHANGELOG.rst b/CHANGELOG.rst index fbdbb611a3..fea8fd127a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Blades: Add role parameter to LTI. BLD-583. + Blades: Bugfix "In Firefox YouTube video with start time plays from 00:00:00". BLD-708. diff --git a/cms/djangoapps/contentstore/tests/test_access.py b/cms/djangoapps/contentstore/tests/test_access.py new file mode 100644 index 0000000000..63441d60f9 --- /dev/null +++ b/cms/djangoapps/contentstore/tests/test_access.py @@ -0,0 +1,42 @@ +""" +Tests access.py +""" +from django.test import TestCase +from django.contrib.auth.models import User +from xmodule.modulestore import Location + +from student.roles import CourseInstructorRole, CourseStaffRole +from student.tests.factories import AdminFactory +from student.auth import add_users +from contentstore.views.access import get_user_role + +class RolesTest(TestCase): + """ + Tests for user roles. + """ + def setUp(self): + """ Test case setup """ + self.global_admin = AdminFactory() + self.instructor = User.objects.create_user('testinstructor', 'testinstructor+courses@edx.org', 'foo') + self.staff = User.objects.create_user('teststaff', 'teststaff+courses@edx.org', 'foo') + self.location = Location('i4x', 'mitX', '101', 'course', 'test') + + def test_get_user_role_instructor(self): + """ + Verifies if user is instructor. + """ + add_users(self.global_admin, CourseInstructorRole(self.location), self.instructor) + self.assertEqual( + 'instructor', + get_user_role(self.instructor, self.location, self.location.course_id) + ) + + def test_get_user_role_staff(self): + """ + Verifies if user is staff. + """ + add_users(self.global_admin, CourseStaffRole(self.location), self.staff) + self.assertEqual( + 'staff', + get_user_role(self.staff, self.location, self.location.course_id) + ) diff --git a/cms/djangoapps/contentstore/views/access.py b/cms/djangoapps/contentstore/views/access.py index 215c8becf2..9ff328a898 100644 --- a/cms/djangoapps/contentstore/views/access.py +++ b/cms/djangoapps/contentstore/views/access.py @@ -1,6 +1,6 @@ from ..utils import get_course_location_for_item from xmodule.modulestore.locator import CourseLocator -from student.roles import CourseStaffRole, GlobalStaff +from student.roles import CourseStaffRole, GlobalStaff, CourseInstructorRole from student import auth @@ -20,3 +20,17 @@ def has_course_access(user, location, role=CourseStaffRole): # this can be expensive if location is not category=='course' location = get_course_location_for_item(location) return auth.has_access(user, role(location)) + + +def get_user_role(user, location, context): + """ + Return corresponding string if user has staff or instructor role in Studio. + This will not return student role because its purpose for using in Studio. + + :param location: a descriptor.location + :param context: a course_id + """ + if auth.has_access(user, CourseInstructorRole(location, context)): + return 'instructor' + else: + return 'staff' diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 3a8cba68f2..19b351d59c 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -26,6 +26,8 @@ from .session_kv_store import SessionKeyValueStore from .helpers import render_from_lms from ..utils import get_course_for_item +from contentstore.views.access import get_user_role + __all__ = ['preview_handler'] log = logging.getLogger(__name__) @@ -132,6 +134,7 @@ def _preview_module_system(request, descriptor): ), ), error_descriptor_class=ErrorDescriptor, + get_user_role=lambda: get_user_role(request.user, descriptor.location, course_id), ) diff --git a/common/lib/xmodule/xmodule/lti_module.py b/common/lib/xmodule/xmodule/lti_module.py index 7bf97654d6..f963c148a9 100644 --- a/common/lib/xmodule/xmodule/lti_module.py +++ b/common/lib/xmodule/xmodule/lti_module.py @@ -56,7 +56,6 @@ from pkg_resources import resource_string from xblock.core import String, Scope, List, XBlock from xblock.fields import Boolean, Float - log = logging.getLogger(__name__) @@ -328,6 +327,25 @@ class LTIModule(LTIFields, XModule): """ return u':'.join(urllib.quote(i) for i in (self.lti_id, self.get_resource_link_id(), self.get_user_id())) + def get_course(self): + """ + Return course by course id. + """ + course_location = CourseDescriptor.id_to_location(self.course_id) + course = self.descriptor.runtime.modulestore.get_item(course_location) + return course + + @property + def role(self): + """ + Get system user role and convert it to LTI role. + """ + roles = { + 'student': u'Student', + 'staff': u'Administrator', + 'instructor': u'Instructor', + } + return roles.get(self.system.get_user_role(), u'Student') def oauth_params(self, custom_parameters, client_key, client_secret): """ @@ -351,7 +369,7 @@ class LTIModule(LTIFields, XModule): u'launch_presentation_return_url': '', u'lti_message_type': u'basic-lti-launch-request', u'lti_version': 'LTI-1p0', - u'role': u'student', + u'roles': self.role, # Parameters required for grading: u'resource_link_id': self.get_resource_link_id(), @@ -607,10 +625,7 @@ oauth_consumer_key="", oauth_signature="frVp4JuvT1mVXlxktiAUjQ7%2F1cw%3D"'} """ Obtains client_key and client_secret credentials from current course. """ - course_id = self.course_id - course_location = CourseDescriptor.id_to_location(course_id) - course = self.descriptor.runtime.modulestore.get_item(course_location) - + course = self.get_course() for lti_passport in course.lti_passports: try: lti_id, key, secret = [i.strip() for i in lti_passport.split(':')] diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index 543d0eb095..caac60e71f 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -77,6 +77,7 @@ def get_test_system(course_id=''): open_ended_grading_interface=open_ended_grading_interface, course_id=course_id, error_descriptor_class=ErrorDescriptor, + get_user_role=Mock(is_staff=False), ) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index fde6cbd024..51d077c686 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1029,7 +1029,7 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable=abs open_ended_grading_interface=None, s3_interface=None, cache=None, can_execute_unsafe_code=None, replace_course_urls=None, replace_jump_to_id_urls=None, error_descriptor_class=None, get_real_user=None, - field_data=None, + field_data=None, get_user_role=None, **kwargs): """ Create a closure around the system environment. @@ -1082,6 +1082,9 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable=abs get_real_user - function that takes `anonymous_student_id` and returns real user_id, associated with `anonymous_student_id`. + get_user_role - A function that returns user role. Implementation is different + for LMS and Studio. + field_data - the `FieldData` to use for backing XBlock storage. """ @@ -1119,6 +1122,8 @@ class ModuleSystem(ConfigurableFragmentWrapper, Runtime): # pylint: disable=abs self.get_real_user = get_real_user + self.get_user_role = get_user_role + def get(self, attr): """ provide uniform access to attributes (like etree).""" return self.__dict__.get(attr) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 3a7de60ea7..e5cfc85038 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -466,3 +466,20 @@ def _has_staff_access_to_descriptor(user, descriptor, course_context=None): descriptor: something that has a location attribute """ return _has_staff_access_to_location(user, descriptor.location, course_context) + + +def get_user_role(user, course_id): + """ + Return corresponding string if user has staff, instructor or student + course role in LMS. + """ + from courseware.courses import get_course + course = get_course(course_id) + if is_masquerading_as_student(user): + return 'student' + elif has_access(user, course, 'instructor'): + return 'instructor' + elif has_access(user, course, 'staff'): + return 'staff' + else: + return 'student' diff --git a/lms/djangoapps/courseware/features/lti.feature b/lms/djangoapps/courseware/features/lti.feature index e004f4bd5b..9804d865c9 100644 --- a/lms/djangoapps/courseware/features/lti.feature +++ b/lms/djangoapps/courseware/features/lti.feature @@ -56,6 +56,17 @@ Feature: LMS.LTI component And I see in the gradebook table that "Total" is "5" #7 + Scenario: Graded LTI component in LMS role's masquerading correctly works + Given the course has correct LTI credentials with registered Instructor + And the course has an LTI component with correct fields: + | open_in_a_new_page | has_score | + | False | True | + And I view the LTI and it is rendered in iframe + And I see in iframe that LTI role is Instructor + And I switch to Student view + Then I see in iframe that LTI role is Student + + #8 Scenario: Graded LTI component in LMS is correctly works with beta testers Given the course has correct LTI credentials with registered BetaTester And the course has an LTI component with correct fields: @@ -65,5 +76,3 @@ Feature: LMS.LTI component And I click on the "Progress" tab Then I see text "Problem Scores: 5/10" And I see graph with total progress "5%" - - diff --git a/lms/djangoapps/courseware/features/lti.py b/lms/djangoapps/courseware/features/lti.py index d463eaace2..5c47c6ea85 100644 --- a/lms/djangoapps/courseware/features/lti.py +++ b/lms/djangoapps/courseware/features/lti.py @@ -1,9 +1,11 @@ #pylint: disable=C0111 - import datetime import os import pytz from mock import patch +from pytz import UTC +from nose.tools import assert_equal +from splinter.exceptions import ElementDoesNotExist from django.contrib.auth.models import User from django.core.urlresolvers import reverse @@ -12,9 +14,13 @@ from lettuce.django import django_url from common import course_id, visit_scenario_item from courseware.tests.factories import InstructorFactory, BetaTesterFactory + from courseware.access import has_access from student.tests.factories import UserFactory +from nose.tools import assert_equals +from common import course_id, visit_scenario_item + @step('I view the LTI and error is shown$') def lti_is_not_rendered(_step): @@ -66,6 +72,7 @@ def incorrect_lti_is_rendered(_step): assert world.is_css_present('iframe', wait_time=2) assert not world.is_css_present('.link_lti_new_window', wait_time=0) assert not world.is_css_present('.error_message', wait_time=0) + #inside iframe test content is presented check_lti_iframe_content("Wrong LTI signature") @@ -79,6 +86,7 @@ def set_correct_lti_passport(_step, user='Instructor'): world.lti_server.oauth_settings['client_secret'] )] } + i_am_registered_for_the_course(coursenum, metadata, user) @@ -91,8 +99,10 @@ def set_incorrect_lti_passport(_step): "incorrect_lti_secret_key" )] } + i_am_registered_for_the_course(coursenum, metadata) + @step('the course has an LTI component with (.*) fields(?:\:)?$') #, new_page is(.*), is_graded is(.*) def add_correct_lti_to_course(_step, fields): category = 'lti' @@ -100,6 +110,7 @@ def add_correct_lti_to_course(_step, fields): 'lti_id': 'correct_lti_id', 'launch_url': world.lti_server.oauth_settings['lti_base'] + world.lti_server.oauth_settings['lti_endpoint'], } + if fields.strip() == 'incorrect_lti_id': # incorrect fields metadata.update({ 'lti_id': 'incorrect_lti_id' @@ -132,7 +143,6 @@ def add_correct_lti_to_course(_step, fields): def create_course_for_lti(course, metadata): - # First clear the modulestore so we don't try to recreate # the same course twice # This also ensures that the necessary templates are loaded @@ -202,10 +212,12 @@ def i_am_registered_for_the_course(coursenum, metadata, user='Instructor'): user = BetaTesterFactory(course=course_location) normal_student = UserFactory() instructor = InstructorFactory(course=course_location) + assert not has_access(normal_student, course_descriptor, 'load') assert has_access(user, course_descriptor, 'load') assert has_access(instructor, course_descriptor, 'load') else: + metadata.update({'start': datetime.datetime(1970, 1, 1, tzinfo=UTC)}) create_course_for_lti(coursenum, metadata) course_descriptor = world.scenario_dict['COURSE'] course_location = world.scenario_dict['COURSE'].location @@ -214,6 +226,7 @@ def i_am_registered_for_the_course(coursenum, metadata, user='Instructor'): # Enroll the user in the course and log them in if has_access(user, course_descriptor, 'load'): world.enroll_user(user, course_id(coursenum)) + world.log_in(username=user.username, password='test') @@ -229,11 +242,11 @@ def check_lti_popup(): url = world.browser.url basename = os.path.basename(url) pathname = os.path.splitext(basename)[0] - if pathname == u'correct_lti_endpoint': break result = world.css_find('.result').first.text + assert result == u'This is LTI tool. Success.' world.browser.driver.close() # Close the pop-up window @@ -279,3 +292,26 @@ def click_grade(_step): iframe.find_by_name('submit-button').first.click() assert iframe.is_text_present('LTI consumer (edX) responded with XML content') + +@step('I see in iframe that LTI role is (.*)$') +def check_role(_step, role): + world.is_css_present('iframe') + location = world.scenario_dict['LTI'].location.html_id() + iframe_name = 'ltiFrame-' + location + with world.browser.get_iframe(iframe_name) as iframe: + expected_role = 'Role: ' + role + role = world.retry_on_exception( + lambda: iframe.find_by_tag('h5').first.value, + max_attempts=5, + ignored_exceptions=ElementDoesNotExist + ) + assert_equal(expected_role, role) + + +@step('I switch to (.*)$') +def switch_view(_step, view): + staff_status = world.css_find('#staffstatus').first + if staff_status.text != view: + world.css_click('#staffstatus') + world.wait_for_ajax_complete() + diff --git a/lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py b/lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py index 9b86592e16..235e2f851f 100644 --- a/lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py +++ b/lms/djangoapps/courseware/mock_lti_server/mock_lti_server.py @@ -185,7 +185,7 @@ class MockLTIRequestHandler(BaseHTTPRequestHandler): ''' self._send_head(status_code) if getattr(self.server, 'grade_data', False): # lti can be graded - url = "//{}:{}".format(self.server.server_host, self.server.server_port) + url = "//%s:%s" % self.server.server_address response_str = textwrap.dedent(""" @@ -196,13 +196,14 @@ class MockLTIRequestHandler(BaseHTTPRequestHandler):

Graded IFrame loaded

Server response is:

{}

+
Role: {role}
- """).format(message, url=url) + """).format(message, role=self.post_dict['roles'], url=url) else: # lti can't be graded response_str = textwrap.dedent(""" @@ -214,6 +215,7 @@ class MockLTIRequestHandler(BaseHTTPRequestHandler):

IFrame loaded

Server response is:

{}

+ diff --git a/lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py b/lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py index d6f3c5df3c..c60e095c15 100644 --- a/lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py +++ b/lms/djangoapps/courseware/mock_lti_server/test_mock_lti_server.py @@ -57,7 +57,7 @@ class MockLTIServerTest(unittest.TestCase): #wrong number of params and no signature payload = { 'user_id': 'default_user_id', - 'role': 'student', + 'roles': 'Student', 'oauth_nonce': '', 'oauth_timestamp': '', } @@ -73,7 +73,7 @@ class MockLTIServerTest(unittest.TestCase): """ payload = { 'user_id': 'default_user_id', - 'role': 'student', + 'roles': 'Student', 'oauth_nonce': '', 'oauth_timestamp': '', 'oauth_consumer_key': 'test_client_key', @@ -100,7 +100,7 @@ class MockLTIServerTest(unittest.TestCase): """ payload = { 'user_id': 'default_user_id', - 'role': 'student', + 'roles': 'Student', 'oauth_nonce': '', 'oauth_timestamp': '', 'oauth_consumer_key': 'test_client_key', @@ -126,7 +126,7 @@ class MockLTIServerTest(unittest.TestCase): payload = { 'user_id': 'default_user_id', - 'role': 'student', + 'roles': 'Student', 'oauth_nonce': '', 'oauth_timestamp': '', 'oauth_consumer_key': 'test_client_key', diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index a988b332bc..51f236adc7 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -17,7 +17,7 @@ import django.utils from django.views.decorators.csrf import csrf_exempt from capa.xqueue_interface import XQueueInterface -from courseware.access import has_access +from courseware.access import has_access, get_user_role from courseware.masquerade import setup_masquerade from courseware.model_data import FieldDataCache, DjangoKeyValueStore from lms.lib.xblock.field_data import LmsFieldData @@ -432,6 +432,7 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours # directly as the runtime i18n service. 'i18n': django.utils.translation, }, + get_user_role=lambda: get_user_role(user, course_id), ) # pass position specified in URL to module through ModuleSystem diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 3b24d5d4c7..29ef87b22d 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -102,3 +102,48 @@ class AccessTestCase(TestCase): def test__user_passed_as_none(self): """Ensure has_access handles a user being passed as null""" access.has_access(None, 'global', 'staff', None) + +class UserRoleTestCase(TestCase): + """ + Tests for user roles. + """ + def setUp(self): + self.course = Location('i4x://edX/toy/course/2012_Fall') + self.anonymous_user = AnonymousUserFactory() + self.student = UserFactory() + self.global_staff = UserFactory(is_staff=True) + self.course_staff = StaffFactory(course=self.course) + self.course_instructor = InstructorFactory(course=self.course) + + def test_user_role_staff(self): + """Ensure that user role is student for staff masqueraded as student.""" + self.assertEqual( + 'staff', + access.get_user_role(self.course_staff, self.course.course_id) + ) + # Masquerade staff + self.course_staff.masquerade_as_student = True + self.assertEqual( + 'student', + access.get_user_role(self.course_staff, self.course.course_id) + ) + + def test_user_role_instructor(self): + """Ensure that user role is student for instructor masqueraded as student.""" + self.assertEqual( + 'instructor', + access.get_user_role(self.course_instructor, self.course.course_id) + ) + # Masquerade instructor + self.course_instructor.masquerade_as_student = True + self.assertEqual( + 'student', + access.get_user_role(self.course_instructor, self.course.course_id) + ) + + def test_user_role_anonymous(self): + """Ensure that user role is student for anonymous user.""" + self.assertEqual( + 'student', + access.get_user_role(self.anonymous_user, self.course.course_id) + ) diff --git a/lms/djangoapps/courseware/tests/test_lti_integration.py b/lms/djangoapps/courseware/tests/test_lti_integration.py index 178cef57f1..eb04684e7f 100644 --- a/lms/djangoapps/courseware/tests/test_lti_integration.py +++ b/lms/djangoapps/courseware/tests/test_lti_integration.py @@ -43,7 +43,7 @@ class TestLTI(BaseTestXmodule): u'launch_presentation_return_url': '', u'lti_message_type': u'basic-lti-launch-request', u'lti_version': 'LTI-1p0', - u'role': u'student', + u'roles': u'Student', u'resource_link_id': module_id, u'lis_result_sourcedid': sourcedId,