Juniper upgrade Studio standalone login/logout by johnbaldwin · Pull Request #842 · appsembler/edx-platform

johnbaldwin · 2021-03-12T21:45:44Z

Studio local login should work now and the signin form should look like Hawthorn.

There may need to be some improvements, but I think it is good enough to get this pushed to Juniper staging

https://appsembler.atlassian.net/browse/RED-1571

OmarIthawi

Thanks @johnbaldwin! I've checked it quickly knowing that it's WIP.

I like that you're working on a simple implementation for this feature set.

I see some external authentication code, which is probably just leftovers from LMS views. I think we should not plan for external auth on this PR.

Please let me know whenever this is ready for review!

OmarIthawi

@johnbaldwin I've answered your comment and went for a quick review.

Nothing big, but mostly to reply to inline comments and suggest follow up refactoring once the form works.

OmarIthawi · 2021-03-15T19:11:38Z

I agree! feature flags should help to preserve upstream behaviour in standalone mode.

johnbaldwin · 2021-03-15T21:26:54Z

@OmarIthawi Ready for review now. I did a bunch of refactoring. Code much smaller now. Also fixed the login error message so it looks as it was before

johnbaldwin · 2021-03-15T22:37:55Z

Class based views are the thing to do. Much better than function based views with conditional request.method == 'GET_ME_OUT_OF_HERE' checks

johnbaldwin · 2021-03-15T22:42:05Z

At some point, we probably want to get this implemented with Django Forms. Did this as a quick hack to save time

johnbaldwin · 2021-03-15T23:02:44Z

@OmarIthawi This is where all the MTE magic happens. We might want to pull this code out of './openedx/core/djangoapps/user_authn/views/login.py' and make it a Python API instead of pinky-swear private functions.

johnbaldwin · 2021-03-15T23:06:08Z

@OmarIthawi I think we need to add a check here for authorization to be a studio author/admin

The basic workflow works. Should work well enough for staging. Absolutely needs a bunch more work for production

Refactored, simplified and cleaned up studio login code

