User Authn: Remove deprecated, ENABLE_COMBINED_LOGIN_REGISTRATION #22086

nasthagiri · 2019-10-19T17:33:30Z

This PR removes an older implementation of the login and registration APIs. They were moved to openedx/core/djangoapps/user_authn/views/deprecated.py as part of a past login/authentication refactor PR.

This PR also removes the ENABLE_COMBINED_LOGIN_REGISTRATION Django setting, as part of our effort to consolidate towards a single implementation.

Further removal and refactor will follow after removal of this earlier version.

nasthagiri · 2019-10-21T13:37:45Z

common/djangoapps/student/tests/test_long_username_email.py

        obj = json.loads(response.content.decode('utf-8'))
        self.assertEqual(
-            obj['value'],
+            obj['username'][0]['user_message'],


Note to reviewers: These test changes are needed since the old registration API returned the validation error in a value field instead of the new format of field_name (e.g., username) and user_message.

nasthagiri · 2019-10-21T13:41:57Z

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

        # Check that the correct provider was selected.
        self.assertContains(
            response,
-            u'successfully signed in with <strong>%s</strong>' % self.provider.name,


Note to reviewers: Since the new implementation presents this string via Javascript in Backbone, we check for the field value in the response instead.

nasthagiri · 2019-10-21T13:44:10Z

openedx/core/djangoapps/user_authn/views/login_form.py

-                    )
-                third_party_auth_hint = provider_id
-                initial_mode = "hinted_login"
+            if 'tpa_hint' in next_args:


Note to reviewers: I added this extra if statement here since it is an expected condition for tpa_hint to not be included as a parameter. We receive on the order 100k exceptions a day due to this lack of check.

nasthagiri · 2019-10-21T13:45:27Z

openedx/core/djangoapps/user_api/views.py

        if data.get("honor_code") and "terms_of_service" not in data:
            data["terms_of_service"] = data["honor_code"]

+    def _get_third_party_auth_redirect_url(self, request):


Note to reviewers: I added a redirect_url to the new registration response in order to make old tests pass. I am currently testing removal of this (for now) to see if this addition is actually needed in this cleanup task.

Testing removal of redirect_url in this PR: https://github.com/edx/edx-platform/pull/22090

This is the only weird part of this PR, because everything is about removal (or refactor), but this introduces a change to an existing API that is already in use.

It would be cleanest if this code weren't needed in this PR. If it is, I would comment the code (in addition to the comments in the PR) to clarify that this code is not used and is just required temporarily for old tests that will be removed.

Update: The third_party_auth...test_full_pipeline_succeeds_registering_new_account test currently requires the redirect_url field in the registration response.

I’m missing something. We had two views and you are removing The deprecated one that had this field, but the newer did not.

Does that mean we had a bug in the newer and just never tested it? Or is this testing something that we don’t need? Or other?

robrap

Nice. Thank you. I especially appreciate you keeping it small even though there is much more to do. It makes it simpler in pieces.

The only potential blocker is double-checking White Label (see comment below).

robrap · 2019-10-21T14:04:02Z

common/djangoapps/student/tests/test_password_policy.py

+            "This password must contain at least 3 punctuation marks.",
+        ]
+        for i in range(3):
+            self.assertEqual(obj['password'][i]['user_message'], error_strings[i])


Minor: you could add an assertion around length to ensure no additional errors appear that were unexpected.

robrap · 2019-10-21T14:05:28Z

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


 from lms.djangoapps.commerce.tests import TEST_API_URL
-from openedx.core.djangoapps.user_authn.views.deprecated import signin_user, create_account, register_user
+from openedx.core.djangoapps.user_api.views import RegistrationView


I like that you aren't doing too much in this single PR. I imagine we might move RegistrationView to user_authn in a later pass?

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

robrap · 2019-10-21T14:20:41Z

openedx/core/djangoapps/user_authn/views/login_form.py

-                    )
-                third_party_auth_hint = provider_id
-                initial_mode = "hinted_login"
+            if 'tpa_hint' in next_args:


openedx/core/djangoapps/user_authn/views/login_form.py

robrap · 2019-10-21T14:49:54Z

openedx/core/djangoapps/user_api/views.py

-    def dispatch(self, request, *args, **kwargs):
-        return super(RegistrationView, self).dispatch(request, *args, **kwargs)
+    def _create_response(self, response_dict, status_code):
+        response_dict['success'] = (status_code == 200)


Note: Having errors go from not having a success key, to now having one with a value of 'False' could theoretically throw off a client, but hopefully not. :)

Sadly this change did cause a spike of registration errors (with accompanying bad reviews) on our edX Android PlayStore app.

robrap · 2019-10-21T15:02:12Z

openedx/core/djangoapps/user_api/views.py

        if data.get("honor_code") and "terms_of_service" not in data:
            data["terms_of_service"] = data["honor_code"]

+    def _get_third_party_auth_redirect_url(self, request):


This is the only weird part of this PR, because everything is about removal (or refactor), but this introduces a change to an existing API that is already in use.

It would be cleanest if this code weren't needed in this PR. If it is, I would comment the code (in addition to the comments in the PR) to clarify that this code is not used and is just required temporarily for old tests that will be removed.

felipemontoya · 2019-10-21T17:47:55Z

