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

felipemontoya · 2019-02-20T22:08:35Z

Description

This PR creates a new FEATURE flag DISABLE_STUDIO_SSO_OVER_LMS that defaults to False.
When turned on, the flag will make studio behave with the old login flow. This means that it the button on the header will take users to /signin where they can use the same user/pass they had before.

Previous discussion

This PR comes as a follow up to the discussion on the Community Arch meeting where the point was raised that not every openedx installation can immediately change to the new operation under compatible domains with the LMS for cookie sharing.

Some related links:
https://openedx.atlassian.net/browse/ARCH-218
https://openedx.atlassian.net/browse/DEPR-6

Stakeholders

Sandbox (not installed yet)

LMS: ~~https://pr19845.sandbox.opencraft.hosting/~~
Studio: ~~https://studio-pr19845.sandbox.opencraft.hosting/~~

Settings

FEATURES:
  DISABLE_STUDIO_SSO_OVER_LMS: True

openedx-webhooks · 2019-02-20T22:08:49Z

Thanks for the pull request, @felipemontoya! I've created OSPR-3103 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

nasthagiri · 2019-02-20T22:17:11Z

cms/envs/production.py

            CAS_ATTRIBUTE_CALLBACK['function']
        )

+# Login using the LMS as the identity provider


Need to update this comment to say "Do NOT login using the LMS as an Identity Provider if disabled".

nasthagiri · 2019-02-20T22:26:47Z

cms/templates/widgets/header.html

+          login_url = '{lms_root_url}/login?next={next_url}'.format(
+            lms_root_url=settings.LMS_ROOT_URL,
+            next_url=current_url,
+          )


Can we consolidate the use of DISABLE_STUDIO_SSO_OVER_LMS to only the settings file? So it's logic isn't spread throughout these other .html files? As a concrete suggestion:

Add new settings in production.py for FRONTEND_LOGIN_URL and FRONTEND_LOGOUT_URL.

When DISABLE_STUDIO_SSO_OVER_LMS is True

Set FRONTEND_LOGIN_URL to LOGIN_URL

Set FRONTEND_LOGOUT_URL to EDX_ROOT_URL + '/signout'

When DISABLE_STUDIO_SSO_OVER_LMS is False

