fix: added logout user before sso pipeline starts#2
Conversation
robrap
left a comment
There was a problem hiding this comment.
Thanks. I added questions for follow-up.
robrap
left a comment
There was a problem hiding this comment.
Thanks @Akanshu-2u. I found all this very helpful. Why don't you proceed with some unit tests and a temporary rollout toggle. You can add observability for the state of the toggle and for when the user is logged out.
Co-authored-by: Robert Raposa <rraposa@gmail.com>
fd1aa99 to
2968468
Compare
jcapphelix
left a comment
There was a problem hiding this comment.
@Akanshu-2u I've given few changes.
|
This can go for further approvals. |
robrap
left a comment
There was a problem hiding this comment.
Thanks. I'm looking forward to getting this more comprehensive fix out.
Co-authored-by: Robert Raposa <rraposa@gmail.com>
Co-authored-by: Robert Raposa <rraposa@gmail.com>
Co-authored-by: Robert Raposa <rraposa@gmail.com>
Co-authored-by: Robert Raposa <rraposa@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Introduces a session cleanup step before initiating a new OAuth2 flow to avoid user association conflicts by logging out any currently authenticated user, guarded by a new feature toggle and enhanced observability.
- Adds ENABLE_OAUTH_SESSION_CLEANUP toggle plus monitoring/logging in start()
- Implements comprehensive tests for start() behavior across toggle and request/user states
- Updates test dependency versions (one version pin appears potentially invalid)
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| requirements/test.txt | Bumps multiple dependency versions (one questionable version pin: cffi==2.0.0). |
| auth_backends/backends.py | Adds toggle, logging, and session cleanup logic to OAuth start() with observability attributes. |
| auth_backends/tests/test_backends.py | Adds parametrized tests covering authenticated/unauthenticated, missing request, and request without user paths. |
| CHANGELOG.rst | Reclassifies a patch release section to Added with new feature notes (potential semver concern). |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Robert Raposa <rraposa@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Robert Raposa <rraposa@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Robert Raposa <rraposa@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
auth_backends/backends.py:1
- Custom attribute keys used in set_custom_attribute do not match the documented custom_attribute_name comments or the expectations in the new tests (which assert calls like 'start.session_cleanup_toggle_enabled', 'start.user_authenticated_before_cleanup', and 'start.session_cleanup_performed'). This mismatch will cause the tests to fail and leads to inconsistent monitoring data. Align the keys by changing 'session_cleanup.toggle_enabled' -> 'start.session_cleanup_toggle_enabled', 'session_cleanup.logout_required' -> 'start.user_authenticated_before_cleanup', and 'session_cleanup.logout_performed' -> 'start.session_cleanup_performed'.
"""Django authentication backends.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@Akanshu-2u: Also, can you please update the PR description in general, and also add a link to our private jira ticket (and a link in the ticket to this PR, if that doesn't exist). Thanks. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
jcapphelix
left a comment
There was a problem hiding this comment.
All my comments are addressed.
PR description and JIRA links to be added, post that, it can be sent for re-review.
Addressed. |
robrap
left a comment
There was a problem hiding this comment.
Minor fixes that I will commit and then squash and merge.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| user_authenticated = ( | ||
| request is not None and | ||
| hasattr(request, 'user') and |
There was a problem hiding this comment.
The hasattr(request, 'user') check is unnecessary and potentially misleading. Django's request objects always have a user attribute (either an authenticated user or AnonymousUser). This check should be removed as it adds no value and could mask real issues.
| hasattr(request, 'user') and |
| set_custom_attribute('session_cleanup.logout_required', user_authenticated) | ||
|
|
||
| if user_authenticated and ENABLE_OAUTH_SESSION_CLEANUP.is_enabled(): | ||
| existing_username = getattr(request.user, 'username', 'unknown') |
There was a problem hiding this comment.
Using getattr with a default value of 'unknown' is inappropriate here. Since we've already verified request.user.is_authenticated is True, the user object is guaranteed to have a username attribute. This should be simplified to request.user.username to avoid logging misleading 'unknown' values.
| existing_username = getattr(request.user, 'username', 'unknown') | |
| existing_username = request.user.username |
Description:
Users logging in through OAuth SSO could retain session data from previous logins, creating potential security risks and confusion when switching between accounts.
Solution:
Added
logout(request)call inEdXOAuth2.start()method to clear existing sessions before OAuth authentication begins.JIRA:
BOMS-3