Merge pull request #552 from edx-solutions/rc/2015-10-21

Rc/2015 10 21

common/djangoapps/third_party_auth/pipeline.py

@@ -535,7 +535,6 @@ def ensure_user_information(strategy, auth_entry, backend=None, user=None, socia
     Ensure that we have the necessary information about a user (either an
     existing account or registration data) to proceed with the pipeline.
     """
-
     # We're deliberately verbose here to make it clear what the intended
     # dispatch behavior is for the various pipeline entry points, given the
     # current state of the pipeline. Keep in mind the pipeline is re-entrant
@@ -725,7 +724,9 @@ def associate_by_email_if_login_api(auth_entry, backend, details, user, *args, *
 
     This association is done ONLY if the user entered the pipeline through a LOGIN API.
     """
-    if auth_entry == AUTH_ENTRY_LOGIN_API:
+    custom_auth_entry = AUTH_ENTRY_CUSTOM.get(auth_entry)
+
+    if auth_entry == AUTH_ENTRY_LOGIN_API or (custom_auth_entry and custom_auth_entry.get('link_by_email')):
         association_response = associate_by_email(backend, details, user, *args, **kwargs)
         if (
             association_response and

common/djangoapps/third_party_auth/tests/specs/test_testshib.py

@@ -8,14 +8,22 @@ from mock import patch
 from student.tests.factories import UserFactory
 from third_party_auth.tasks import fetch_saml_metadata
 from third_party_auth.tests import testutil
+from third_party_auth import pipeline
 import unittest
 
 TESTSHIB_ENTITY_ID = 'https://idp.testshib.org/idp/shibboleth'
 TESTSHIB_METADATA_URL = 'https://mock.testshib.org/metadata/testshib-providers.xml'
 TESTSHIB_SSO_URL = 'https://idp.testshib.org/idp/profile/SAML2/Redirect/SSO'
 
-TPA_TESTSHIB_LOGIN_URL = '/auth/login/tpa-saml/?auth_entry=login&next=%2Fdashboard&idp=testshib'
-TPA_TESTSHIB_REGISTER_URL = '/auth/login/tpa-saml/?auth_entry=register&next=%2Fdashboard&idp=testshib'
+
+def _make_entrypoint_url(auth_entry):
+    """
+    Builds TPA saml entrypoint with specified auth_entry value
+    """
+    return '/auth/login/tpa-saml/?auth_entry={auth_entry}&next=%2Fdashboard&idp=testshib'.format(auth_entry=auth_entry)
+
+TPA_TESTSHIB_LOGIN_URL = _make_entrypoint_url('login')
+TPA_TESTSHIB_REGISTER_URL = _make_entrypoint_url('register')
 TPA_TESTSHIB_COMPLETE_URL = '/auth/complete/tpa-saml/'
 
 
@@ -173,6 +181,54 @@ class TestShibIntegrationTest(testutil.SAMLTestCase):
 
         self._test_autoprovision(TPA_TESTSHIB_REGISTER_URL)
 
+    def test_custom_form_does_not_link_by_email(self):
+        self._configure_testshib_provider(autoprovision_account=False)
+        self._freeze_time(timestamp=1434326820)  # This is the time when the saved request/response was recorded.
+
+        email = 'myself@testshib.org'
+        UserFactory(username='myself', email=email, password='irrelevant')
+        self._verify_user_email(email)
+        self._assert_user_exists('myself', have_social=False)
+
+        custom_url = pipeline.get_login_url('saml-testshib', 'custom1')
+        self.client.get(custom_url)
+
+        testshib_response = self._fake_testshib_login_and_return()
+
+        # We should be redirected to the custom form since this account is not linked to an edX account, and
+        # automatic linking is not enabled for custom1 entrypoint:
+        self.assertEqual(testshib_response.status_code, 302)
+        self.assertEqual(testshib_response['Location'], self.url_prefix + '/auth/custom_auth_entry')
+
+    def test_custom_form_links_by_email(self):
+        self._configure_testshib_provider(autoprovision_account=False)
+        self._freeze_time(timestamp=1434326820)  # This is the time when the saved request/response was recorded.
+
+        email = 'myself@testshib.org'
+        UserFactory(username='myself', email=email, password='irrelevant')
+        self._verify_user_email(email)
+        self._assert_user_exists('myself', have_social=False)
+
+        custom_url = pipeline.get_login_url('saml-testshib', 'custom2')
+        self.client.get(custom_url)
+
+        testshib_response = self._fake_testshib_login_and_return()
+        # We should be redirected to TPA-complete endpoint
+        self.assertEqual(testshib_response.status_code, 302)
+        self.assertEqual(testshib_response['Location'], self.url_prefix + TPA_TESTSHIB_COMPLETE_URL)
+
+        complete_response = self.client.get(testshib_response['Location'])
+        # And we should be redirected to the dashboard
+        self.assertEqual(complete_response.status_code, 302)
+        self.assertEqual(complete_response['Location'], self.url_prefix + self.dashboard_page_url)
+
+        # And account should now be linked to social
+        self._assert_user_exists('myself', have_social=True)
+
+        # Now check that we can login again:
+        self.client.logout()
+        self._test_return_login()
+
     def _test_autoprovision(self, entry_point):
         """ Actual autoprovision code """
         # The user clicks on the TestShib button:
@@ -280,6 +336,20 @@ class TestShibIntegrationTest(testutil.SAMLTestCase):
         user.is_active = True
         user.save()
 
+    def _assert_user_exists(self, username, have_social=False, is_active=True):
+        """
+        Asserts user exists, checks activation status and social_auth links
+        """
+        user = User.objects.get(username=username)
+        self.assertEqual(user.is_active, is_active)
+        social_auths = user.social_auth.all()
+
+        if have_social:
+            self.assertEqual(1, len(social_auths))
+            self.assertEqual('tpa-saml', social_auths[0].provider)
+        else:
+            self.assertEqual(0, len(social_auths))
+
     def _assert_user_does_not_exist(self, username):
         """ Asserts that user with specified username does not exist """
         with self.assertRaises(User.DoesNotExist):

lms/djangoapps/api_manager/users/tests.py

@@ -374,15 +374,34 @@ class UsersApiTests(ModuleStoreTestCase):
     def test_user_list_post_duplicate(self):
         test_uri = self.users_base_uri
         local_username = self.test_username + str(randint(11, 99))
+
+        def post_duplicate_and_assert_409(email, username):
+            """
+            Posts user data with and asserts that return status code was 409 CONFLICT
+            """
+            data = {'email': email, 'username': username, 'password': self.test_password}
+            expected_message = "Username '{username}' or email '{email}' already exists".format(
+                username=username, email=email
+            )
+            response = self.do_post(test_uri, data)
+            self.assertEqual(response.status_code, 409)
+            self.assertEqual(response.data['message'], expected_message)
+            self.assertEqual(response.data['field_conflict'], 'username or email')
+
         data = {'email': self.test_email, 'username': local_username, 'password':
                 self.test_password, 'first_name': self.test_first_name, 'last_name': self.test_last_name}
         response = self.do_post(test_uri, data)
-        expected_message = "Username '{username}' or email '{email}' already exists".format(
-            username=local_username, email=self.test_email
-        )
-        self.assertEqual(response.status_code, 409)
-        self.assertEqual(response.data['message'], expected_message)
-        self.assertEqual(response.data['field_conflict'], 'username or email')
+        self.assertEqual(response.status_code, 201)
+
+        # try creating a user with same email and username
+        post_duplicate_and_assert_409(self.test_email, local_username)
+
+        # try creating a user with same username
+        post_duplicate_and_assert_409(str(uuid.uuid4()) + '@test.org', local_username)
+
+        # creating a user with same email - does not work, since auth_user table in test database does not have unique
+        # constraint on email, added by a migration in common/djangoapps/student/migrations/0004_add_email_index.py
+        # Test engine uses in-memory sqlite DB, so migrations are not applied to it
 
     @mock.patch.dict("student.models.settings.FEATURES", {"ENABLE_DISCUSSION_EMAIL_DIGEST": True})
     def test_user_list_post_discussion_digest_email(self):
@@ -840,6 +859,26 @@ class UsersApiTests(ModuleStoreTestCase):
         self.assertEqual(response.data['id'], unicode(self.course.id))
         self.assertTrue(response.data['is_active'])
 
+    def test_user_courses_list_post_duplicate(self):
+        # creating user
+        test_uri = self.users_base_uri
+        local_username = self.test_username + str(randint(11, 99))
+        data = {'email': self.test_email, 'username': local_username, 'password':
+                self.test_password, 'first_name': self.test_first_name, 'last_name': self.test_last_name}
+        response = self.do_post(test_uri, data)
+        user_id = response.data['id']
+
+        # adding it to a cohort
+        test_uri = '{}/{}/courses'.format(test_uri, str(user_id))
+        data = {'course_id': unicode(self.course.id)}
+        response = self.do_post(test_uri, data)
+        self.assertEqual(response.status_code, 201)
+
+        # and trying to add it second time
+        response = self.do_post(test_uri, data)
+        self.assertEqual(response.status_code, 409)
+        self.assertIn("already added to cohort", response.data['message'])
+
     def test_user_courses_list_post_undefined_user(self):
         course = CourseFactory.create(org='TUCLPUU', run='TUCLPUU1')
         test_uri = self.users_base_uri

lms/djangoapps/api_manager/users/views.py

@@ -34,7 +34,8 @@ from openedx.core.djangoapps.course_groups.cohorts import (
     get_cohort_by_name,
     add_cohort,
     add_user_to_cohort,
-    remove_user_from_cohort
+    remove_user_from_cohort,
+    AlreadyAddedToCohortException
 )
 from openedx.core.djangoapps.user_api.models import UserPreference
 from openedx.core.djangoapps.user_api.preferences.api import set_user_preference
@@ -769,7 +770,18 @@ class UsersCoursesList(SecureAPIView):
             default_cohort = get_cohort_by_name(course_key, CourseUserGroup.default_cohort_name)
         except CourseUserGroup.DoesNotExist:
             default_cohort = add_cohort(course_key, CourseUserGroup.default_cohort_name, CourseCohort.RANDOM)
-        add_user_to_cohort(default_cohort, user.username)
+        try:
+            add_user_to_cohort(default_cohort, user.username)
+        except AlreadyAddedToCohortException:
+            msg_tpl = _('Student {student} already added to cohort {cohort_name} for course {course}')
+            # pylint reports msg_tpl to not have `format` member, which is obviously a type resolution issue
+            # related - http://stackoverflow.com/questions/10025710/pylint-reports-as-not-callable
+            # pylint: disable=no-member
+            response_data = {
+                'message': msg_tpl.format(student=user.username, cohort_name=default_cohort.name, course=course_key)
+            }
+            return Response(response_data, status=status.HTTP_409_CONFLICT)
+
         log.debug('User "{}" has been automatically added in cohort "{}" for course "{}"'.format(
             user.username, default_cohort.name, course_descriptor.display_name)
         )

lms/envs/test.py

@@ -267,6 +267,12 @@ THIRD_PARTY_AUTH_CUSTOM_AUTH_FORMS = {
         'url': '/misc/my-custom-registration-form',
         'error_url': '/misc/my-custom-sso-error-page'
     },
+    'custom2': {
+        'secret_key': 'opensesame',
+        'url': '/misc/my-custom-registration-form',
+        'error_url': '/misc/my-custom-sso-error-page',
+        'link_by_email': True
+    },
 }
 
 ################################## OPENID #####################################

openedx/core/djangoapps/course_groups/cohorts.py

@@ -20,6 +20,13 @@ from student.models import get_user_by_username_or_email
 from .models import CourseUserGroup, CourseCohort, CourseCohortsSettings, CourseUserGroupPartitionGroup
 
 
+class AlreadyAddedToCohortException(ValueError):
+    """
+    Raised when an attempt is made to add user to a cohort he's already added to
+    """
+    pass
+
+
 log = logging.getLogger(__name__)
 
 
@@ -386,7 +393,7 @@ def add_user_to_cohort(cohort, username_or_email):
     )
     if course_cohorts.exists():
         if course_cohorts[0] == cohort:
