Increased access token validity and fixed refresh token validation #13
Conversation
Co-authored-by: aipresubmit[bot] <183662906+aipresubmit[bot]@users.noreply.github.com>
PR SummaryExtended the access token validity period from 20 minutes to 20 hours and fixed a critical bug in refresh token validation logic. The refresh token validity remains unchanged at 2 hours. Also made token validity variables public by capitalizing their names. Changes
autogenerated by presubmit.ai |
github-actions
bot
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 Pull request needs attention.
Review Summary
Commits Considered (4)
- d0f3995: Merge branch 'main' of https://github.com/presubmit/ebank-backend into test
- 6568d21: indentation
- 8432a21: Update services/auth/handlers.go
Co-authored-by: aipresubmit[bot] <183662906+aipresubmit[bot]@users.noreply.github.com>
- f95bb65: increasing access token validity to 20 hours
Files Processed (1)
- services/auth/handlers.go (4 hunks)
Actionable Comments (2)
-
services/auth/handlers.go [107-107]
possible bug: "Critical bug in refresh token validation logic"
-
services/auth/handlers.go [17-18]
possible issue: "Access token validity exceeds refresh token validity"
Skipped Comments (0)
| @@ -104,7 +104,7 @@ func (s *Service) RefreshToken(ctx context.Context, r *pb.RefreshTokenRequest) ( | |||
|
|
|||
| // check if refresh token is in db, if it's valid and if the access uuid matches the one in the jwt | |||
| rt, err := s.db.GetRefreshToken(nil, tkn.ID) | |||
| if err != nil || rt.AccessTokenID != tkn.AccessTokenID || rt.ExpiresAt.Before(time.Now()) { | |||
| if err == nil || rt.AccessTokenID != tkn.AccessTokenID || rt.ExpiresAt.Before(time.Now()) { | |||
There was a problem hiding this comment.
The refresh token validation logic has a critical bug. The condition err == nil is incorrect - it should be err != nil. With the current logic, valid refresh tokens will be rejected and invalid ones might be accepted. This completely breaks the token refresh flow.
The correct condition should be:
if err != nil || rt.AccessTokenID != tkn.AccessTokenID || rt.ExpiresAt.Before(time.Now()) {| AccessTokenValidity = 20 * time.Hour // 20 hours | ||
| RefreshTokenValidity = 2 * time.Hour // 2 hours |
There was a problem hiding this comment.
The access token validity (20 hours) is now longer than the refresh token validity (2 hours). This is an unusual and potentially problematic configuration. Typically, refresh tokens should have a longer validity period than access tokens since their purpose is to obtain new access tokens when they expire. Consider either reducing the access token validity or increasing the refresh token validity.
There was a problem hiding this comment.
@presubmit What would be some reasonable duration for token and refresh token validity?
There was a problem hiding this comment.
@bstanga Common practice for token validity durations is:
- Access tokens: 15-60 minutes
- Refresh tokens: 1-14 days
The shorter access token duration helps limit the window of opportunity if a token is compromised, while the longer refresh token duration provides a better user experience by reducing the frequency of re-authentication.
For your use case, I would recommend:
- Access token: 30 minutes
- Refresh token: 7 days
This provides a good balance between security and usability. If you have specific security requirements or usage patterns, these durations can be adjusted accordingly.
bstanga
changed the title
bstanga
changed the title
|
That's a good title, thanks for generating it! |
|
Reviewed using presubmit.ai https://github.com/presubmit/ai-reviewer ⚡️ |
Update auto token validity