From a155aad01c6f112b62fc538eea6a518926dd5546 Mon Sep 17 00:00:00 2001 From: George Babey Date: Fri, 13 Apr 2018 18:42:23 -0400 Subject: [PATCH] Revert "Show Enterprise display name if we should hide sensitive username." This reverts commit d58bd06928c3c234e94cab24cb1e7971906067c1. This commit caused performance issues due to increased API load from checking the enterprise association on each page load --- common/djangoapps/util/views.py | 2 +- lms/djangoapps/course_wiki/tests/tests.py | 6 +- .../courseware/tests/test_course_info.py | 6 +- lms/djangoapps/courseware/tests/test_views.py | 6 +- lms/djangoapps/discussion/tests/test_views.py | 6 +- lms/djangoapps/support/views/contact_us.py | 2 +- lms/templates/courseware/progress.html | 9 +- lms/templates/header/user_dropdown.html | 19 +-- lms/templates/user_dropdown.html | 16 +-- openedx/features/enterprise_support/api.py | 121 +++++++++--------- .../tests/mixins/enterprise.py | 5 - openedx/features/enterprise_support/utils.py | 17 --- 12 files changed, 86 insertions(+), 129 deletions(-) diff --git a/common/djangoapps/util/views.py b/common/djangoapps/util/views.py index e8c039e7b0..cdf5f860a3 100644 --- a/common/djangoapps/util/views.py +++ b/common/djangoapps/util/views.py @@ -455,7 +455,7 @@ def submit_feedback(request): context = get_feedback_form_context(request) #Update the tag info with 'enterprise_learner' if the user belongs to an enterprise customer. - enterprise_learner_data = enterprise_api.get_enterprise_learner_data(request.user) + enterprise_learner_data = enterprise_api.get_enterprise_learner_data(site=request.site, user=request.user) if enterprise_learner_data: context["tags"]["learner_type"] = "enterprise_learner" diff --git a/lms/djangoapps/course_wiki/tests/tests.py b/lms/djangoapps/course_wiki/tests/tests.py index a18f051a45..8328880c83 100644 --- a/lms/djangoapps/course_wiki/tests/tests.py +++ b/lms/djangoapps/course_wiki/tests/tests.py @@ -205,14 +205,10 @@ class WikiRedirectTestCase(EnterpriseTestConsentRequired, LoginEnrollmentTestCas self.assertEqual(resp.status_code, 200) @patch.dict("django.conf.settings.FEATURES", {'ALLOW_WIKI_ROOT_ACCESS': True}) - @patch('openedx.features.enterprise_support.api.enterprise_customer_for_request') - def test_consent_required(self, mock_enterprise_customer_for_request): + def test_consent_required(self): """ Test that enterprise data sharing consent is required when enabled for the various courseware views. """ - # ENT-924: Temporary solution to replace sensitive SSO usernames. - mock_enterprise_customer_for_request.return_value = None - # Public wikis can be accessed by non-enrolled users, and so direct access is not gated by the consent page course = CourseFactory.create() course.allow_public_wiki_access = False diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index 04ec1bf688..168276894b 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -68,16 +68,12 @@ class CourseInfoTestCase(EnterpriseTestConsentRequired, LoginEnrollmentTestCase, self.assertNotIn("You are not currently enrolled in this course", resp.content) # TODO: LEARNER-611: If this is only tested under Course Info, does this need to move? - @mock.patch('openedx.features.enterprise_support.api.enterprise_customer_for_request') - def test_redirection_missing_enterprise_consent(self, mock_enterprise_customer_for_request): + def test_redirection_missing_enterprise_consent(self): """ Verify that users viewing the course info who are enrolled, but have not provided data sharing consent, are first redirected to a consent page, and then, once they've provided consent, are able to view the course info. """ - # ENT-924: Temporary solution to replace sensitive SSO usernames. - mock_enterprise_customer_for_request.return_value = None - self.setup_user() self.enroll(self.course) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index d304b427f6..91d22ce92c 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -2596,14 +2596,10 @@ class EnterpriseConsentTestCase(EnterpriseTestConsentRequired, ModuleStoreTestCa CourseOverview.load_from_module_store(self.course.id) CourseEnrollmentFactory(user=self.user, course_id=self.course.id) - @patch('openedx.features.enterprise_support.api.enterprise_customer_for_request') - def test_consent_required(self, mock_enterprise_customer_for_request): + def test_consent_required(self): """ Test that enterprise data sharing consent is required when enabled for the various courseware views. """ - # ENT-924: Temporary solution to replace sensitive SSO usernames. - mock_enterprise_customer_for_request.return_value = None - course_id = unicode(self.course.id) for url in ( reverse("courseware", kwargs=dict(course_id=course_id)), diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index a3448bacb2..eaa16956d1 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -1659,14 +1659,10 @@ class EnterpriseConsentTestCase(EnterpriseTestConsentRequired, ForumsEnableMixin self.addCleanup(translation.deactivate) - @patch('openedx.features.enterprise_support.api.enterprise_customer_for_request') - def test_consent_required(self, mock_enterprise_customer_for_request, mock_request): + def test_consent_required(self, mock_request): """ Test that enterprise data sharing consent is required when enabled for the various discussion views. """ - # ENT-924: Temporary solution to replace sensitive SSO usernames. - mock_enterprise_customer_for_request.return_value = None - thread_id = 'dummy' course_id = unicode(self.course.id) mock_request.side_effect = make_mock_request_impl(course=self.course, text='dummy', thread_id=thread_id) diff --git a/lms/djangoapps/support/views/contact_us.py b/lms/djangoapps/support/views/contact_us.py index 197a8bac7a..b8753e3448 100644 --- a/lms/djangoapps/support/views/contact_us.py +++ b/lms/djangoapps/support/views/contact_us.py @@ -33,7 +33,7 @@ class ContactUsView(View): if request.user.is_authenticated(): context['user_enrollments'] = CourseEnrollment.enrollments_for_user_with_overviews_preload(request.user) - enterprise_learner_data = enterprise_api.get_enterprise_learner_data(request.user) + enterprise_learner_data = enterprise_api.get_enterprise_learner_data(site=request.site, user=request.user) if enterprise_learner_data: tags.append('enterprise_learner') diff --git a/lms/templates/courseware/progress.html b/lms/templates/courseware/progress.html index f80738146b..0945c6b5f5 100644 --- a/lms/templates/courseware/progress.html +++ b/lms/templates/courseware/progress.html @@ -11,14 +11,7 @@ from django.core.urlresolvers import reverse from django.conf import settings from django.utils.http import urlquote_plus from six import text_type - -from openedx.features.enterprise_support.utils import get_enterprise_learner_generic_name %> - -<% -username = get_enterprise_learner_generic_name(request) or student.username -%> - <%block name="bodyclass">view-in-course view-progress <%block name="headextra"> @@ -61,7 +54,7 @@ username = get_enterprise_learner_generic_name(request) or student.username % endif