-            raise ValueError("User {user_name} already present in cohort {cohort_name}".format(
+            raise AlreadyAddedToCohortException("User {user_name} already present in cohort {cohort_name}".format(
                 user_name=user.username,
                 cohort_name=cohort.name
             ))

openedx/core/djangoapps/course_groups/tests/test_cohorts.py

@@ -706,9 +706,9 @@ class TestCohorts(ModuleStoreTestCase):
             }
         )
         # Error cases
-        # Should get ValueError if user already in cohort
+        # Should get AlreadyAddedToCohortException if user already in cohort
         self.assertRaises(
-            ValueError,
+            cohorts.AlreadyAddedToCohortException,
             lambda: cohorts.add_user_to_cohort(second_cohort, "Username")
         )
         # UserDoesNotExist if user truly does not exist

requirements/edx/custom.txt

 # Custom requirements to be customized by individual OpenEdX instances
 
-# When updating a hash of an XBlock that uses xblock-utils, please update xblock-utils version hash here and in
-# github.txt as well. Deployments install custom.txt before github.txt and github.txt installs xblock-utils. This might
-# lead to installing an outdated version of xblock-utils and causing regressions. A note in github.txt is added to
-# keep xblock-utils version there in sync with this one.
+# When updating a hash of an XBlock that users xblock-utils, please update its version hash in github.txt
 -e git+https://github.com/edx/xblock-utils.git@3b58c757f06943072b170654d676e95b9adb37b0#egg=xblock-utils
 -e git+https://github.com/edx-solutions/xblock-mentoring.git@bd0b3f413ae7e8274985555adfd7de7af3eca84c#egg=xblock-mentoring
 -e git+https://github.com/edx-solutions/xblock-image-explorer.git@21b9bcc4f2c7917463ab18a596161ac6c58c9c4a#egg=xblock-image-explorer
@@ -14,7 +11,7 @@
 -e git+https://github.com/edx-solutions/xblock-adventure.git@effa22006bb6528bc6d3788787466eb4e74e1161#egg=xblock-adventure
 -e git+https://github.com/mckinseyacademy/xblock-poll.git@ca0e6eb4ef10c128d573c3cec015dcfee7984730#egg=xblock-poll
 -e git+https://github.com/edx/edx-notifications.git@275b8354593048ecae3e06642985b702b81140cc#egg=edx-notifications
--e git+https://github.com/open-craft/problem-builder.git@fa5d5e59133b2fd95ebea1aabcaa36578775eb21#egg=problem-builder
--e git+https://github.com/open-craft/xblock-group-project-v2.git@648c357c2b57fe6fa5ff68a0c29e6e72f309b9ca#egg=xblock-group-project-v2
+-e git+https://github.com/open-craft/problem-builder.git@c6e606027155d92ca78e96e6bc23ea86dcc588fc#egg=problem-builder
+-e git+https://github.com/open-craft/xblock-group-project-v2.git@533a3d70b8ff58af0c4f5e22abc674c60881cea3#egg=xblock-group-project-v2
 -e git+https://github.com/OfficeDev/xblock-officemix.git@86238f5968a08db005717dbddc346808f1ed3716#egg=xblock-officemix
 -e git+https://github.com/edx-solutions/xblock.git@80d11e883cb0f4b554e1e566294cb7de383cffed#egg=xblock