This should be ready for staging work. Should only need the following: The login 'Sign in here' button needs to be disabled until form fields are valid (having email in the email field, some text in the password field

OmarIthawi

I did one more review round for this PR aiming to reduce conflicts in large settings files. It with the Koa upgrade:

We do IdP before Koa: It makes this PR easy to revert.
We do IdP after Koa: We can do an easier merge.

Please add few basic test cases to ensure that login/logout actually works as expected for correct/incorrect passwords.

OmarIthawi · 2021-03-16T11:59:31Z

+from django.utils.decorators import method_decorator
+
+from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
+from openedx.core.djangoapps.user_authn.views.login import _get_user_by_email


I like your suggestion. I think we should do that now, the function wasn't used outside login until now. A good step is to make a function that's more usable and discoverable by other modules:

I think for now we can do the following until we can refactor it better:

# user_authn/api.py def get_user_from_login_request(request): """ Gets the email from a login POST request. Performs needed checks and handle Multi-Tenant Emails logic. """ return _get_user_by_email(request)

OmarIthawi · 2021-03-16T12:41:35Z

+    # Can we remove this one or maybe use settings to conditionally include
    url(r'', include('openedx.core.djangoapps.user_authn.urls_common')),


I read the module comments and I'm thinking that we probably should keep these lines as-is.

edx-platform/openedx/core/djangoapps/user_authn/urls_common.py

Lines 1 to 11 in 2827f54

"""

Common URLs for User Authentication

Note: The split between urls.py and urls_common.py is hopefully temporary.

For now, this is needed because of difference in CMS and LMS that have

not yet been cleaned up.

This is also home to urls for endpoints that have been consolidated from other djangoapps,

which leads to inconsistent prefixing.

"""

Suggested change

# Can we remove this one or maybe use settings to conditionally include

url(r'', include('openedx.core.djangoapps.user_authn.urls_common')),

url(r'', include('openedx.core.djangoapps.user_authn.urls_common')),

Ok, I'll remove the comment then

OmarIthawi · 2021-03-16T12:45:24Z

+    # Keeping the original Juniper LMD redirect login code
+    # TBD if we want to conditionally include via settings so that it is
+    # disabled on prod
    url(r'^signin_redirect_to_lms$', contentstore.views.login_redirect_to_lms, name='login_redirect_to_lms'),


Yup, I think we should use settings to disable the redirect view. A basic start would be:

Suggested change

# Keeping the original Juniper LMD redirect login code

# TBD if we want to conditionally include via settings so that it is

# disabled on prod

url(r'^signin_redirect_to_lms$', contentstore.views.login_redirect_to_lms, name='login_redirect_to_lms'),

( # Tahoe: Enable the Hawthorn-like Studio login form

url(r'', include('cms.djangoapps.appsembler.urls'))

if settings.FEATURES['TAHOE_STUDIO_LOGIN'] else

url(r'^signin_redirect_to_lms$', contentstore.views.login_redirect_to_lms, name='login_redirect_to_lms')

),

OmarIthawi · 2021-03-16T12:49:51Z

 ENTERPRISE_API_URL = LMS_INTERNAL_ROOT_URL + '/enterprise/api/v1/'
 ENTERPRISE_CONSENT_API_URL = LMS_INTERNAL_ROOT_URL + '/consent/api/v1/'
 ENTERPRISE_MARKETING_FOOTER_QUERY_PARAMS = {}



settings files are very large which creates a challenge during merge

Suggested change

# Public domain name of Studio (should be resolvable from the end-user's browser)

CMS_BASE = 'localhost:18010'

OmarIthawi · 2021-03-16T12:59:16Z

+# Public domain name of Studio (should be resolvable from the end-user's browser)
+CMS_BASE = 'localhost:18010'
+CMS_ROOT_URL = '//localhost:18010'
+


To reduce conflicts: please see the lines below.

Suggested change

# Public domain name of Studio (should be resolvable from the end-user's browser)

CMS_BASE = 'localhost:18010'

CMS_ROOT_URL = '//localhost:18010'

OmarIthawi · 2021-03-16T13:05:41Z

+# Original 'LOGIN_URL' renamed to 'LMS_REDIRECT_LOGIN_URL'
+# This is a candidate for conditional setings to switch between Studio local
+# login and LMS redirection login
+LMS_REDIRECT_LOGIN_URL = reverse_lazy('login_redirect_to_lms')
+LOGIN_URL = reverse_lazy('login')
+FRONTEND_LOGIN_URL = lambda settings: settings.CMS_ROOT_URL + '/login'


Reduce the change with clearer # Tahoe: comments so it's more obvious during release merge:

Suggested change

# Original 'LOGIN_URL' renamed to 'LMS_REDIRECT_LOGIN_URL'

# This is a candidate for conditional setings to switch between Studio local

# login and LMS redirection login

LMS_REDIRECT_LOGIN_URL = reverse_lazy('login_redirect_to_lms')

LOGIN_URL = reverse_lazy('login')

FRONTEND_LOGIN_URL = lambda settings: settings.CMS_ROOT_URL + '/login'

LOGOUT_REDIRECT_URL = reverse_lazy('home') # Tahoe: To make `TAHOE_STUDIO_LOGIN` feature work

LOGIN_URL = reverse_lazy('login') # Tahoe: To make `TAHOE_STUDIO_LOGIN` feature work

FRONTEND_LOGIN_URL = lambda settings: settings.LMS_ROOT_URL + '/login'

OmarIthawi · 2021-03-16T13:07:00Z

+FRONTEND_LOGOUT_URL = lambda settings: settings.CMS_ROOT_URL + '/logout/'
 derived('FRONTEND_LOGOUT_URL')


Simplify the change with clearer comments for the release merge

Suggested change

FRONTEND_LOGOUT_URL = lambda settings: settings.CMS_ROOT_URL + '/logout/'

derived('FRONTEND_LOGOUT_URL')

# Tahoe: in Juniper.master was `FRONTEND_LOGOUT_URL = lambda settings: settings.LMS_ROOT_URL + '/logout'`

# Tahoe: in Juniper.master was `derived('FRONTEND_LOGOUT_URL')`

FRONTEND_LOGOUT_URL = reverse_lazy('logout') # Tahoe: To make `TAHOE_STUDIO_LOGIN` feature work

OmarIthawi · 2021-03-16T13:07:45Z

+LOGOUT_REDIRECT_URL = reverse_lazy('home')
+


Added above with # Tahoe: comments

Suggested change

LOGOUT_REDIRECT_URL = reverse_lazy('home')

OmarIthawi · 2021-03-16T14:19:12Z

@@ -0,0 +1,50 @@
+<%namespace name='static' file='/static_content.html'/>


This could resolve the test failures.

Suggested change

<%namespace name='static' file='/static_content.html'/>

## mako

<%namespace name='static' file='/static_content.html'/>

I don't think ## mako has anything to do with test failures.
It appears to be just a Mako comment line. Perhaps it's a hint to the developers?
I see it unevenly used in edx-platform Mako templates
I don't see it mentioned anywhere as some kind of compiler hint or directive in the Mako documentation or on the web. Do you?

I don't see it mentioned anywhere as some kind of compiler hint or directive in the Mako documentation or on the web. Do you?

Yes, it's a compiler hint that is useful in Class Based Views. Not to say that it's required.

This is just to get the tests passing. We'll follow on with adding confugration so the default state (login direct to LMS) will be back for the nominal tests

coveralls · 2021-03-16T17:23:07Z

Coverage increased (+0.004%) to 40.155% when pulling 58b1f91 on john/studio-signin into 09601e1 on main.

johnbaldwin · 2021-03-17T16:38:51Z

Closing. This PR has been superseded by #847

johnbaldwin requested review from OmarIthawi and melvinsoft March 12, 2021 21:46

johnbaldwin commented Mar 12, 2021

View reviewed changes

Comment thread cms/djangoapps/appsembler/views.py Outdated

OmarIthawi reviewed Mar 14, 2021

View reviewed changes

OmarIthawi self-requested a review March 14, 2021 12:29

johnbaldwin commented Mar 15, 2021

View reviewed changes

Comment thread cms/djangoapps/appsembler/views.py Outdated

OmarIthawi reviewed Mar 15, 2021

View reviewed changes

johnbaldwin force-pushed the john/studio-signin branch from aa79af6 to 438c083 Compare March 15, 2021 21:21

johnbaldwin marked this pull request as ready for review March 15, 2021 21:23

johnbaldwin force-pushed the john/studio-signin branch 3 times, most recently from ede8a55 to 11c4d39 Compare March 15, 2021 22:35

johnbaldwin commented Mar 15, 2021

View reviewed changes

johnbaldwin changed the title ~~WIP Juniper upgrade Studio standalone login/logout~~ Juniper upgrade Studio standalone login/logout Mar 15, 2021

johnbaldwin commented Mar 15, 2021

View reviewed changes

johnbaldwin added 4 commits March 16, 2021 11:15

WIP Juniper upgrade Studio standalone login/logout

1c9e712

The basic workflow works. Should work well enough for staging. Absolutely needs a bunch more work for production

Added login page

cf7594d

Juniper upgrade - Enable Studio local login

69ca5e5

Refactored, simplified and cleaned up studio login code

johnbaldwin force-pushed the john/studio-signin branch from 11c4d39 to 3cafaa6 Compare March 16, 2021 10:16

Fix breaking test for the new studio login

c73a886

OmarIthawi reviewed Mar 16, 2021

View reviewed changes

fix broken studio login test

b25daa1

OmarIthawi reviewed Mar 16, 2021

View reviewed changes

Comment thread cms/djangoapps/appsembler/urls.py

Juniper upgrade - temporary xfail added to test

58b1f91

This is just to get the tests passing. We'll follow on with adding confugration so the default state (login direct to LMS) will be back for the nominal tests

johnbaldwin requested a review from OmarIthawi March 16, 2021 17:22

johnbaldwin closed this Mar 17, 2021

johnbaldwin deleted the john/studio-signin branch March 17, 2021 16:38

natea mentioned this pull request May 13, 2022

[Snyk] Fix for 1 vulnerabilities #1141

Closed

		# Can we remove this one or maybe use settings to conditionally include
		url(r'', include('openedx.core.djangoapps.user_authn.urls_common')),

	"""
	Common URLs for User Authentication

	Note: The split between urls.py and urls_common.py is hopefully temporary.
	For now, this is needed because of difference in CMS and LMS that have
	not yet been cleaned up.

	This is also home to urls for endpoints that have been consolidated from other djangoapps,
	which leads to inconsistent prefixing.

	"""



	# Public domain name of Studio (should be resolvable from the end-user's browser)
	CMS_BASE = 'localhost:18010'

	# Public domain name of Studio (should be resolvable from the end-user's browser)
	CMS_BASE = 'localhost:18010'
	CMS_ROOT_URL = '//localhost:18010'

		FRONTEND_LOGOUT_URL = lambda settings: settings.CMS_ROOT_URL + '/logout/'
		derived('FRONTEND_LOGOUT_URL')

-FRONTEND_LOGOUT_URL = lambda settings: settings.CMS_ROOT_URL + '/logout/'
-derived('FRONTEND_LOGOUT_URL')
+# Tahoe: in Juniper.master was `FRONTEND_LOGOUT_URL = lambda settings: settings.LMS_ROOT_URL + '/logout'`
+# Tahoe: in Juniper.master was `derived('FRONTEND_LOGOUT_URL')`
+FRONTEND_LOGOUT_URL = reverse_lazy('logout')  # Tahoe: To make `TAHOE_STUDIO_LOGIN` feature work

		@@ -0,0 +1,50 @@
		<%namespace name='static' file='/static_content.html'/>

	<%namespace name='static' file='/static_content.html'/>
	## mako
	<%namespace name='static' file='/static_content.html'/>

Uh oh!

Conversation

johnbaldwin commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

OmarIthawi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OmarIthawi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnbaldwin commented Mar 15, 2021 • 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!

OmarIthawi 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Mar 16, 2021

Uh oh!

johnbaldwin commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

johnbaldwin commented Mar 12, 2021 •

edited

Loading

johnbaldwin commented Mar 15, 2021 •

edited

Loading