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.

Sign up for GitHub

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

Jump to bottom

Increased access token validity and fixed refresh token validation #13

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Increased access token validity and fixed refresh token validation #13

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f95bb65
increasing access token validity to 20 hours
Oct 10, 2024
8432a21
Update services/auth/handlers.go
bstanga Oct 10, 2024
6568d21
indentation
Oct 10, 2024
d0f3995
Merge branch 'main' of https://github.com/presubmit/ebank-backend int…
Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions services/auth/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import (
)

var (
accessTokenValidity = 20 * time.Minute // 20 minutes
refreshTokenValidity = 2 * time.Hour // 2 hours
AccessTokenValidity = 20 * time.Hour // 20 hours
RefreshTokenValidity = 2 * time.Hour // 2 hours
Comment on lines +17 to +18
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@presubmit What would be some reasonable duration for token and refresh token validity?

Copy link

Choose a reason for hiding this comment

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

@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.

)

type Service struct {
Expand Down Expand Up @@ -57,15 +57,15 @@ func (s *Service) GenerateToken(ctx context.Context, r *pb.GenerateTokenRequest)
at := &tokens.AccessToken{
ID: uuid.New().String(),
UserID: userId,
ExpiresAt: time.Now().Add(accessTokenValidity),
ExpiresAt: time.Now().Add(AccessTokenValidity),
}
// generate jwt access token
accessToken, err := at.GenerateJWT(s.jwtAccessSecret)
if err != nil {
return nil, err
}
// save access token in cache
if err := s.cache.Set(at.ID, at.UserID, accessTokenValidity).Err(); err != nil {
if err := s.cache.Set(at.ID, at.UserID, AccessTokenValidity).Err(); err != nil {
return nil, err
}

Expand All @@ -74,7 +74,7 @@ func (s *Service) GenerateToken(ctx context.Context, r *pb.GenerateTokenRequest)
ID: uuid.New().String(),
UserID: userId,
AccessTokenID: at.ID,
ExpiresAt: time.Now().Add(refreshTokenValidity),
ExpiresAt: time.Now().Add(RefreshTokenValidity),
}
// generate jwt refresh token
refreshToken, err := rt.GenerateJWT(s.jwtRefreshSecret)
Expand Down Expand Up @@ -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()) {
Copy link

Choose a reason for hiding this comment

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

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()) {

return nil, er.NotAuthenticatedf("invalid refresh token")
}

Expand Down