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

Add JWT refresh token model #2569

Closed
wants to merge 30 commits into from
Closed

Conversation

stveit
Copy link
Contributor

@stveit stveit commented Feb 6, 2023

Adds model representing JWT refresh token, with class functions for generating both refresh and access tokens.

The model is basically a wrapper around a JWT, with some functions to interact with it. The intended use is for the API view accepting refresh tokens to use JWTRefreshToken.objects.get(token=sometoken) to see if the refresh token is real, and use is_active() to see if its still active. If the token active, then an access token is generated and returned to the user, alongside a new refresh token overriding the current one. expire() exists in case a refresh token is leaked, or any other reason why you would want to effectively disable a refresh token.

A JWT being stored in the database does go against the idea of having all necessary info inside the token,
but this is more or less necessary since it allows deactivating a refresh token. If an attacker got a hold of an active refresh token
with no way for the NAV admins to disable the token, then the attacker can generate access tokens forever.

The alternative is using a blacklist, maybe some way to detect if a refresh token is used twice or something along those lines.
I figured a whitelist approach is more reasonable. Access tokens will still exist entirely by themselves, with no reference in the DB. This works fine since they have a limited lifespan, so its not the end of the world if an attacker gets a hold of one.

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

Test results

       8 files         8 suites   4m 19s ⏱️
3 335 tests 3 335 ✔️ 0 💤 0
4 986 runs  4 986 ✔️ 0 💤 0

Results for commit 58b9535.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Attention: Patch coverage is 97.87234% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 55.51%. Comparing base (e879e67) to head (a9cd2d1).
Report is 453 commits behind head on master.

❗ Current head a9cd2d1 differs from pull request most recent head 58b9535. Consider uploading reports for the commit 58b9535 to get more accurate results

Files Patch % Lines
python/nav/models/api.py 97.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2569      +/-   ##
==========================================
+ Coverage   55.19%   55.51%   +0.31%     
==========================================
  Files         561      567       +6     
  Lines       40917    41221     +304     
==========================================
+ Hits        22584    22883     +299     
- Misses      18333    18338       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stveit stveit self-assigned this Feb 6, 2023
@stveit stveit force-pushed the jwt-refresh-model branch from 0280cc8 to 10b0c13 Compare February 6, 2023 21:16
@sonarcloud
Copy link

sonarcloud bot commented Feb 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@stveit stveit marked this pull request as draft February 8, 2023 07:55
@stveit stveit force-pushed the jwt-refresh-model branch from 10b0c13 to b7a8fa6 Compare November 7, 2023 12:07
stveit added 14 commits November 7, 2023 14:11
Now shows very clearly that you are getting exp calim or nbf claim
no longer needed for testing, found better way
doesnt actually do anything. if you set it to the past, then its valid
and if you set it to the future then it will eventually be valid.
setting exp to the past is enought to deactivate it for good
doesnt really add anything
@stveit stveit marked this pull request as ready for review November 13, 2023 09:58
"data" and "body" were both used for the same thing, so
changed to only use one of the terms for clarity
Copy link

sonarcloud bot commented Nov 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@stveit stveit force-pushed the jwt-refresh-model branch from 08c1893 to 5921a3e Compare April 29, 2024 11:55
Copy link

sonarcloud bot commented Apr 29, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@stveit
Copy link
Contributor Author

stveit commented Jul 24, 2024

Closing this on the premise that storing JWTs really does go against the point of it, and if we want to store refresh tokens in the database then we should probably have a different approach alltogether

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.

1 participant