Skip to content

feat: implement login redirect middleware [BB-5090] [OSPR-6223] - #29316

Closed
Cup0fCoffee wants to merge 1 commit into
openedx:masterfrom
open-craft:maxim/bb-5090-login-redirect-middleware-upstream
Closed

feat: implement login redirect middleware [BB-5090] [OSPR-6223]#29316
Cup0fCoffee wants to merge 1 commit into
openedx:masterfrom
open-craft:maxim/bb-5090-login-redirect-middleware-upstream

Conversation

@Cup0fCoffee

@Cup0fCoffee Cup0fCoffee commented Nov 15, 2021

Copy link
Copy Markdown
Contributor

Description

This PR implements new middleware that redirects all unathenticated users to login page. It enables admins to enable the middleware through settings for LMS to prevent unathenticated users from seeing any content.

Supporting information

BB-5090
OSPR-6223

Testing instructions

  • Run make lms-up in the devstack
  • Visit http://localhost:18000/courses/ (logout before that, if you're logged in) - you will be able to see the available courses
  • Run make lms-shell in the devstack, open /edx/etc/lms.yml and under 'FEATURES' add ENABLE_REDIRECT_UNATHENTICATED_USERS_TO_LOGIN: true
  • Exit the lms shell and run make lms-restart-devserver
  • Now if you visit http://localhost:18000/courses/, you'll be redirected to login page
  • If you log in, you should get redirected to the page you've tried to visit before logging in (in this case it's /courses/)
  • You can try to repeat the last two steps with any page that doesn't usually require user to be authenticated, and you can add query string to the url, to check that it doesn't get lost after login

Other information

The middleware is implemented in the "new" style, i.e. without the use of MiddlewareMixin, to not introduce more things to refactor in the future. If there are reasons for why the MiddlewareMixin is preferred, I'll happily change the implementation to use it.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @Cup0fCoffee! I've created OSPR-6223 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

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

  • supporting documentation
  • Open edX discussion forum 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 be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Nov 15, 2021
@Cup0fCoffee Cup0fCoffee changed the title feat: implement login redirect middleware [BB-5090] feat: implement login redirect middleware [BB-5090] [OSPR-6223] Nov 15, 2021
Implemented a new middleware, that redirects all unathorised user, that
are trying to vising any page but login and register, to login page.
The middleware is enabled via a ENABLE_REDIRECT_UNAUTHORISED_USERS_TO_LOGIN
setting.

Added the middleware and the setting to the LMS.
@Cup0fCoffee
Cup0fCoffee force-pushed the maxim/bb-5090-login-redirect-middleware-upstream branch from 955cd24 to 8ae40dc Compare November 15, 2021 17:29
@natabene

Copy link
Copy Markdown
Contributor

@Cup0fCoffee Thank you for your contribution. Is this ready for our review?

@edx-status-bot

Copy link
Copy Markdown

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

@Cup0fCoffee

Copy link
Copy Markdown
Contributor Author

@natabene Yes.

@Cup0fCoffee

Cup0fCoffee commented Nov 23, 2021

Copy link
Copy Markdown
Contributor Author

@natabene, @0x29a has discovered that the current implementation of the feature breaks the third party authentication when enabled. We have a few ideas for how to fix it, and are working on it. However, we are not confident that our implementation can be comprehensive enough to be upstreamed.

If there is interest in the feature, we can discuss possible implementations and how to cover all corner cases on the forum in the future. For now this PR can be closed.

@openedx-webhooks

Copy link
Copy Markdown

@Cup0fCoffee Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U rejected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants