fix: JTI revocation returns error, LogoutHandler returns 500 on failure
This commit is contained in:
+10
-10
@@ -38,35 +38,35 @@ func generateJTI() (string, error) {
|
||||
b[0:4], b[4:6], b[6:8], b[8:10], b[10:16]), nil
|
||||
}
|
||||
|
||||
// RevokeJTI adds a JTI to the revoked set in PostgreSQL
|
||||
func RevokeJTI(ctx context.Context, jti string, expiresAt time.Time) {
|
||||
// RevokeJTI adds a JTI to the revoked set in PostgreSQL.
|
||||
// Returns an error if the operation fails.
|
||||
func RevokeJTI(ctx context.Context, jti string, expiresAt time.Time) error {
|
||||
if db.Conn == nil {
|
||||
return
|
||||
return fmt.Errorf("revoke JTI: db.Conn is nil")
|
||||
}
|
||||
tx, err := db.Conn.Begin(ctx)
|
||||
if err != nil {
|
||||
log.Printf("WARN: Failed to begin transaction for JTI revocation: %v", err)
|
||||
return
|
||||
return fmt.Errorf("revoke JTI: begin transaction: %w", err)
|
||||
}
|
||||
defer func() {
|
||||
if err := tx.Rollback(ctx); err != nil && err.Error() != "tx is closed" {
|
||||
slog.Error("failed to rollback transaction", "err", err)
|
||||
}
|
||||
}()
|
||||
}()
|
||||
|
||||
_, err = tx.Exec(ctx,
|
||||
`INSERT INTO revoked_jtis (jti, expires_at) VALUES ($1, $2)
|
||||
ON CONFLICT (jti) DO NOTHING`,
|
||||
jti, expiresAt)
|
||||
if err != nil {
|
||||
// Log but don't fail - this is best effort
|
||||
log.Printf("WARN: Failed to revoke JTI %s: %v", jti, err)
|
||||
return
|
||||
return fmt.Errorf("revoke JTI %s: %w", jti, err)
|
||||
}
|
||||
|
||||
if err := tx.Commit(ctx); err != nil {
|
||||
log.Printf("WARN: Failed to commit transaction for JTI revocation: %v", err)
|
||||
return fmt.Errorf("revoke JTI: commit transaction: %w", err)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// IsJTIRevoked checks if a JTI is in the revoked set via PostgreSQL.
|
||||
|
||||
@@ -165,7 +165,9 @@ func TestVerifyToken_RevokedJTI(t *testing.T) {
|
||||
t.Fatalf("GenerateToken() failed: %v", err)
|
||||
}
|
||||
|
||||
RevokeJTI(ctx, jti, clock.Now().Add(30*24*time.Hour))
|
||||
if err := RevokeJTI(ctx, jti, clock.Now().Add(30*24*time.Hour)); err != nil {
|
||||
t.Fatalf("RevokeJTI() failed: %v", err)
|
||||
}
|
||||
|
||||
_, _, _, err = VerifyToken(token, ctx)
|
||||
if err == nil {
|
||||
@@ -225,7 +227,9 @@ func TestRevokeJTI_AddsToSet(t *testing.T) {
|
||||
t.Fatal("JTI should not be revoked before calling RevokeJTI")
|
||||
}
|
||||
|
||||
RevokeJTI(ctx, jti, clock.Now().Add(30*24*time.Hour))
|
||||
if err := RevokeJTI(ctx, jti, clock.Now().Add(30*24*time.Hour)); err != nil {
|
||||
t.Fatalf("RevokeJTI() failed: %v", err)
|
||||
}
|
||||
|
||||
if !IsJTIRevoked(ctx, jti) {
|
||||
t.Error("expected IsJTIRevoked to return true after RevokeJTI")
|
||||
@@ -255,7 +259,9 @@ func TestCleanupRevokedJTIs_RemovesExpired(t *testing.T) {
|
||||
}
|
||||
|
||||
// Add with future expiry so IsJTIRevoked sees it
|
||||
RevokeJTI(ctx, jti, clock.Now().Add(1*time.Hour))
|
||||
if err := RevokeJTI(ctx, jti, clock.Now().Add(1*time.Hour)); err != nil {
|
||||
t.Fatalf("RevokeJTI() failed: %v", err)
|
||||
}
|
||||
|
||||
if !IsJTIRevoked(ctx, jti) {
|
||||
t.Fatal("JTI should be in revoked set after RevokeJTI")
|
||||
@@ -285,7 +291,9 @@ func TestCleanupRevokedJTIs_KeepsValid(t *testing.T) {
|
||||
}
|
||||
|
||||
// Add with future expiry
|
||||
RevokeJTI(ctx, jti, clock.Now().Add(30*24*time.Hour))
|
||||
if err := RevokeJTI(ctx, jti, clock.Now().Add(30*24*time.Hour)); err != nil {
|
||||
t.Fatalf("RevokeJTI() failed: %v", err)
|
||||
}
|
||||
|
||||
if !IsJTIRevoked(ctx, jti) {
|
||||
t.Fatal("JTI should be in revoked set before cleanup")
|
||||
@@ -491,8 +499,10 @@ func TestRevokeJTI_NilConn(t *testing.T) {
|
||||
db.Conn = nil
|
||||
t.Cleanup(func() { db.Conn = savedConn })
|
||||
|
||||
// Should not panic when db.Conn is nil
|
||||
RevokeJTI(context.Background(), "test-jti", time.Now())
|
||||
// Should return an error when db.Conn is nil
|
||||
if err := RevokeJTI(context.Background(), "test-jti", time.Now()); err == nil {
|
||||
t.Error("expected error when db.Conn is nil, got nil")
|
||||
}
|
||||
}
|
||||
|
||||
// TestIsJTIRevoked_NilConn verifies that IsJTIRevoked returns false when db.Conn is nil.
|
||||
|
||||
@@ -1731,7 +1731,9 @@ func TestJTI_Revocation_PostgreSQL(t *testing.T) {
|
||||
t.Fatalf("token should be valid before revocation: %v", err)
|
||||
}
|
||||
|
||||
auth.RevokeJTI(ctx, jti, clock.Now().Add(1*time.Hour))
|
||||
if err := auth.RevokeJTI(ctx, jti, clock.Now().Add(1*time.Hour)); err != nil {
|
||||
t.Fatalf("RevokeJTI() failed: %v", err)
|
||||
}
|
||||
|
||||
if !auth.IsJTIRevoked(ctx, jti) {
|
||||
t.Error("JTI should be revoked after RevokeJTI call")
|
||||
|
||||
@@ -485,7 +485,10 @@ func RefreshTokenHandler(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
// Revoke the old token's JTI before issuing a new one (rotation)
|
||||
if oldJTI != "" {
|
||||
auth.RevokeJTI(r.Context(), oldJTI, clock.Now().Add(90*24*time.Hour)) // match refresh token lifetime
|
||||
// Best-effort revocation: log the error but continue with the refresh
|
||||
if err := auth.RevokeJTI(r.Context(), oldJTI, clock.Now().Add(90*24*time.Hour)); err != nil {
|
||||
slog.Error("refresh: failed to revoke old JTI", "oldJTI", oldJTI, "err", err)
|
||||
}
|
||||
}
|
||||
|
||||
// Generate new token
|
||||
@@ -510,7 +513,11 @@ func LogoutHandler(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
|
||||
// Revoke the JTI — match the access token lifetime (1 hour)
|
||||
auth.RevokeJTI(r.Context(), jti, clock.Now().Add(1*time.Hour))
|
||||
if err := auth.RevokeJTI(r.Context(), jti, clock.Now().Add(1*time.Hour)); err != nil {
|
||||
slog.Error("logout: failed to revoke JTI", "err", err)
|
||||
http.Error(w, "Failed to revoke token. Please try again.", http.StatusInternalServerError)
|
||||
return
|
||||
}
|
||||
|
||||
_ = json.NewEncoder(w).Encode(map[string]bool{"success": true})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user