Set FRONTEND_LOGIN_URL to LMS_ROOT_URL + '/login'`

Set FRONTEND_LOGOUT_URL to LMS_ROOT_URL + '/logout'

Update these .html files to always append ?next={next_url} regardless of the DISABLE_STUDIO_SSO_OVER_LMS value. I believe the old Studio endpoints will just ignore the next parameter.

natabene · 2019-02-21T02:47:54Z

@felipemontoya Thank you for your contribution. Can you let me know once you have addressed requests from @nasthagiri ?

felipemontoya · 2019-02-26T18:08:15Z

Hey @nasthagiri sorry It took me long to address the feedback. I created a devstack for this on my home computer and only today I stayed to work from home again.

I did all the logic on the settings modules this time. Not only on production.py since the base existence of the keys is something that should live in common.py in case some other module is not inheriting from production.

I did have to change something else on the LogoutView. See the inline comment

felipemontoya · 2019-02-26T18:12:05Z

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

This logic of looking at oauth_client_ids, I brought it back from Hawthorn. Without it, what happens is that studio will try to render a page that inherits from "main_django.html" and that template only exists in the LMS.

Now, I could also change the logic to make the if exclusively targeting studio and not all the cases where there are no oauth_clients. If that was the case, should I check for SERVICE_VARIANT == 'cms' or is there a preferred way of doing this?

The rendered page is a harmless, but ugly looking bug in the master version. You can see what I mean if you go to https://studio.edx.org/logout (even without a valid session)

Also this part here is what is breaking the tests because they are expecting a 200 http status, but they get a redirect 302. I'll fix the tests once I have a clear indication about what I should do in this case

I believe @pwnage101 and @doctoryes had reasons to remove this code as part of supporting SSO via DOT (rather than DOP).

@felipemontoya Can you put this logic behind the DISABLE_STUDIO_SSO_OVER_LMS feature toggle? When Studio redirects to the LMS for both login/logout, then Studio's own logout page should never be rendered and the ugly page you point out should not appear. Eventually, we will want to remove the old studio login/logout code entirely, including this now-ugly logout page.

This makes sense to me. I just made the necessary changes.
I also rebased on top of master to have a more up-to-date PR

nasthagiri

Looks good to me! Just one request to keep the oauth_client changes behind the feature toggle. Thanks Felipe.

nasthagiri · 2019-03-11T13:08:06Z

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

I believe @pwnage101 and @doctoryes had reasons to remove this code as part of supporting SSO via DOT (rather than DOP).

@felipemontoya Can you put this logic behind the DISABLE_STUDIO_SSO_OVER_LMS feature toggle? When Studio redirects to the LMS for both login/logout, then Studio's own logout page should never be rendered and the ugly page you point out should not appear. Eventually, we will want to remove the old studio login/logout code entirely, including this now-ugly logout page.

…ure flag

felipemontoya · 2019-03-11T18:09:41Z

My bad, I left a typo in. It is running tests again

pwnage101 · 2019-03-11T18:24:07Z

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

+        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)


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.

nasthagiri

looks good to merge once tests pass

felipemontoya · 2019-03-11T19:34:09Z

jenkins run python

felipemontoya · 2019-03-11T20:25:20Z

@nasthagiri I'm not quite sure on how to proceed. Lettuce tests are complaining about the absence of FRONTEND_LOGOUT_URL, but they are complaining when importing from lms.envs.test .... Nowhere in this PR is the FRONTEND_LOGOUT_URL being called from the LMS side.

I suppose that setting a value for it in the lms.envs.common would fix it, but It does not seem to be correct to add it there if it is not ever needed at the lms.

Traceback (most recent call last):
  File "./manage.py", line 119, in <module>
    startup = importlib.import_module(edx_args.startup)
  File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/home/jenkins/workspace/edx-platform-lettuce-pipeline-pr/cms/startup.py", line 9, in <module>
    settings.INSTALLED_APPS  # pylint: disable=pointless-statement
  File "/home/jenkins/edx-venv/local/lib/python2.7/site-packages/django/conf/__init__.py", line 56, in __getattr__
    self._setup(name)
  File "/home/jenkins/edx-venv/local/lib/python2.7/site-packages/django/conf/__init__.py", line 41, in _setup
    self._wrapped = Settings(settings_module)
  File "/home/jenkins/edx-venv/local/lib/python2.7/site-packages/django/conf/__init__.py", line 110, in __init__
    mod = importlib.import_module(self.SETTINGS_MODULE)
  File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/home/jenkins/workspace/edx-platform-lettuce-pipeline-pr/cms/envs/acceptance.py", line 10, in <module>
    from .test import *
  File "/home/jenkins/workspace/edx-platform-lettuce-pipeline-pr/cms/envs/test.py", line 27, in <module>
    from lms.envs.test import (
  File "/home/jenkins/workspace/edx-platform-lettuce-pipeline-pr/lms/envs/test.py", line 608, in <module>
    derive_settings(__name__)
  File "/home/jenkins/workspace/edx-platform-lettuce-pipeline-pr/openedx/core/lib/derived.py", line 50, in derive_settings
    setting = getattr(module, derived)
AttributeError: 'module' object has no attribute 'FRONTEND_LOGOUT_URL'

Should I add it to the LMS? or can anyone else look at this and let me know if I should use the derive_settings differently?

nasthagiri · 2019-03-11T22:09:31Z

@felipemontoya I do believe you are using derive_settings correctly. I also believe the current settings implementation is far from ideal - in having CMS importing from LMS. For the time being, you can:

Define FRONTEND_LOGOUT_URL in lms/envs/test.py (not common.py).
Add a TODO comment with Put back in the blocks for Advanced Settings. #1 that it is currently needed for Lettuce tests, with a reference to DEPR-19. We can then remove it when we remove Lettuce altogether soon.

I am pinging @doctoryes who wrote the derived implementation if she has any better ideas.

Thanks.

doctoryes · 2019-03-12T16:20:10Z

@nasthagiri 's suggestions are 👍 .

nasthagiri · 2019-03-14T22:51:26Z

lms/envs/test.py


+# 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'


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

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

Thanks for trying @felipemontoya.

edx-status-bot · 2019-03-15T04:48:32Z

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

openedx-webhooks · 2019-03-15T12:23:06Z

@felipemontoya 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2019-03-15T15:46:41Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Monday, March 18, 2019.

edx-pipeline-bot · 2019-03-18T16:08:32Z

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

robrap · 2019-11-26T22:13:53Z

@felipemontoya: We are looking to remove the old Studio login page and this feature flag. Do you now have a solution for the cookie sharing problem? Thank you.

Also, can you tell me why FRONTEND_LOGIN_URL exists, and why we can't just use LOGIN_URL? Thanks again.

felipemontoya · 2019-11-27T16:14:56Z

Hello @robrap.

I don't see this issue as we having a problem for the cookie sharing problem. I know exactly how to share the cookies between lms and cms. The thing there is that this forces a requirement on the possible domains where studio might be hosted in relation to the lms.

Hosting studio and the lms as subdomains of the same domain and more specifically sharing the cookie using the SESSION_COOKIE_DOMAIN = '.yourdomain.com' has been the use case for the edx.org installation since the beginning but it is not a hard requirement for using studio until this comes into effect. If you want to move ahead with the deprecation, go for it, but make it super clear on the release notes that this is the requirement now so that people upgrading and installing for the first time are not tripping over this.

Optionally I see 2 other paths going forward:

convert the studio auth into a real SSO third party auth authentication using Oauth2
use the plugins mechanism to create authentication view for studio (can be done independently by the community. No need to merge anything to the core)

About the LOGIN_URL from static reading the code I also don't see a need for it. That might have been something that changed during the discussion and could have been further optimized. However I have not fired up a running dev environment to try and see if there are unintended consequences.

sambapete · 2019-11-27T17:05:40Z

If I can add something to what @felipemontoya is saying is that for example here we use cours.edulib.org for the LMS and studio.edulib.org for the CMS. The authentication change that was made was not allowing us to connect directly to studio.edulib.org because we are behind two ELB and the FQDN were set at the ELB level (even if SSO was previously working when set to SESSION_COOKIE_DOMAIN = ".edulib.org"). Basically, it was not recognizing studio.edulib.org to be in the same domain as cours.edulib.org.

Since many "existing" installations will have different requirement than edx.org, maybe these should also be taken into consideration moving forward and not force an architectural / infrastructure change on the part of the community.

As I said previously in the thread in Slack, documentation is key here.

nedbat · 2019-11-27T18:10:19Z

@robrap This would be a good topic for https://discuss.openedx.org so that we can get more viewpoints and ideas.

robrap · 2019-11-27T21:02:30Z

@felipemontoya @sambapete: Sharing the LMS login page better aligns with the other IDAs, it allows Studio to get all the SSO integrations supported by LMS, and it simplifies the security story for Open edX.
@felipemontoya: It sounds like you have some good possible alternatives in mind.

I added notes to the upcoming Juniper release page that we can enhance as we learn more, including any future alternatives that are implemented. See https://openedx.atlassian.net/wiki/spaces/COMM/pages/940048716/Juniper

robrap · 2019-11-27T21:03:09Z

If you want to continue discussion, maybe move to Discourse as @nedbat asked. Thanks.

This completes the work started in https://github.com/edx/edx-platform/pull/19453 to use the LMS login and registration for Studio, rather than Studio providing its own implementation. LMS login/registration are being used for the following reasons: 1. LMS logistration properly handles all SSO integrations. 2. A single logistration is simpler to maintain and understand. 3. Allows Studio to work more like all other IDAs that use LMS logistration. The original switch to use LMS logistration for Studio also added the toggle `DISABLE_STUDIO_SSO_OVER_LMS` to provide the community some additional time for switching. This commit removes this toggle, which at this point means all deployments will use the LMS logistration. This change requires sharing cookies across LMS and Studio. Should that prove to be a problem for certain Open edX instances, there are discussions of possible alternative solutions. See https://github.com/edx/edx-platform/pull/19845#issuecomment-559154256 Detailed changes: * Fix some Studio links that still went to old Studio signin and signup. * Remove DISABLE_STUDIO_SSO_OVER_LMS feature toggle. * Remove old studio signin and signup pages and templates. * Fix url name "login", which had different meanings for Studio and LMS. * Use the following settings: LOGIN_URL, FRONTEND_LOGIN_URL, FRONTEND_LOGOUT_URL, and FRONTEND_REGISTER_URL. * Redirect /signin and /signup to the LMS logistration. * Add custom metric `uses_pattern_library`. * Add custom metric `student_activate_account`. * Add Django Settings to allow /signin, /signup, and /login_post to be disabled once ready. This work also relates to ARCH-218 and DEPR-6. ARCH-1253

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Feb 20, 2019

nasthagiri reviewed Feb 20, 2019

View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Feb 21, 2019

felipemontoya force-pushed the fmo/studio_sso_over_lms_flag branch from 4155e6e to 031cb45 Compare February 26, 2019 18:05

felipemontoya commented Feb 26, 2019

View reviewed changes

felipemontoya force-pushed the fmo/studio_sso_over_lms_flag branch from 031cb45 to d585438 Compare February 26, 2019 18:15

nasthagiri reviewed Mar 11, 2019

View reviewed changes

felipemontoya added 2 commits March 11, 2019 12:01

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

80b977f

Addressing feedback

14b4223

felipemontoya force-pushed the fmo/studio_sso_over_lms_flag branch from d585438 to 947fe19 Compare March 11, 2019 17:32

Addressing second feedback about redirect logic on logout behing feat…

9195ec9

…ure flag

felipemontoya force-pushed the fmo/studio_sso_over_lms_flag branch from 947fe19 to 9195ec9 Compare March 11, 2019 18:09

pwnage101 reviewed Mar 11, 2019

View reviewed changes

nasthagiri approved these changes Mar 11, 2019

View reviewed changes

Fixing lettuce tests

923a917

nasthagiri reviewed Mar 14, 2019

View reviewed changes

felipemontoya force-pushed the fmo/studio_sso_over_lms_flag branch from 987bff1 to 923a917 Compare March 15, 2019 04:09

nasthagiri merged commit 9c3472a into openedx:master Mar 15, 2019

nedbat mentioned this pull request Mar 15, 2019

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

Merged

robrap mentioned this pull request Nov 27, 2019

ARCH-1253: remove studio signin and signup pages #22416

Merged

openedx-webhooks added merged and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 22, 2021

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

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

Conversation

felipemontoya commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Previous discussion

Stakeholders

Sandbox (not installed yet)

Uh oh!

openedx-webhooks commented Feb 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natabene commented Feb 21, 2019

Uh oh!

felipemontoya commented Feb 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipemontoya Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nasthagiri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipemontoya commented Mar 11, 2019

Uh oh!

pwnage101 Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasthagiri left a comment

Choose a reason for hiding this comment

Uh oh!

felipemontoya commented Mar 11, 2019

Uh oh!

felipemontoya commented Mar 11, 2019

Uh oh!

nasthagiri commented Mar 11, 2019

Uh oh!

doctoryes commented Mar 12, 2019

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!

edx-status-bot commented Mar 15, 2019

Uh oh!

openedx-webhooks commented Mar 15, 2019

Uh oh!

edx-pipeline-bot commented Mar 15, 2019

Uh oh!

edx-pipeline-bot commented Mar 18, 2019

Uh oh!

robrap commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felipemontoya commented Nov 27, 2019

Uh oh!

sambapete commented Nov 27, 2019

Uh oh!

nedbat commented Nov 27, 2019

Uh oh!

robrap commented Nov 27, 2019

Uh oh!

robrap commented Nov 27, 2019

felipemontoya commented Feb 20, 2019 •

edited

Loading

felipemontoya Feb 26, 2019 •

edited

Loading

pwnage101 Mar 11, 2019 •

edited

Loading

robrap commented Nov 26, 2019 •

edited

Loading