-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
feat: tpa automatic logout #32193
feat: tpa automatic logout #32193
Conversation
Thanks for the pull request, @kaustavb12! 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:
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. |
7e16183
to
f82f66d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 @kaustavb12 Looks good, just added few suggestions related to docs.
- I tested this: Tested logging in and out from sandbox.
- I read through the code
-
I checked for accessibility issues - Includes documentation
-
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-secure
repository.
d450789
to
2a5f33a
Compare
@timmc-edx I see you as an author of logout.py. Is this something you could review or could you recommend a reviewer. This definitely looks useful. |
I'll leave a few comments for the parts I understand, but I'll see if I can find someone on Vanguards to do a proper review. (They're listed as owners for the |
cms/envs/common.py
Outdated
# .. toggle_creation_date: 2023-05-07 | ||
# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/32193 | ||
TPA_AUTOMATIC_LOGOUT_ENABLED = False | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only have this defined on the LMS side -- the CMS side just redirects to LMS for logout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timmc-edx You are right, I added this in the CMS side only because some CMS test cases were failing. In hindsight, that was the incorrect way to go about it, I have now handled it differently and removed it from the CMS side.
lms/envs/common.py
Outdated
# from the different IDAs. To ensure the user is logged out of all the IDAs be sure to redirect | ||
# back to <LMS>/logout after logging out of the TPA. | ||
# .. toggle_creation_date: 2023-05-07 | ||
# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/32193 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think toggle_tickets
is primarily used for temporary toggles, whereas this one would be intended as permanent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this.
lms/envs/common.py
Outdated
# .. toggle_default: False | ||
# .. toggle_description: Redirect the user to the TPA logout URL if this flag is enabled, the | ||
# TPA logout URL is configured, and the user logs in through TPA. | ||
# .. toggle_use_cases: open_edx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] I believe this would generally be considered an opt_in
toggle, although I don't have very strong feelings on this. Docs: https://edx.readthedocs.io/projects/edx-toggles/en/latest/how_to/documenting_new_feature_toggles.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for linking the docs.
(Actually, I'll let the usual contributions/maintainership process happen, rather than interfering.) |
Hi @timmc-edx! Just seeing if you were able to find someone on the Vanguards to review/merge this? |
48a5aa7
to
13a1138
Compare
@openedx/vanguards Hi! Could someone please review this, and merge if all looks good? Thanks! |
@mphilbrick211 we are already reviewing it. @mubbsharanwar has been assigned this ticket. Please note that we pick these reviews based on the team's capacity for external requests and delays are to be expected. |
@kaustavb12 🎉 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 Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
EdX Release Notice: This PR has been deployed to the production environment. |
Thank you @timmc-edx and @mubbsharanwar for the reviews and your help to move this forward. |
Description
This PR adds the ability to enable automated TPA logout.
Currently, if a user is logged in through TPA and a
logout_url
is configured, when logging out, the user is presented with the promptClick here to delete your single signed-on (SSO) session
.However, some Open edX operators would not want a two-step logout process and instead would prefer the user is logged out of both Open edX and the SSO with a single click. This is especially critical for use-cases such as when the learners might share terminals in their educational institutions in countries/regions with low internet penetration.
This PR adds the
TPA_AUTOMATIC_LOGOUT_ENABLED
django setting, which when enabled, redirects the user automatically to the TPAlogout_url
if it is configured.This new django setting has no effect if
logout_url
is either not configured and the user is logged in directly though the LMS(using username and password).Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
User sign-in using third party SSO
Sign In
and selectCognito
from thesign in with
section below. You will now be redirected to the Cognito sign-in page.Username :
openedx
Email :
[email protected]
Password:
openedx
openedx
user.Network
tab and selectPreserve log
orPersist logs
Sign Out
from the drop-down menu on the top right.LMS /logout
->Cognito /logout
->LMS /logout
->LMS /
User sign-in without using third party SSO
Sign In
and login through the default login form using the following credentials:Username :
openedx
Email :
[email protected]
Password:
openedx
openedx
user.Network
tab and selectPreserve log
orPersist logs
Sign Out
from the drop-down menu on the top right.LMS /logout
->LMS /
.Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
OpenCraft internal ticket BB-7398