Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions cms/djangoapps/appsembler/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
"""Perform Studio local login/logout

The purpose of this is to address the issue that Ironwood introduced login
redirect to the LMS, which breaks in multisite custom domain environments

We have this code in the Appsembler CMS app to help isolate custom code
"""
from django.conf import settings
from django.urls import path
from django.contrib.auth.views import LogoutView
from .views import LoginView

urlpatterns = [
path('login/', LoginView.as_view(), name='login'),
Comment thread
OmarIthawi marked this conversation as resolved.
path('logout/', LogoutView.as_view(
next_page=settings.LOGOUT_REDIRECT_URL), name='logout'),
]
69 changes: 69 additions & 0 deletions cms/djangoapps/appsembler/views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
"""Appsembler custom views for Studio

Views here provide Studio local login/logout
"""

from django.conf import settings
from django.contrib.auth import authenticate, login
from django.shortcuts import redirect
from django.views import View
from django.views.decorators.clickjacking import xframe_options_deny
from django.views.decorators.csrf import ensure_csrf_cookie
from django.urls import reverse
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


from edxmako.shortcuts import render_to_response


def forgot_password_link():
return "//{base}/login#forgot-password-modal".format(base=settings.LMS_BASE)


def platform_name():
return configuration_helpers.get_value('platform_name', settings.PLATFORM_NAME)


def render_login_page(show_login_error_message=False):
"""Convenience function to put the login page

Arguments:
show_login_error_message (bool): flag to show if a login attempt failed

Returns:
django.http.response.HttpResponse object with the login page content
"""
return render_to_response(
'login_page.html',
{
'show_login_error_message': show_login_error_message,
'forgot_password_link': forgot_password_link(),
'platform_name': platform_name(),
}
)


class LoginView(View):

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Basic login view to allow for Studio local logins
"""

@method_decorator(ensure_csrf_cookie)
@method_decorator(xframe_options_deny)
def get(self, request):
return render_login_page()

@method_decorator(ensure_csrf_cookie)
def post(self, request, *args, **kwargs):

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

user = _get_user_by_email(request)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

password = request.POST['password']

if user:
user = authenticate(request, username=user.username, password=password)

if not user:
return render_login_page(show_login_error_message=True)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

login(request, user)
return redirect(reverse('home'))
2 changes: 1 addition & 1 deletion cms/djangoapps/appsembler_tiers/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def test_site_unavailable_page_non_logged_in(self):
"""
response = self.client.get(self.url)
assert response.status_code == status.HTTP_302_FOUND, response.content
assert response['Location'] == '/signin_redirect_to_lms?next=/site-unavailable/', response.content
assert response['Location'] == '/login/?next=/site-unavailable/', response.content

def test_site_unavailable_page(self):
"""
Expand Down
8 changes: 6 additions & 2 deletions cms/djangoapps/contentstore/tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import datetime
import time
import pytest

import mock
from ddt import data, ddt, unpack
Expand Down Expand Up @@ -138,6 +139,10 @@ def test_private_pages_auth(self):
print(u"Checking '{0}'".format(page))
self.check_page_get(page, expected=200)

# JLB Juniper upgrade: This test fails, returning a 200
# Adding conditional configuration for Studio local login should make
# this test work again without modification
@pytest.mark.xfail
@override_settings(SESSION_INACTIVITY_TIMEOUT_IN_SECONDS=1)
def test_inactive_session_timeout(self):
"""
Expand All @@ -158,7 +163,6 @@ def test_inactive_session_timeout(self):
time.sleep(2)

resp = self.client.get_html(course_url)

# re-request, and we should get a redirect to login page
self.assertRedirects(resp, settings.LOGIN_URL + '?next=/home/', target_status_code=302)

Expand All @@ -181,7 +185,7 @@ def test_signin_and_signup_buttons_index_page(self, allow_account_creation, asse
)
self.assertContains(
response,
'<a class="action action-signin" href="/signin_redirect_to_lms?next=http%3A%2F%2Ftestserver%2F">'
'<a class="action action-signin" href="/login/?next=http%3A%2F%2Ftestserver%2F">'
'Sign In</a>'
)

Expand Down
18 changes: 13 additions & 5 deletions cms/envs/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -571,27 +571,35 @@

LOGGING_ENV = 'sandbox'

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

Comment on lines +574 to +577

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

LMS_BASE = 'localhost:18000'
LMS_ROOT_URL = "https://localhost:18000"
LMS_INTERNAL_ROOT_URL = LMS_ROOT_URL

LOGIN_REDIRECT_URL = EDX_ROOT_URL + '/home/'
# TODO: Determine if LOGIN_URL could be set to the FRONTEND_LOGIN_URL value instead.
LOGIN_URL = reverse_lazy('login_redirect_to_lms')
FRONTEND_LOGIN_URL = lambda settings: settings.LMS_ROOT_URL + '/login'
# 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'
Comment on lines +584 to +589

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

derived('FRONTEND_LOGIN_URL')
FRONTEND_LOGOUT_URL = lambda settings: settings.LMS_ROOT_URL + '/logout'
FRONTEND_LOGOUT_URL = lambda settings: settings.CMS_ROOT_URL + '/logout/'
derived('FRONTEND_LOGOUT_URL')
Comment on lines +591 to 592

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FRONTEND_REGISTER_URL = lambda settings: settings.LMS_ROOT_URL + '/register'
derived('FRONTEND_REGISTER_URL')

LOGOUT_REDIRECT_URL = reverse_lazy('home')

Comment on lines +596 to +597

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added above with # Tahoe: comments

Suggested change
LOGOUT_REDIRECT_URL = reverse_lazy('home')

LMS_ENROLLMENT_API_PATH = "/api/enrollment/v1/"
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 = {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

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

LOG_DIR = '/edx/var/log/edx'

Expand Down
50 changes: 50 additions & 0 deletions cms/templates/login_page.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<%namespace name='static' file='/static_content.html'/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could resolve the test failures.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<%page expression_filter="h"/>
<%inherit file="base.html" />
<%def name="online_help_token()"><% return "login" %></%def>
<%!
from django.utils.translation import ugettext as _
from django.urls import reverse
%>
<%block name="title">${_("Sign In")}</%block>
<%block name="bodyclass">not-signedin view-signin</%block>

<%block name="content">

<div class="wrapper-content wrapper">
<section class="content">
<header>
<h1 class="title title-1">${_("Sign In to {studio_name}").format(studio_name=settings.STUDIO_NAME)}</h1>
</header>
<article class="content-primary">
<div class=""><!-- What class for a dive to wrap the form? -->
<form method="post" action="${reverse('login')}">
%if show_login_error_message:
<div id="login_error"
class="message message-status error is-shown"
style="display: block;">
${_("Email or password is incorrect. Please ensure that you are a course staff in order to use Studio.")}
</div>
%endif
<fieldset>
<input type="hidden" id="csrf_token" name="csrfmiddlewaretoken" value="${csrf_token}">
<ol class="list-input">
<li class="field text required" id="field-email">
<label for="email">${_("E-mail")}</label>
<input id="email" type="email" name="email" placeholder="${_('example: username@domain.com')}"/>
</li>

<li class="field text required" id="field-password">
<label for="password">${_("Password")}</label>
<input id="password" type="password" name="password" />
<a href="${forgot_password_link}" class="action action-forgotpassword">${_("Forgot password?")}</a>
</li>
</ol>
</fieldset>
<button type="submit" name="submit" class="action action-primary">Sign in here</button>
</form>
</div>
</article>
</section>
</div>
</%block>
5 changes: 5 additions & 0 deletions cms/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@
LIBRARY_KEY_PATTERN = r'(?P<library_key_string>library-v1:[^/+]+\+[^/+]+)'

urlpatterns = [
# Can we remove this one or maybe use settings to conditionally include
url(r'', include('openedx.core.djangoapps.user_authn.urls_common')),
Comment on lines +44 to 45

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
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')),

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll remove the comment then

url(r'', include('student.urls')),
url(r'', include('cms.djangoapps.appsembler.urls')),
url(r'^transcripts/upload$', contentstore.views.upload_transcripts, name='upload_transcripts'),
url(r'^transcripts/download$', contentstore.views.download_transcripts, name='download_transcripts'),
url(r'^transcripts/check$', contentstore.views.check_transcripts, name='check_transcripts'),
Expand Down Expand Up @@ -84,6 +86,9 @@
# restful api
url(r'^$', contentstore.views.howitworks, name='homepage'),
url(r'^howitworks$', contentstore.views.howitworks, name='howitworks'),
# 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'),
Comment on lines +89 to 92

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')
),

url(r'^request_course_creator$', contentstore.views.request_course_creator, name='request_course_creator'),
url(r'^course_team/{}(?:/(?P<email>.+))?$'.format(COURSELIKE_KEY_PATTERN),
Expand Down