requirements/edx/github.txt

@@ -51,7 +51,7 @@ git+https://github.com/edx/ease.git@release-2015-07-14#egg=ease==0.1.3
 -e git+https://github.com/edx/edx-milestones.git@release-2015-06-17#egg=edx-milestones
 git+https://github.com/edx/edx-lint.git@ed8c8d2a0267d4d42f43642d193e25f8bd575d9b#egg=edx_lint==0.2.3
 # Note for the next rebase: custom.txt or one of XBlocks installed there might require a newer version of xblock-utils - please check versions
--e git+https://github.com/edx/xblock-utils.git@3b58c757f06943072b170654d676e95b9adb37b0#egg=xblock-utils
+-e git+https://github.com/edx/xblock-utils.git@588f7fd3ee88847c57cf09d10e81caa6b267ec51#egg=xblock-utils
 -e git+https://github.com/edx-solutions/xblock-google-drive.git@138e6fa0bf3a2013e904a085b9fed77dab7f3f21#egg=xblock-google-drive
 -e git+https://github.com/edx/edx-reverification-block.git@a286e89c73e1b788e35ac5b08a54b71a9fa63cfd#egg=edx-reverification-block
 git+https://github.com/edx/ecommerce-api-client.git@1.0.0#egg=ecommerce-api-client==1.0.0