Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@aibaq this is a great start. to help me understand the issue a bit more, in what scenario did you encounter this issue? I would expect that a user would have logged in at least once by the time get_id_token_dictionary was called. I'd like to understand how this was occurring to both correct my expectations and ensure there isn't a bug somewhere else that is allow a token to be generated for a user that isn't authenticated. It also looks like there is a code path that is uncovered that will need to be tested. |
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a TypeError that occurs at the /o/token/ endpoint when using OIDC and the Django User's last_login field is null. The fix adds a null check for last_login and provides a fallback value using the current timestamp.
- Added null safety check for
request.user.last_loginbefore accessing it - Set
timezone.now().timestamp()as fallback whenlast_loginis null - Added test case to verify the fix works with null
last_loginvalues
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| oauth2_provider/oauth2_validators.py | Added null check for last_login and fallback to current timestamp |
| tests/test_oauth2_validators.py | Added test setup to simulate user with null last_login |
| CHANGELOG.md | Added entry for the TypeError fix |
| AUTHORS | Added contributor name |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@aibaq We won't be able to close this without the test coverage and details so we can reproduce the bug. |
|
For context, we recently hit the same error after upgrading oauthlib from 3.2 to 3.3. The "id_token" is now included in refresh token responses when the the However, it's unclear for me how this should behave for users who interact only with OAuth applications. For these users, their |
|
@amatissart thanks for the additional context. I'm definitely open to the PR. As you noted the guidance with regard to last_login is unclear. I see there is a test for the case, but it explicitly sets user.last_login to None. I'd like a clear understanding of how this happens in the wild and for that to be clearly documented in the code at the conditional or in a test. The test explicitly sets the user.last_login to none, which is a bit synthetic. I feel like the field being nullabe is probably sufficient for having some sort of fallback... but that raises the question of how should we handle this when user.last_login is not set? Is now appropriate? Should auth_time be omitted in these cases, it is optional after all? Should we look to the token chain and look for a a created time on the earliest token there? Or are we missing a mechanism to track session time that allows us to provide a reliable auth_time that can be used for session expiration? If user.last_login is not set, should we set it and then use that for auth_time? I want to be sure we're doing the right thing, but I have limited time to research these things as a maintainer. We really depend on the support of the wider community and contributors who are focused on these issues to help us ensure we're doing what's right for the wider user community... |
| expiration_time = timezone.now() + timedelta(seconds=oauth2_settings.ID_TOKEN_EXPIRE_SECONDS) | ||
| if request.user.last_login: | ||
| auth_time = int(dateformat.format(request.user.last_login, "U")) | ||
| else: | ||
| auth_time = int(timezone.now().timestamp()) | ||
| # Required ID Token claims | ||
| claims.update( | ||
| **{ | ||
| "iss": self.get_oidc_issuer_endpoint(request), | ||
| "exp": int(dateformat.format(expiration_time, "U")), | ||
| "auth_time": int(dateformat.format(request.user.last_login, "U")), | ||
| "auth_time": auth_time, | ||
| "jti": str(uuid.uuid4()), |
There was a problem hiding this comment.
int(timezone.now().timestamp()) can differ from Django’s dateformat.format(..., 'U') behavior (notably when USE_TZ=False, where .timestamp() on naive datetimes is interpreted in local time). For consistency with the existing codepath and Django’s formatting semantics, compute the fallback using the same mechanism as the non-null branch (i.e., format timezone.now() with 'U').
| self.user.last_login = None | ||
| self.user.save() |
There was a problem hiding this comment.
Setting last_login = None in setUp() adds an extra DB write for every test in this class and makes the entire class depend on a special user state (even in tests unrelated to OIDC auth_time). Prefer moving this setup into the specific test(s) that exercise the last_login is None path, or create a dedicated user within that test to keep unrelated tests isolated and faster.
| self.user.last_login = None | |
| self.user.save() |
| * #1252 Fix crash when 'client' is in token request body | ||
| * #1496 Fix error when Bearer token string is empty but preceded by `Bearer` keyword. | ||
| * #1630 use token_checksum for lookup in _get_token_from_authentication_server | ||
| * #1597 Fix: TypeError at /s/auth/o/token/ |
There was a problem hiding this comment.
The PR description refers to the /o/token/ endpoint, but the changelog entry records /s/auth/o/token/. This looks inconsistent and may mislead users; update the changelog entry to match the actual endpoint and wording used by the project.
|
@aibaq can you address the copilot comments and rebase this branch? |
Fixes #
Description of the Change
Django User's last_login field is nullable, so
/o/token/endpoint raises TypeError when last_login is null when using oidcThis fix sets timezone.now() as a default value for auth_time
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS