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