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

JWT expiration check #43

Merged
merged 4 commits into from
Jul 21, 2024
Merged

JWT expiration check #43

merged 4 commits into from
Jul 21, 2024

Conversation

Scurrra
Copy link
Contributor

@Scurrra Scurrra commented Jul 20, 2024

Motivation:

Doing a project I found this library for easy oauth. Because of lack of experience in the field I decided to learn how the library works. I had a big question on how jwts work, especially in this library. While doing this I found out that there is no check if the token is expired. I added this check to OAuth2Backend.authenticate. I also fixed deprecated datetime.utcnow() function.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you updated the documentation related to the changes you have made?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

Copy link
Member

@ArtyomVancyan ArtyomVancyan left a comment

Choose a reason for hiding this comment

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

Hi @Scurrra, thanks for pointing it out. I suggest changing the UTC alias with timezone.utc as it is not available in all versions of the datetime package.

src/fastapi_oauth2/middleware.py Outdated Show resolved Hide resolved
src/fastapi_oauth2/middleware.py Outdated Show resolved Hide resolved
src/fastapi_oauth2/middleware.py Outdated Show resolved Hide resolved
@Scurrra Scurrra requested a review from ArtyomVancyan July 21, 2024 13:23
@ArtyomVancyan
Copy link
Member

@Scurrra, you requested me a review, but I requested a few changes before merging (you shouldn't click "Resolve", you should do "Commit suggestion"). I will wait for your update. Also, I didn't get what was wrong with the old datetime.utcnow(). Isn't that the same as what you suggested (datetime.now(UTC))?

@Scurrra
Copy link
Contributor Author

Scurrra commented Jul 21, 2024

you should do "Commit suggestion"

My bad, sorry.

datetime.utcnow() will be deprecated.

@ArtyomVancyan ArtyomVancyan merged commit 1e6d30a into pysnippet:master Jul 21, 2024
12 checks passed
@Scurrra
Copy link
Contributor Author

Scurrra commented Jul 23, 2024

I don't know is it my fault or not, but playground is broken.

image

Probably this is because of troubles with OAUTH2_GITHUB_CLIENT_ID in playground, which wasn't affected by me.

Update: same with github auth option.

@ArtyomVancyan
Copy link
Member

It was a missdeployment issue, I have fixed it. Thanks for letting me know. Also, it could not be your fault, as these changes haven't been released yet :)

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.

2 participants