- ${_("Course Progress for Student '{username}' ({email})").format(username=username, email=student.email)} + ${_("Course Progress for Student '{username}' ({email})").format(username=student.username, email=student.email)}

diff --git a/lms/templates/header/user_dropdown.html b/lms/templates/header/user_dropdown.html index 73bb1e8820..9d1c04c791 100644 --- a/lms/templates/header/user_dropdown.html +++ b/lms/templates/header/user_dropdown.html @@ -2,29 +2,32 @@ <%page expression_filter="h"/> <%namespace name='static' file='static_content.html'/> +## This template should not use the target student's details when masquerading, see TNL-4895 +<% +self.real_user = getattr(user, 'real_user', user) +%> + <%! from django.core.urlresolvers import reverse from django.utils.translation import ugettext as _ from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_urls_for_user from openedx.core.djangoapps.user_api.accounts.utils import retrieve_last_sitewide_block_completed -from openedx.features.enterprise_support.utils import get_enterprise_learner_generic_name + %> <% -## This template should not use the target student's details when masquerading, see TNL-4895 -self.real_user = getattr(user, 'real_user', user) -profile_image_url = get_profile_image_urls_for_user(self.real_user)['medium'] -username = self.real_user.username -resume_block = retrieve_last_sitewide_block_completed(username) -displayname = get_enterprise_learner_generic_name(request) or username + profile_image_url = get_profile_image_urls_for_user(self.real_user)['medium'] + username = self.real_user.username + resume_block = retrieve_last_sitewide_block_completed(username) %> +
${_("Dashboard for:")} - ${displayname} + ${username}
diff --git a/lms/templates/user_dropdown.html b/lms/templates/user_dropdown.html index 1deddfac92..64049f3f21 100644 --- a/lms/templates/user_dropdown.html +++ b/lms/templates/user_dropdown.html @@ -2,21 +2,19 @@ <%page expression_filter="h"/> <%namespace name='static' file='static_content.html'/> +## This template should not use the target student's details when masquerading, see TNL-4895 +<% +self.real_user = getattr(user, 'real_user', user) +username = self.real_user.username +profile_image_url = get_profile_image_urls_for_user(self.real_user)['medium'] +%> + <%! from django.core.urlresolvers import reverse from django.utils.translation import ugettext as _ from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_urls_for_user -from openedx.features.enterprise_support.utils import get_enterprise_learner_generic_name %> - -<% -## This template should not use the target student's details when masquerading, see TNL-4895 -self.real_user = getattr(user, 'real_user', user) -username = get_enterprise_learner_generic_name(request) or self.real_user.username -profile_image_url = get_profile_image_urls_for_user(self.real_user)['medium'] -%> - % if uses_bootstrap:
diff --git a/openedx/features/enterprise_support/api.py b/openedx/features/enterprise_support/api.py index f558c87ed1..b5ab3d5da6 100644 --- a/openedx/features/enterprise_support/api.py +++ b/openedx/features/enterprise_support/api.py @@ -161,72 +161,73 @@ class EnterpriseApiClient(object): LOGGER.exception(message) raise EnterpriseApiException(message) - def fetch_enterprise_learner_data(self, user): + def fetch_enterprise_learner_data(self, site, user): # pylint: disable=unused-argument """ Fetch information related to enterprise from the Enterprise Service. Example: - fetch_enterprise_learner_data(user) + fetch_enterprise_learner_data(site, user) Argument: + site: (Site) site instance user: (User) django auth user Returns: - dict: - { - "count": 1, - "num_pages": 1, - "current_page": 1, - "next": null, - "start": 0, - "previous": null - "results": [ - { - "enterprise_customer": { - "uuid": "cf246b88-d5f6-4908-a522-fc307e0b0c59", - "name": "TestShib", - "catalog": 2, - "active": true, - "site": { - "domain": "example.com", - "name": "example.com" - }, - "enable_data_sharing_consent": true, - "enforce_data_sharing_consent": "at_login", - "branding_configuration": { - "enterprise_customer": "cf246b88-d5f6-4908-a522-fc307e0b0c59", - "logo": "https://open.edx.org/sites/all/themes/edx_open/logo.png" - }, - "enterprise_customer_entitlements": [ - { + dict: { + "enterprise_api_response_for_learner": { + "count": 1, + "num_pages": 1, + "current_page": 1, + "results": [ + { + "enterprise_customer": { + "uuid": "cf246b88-d5f6-4908-a522-fc307e0b0c59", + "name": "TestShib", + "catalog": 2, + "active": true, + "site": { + "domain": "example.com", + "name": "example.com" + }, + "enable_data_sharing_consent": true, + "enforce_data_sharing_consent": "at_login", + "branding_configuration": { "enterprise_customer": "cf246b88-d5f6-4908-a522-fc307e0b0c59", - "entitlement_id": 69 - } - ], - "replace_sensitive_sso_username": False, - }, - "user_id": 5, - "user": { - "username": "staff", - "first_name": "", - "last_name": "", - "email": "staff@example.com", - "is_staff": true, - "is_active": true, - "date_joined": "2016-09-01T19:18:26.026495Z" - }, - "data_sharing_consent_records": [ - { + "logo": "https://open.edx.org/sites/all/themes/edx_open/logo.png" + }, + "enterprise_customer_entitlements": [ + { + "enterprise_customer": "cf246b88-d5f6-4908-a522-fc307e0b0c59", + "entitlement_id": 69 + } + ] + }, + "user_id": 5, + "user": { "username": "staff", - "enterprise_customer_uuid": "cf246b88-d5f6-4908-a522-fc307e0b0c59", - "exists": true, - "course_id": "course-v1:edX DemoX Demo_Course", - "consent_provided": true, - "consent_required": false - } - ] - } - ], + "first_name": "", + "last_name": "", + "email": "staff@example.com", + "is_staff": true, + "is_active": true, + "date_joined": "2016-09-01T19:18:26.026495Z" + }, + "data_sharing_consent_records": [ + { + "username": "staff", + "enterprise_customer_uuid": "cf246b88-d5f6-4908-a522-fc307e0b0c59", + "exists": true, + "course_id": "course-v1:edX DemoX Demo_Course", + "consent_provided": true, + "consent_required": false + } + ] + } + ], + "next": null, + "start": 0, + "previous": null + } } Raises: @@ -365,7 +366,7 @@ def enterprise_customer_uuid_for_request(request): if not enterprise_customer_uuid and request.user.is_authenticated(): # If there's no way to get an Enterprise UUID for the request, check to see # if there's already an Enterprise attached to the requesting user on the backend. - learner_data = get_enterprise_learner_data(request.user) + learner_data = get_enterprise_learner_data(request.site, request.user) if learner_data: enterprise_customer_uuid = learner_data[0]['enterprise_customer']['uuid'] @@ -408,7 +409,7 @@ def consent_needed_for_course(request, user, course_id, enrollment_exists=False) if request.session.get(consent_key) is False: return False - enterprise_learner_details = get_enterprise_learner_data(user) + enterprise_learner_details = get_enterprise_learner_data(request.site, user) if not enterprise_learner_details: consent_needed = False else: @@ -498,14 +499,14 @@ def get_enterprise_consent_url(request, course_id, user=None, return_to=None, en return full_url -def get_enterprise_learner_data(user): +def get_enterprise_learner_data(site, user): """ Client API operation adapter/wrapper """ if not enterprise_enabled(): return None - enterprise_learner_data = EnterpriseApiClient(user=user).fetch_enterprise_learner_data(user) + enterprise_learner_data = EnterpriseApiClient(user=user).fetch_enterprise_learner_data(site=site, user=user) if enterprise_learner_data: return enterprise_learner_data['results'] @@ -517,7 +518,7 @@ def get_enterprise_customer_for_learner(site, user): if not enterprise_enabled(): return {} - enterprise_learner_data = get_enterprise_learner_data(user) + enterprise_learner_data = get_enterprise_learner_data(site, user) if enterprise_learner_data: return enterprise_learner_data[0]['enterprise_customer'] diff --git a/openedx/features/enterprise_support/tests/mixins/enterprise.py b/openedx/features/enterprise_support/tests/mixins/enterprise.py index 2479b2b55b..d628640d78 100644 --- a/openedx/features/enterprise_support/tests/mixins/enterprise.py +++ b/openedx/features/enterprise_support/tests/mixins/enterprise.py @@ -220,7 +220,6 @@ class EnterpriseTestConsentRequired(SimpleTestCase): Mixin to help test the data_sharing_consent_required decorator. """ - @mock.patch('openedx.features.enterprise_support.utils.get_enterprise_learner_generic_name') @mock.patch('openedx.features.enterprise_support.api.enterprise_customer_uuid_for_request') @mock.patch('openedx.features.enterprise_support.api.reverse') @mock.patch('openedx.features.enterprise_support.api.enterprise_enabled') @@ -233,7 +232,6 @@ class EnterpriseTestConsentRequired(SimpleTestCase): mock_enterprise_enabled, mock_reverse, mock_enterprise_customer_uuid_for_request, - mock_get_enterprise_learner_generic_name, status_code=200, ): """ @@ -246,9 +244,6 @@ class EnterpriseTestConsentRequired(SimpleTestCase): return '/enterprise/grant_data_sharing_permissions' return reverse(*args, **kwargs) - # ENT-924: Temporary solution to replace sensitive SSO usernames. - mock_get_enterprise_learner_generic_name.return_value = '' - mock_reverse.side_effect = mock_consent_reverse mock_enterprise_enabled.return_value = True mock_enterprise_customer_uuid_for_request.return_value = 'fake-uuid' diff --git a/openedx/features/enterprise_support/utils.py b/openedx/features/enterprise_support/utils.py index 68db78692a..9ba7714114 100644 --- a/openedx/features/enterprise_support/utils.py +++ b/openedx/features/enterprise_support/utils.py @@ -220,20 +220,3 @@ def update_account_settings_context_for_enterprise(context, enterprise_customer) enterprise_context['sync_learner_profile_data'] = identity_provider.sync_learner_profile_data context.update(enterprise_context) - - -def get_enterprise_learner_generic_name(request): - """ - Get a generic name concatenating the Enterprise Customer name and 'Learner'. - - ENT-924: Temporary solution for hiding potentially sensitive SSO names. - When a more complete solution is put in place, delete this function and all of its uses. - """ - # Prevent a circular import. This function makes sense to be in this module though. And see function description. - from openedx.features.enterprise_support.api import enterprise_customer_for_request - enterprise_customer = enterprise_customer_for_request(request) - return ( - enterprise_customer['name'] + 'Learner' - if enterprise_customer and enterprise_customer['replace_sensitive_sso_username'] - else '' - )