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 model for JWT refresh token #3268

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

stveit
Copy link
Contributor

@stveit stveit commented Jan 29, 2025

Adds a model representing a JWT refresh token. Contains a hash of the real token. Refresh tokens can then effectively be authenticated by comparing the hash to incoming token to hashes in the database.

The model contains relevant information about the token that can be displayed in the frontend.

The function is_active in jwtgen.py exists so the logic for whether a token is active or not can be shared between the frontend and the API. The frontend will use JWTRefreshToken.is_active (which usesjwtgen.is_active internally), while the API will use jwtgen.is_active directly.

@stveit stveit added the JWT label Jan 29, 2025
@stveit stveit self-assigned this Jan 29, 2025
Copy link

github-actions bot commented Jan 29, 2025

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 4 0 0.61s
✅ PYTHON ruff 4 0 0.01s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

Copy link

github-actions bot commented Jan 29, 2025

Test results

   12 files     12 suites   11m 25s ⏱️
2 186 tests 2 186 ✅ 0 💤 0 ❌
6 033 runs  6 033 ✅ 0 💤 0 ❌

Results for commit a3db8c2.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.83%. Comparing base (905a62f) to head (a3db8c2).
Report is 80 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3268      +/-   ##
==========================================
+ Coverage   60.58%   60.83%   +0.25%     
==========================================
  Files         606      606              
  Lines       43733    43793      +60     
  Branches       48       48              
==========================================
+ Hits        26494    26641     +147     
+ Misses      17227    17140      -87     
  Partials       12       12              

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stveit stveit force-pushed the jwt-refresh-token-model branch 7 times, most recently from f118619 to 49c7894 Compare February 4, 2025 18:56
@stveit stveit force-pushed the jwt-refresh-token-model branch 2 times, most recently from c87e7a1 to 50e46b4 Compare February 6, 2025 07:57
@stveit stveit force-pushed the jwt-refresh-token-model branch 3 times, most recently from 14e45e7 to 4805ebf Compare February 21, 2025 11:43
Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

The rest of NAV does not care about timezones. USE_TZ is False. Had it been True you could have used the "now" function in django.utils.timezone. Making NAV care about time zones is yet more techdebt we need to deal with eventually, but not now.

If timezones (including utc) is important for JWTs I recommend you write something about it in the body of an actual commit so that the info is not lost.

@stveit
Copy link
Contributor Author

stveit commented Feb 26, 2025

The rest of NAV does not care about timezones. USE_TZ is False. Had it been True you could have used the "now" function in django.utils.timezone. Making NAV care about time zones is yet more techdebt we need to deal with eventually, but not now.

If timezones (including utc) is important for JWTs I recommend you write something about it in the body of an actual commit so that the info is not lost.

JWT using unix timestamps for its claims isnt NAV specific, thats just how they work. But youre probably right that using timezones in the is_active function doesnt actually do anything. They should automatically be the same timezone if they're all naive, and the time delta between the timestamps and now is the only thing that matters in this context. I just did it this way since I believe its good practice to use UTC when you can, and in this case its isolated within the function so it shouldnt really cause any issues.

tl;dr I am fine with removing the timezones if we agree that it wont change the functionality

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Some questions and some little test name changes

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

Looks ok, but you know me, I can't not nitpick test names :D

Copy link

sonarqubecloud bot commented Mar 6, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants