From b1cfa932bea0d06291e8f4ee5809cc442047426e Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 26 Sep 2026 02:21:16 -0400 Subject: [PATCH] fix(api): make logout idempotent and hide grants left on a stale epoch Logout answered an undeclared 404 when its grant was already gone, for example revoked by another node inside the liveness cache window or by a concurrent logout. It now treats a missing grant as success and still evicts the cache entry, so logout always answers 200. Grants left on an older user epoch (after a password reset through the existing UI, or a login that raced a password change) are dead but only deleted when presented. Listing and counting grants now filter on the user's current epoch, so those grants no longer show up. dropGrant now deletes before evicting, like RevokeGrant, so a concurrent cache fill cannot re-cache a grant that is being dropped. --- core/apiauth/service.go | 15 ++++++++++---- core/apiauth/service_session_test.go | 29 ++++++++++++++++++++++++++++ model/grant.go | 4 ++-- persistence/grant_repository.go | 13 +++++++------ persistence/grant_repository_test.go | 13 ++++++++----- server/apiv1/auth_test.go | 11 +++++++++++ 6 files changed, 68 insertions(+), 17 deletions(-) diff --git a/core/apiauth/service.go b/core/apiauth/service.go index 04012c77c..29b5075b3 100644 --- a/core/apiauth/service.go +++ b/core/apiauth/service.go @@ -192,11 +192,12 @@ func (s *Service) loadUser(ctx context.Context, userID string) (*model.User, err return u, err } +// dropGrant evicts after deleting, so a concurrent fill cannot re-cache the dead grant. func (s *Service) dropGrant(ctx context.Context, id string) { - s.cache.evict(id) if err := s.ds.Grant().Delete(ctx, id); err != nil { log.Warn(ctx, "API v1: could not delete dead grant", "grant", id, err) } + s.cache.evict(id) } // settleEpoch re-reads grant and user in one read transaction: separate reads can straddle a password @@ -303,11 +304,11 @@ func (s *Service) liveGrant(ctx context.Context, id string, u *model.User) (live func (s *Service) ListGrants(ctx context.Context, p *Principal, offset, limit int) (model.Grants, int64, error) { idleSince := s.now().Add(-IdleExpiry) - grants, err := s.ds.Grant().GetAllForUser(ctx, p.User.ID, idleSince, offset, limit) + grants, err := s.ds.Grant().GetAllForUser(ctx, p.User.ID, p.User.TokenEpoch, idleSince, offset, limit) if err != nil { return nil, 0, err } - total, err := s.ds.Grant().CountForUser(ctx, p.User.ID, idleSince) + total, err := s.ds.Grant().CountForUser(ctx, p.User.ID, p.User.TokenEpoch, idleSince) return grants, total, err } @@ -319,8 +320,14 @@ func (s *Service) RevokeGrant(ctx context.Context, p *Principal, grantID string) return nil } +// Logout succeeds when the grant is already gone, e.g. revoked by another node or a concurrent logout. func (s *Service) Logout(ctx context.Context, p *Principal) error { - return s.RevokeGrant(ctx, p, p.GrantID) + err := s.RevokeGrant(ctx, p, p.GrantID) + if errors.Is(err, model.ErrNotFound) { + s.cache.evict(p.GrantID) + return nil + } + return err } // ChangePassword does every check inside the locked transaction, so a reset that lands first is never overwritten. diff --git a/core/apiauth/service_session_test.go b/core/apiauth/service_session_test.go index 929899422..de0d19033 100644 --- a/core/apiauth/service_session_test.go +++ b/core/apiauth/service_session_test.go @@ -199,6 +199,35 @@ var _ = Describe("Service: sessions", func() { Expect([]string{grants[0].ID, grants[1].ID}).To(ContainElements(first.Grant.ID, p.GrantID)) }) + It("lists only grants on the user's current epoch", func() { + u := createUser(ctx, "pw", false) + login(u) + u.NewPassword = "reset-by-admin" // old-UI reset leaves the old grant on the previous epoch + Expect(realDS.User().Put(ctx, &u)).To(Succeed()) + issued, err := svc.Login(ctx, u.UserName, "reset-by-admin", meta, nil) + Expect(err).ToNot(HaveOccurred()) + p, err := svc.ResolveGrant(ctx, issued.Secret, "") + Expect(err).ToNot(HaveOccurred()) + + grants, total, err := svc.ListGrants(ctx, p, 0, 10) + Expect(err).ToNot(HaveOccurred()) + Expect(total).To(Equal(int64(1))) + Expect(grants).To(HaveLen(1)) + Expect(grants[0].ID).To(Equal(issued.Grant.ID)) + }) + + It("logs out successfully when the grant is already gone", func() { + u := createUser(ctx, "pw", false) + _, p, tok := login(u) + _, err := svc.Authenticate(ctx, tok.Token, "") + Expect(err).ToNot(HaveOccurred()) + Expect(realDS.Grant().Delete(ctx, p.GrantID)).To(Succeed()) // another node + + Expect(svc.Logout(ctx, p)).To(Succeed()) + _, err = svc.Authenticate(ctx, tok.Token, "") + Expect(err).To(MatchError(model.ErrInvalidAuth)) + }) + It("refuses to revoke another user's grant", func() { alice := createUser(ctx, "pw", false) bob := createUser(ctx, "pw", false) diff --git a/model/grant.go b/model/grant.go index 875c3bc86..f1475c93f 100644 --- a/model/grant.go +++ b/model/grant.go @@ -50,8 +50,8 @@ type GrantRepository interface { Put(ctx context.Context, g *Grant) error Get(ctx context.Context, id string) (*Grant, error) FindBySecretHash(ctx context.Context, hash string) (*Grant, error) - GetAllForUser(ctx context.Context, userID string, idleSince time.Time, offset, limit int) (Grants, error) - CountForUser(ctx context.Context, userID string, idleSince time.Time) (int64, error) + GetAllForUser(ctx context.Context, userID string, epoch int, idleSince time.Time, offset, limit int) (Grants, error) + CountForUser(ctx context.Context, userID string, epoch int, idleSince time.Time) (int64, error) Delete(ctx context.Context, id string) error DeleteForUser(ctx context.Context, userID, id string) error DeleteOtherEpochs(ctx context.Context, userID string, epoch int) error diff --git a/persistence/grant_repository.go b/persistence/grant_repository.go index 7a9ca9fe8..fcd58bfa3 100644 --- a/persistence/grant_repository.go +++ b/persistence/grant_repository.go @@ -60,12 +60,13 @@ func (r *grantRepository) findOne(ctx context.Context, cond Sqlizer) (*model.Gra return &g, nil } -func (r *grantRepository) activeForUser(userID string, idleSince time.Time) Sqlizer { - return And{Eq{"user_id": userID}, Expr(grantLastActivity+" >= ?", idleSince.UTC())} +// activeForUser skips grants left on an older epoch: they are dead but only deleted when presented. +func (r *grantRepository) activeForUser(userID string, epoch int, idleSince time.Time) Sqlizer { + return And{Eq{"user_id": userID, "user_epoch": epoch}, Expr(grantLastActivity+" >= ?", idleSince.UTC())} } -func (r *grantRepository) GetAllForUser(ctx context.Context, userID string, idleSince time.Time, offset, limit int) (model.Grants, error) { - sel := r.newSelect(ctx).Columns("*").Where(r.activeForUser(userID, idleSince)). +func (r *grantRepository) GetAllForUser(ctx context.Context, userID string, epoch int, idleSince time.Time, offset, limit int) (model.Grants, error) { + sel := r.newSelect(ctx).Columns("*").Where(r.activeForUser(userID, epoch, idleSince)). OrderBy("last_used_at IS NULL", "last_used_at desc", "created_at desc", "id"). Offset(uint64(offset)).Limit(uint64(limit)) var res model.Grants @@ -73,8 +74,8 @@ func (r *grantRepository) GetAllForUser(ctx context.Context, userID string, idle return res, err } -func (r *grantRepository) CountForUser(ctx context.Context, userID string, idleSince time.Time) (int64, error) { - return r.count(ctx, Select().Where(r.activeForUser(userID, idleSince))) +func (r *grantRepository) CountForUser(ctx context.Context, userID string, epoch int, idleSince time.Time) (int64, error) { + return r.count(ctx, Select().Where(r.activeForUser(userID, epoch, idleSince))) } func (r *grantRepository) Delete(ctx context.Context, id string) error { diff --git a/persistence/grant_repository_test.go b/persistence/grant_repository_test.go index f621a5cc2..bb140ebd6 100644 --- a/persistence/grant_repository_test.go +++ b/persistence/grant_repository_test.go @@ -54,7 +54,7 @@ var _ = Describe("GrantRepository", func() { Expect(err).To(MatchError(model.ErrNotFound)) }) - It("lists and counts only the user's non-idle grants by lastUsedAt, never-used ones last", func() { + It("lists and counts only the user's non-idle grants on the given epoch by lastUsedAt, never-used ones last", func() { old := newGrant(adminUser.ID, "h-old") old.CreatedAt = now.Add(-100 * 24 * time.Hour) usedEarly := newGrant(adminUser.ID, "h-used-early") @@ -67,18 +67,21 @@ var _ = Describe("GrantRepository", func() { usedLate.LastUsedAt = &lateUse freshNeverUsed := newGrant(adminUser.ID, "h-fresh") // newer than both uses, but never used other := newGrant(regularUser.ID, "h-other") - for _, g := range []*model.Grant{old, usedEarly, usedLate, freshNeverUsed, other} { + staleEpoch := newGrant(adminUser.ID, "h-stale-epoch") + staleEpoch.UserEpoch = 1 + for _, g := range []*model.Grant{old, usedEarly, usedLate, freshNeverUsed, other, staleEpoch} { Expect(repo.Put(ctx, g)).To(Succeed()) } idleSince := now.Add(-90 * 24 * time.Hour) - list, err := repo.GetAllForUser(ctx, adminUser.ID, idleSince, 0, 10) + list, err := repo.GetAllForUser(ctx, adminUser.ID, 0, idleSince, 0, 10) Expect(err).ToNot(HaveOccurred()) + Expect(list).To(HaveLen(3)) Expect([]string{list[0].ID, list[1].ID, list[2].ID}).To(Equal([]string{usedLate.ID, usedEarly.ID, freshNeverUsed.ID})) - Expect(repo.CountForUser(ctx, adminUser.ID, idleSince)).To(Equal(int64(3))) + Expect(repo.CountForUser(ctx, adminUser.ID, 0, idleSince)).To(Equal(int64(3))) - page, err := repo.GetAllForUser(ctx, adminUser.ID, idleSince, 1, 1) + page, err := repo.GetAllForUser(ctx, adminUser.ID, 0, idleSince, 1, 1) Expect(err).ToNot(HaveOccurred()) Expect(page).To(HaveLen(1)) Expect(page[0].ID).To(Equal(usedEarly.ID)) diff --git a/server/apiv1/auth_test.go b/server/apiv1/auth_test.go index da45a2141..d360e2b49 100644 --- a/server/apiv1/auth_test.go +++ b/server/apiv1/auth_test.go @@ -171,6 +171,17 @@ var _ = Describe("auth endpoints", func() { Expect(call(http.MethodPost, "/api/v1/auth/token", gc.Secret, nil).Code).To(Equal(http.StatusUnauthorized)) }) + It("logs out with 200 when another node already revoked the grant", func() { + gc := setup() + at := mint(gc.Secret, nil) + Expect(call(http.MethodGet, "/api/v1/auth/grants", at.AccessToken, nil).Code).To(Equal(http.StatusOK)) // caches the grant + Expect(realDS.Grant().Delete(ctx, gc.Grant.Id)).To(Succeed()) + + w := call(http.MethodPost, "/api/v1/auth/logout", at.AccessToken, nil) + Expect(w.Code).To(Equal(http.StatusOK), w.Body.String()) + Expect(w.Body.String()).To(ContainSubstring(`"logoutUrl":null`)) + }) + It("answers 404 for a grant id the caller does not own, and 400 for an over-long id", func() { gc := setup() tok := mint(gc.Secret, nil).AccessToken