@nasthagiri we at eduNEXT use those deprecated view extensively. The reason for it, is that theming the new combined login_registration views is a nightmare.

I understand this has been in the works for quite some time, but I'd like to ask if the merging of this can be made after the juniper cut. This way we can have one extra release to move away from our registration theming model. This will be made simpler by the fact that during juniper we will need to implement theming on the MFE and we will gather a lot of experience from that.

nasthagiri · 2019-10-21T23:20:03Z

@felipemontoya I have created a DEPR ticket for this effort. We can continue the discussion there as well: https://openedx.atlassian.net/browse/DEPR-52

At this point, the edX architecture team is resourced to replatform the login and registration pages, which includes simplifying the underlying backend logic and implementation. Unfortunately, this code is unnecessarily complicated because of half-completed past efforts. Because of this gnarly situation, our current approach is to remove old and deprecated code along the way - that is, strip out one spaghetti strand after another.

As for Juniper, we are targeting the upgrade of our massive platform to Python-3 and Django-2. In order to minimize these arduous maintenances costs, we prefer removing any unneeded code prior to upgrades.

For your case, I wonder if there's a path that helps you move forward - where you can use our new Login and Registration micro-frontends - since they will be available for use in Juniper. We will be using them on edx.org by then. Or perhaps you can see how other open edX instances have successfully themed the newer login/registration pages.

felipemontoya · 2019-10-22T15:38:42Z

@nasthagiri thanks a lot for the detailed answer. I know how to theme the underscore + backbone based login views. We have done it for several clients. However I find that mechanism cumbersome and very little can be done. I'm all for simplifying the login/registration views. In fact, if by Juniper the MFE are going to be used for login/registration, that is good enough for me, since we can move the bulk of our sites to MFEs directly.

Furthermore, any custom logic that we want to modify in the login/registration process at the backend we should be able to add via plugins then. Does the Arch team have a specific pattern in mind for this? or should we propose something in the form of a PR and have a discussion there?

edx-status-bot · 2019-10-29T13:25:06Z

Your PR has finished running tests. There were no failures.

nasthagiri · 2019-10-29T13:35:33Z

Furthermore, any custom logic that we want to modify in the login/registration process at the backend we should be able to add via plugins then. Does the Arch team have a specific pattern in mind for this? or should we propose something in the form of a PR and have a discussion there?

@felipemontoya Feel free to post in a PR and we can discuss there. My suggestion is to wait for 2-3 weeks, however, while the team continues to simplify the current backend logic. After that, perhaps you can create a Django App Plugin to address your needs.

edx-pipeline-bot · 2019-10-29T14:37:57Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Wednesday, October 30, 2019.

edx-pipeline-bot · 2019-10-29T15:06:37Z

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/STAGE_edxapp_M-D

edx-pipeline-bot · 2019-10-30T15:15:52Z

EdX Release Notice: This PR has been deployed to the production environment.

…om/edx/edx-platform/pull/22086

nasthagiri requested review from a team October 19, 2019 17:33

nasthagiri force-pushed the arch/user-authn-delete-deprecated branch 7 times, most recently from f7cd906 to 67d0d10 Compare October 20, 2019 16:28

nasthagiri commented Oct 21, 2019

View reviewed changes

nasthagiri requested review from dianakhuang and robrap October 21, 2019 13:45

robrap approved these changes Oct 21, 2019

View reviewed changes

nasthagiri force-pushed the arch/user-authn-delete-deprecated branch from 67d0d10 to 5271f88 Compare October 29, 2019 01:35

User Authn: Remove deprecated, ENABLE_COMBINED_LOGIN_REGISTRATION

db42c7e

nasthagiri force-pushed the arch/user-authn-delete-deprecated branch from 5271f88 to db42c7e Compare October 29, 2019 12:58

nasthagiri merged commit f533134 into master Oct 29, 2019

edx-secure deleted the arch/user-authn-delete-deprecated branch November 10, 2019 15:15

felipemontoya referenced this pull request in eduNEXT/edunext-platform Apr 7, 2020

Fixing the line that throws the error by backporting https://github.c…

e7e44e3

…om/edx/edx-platform/pull/22086

felipemontoya mentioned this pull request Apr 7, 2020

Preventing KeyError on missing tpa_hint eduNEXT/edunext-platform#386

Merged

morenol referenced this pull request in Zollege/edx-platform Apr 9, 2020

Fixing the line that throws the error by backporting https://github.c…

6ec3fec

…om/edx/edx-platform/pull/22086

User Authn: Remove deprecated, ENABLE_COMBINED_LOGIN_REGISTRATION #22086

User Authn: Remove deprecated, ENABLE_COMBINED_LOGIN_REGISTRATION #22086

Uh oh!

Conversation

nasthagiri commented Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipemontoya commented Oct 21, 2019

Uh oh!

nasthagiri commented Oct 21, 2019

Uh oh!

felipemontoya commented Oct 22, 2019

Uh oh!

edx-status-bot commented Oct 29, 2019

Uh oh!

nasthagiri commented Oct 29, 2019

Uh oh!

edx-pipeline-bot commented Oct 29, 2019

Uh oh!

edx-pipeline-bot commented Oct 29, 2019

Uh oh!

edx-pipeline-bot commented Oct 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nasthagiri commented Oct 19, 2019 •

edited

Loading