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

Terminate all UAA sessions at the server level #814

Merged
merged 3 commits into from
Jun 14, 2018

Conversation

rogeruiz
Copy link
Contributor

This addresses #810 with the following solution.

If the user is authenticated
  Clear Django user session
  Redirect to UAA server with Client ID and Redirect parameters
  Redirect to application logout page
If the user isn't authenticated
  Show the application logout page

@rogeruiz
Copy link
Contributor Author

In order to deploy this to production, we need to add the https://tock.18f.gov/logout URL to the redirects array in the client.

@rogeruiz rogeruiz force-pushed the terminate-all-uaa-sessions-on-logout branch from a46e519 to 97ed1d9 Compare June 13, 2018 17:45
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Woohoo, thank you so much for tracking this down and following up on it, @rogeruiz!

@@ -12,7 +12,7 @@ def successful_login(sender, request, user, **kwargs):


def successful_logout(sender, request, user, **kwargs):
logger.info(f'Successful logout event for {user.username}.')
logger.info(f'Successful logout event for {user}.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to leave this as user.username, or are you going for the repr/str defined on the model instead by doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value that got sent over after the redirect was no longer a Django user but rather the string of the user. This was occurring after the redirect I believe. when I user user.username it was hitting an error. I figured it's also easier to just rely on the str defined on the model anyway here. It never felt right to drill down into when the string representation of the User model could change and be more useful.

@rogeruiz
Copy link
Contributor Author

Whoops, gotta fix the tests here! 🐠

@rogeruiz rogeruiz requested a review from ccostino June 14, 2018 12:58
@codecov-io
Copy link

Codecov Report

Merging #814 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #814      +/-   ##
==========================================
- Coverage   90.85%   90.82%   -0.03%     
==========================================
  Files          38       38              
  Lines        1716     1722       +6     
==========================================
+ Hits         1559     1564       +5     
- Misses        157      158       +1
Impacted Files Coverage Δ
tock/views.py 71.87% <0%> (+2.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe78bec...a555071. Read the comment docs.

Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks @rogeruiz! Let's verify once more in staging too, once it's in there.

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

Successfully merging this pull request may close these issues.

3 participants