nav icon indicating copy to clipboard operation
nav copied to clipboard

Add JWT refresh token model

Open stveit opened this issue 3 years ago • 5 comments

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.

stveit avatar Feb 06 '23 08:02 stveit

Test results

       8 files         8 suites   4m 19s :stopwatch: 3 335 tests 3 335 :heavy_check_mark: 0 :zzz: 0 :x: 4 986 runs  4 986 :heavy_check_mark: 0 :zzz: 0 :x:

Results for commit 58b95356.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Feb 06 '23 08:02 github-actions[bot]

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.

:exclamation: 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 :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 06 '23 08:02 codecov[bot]

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

sonarqubecloud[bot] avatar Feb 06 '23 21:02 sonarqubecloud[bot]

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

sonarqubecloud[bot] avatar Nov 15 '23 13:11 sonarqubecloud[bot]

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

stveit avatar Jul 24 '24 07:07 stveit