Skip to content
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

191 back end logout flow #198

Closed
wants to merge 7 commits into from
Closed

Conversation

collinpreston
Copy link
Contributor

@collinpreston collinpreston commented Nov 8, 2023

What are the relevant tickets?

#191

Description (What does it do?)

This PR solves the issue described in this RFC: https://docs.google.com/document/d/17tJ-C2EwWoSpJWZKjuhMVgsqGtyPH0IN9KakXvSKU0M/edit#heading=h.96o2gm3cfv2

This also enables the future implementation of the flow here: https://docs.google.com/document/d/17tJ-C2EwWoSpJWZKjuhMVgsqGtyPH0IN9KakXvSKU0M/edit#heading=h.qr1x27po61aq

How can this be tested?

These instructions assume that you have a local instance of Keycloak and MIT Open running and connected via an Open ID client in Keycloak.

Configure the MIT Open client in Keycloak:

  1. The client can match the existing ol-open-discussions-local client found in the olapps realm in QA.

  2. This client must have Backchannel logout URL set to http://open.ol.local:8063/backend-logout/

  3. Login to MIT Open via http://open.ol.local:8063/login/ol-oidc

  4. You should now be logged into Keycloak (verify by visiting http://localhost:8080/admin/master/console/)

  5. You should now be logged into MIT Open (verify by visiting http://open.ol.local:8063/admin)

  6. In a separate browser or incognito window, perform step 1, 2, and 3 again.

  7. In one of the browser windows, navigate to http://open.ol.local:8063/logout.

  8. You should now be logged out of Keycloak in only one of the browsers (verify by visiting http://localhost:8080/admin/master/console/)

  9. Perform step 6 in the browser not originally used to complete step 6.

  10. You should now be logged out of MIT Open both browsers (verify by visiting http://open.ol.local:8063/admin)

@collinpreston collinpreston linked an issue Nov 8, 2023 that may be closed by this pull request
1 task
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Merging #198 (62de102) into main (acdf97c) will decrease coverage by 0.06%.
The diff coverage is 64.70%.

@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
- Coverage   77.73%   77.67%   -0.06%     
==========================================
  Files         310      310              
  Lines       13441    13489      +48     
  Branches     2210     2219       +9     
==========================================
+ Hits        10448    10478      +30     
- Misses       2719     2737      +18     
  Partials      274      274              
Files Coverage Δ
authentication/urls.py 100.00% <100.00%> (ø)
open_discussions/settings.py 91.44% <100.00%> (+0.03%) ⬆️
open_discussions/urls.py 78.57% <ø> (ø)
authentication/views.py 72.34% <92.59%> (+27.34%) ⬆️
authentication/backends/ol_open_id_connect.py 30.43% <20.00%> (-69.57%) ⬇️

... and 3 files with indirect coverage changes

@collinpreston collinpreston marked this pull request as ready for review November 9, 2023 14:10
@collinpreston collinpreston added the Needs Review An open Pull Request that is ready for review label Nov 9, 2023
@mbertrand mbertrand self-assigned this Nov 9, 2023
README.md Outdated

Provder endpoint where client sends requests for identity claims.

- `FEATURE_KEYCLOAK_ENABLED`
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be used anymore. Keycloak authentication is enabled if I leave this out altogether or set it to False. Remove it here and from app.json?

"django.middleware.common.CommonMiddleware",
"django.middleware.csrf.CsrfViewMiddleware",
# "django.middleware.csrf.CsrfViewMiddleware",
Copy link
Member

Choose a reason for hiding this comment

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

Uncomment this line

@@ -98,6 +98,7 @@ xbundle = "^0.3.1"
social-auth-core = {extras = ["openidconnect"], version = "^4.4.2"}
nh3 = "^0.2.14"
retry2 = "^0.9.5"
django-user-sessions = "^2.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Was this primarily for debugging purposes? I experimented with removing this and using the usual MIDDLEWARE and SESSION_ENGINE settings, everything still seemed to work correctly. I'm a little paranoid about changing those values if not necessary. If it is primarily for debugging, maybe the MIDDLEWARE and SESSION_ENGINE values could be updated conditionally depending on whether or not a KEYCLOAK_DEBUG setting = True?

Copy link
Member

Choose a reason for hiding this comment

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

nvm, it works for 1 browser but logs me out of both if I'm signed into two different ones

Copy link
Member

Choose a reason for hiding this comment

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

Actually something still seems off after reverting back to the usual PR:

Flow A): Log into browser 1. Log into browser 2. Log out of browser 2. Correctly logged out of keycloak and mitopen in browser 2 while still logged into both in browser 1.

Flow B): Log into browser 1. Log into browser 2. Log out of browser 1. Incorrectly logged out of keycloak in browser 2 while still logged into mitopen; still logged into browser 1 keycloak url while logged out of mitopen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is being used to end the Django user's session after receiving a POST request from Keycloak. Django does not provide a way of ending a user's session without that user making a request as the Django session is normally tied to the request.

@mbertrand mbertrand added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Nov 9, 2023
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Something seems off while testing this w/ the same user logging in/out from 2 different browsers, against both the RC instance of keycloak and a local instance. Maybe this is just a consequence of testing against a local docker instance of mitopen, I'm not sure:

  1. Log in as user1 on browser A. Keycloak admin shows one session for user1
  2. Log in as user1 on browser B. Keycloak admin shows two sessions for user1
  3. Log out from browser A. Keycloak admin shows that the session created from browser B is removed, the session created from browser A still exists
  4. Log out from browser B. Keycloak admin is still showing the session created from browser A still exists.
  5. Log back in from browser A (ie http://open.ol.local:8063/login/ol-oidc). Immediately redirected back to mitopen without the keycloak login screen, likely because the session still exists on keycloak - this does not seem right. Would expect to have to log in to keycloak again,
  6. Log back in from browser B (ie http://open.ol.local:8063/login/ol-oidc). Need to login on keycloak again, as expected.

I'm okay with approving this and seeing how things go on RC if you think it's a side effect of running mitopen locally in a docker container and not fully communicating with local/RC keycloak as it would in a live setting.

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

Successfully merging this pull request may close these issues.

Back-end logout flow
2 participants