Skip to content

Make the studio login over the lms optional using a feature flag #19845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nasthagiri merged 4 commits into from
Mar 15, 2019
Merged

Make the studio login over the lms optional using a feature flag #19845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
80b977f
Make the studio login over the lms optional using a feature flag
felipemontoya Feb 20, 2019
14b4223
Addressing feedback
felipemontoya Feb 26, 2019
9195ec9
Addressing second feedback about redirect logic on logout behing feat…
felipemontoya Mar 11, 2019
923a917
Fixing lettuce tests
felipemontoya Mar 14, 2019
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
3 changes: 3 additions & 0 deletions cms/envs/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,9 @@
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/'
FRONTEND_LOGIN_URL = LOGIN_URL
FRONTEND_LOGOUT_URL = lambda settings: settings.LMS_ROOT_URL + '/logout'
derived('FRONTEND_LOGOUT_URL')

# List of logout URIs for each IDA that the learner should be logged out of when they logout of
# Studio. Only applies to IDA for which the social auth flow uses DOT (Django OAuth Toolkit).
Expand Down
10 changes: 10 additions & 0 deletions cms/envs/production.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from xmodule.modulestore.modulestore_settings import convert_module_store_setting_if_needed
from openedx.core.djangoapps.plugins import plugin_settings, constants as plugin_constants
from django.core.exceptions import ImproperlyConfigured
from django.core.urlresolvers import reverse_lazy

from .common import *

Expand Down Expand Up @@ -298,6 +299,15 @@ def get_env_setting(setting):
HEARTBEAT_EXTENDED_CHECKS = ENV_TOKENS.get('HEARTBEAT_EXTENDED_CHECKS', HEARTBEAT_EXTENDED_CHECKS)
HEARTBEAT_CELERY_TIMEOUT = ENV_TOKENS.get('HEARTBEAT_CELERY_TIMEOUT', HEARTBEAT_CELERY_TIMEOUT)

# Login using the LMS as the identity provider.
# Turning the flag to True means that the LMS will NOT be used as the Identity Provider (idp)
if FEATURES.get('DISABLE_STUDIO_SSO_OVER_LMS', False):
LOGIN_URL = reverse_lazy('login')
FRONTEND_LOGIN_URL = LOGIN_URL
FRONTEND_LOGOUT_URL = reverse_lazy('logout')

LOGIN_REDIRECT_WHITELIST = [reverse_lazy('home')]

# Specific setting for the File Upload Service to store media in a bucket.
FILE_UPLOAD_STORAGE_BUCKET_NAME = ENV_TOKENS.get('FILE_UPLOAD_STORAGE_BUCKET_NAME', FILE_UPLOAD_STORAGE_BUCKET_NAME)
FILE_UPLOAD_STORAGE_PREFIX = ENV_TOKENS.get('FILE_UPLOAD_STORAGE_PREFIX', FILE_UPLOAD_STORAGE_PREFIX)
Expand Down
3 changes: 1 addition & 2 deletions cms/templates/widgets/header.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,6 @@ <h3 class="title"><span class="label"><a href="${get_online_help_info(online_hel

% else:
<%
login_url = settings.LMS_ROOT_URL + '/login'
register_url = settings.LMS_ROOT_URL + '/register'
%>
<nav class="nav-not-signedin nav-pitch" aria-label="${_('Account')}">
Expand All @@ -245,7 +244,7 @@ <h2 class="sr-only">${_("Account Navigation")}</h2>
</li>
% endif
<li class="nav-item nav-not-signedin-signin">
<a class="action action-signin" href="${login_url}?next=${current_url}">${_("Sign In")}</a>
<a class="action action-signin" href="${settings.FRONTEND_LOGIN_URL}?next=${current_url}">${_("Sign In")}</a>
</li>
</ol>
</nav>
Expand Down
5 changes: 1 addition & 4 deletions cms/templates/widgets/user_dropdown.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@ <h3 class="title">
</span>
<span class="icon fa fa-caret-down ui-toggle-dd" aria-hidden="true"></span>
</h3>
<%
logout_url = settings.LMS_ROOT_URL + '/logout'
%>
<div class="wrapper wrapper-nav-sub">
<div class="nav-sub">
<ul>
Expand All @@ -54,7 +51,7 @@ <h3 class="title">
</li>
% endif
<li class="nav-item nav-account-signout">
<a class="action action-signout" href="${logout_url}?next=${current_site_url}">${_("Sign Out")}</a>
<a class="action action-signout" href="${settings.FRONTEND_LOGOUT_URL}">${_("Sign Out")}</a>
</li>
</ul>
</div>
Expand Down
4 changes: 4 additions & 0 deletions lms/envs/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,10 @@

LMS_ROOT_URL = "http://localhost:8000"

# TODO (felipemontoya): This key is only needed during lettuce tests.
# To be removed during https://openedx.atlassian.net/browse/DEPR-19
FRONTEND_LOGOUT_URL = LMS_ROOT_URL + '/logout'
Copy link
Contributor

@nasthagiri nasthagiri Mar 14, 2019

Choose a reason for hiding this comment

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

Question: do the lettuce tests not pass with this living in cms/envs/test.py?

Copy link
Member Author

@felipemontoya felipemontoya Mar 15, 2019

Choose a reason for hiding this comment

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

Really good question. I did not know either, so I tried, and the answer is no (https://build.testeng.edx.org/job/edx-platform-lettuce-pipeline-pr/2264/console).
I'm returning to the working version

Copy link
Contributor

@nasthagiri nasthagiri Mar 15, 2019

Choose a reason for hiding this comment

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

Thanks for trying @felipemontoya.


ECOMMERCE_API_URL = 'https://ecommerce.example.com/api/v2/'
ENTERPRISE_API_URL = 'http://enterprise.example.com/enterprise/api/v1/'
ENTERPRISE_CONSENT_API_URL = 'http://enterprise.example.com/consent/api/v1/'
Expand Down
6 changes: 5 additions & 1 deletion openedx/core/djangoapps/user_authn/views/logout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,11 @@ def dispatch(self, request, *args, **kwargs):

logout(request)

response = super(LogoutView, self).dispatch(request, *args, **kwargs)
# If we are using studio logout directly and there is not OIDC logouts we can just redirect the user
if settings.FEATURES.get('DISABLE_STUDIO_SSO_OVER_LMS', False) and not self.oauth_client_ids:
response = redirect(self.target)
else:
response = super(LogoutView, self).dispatch(request, *args, **kwargs)
Copy link
Contributor

@pwnage101 pwnage101 Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, emtiness of the self.oauth_client_ids list does not imply that the user is logged out of all IDAs. IDAs that were converted to use DOT (ecommerce and credentials, on master currently) currently do not get added to that list.

For logging out the user from those DOT-enabled IDAs, we always unconditionally log out the user from them. This is a stop-gap solution until we decide on a better/smarter DOT-specific solution.


# Clear the cookie used by the edx.org marketing site
delete_logged_in_cookies(response)
Expand Down