remote/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-10-08 02:17:25 +02:00
Code Issues Projects Releases Packages Wiki Activity

refactor(api): simplify API v1 auth after dropping access tokens

Browse source 
- Fold Allowed into Expand and drop the ErrInsufficientScope sentinel; the
  gate's scopeError is now the only source of insufficient_scope.
- Replace the two-value authKind with a public flag, inline loadUser, and
  pass the grant to touch.
- Read a declared request body once before validation, so the JSON checks
  and the handler no longer depend on kin-openapi restoring the exact bytes.
- Merge the Service tests into one file and drop specs that only covered
  the removed liveness cache. The revoked-during-password-change spec now
  revokes after the gate authenticates, so it reaches ChangePassword again.

Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Deluan 2026-09-28 21:47:16 -04:00
commit 0628721006
11 changed files with 361 additions and 456 deletions
Download patch file Download diff file Expand all files Collapse all files

6
core/apiauth/db_test.go
View file

@ -33,9 +33,9 @@ func createUser(ctx context.Context, password string, admin bool) model.User {
return *stored
}
// login signs in a user whose password is "pw" and authenticates with the new grant secret.
func login(ctx context.Context, svc *Service, u model.User) (*Issued, *Principal) {
issued, err := svc.Login(ctx, u.UserName, "pw", meta, nil)
// login signs u in (nil scopes asks for all) and authenticates with the new grant secret.
func login(ctx context.Context, svc *Service, u model.User, password string, scopes []string) (*Issued, *Principal) {
issued, err := svc.Login(ctx, u.UserName, password, meta, scopes)
ExpectWithOffset(1, err).ToNot(HaveOccurred())
p, err := svc.Authenticate(ctx, issued.Secret, "")
ExpectWithOffset(1, err).ToNot(HaveOccurred())

7
core/apiauth/scopes.go
View file

@ -53,12 +53,7 @@ func Expand(granted []string, isAdmin bool) []string {
}
out = append(out, s)
}
return Allowed(out, isAdmin)
}
// Allowed keeps the concrete scopes the user may hold now; unlike Expand it never widens `all`.
func Allowed(scopes []string, isAdmin bool) []string {
out := slices.DeleteFunc(slices.Clone(scopes), func(s string) bool { return !grantable(s, isAdmin) })
out = slices.DeleteFunc(out, func(s string) bool { return !grantable(s, isAdmin) })
return normalize(out)
}

10
core/apiauth/scopes_test.go
View file

@ -39,16 +39,6 @@ var _ = Describe("scopes", func() {
})
})
Describe("Allowed", func() {
It("never widens all", func() {
Expect(Allowed([]string{ScopeAll}, true)).To(BeEmpty())
})
It("keeps known scopes, and admin only for admins", func() {
Expect(Allowed([]string{"read", "retired", "admin"}, false)).To(Equal([]string{"read"}))
Expect(Allowed([]string{"read", "admin"}, true)).To(Equal([]string{"admin", "read"}))
})
})
Describe("Satisfies", func() {
It("accepts the exact scope or its :write form", func() {
Expect(Satisfies([]string{"read"}, "read")).To(BeTrue())

27
core/apiauth/service.go
View file

@ -21,7 +21,6 @@ const (
)
var (
ErrInsufficientScope = errors.New("insufficient scope")
ErrPasswordManagedExternally = errors.New("password is managed externally")
ErrCurrentPasswordMismatch = errors.New("current password does not match")
)
@ -51,14 +50,13 @@ type Service struct {
}
func New(ds model.DataStore) *Service {
s := &Service{
return &Service{
ds: ds,
checkers: func(ds model.DataStore) []CredentialChecker {
return []CredentialChecker{dbChecker{ds: ds}}
},
now: time.Now,
}
return s
}
func PasswordChangeable(u model.User) bool {
@ -126,7 +124,10 @@ func (s *Service) Authenticate(ctx context.Context, secret, ip string) (*Princip
s.dropIdle(ctx, g.ID, idleSince)
return nil, model.ErrInvalidAuth
}
u, err := s.loadUser(ctx, g.UserID)
u, err := s.ds.User().Get(ctx, g.UserID)
if errors.Is(err, model.ErrNotFound) {
return nil, model.ErrInvalidAuth
}
if err != nil {
return nil, err
}
@ -135,18 +136,10 @@ func (s *Service) Authenticate(ctx context.Context, secret, ip string) (*Princip
return nil, err
}
}
s.touch(ctx, g.ID, ip, gg.V(g.LastUsedAt))
s.touch(ctx, g, ip)
return &Principal{User: *u, GrantID: g.ID, Scopes: Expand(g.Scopes, u.IsAdmin)}, nil
}
func (s *Service) loadUser(ctx context.Context, userID string) (*model.User, error) {
u, err := s.ds.User().Get(ctx, userID)
if errors.Is(err, model.ErrNotFound) {
return nil, model.ErrInvalidAuth
}
return u, err
}
// dropIdle deletes only still-idle grants, sparing one renewed meanwhile.
func (s *Service) dropIdle(ctx context.Context, id string, idleSince time.Time) {
if _, err := s.ds.Grant().DeleteIdle(ctx, idleSince); err != nil {
@ -183,13 +176,13 @@ func (s *Service) settleEpoch(ctx context.Context, grantID string) (*model.Grant
}
// touch writes last_used at most every touchInterval (zero lastUsed: never used); the SQL condition holds that across nodes.
func (s *Service) touch(ctx context.Context, id, ip string, lastUsed time.Time) {
func (s *Service) touch(ctx context.Context, g *model.Grant, ip string) {
now := s.now()
if !lastUsed.IsZero() && now.Before(lastUsed.Add(touchInterval)) {
if lastUsed := gg.V(g.LastUsedAt); !lastUsed.IsZero() && now.Before(lastUsed.Add(touchInterval)) {
return
}
if err := s.ds.Grant().Touch(ctx, id, ip, now, now.Add(-touchInterval)); err != nil {
log.Warn(ctx, "API v1: could not record grant use", "grant", id, err)
if err := s.ds.Grant().Touch(ctx, g.ID, ip, now, now.Add(-touchInterval)); err != nil {
log.Warn(ctx, "API v1: could not record grant use", "grant", g.ID, err)
}
}

338
core/apiauth/service_session_test.go
View file

@ -1,338 +0,0 @@
package apiauth
import (
"context"
"errors"
"time"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/conf/configtest"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)
var _ = Describe("Service: sessions", func() {
var ctx context.Context
var svc *Service
var now time.Time
BeforeEach(func() {
ctx = GinkgoT().Context()
DeferCleanup(configtest.SetupConfig())
now = time.Now().UTC().Truncate(time.Second)
svc = New(realDS)
svc.SetClock(func() time.Time { return now })
})
Describe("Authenticate", func() {
It("rejects the secret at once after logout", func() {
u := createUser(ctx, "pw", false)
issued, p := login(ctx, svc, u)
Expect(svc.Logout(ctx, p)).To(Succeed())
_, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("rejects the secret at once when another node revoked its grant", func() {
u := createUser(ctx, "pw", false)
issued, _ := login(ctx, svc, u)
Expect(realDS.Grant().DeleteForUser(ctx, u.ID, issued.Grant.ID)).To(Succeed())
_, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("kills grants when the password changes anywhere else", func() {
u := createUser(ctx, "pw", false)
issued, _ := login(ctx, svc, u)
u.NewPassword = "reset-by-admin"
Expect(realDS.User().Put(ctx, &u)).To(Succeed())
_, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("does not kill a grant kept by a password change made through another node", func() {
u := createUser(ctx, "pw", false)
issued, p := login(ctx, svc, u)
other := New(realDS) // another node
other.SetClock(func() time.Time { return now })
Expect(other.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", false)).To(Succeed())
_, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).ToNot(HaveOccurred())
})
It("does not delete a kept grant when the password changed between reading the grant and the user", func() {
u := createUser(ctx, "pw", false)
issued, p := login(ctx, svc, u)
racing := New(afterFindDS{DataStore: realDS, after: func() {
Expect(svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", false)).To(Succeed())
}})
racing.SetClock(func() time.Time { return now })
_, err := racing.Authenticate(ctx, issued.Secret, "")
Expect(err).ToNot(HaveOccurred())
_, err = realDS.Grant().Get(ctx, p.GrantID)
Expect(err).ToNot(HaveOccurred())
})
It("drops admin from a grant once its user is no longer an admin", func() {
saved := KnownScopes
KnownScopes = []string{ScopeRead, ScopePassword, ScopeAdmin}
DeferCleanup(func() { KnownScopes = saved })
u := createUser(ctx, "pw", true)
issued, p := login(ctx, svc, u)
Expect(p.Scopes).To(ContainElement(ScopeAdmin))
u.IsAdmin = false
Expect(realDS.User().Put(ctx, &u)).To(Succeed())
demoted, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).ToNot(HaveOccurred())
Expect(demoted.Scopes).To(Equal([]string{ScopePassword, ScopeRead}))
})
It("rejects the secret after its user is deleted, and the grant row is gone", func() {
u := createUser(ctx, "pw", false)
issued, _ := login(ctx, svc, u)
Expect(realDS.User().Delete(request.WithUser(ctx, model.User{IsAdmin: true}), u.ID)).To(Succeed())
_, err := realDS.Grant().Get(ctx, issued.Grant.ID)
Expect(err).To(MatchError(model.ErrNotFound))
_, err = svc.Authenticate(ctx, issued.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("leaves a login that raced a password change with a dead grant", func() {
u := createUser(ctx, "pw", false)
reached, release := make(chan struct{}), make(chan struct{})
svc.SetCheckers(func(ds model.DataStore) []CredentialChecker {
return []CredentialChecker{pausingChecker{inner: dbChecker{ds: ds}, reached: reached, release: release}}
})
var issued *Issued
var loginErr error
done := make(chan struct{})
go func() {
defer GinkgoRecover()
defer close(done)
issued, loginErr = svc.Login(ctx, u.UserName, "pw", meta, nil)
}()
<-reached // credentials (and the old epoch) were read
u.NewPassword = "changed-meanwhile"
Expect(realDS.User().Put(ctx, &u)).To(Succeed())
close(release)
<-done
Expect(loginErr).ToNot(HaveOccurred())
_, err := New(realDS).Authenticate(ctx, issued.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
})
Describe("grant management", func() {
It("lists the user's grants and marks the current one", func() {
u := createUser(ctx, "pw", false)
first, _ := login(ctx, svc, u)
_, p := login(ctx, svc, u)
grants, total, err := svc.ListGrants(ctx, p, 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(total).To(Equal(int64(2)))
Expect(grants).To(HaveLen(2))
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(ctx, svc, 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.Authenticate(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)
issued, p := login(ctx, svc, u)
Expect(realDS.Grant().DeleteForUser(ctx, u.ID, p.GrantID)).To(Succeed()) // another node
Expect(svc.Logout(ctx, p)).To(Succeed())
_, err := svc.Authenticate(ctx, issued.Secret, "")
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)
aliceGrant, _ := login(ctx, svc, alice)
_, bobP := login(ctx, svc, bob)
Expect(svc.RevokeGrant(ctx, bobP, aliceGrant.Grant.ID)).To(MatchError(model.ErrNotFound))
_, err := svc.Authenticate(ctx, aliceGrant.Secret, "")
Expect(err).ToNot(HaveOccurred())
})
It("rejects the secret at once after its grant is revoked", func() {
u := createUser(ctx, "pw", false)
other, _ := login(ctx, svc, u)
_, p := login(ctx, svc, u)
Expect(svc.RevokeGrant(ctx, p, other.Grant.ID)).To(Succeed())
_, err := svc.Authenticate(ctx, other.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
})
Describe("ChangePassword", func() {
It("revokes other grants by default and keeps the caller's", func() {
u := createUser(ctx, "pw", false)
other, _ := login(ctx, svc, u)
mine, p := login(ctx, svc, u)
Expect(svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true)).To(Succeed())
_, err := svc.Authenticate(ctx, mine.Secret, "")
Expect(err).ToNot(HaveOccurred())
_, err = svc.Authenticate(ctx, other.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
_, err = svc.Login(ctx, u.UserName, "pw2", meta, nil)
Expect(err).ToNot(HaveOccurred())
})
It("keeps every grant when revokeOthers is false", func() {
u := createUser(ctx, "pw", false)
other, _ := login(ctx, svc, u)
_, p := login(ctx, svc, u)
Expect(svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", false)).To(Succeed())
_, err := svc.Authenticate(ctx, other.Secret, "")
Expect(err).ToNot(HaveOccurred())
})
It("rejects a wrong current password without changing anything", func() {
u := createUser(ctx, "pw", false)
_, p := login(ctx, svc, u)
err := svc.ChangePassword(request.WithUser(ctx, p.User), p, "wrong", "pw2", true)
Expect(err).To(MatchError(ErrCurrentPasswordMismatch))
_, err = svc.Login(ctx, u.UserName, "pw", meta, nil)
Expect(err).ToNot(HaveOccurred())
})
It("is forbidden for non-admins when user editing is off", func() {
conf.Server.EnableUserEditing = false
u := createUser(ctx, "pw", false)
_, p := login(ctx, svc, u)
err := svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true)
Expect(err).To(MatchError(model.ErrNotAuthorized))
})
It("does not revive grants killed by an earlier reset when keeping grants", func() {
u := createUser(ctx, "pw", false)
killed, _ := login(ctx, svc, u)
u.NewPassword = "reset-by-admin" // old-UI reset: the killed grant stays on the old epoch until presented
Expect(realDS.User().Put(ctx, &u)).To(Succeed())
issued2, err := svc.Login(ctx, u.UserName, "reset-by-admin", meta, nil)
Expect(err).ToNot(HaveOccurred())
p2, err := svc.Authenticate(ctx, issued2.Secret, "")
Expect(err).ToNot(HaveOccurred())
Expect(svc.ChangePassword(request.WithUser(ctx, p2.User), p2, "reset-by-admin", "pw3", false)).To(Succeed())
_, err = svc.Authenticate(ctx, killed.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("rejects a caller whose grant was revoked before the change ran", func() {
u := createUser(ctx, "pw", false)
_, p := login(ctx, svc, u)
Expect(realDS.Grant().DeleteForUser(ctx, u.ID, p.GrantID)).To(Succeed())
err := svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true)
Expect(err).To(MatchError(model.ErrInvalidAuth))
_, err = svc.Login(ctx, u.UserName, "pw", meta, nil)
Expect(err).ToNot(HaveOccurred())
})
It("rejects a caller naming another user's grant", func() {
alice := createUser(ctx, "pw", false)
bob := createUser(ctx, "pw", false)
_, aliceP := login(ctx, svc, alice)
bobGrant, _ := login(ctx, svc, bob)
forged := &Principal{User: aliceP.User, GrantID: bobGrant.Grant.ID}
err := svc.ChangePassword(request.WithUser(ctx, alice), forged, "pw", "pw2", true)
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("rolls back the password and epoch when a grant update fails", func() {
u := createUser(ctx, "pw", false)
issued, p := login(ctx, svc, u)
failing := New(failingEpochDS{realDS})
failing.SetClock(func() time.Time { return now })
err := failing.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true)
Expect(err).To(MatchError(ContainSubstring("boom")))
reloaded, _ := realDS.User().Get(ctx, u.ID)
Expect(reloaded.TokenEpoch).To(Equal(u.TokenEpoch))
_, err = svc.Login(ctx, u.UserName, "pw", meta, nil)
Expect(err).ToNot(HaveOccurred())
_, err = svc.Authenticate(ctx, issued.Secret, "")
Expect(err).ToNot(HaveOccurred())
})
})
})
// afterFindDS calls after between finding the grant by its secret and reading its user.
type afterFindDS struct {
model.DataStore
after func()
}
func (d afterFindDS) Grant() model.GrantRepository {
return afterFindGrants{GrantRepository: d.DataStore.Grant(), after: d.after}
}
type afterFindGrants struct {
model.GrantRepository
after func()
}
func (g afterFindGrants) FindBySecretHash(ctx context.Context, hash string) (*model.Grant, error) {
found, err := g.GrantRepository.FindBySecretHash(ctx, hash)
g.after()
return found, err
}
type pausingChecker struct {
inner CredentialChecker
reached, release chan struct{}
}
func (c pausingChecker) Check(ctx context.Context, username, password string) (CredentialResult, error) {
res, err := c.inner.Check(ctx, username, password)
close(c.reached)
<-c.release
return res, err
}
// failingEpochDS makes SetEpoch fail inside WithTxImmediate, to prove the whole change rolls back.
type failingEpochDS struct{ model.DataStore }
func (f failingEpochDS) WithTxImmediate(block func(tx model.DataStore) error, scope ...string) error {
return f.DataStore.WithTxImmediate(func(tx model.DataStore) error {
return block(failingEpochTx{tx})
}, scope...)
}
type failingEpochTx struct{ model.DataStore }
func (f failingEpochTx) Grant() model.GrantRepository { return failingGrants{f.DataStore.Grant()} }
type failingGrants struct{ model.GrantRepository }
func (failingGrants) SetEpoch(context.Context, string, int, int, string) error {
return errors.New("boom")
}

315
core/apiauth/service_test.go
View file

@ -2,6 +2,7 @@ package apiauth
import (
"context"
"errors"
"strings"
"time"
@ -9,33 +10,14 @@ import (
"github.com/navidrome/navidrome/conf/configtest"
"github.com/navidrome/navidrome/core/auth"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/model/request"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)
// renewingDS runs renew right before DeleteIdle, as a node resolving the grant meanwhile would.
type renewingDS struct {
model.DataStore
renew func()
}
func (d renewingDS) Grant() model.GrantRepository {
return renewingGrants{GrantRepository: d.DataStore.Grant(), renew: d.renew}
}
type renewingGrants struct {
model.GrantRepository
renew func()
}
func (g renewingGrants) DeleteIdle(ctx context.Context, idleSince time.Time) (int64, error) {
g.renew()
return g.GrantRepository.DeleteIdle(ctx, idleSince)
}
var meta = ClientMeta{Name: "Living room", Client: "TestApp", ClientVersion: "1.0"}
var _ = Describe("Service: grants", func() {
var _ = Describe("Service", func() {
var ctx context.Context
var svc *Service
var now time.Time
@ -106,10 +88,7 @@ var _ = Describe("Service: grants", func() {
Describe("Authenticate", func() {
It("resolves the secret to its user and the grant's expanded scopes", func() {
u := createUser(ctx, "pw", false)
issued, err := svc.Login(ctx, u.UserName, "pw", meta, nil)
Expect(err).ToNot(HaveOccurred())
p, err := svc.Authenticate(ctx, issued.Secret, "10.0.0.9")
Expect(err).ToNot(HaveOccurred())
issued, p := login(ctx, svc, u, "pw", nil)
Expect(p.User.ID).To(Equal(u.ID))
Expect(p.GrantID).To(Equal(issued.Grant.ID))
Expect(p.Scopes).To(Equal([]string{ScopePassword, ScopeRead}))
@ -117,10 +96,7 @@ var _ = Describe("Service: grants", func() {
It("carries only the scopes stored on a narrow grant", func() {
u := createUser(ctx, "pw", false)
issued, err := svc.Login(ctx, u.UserName, "pw", meta, []string{ScopePassword})
Expect(err).ToNot(HaveOccurred())
p, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).ToNot(HaveOccurred())
_, p := login(ctx, svc, u, "pw", []string{ScopePassword})
Expect(p.Scopes).To(Equal([]string{ScopePassword}))
})
@ -169,11 +145,11 @@ var _ = Describe("Service: grants", func() {
Expect(err).To(MatchError(model.ErrNotFound))
})
It("keeps an idle grant that another node renewed before the delete ran", func() {
It("keeps an idle grant that a concurrent request renewed before the delete ran", func() {
u := createUser(ctx, "pw", false)
issued, _ := svc.Login(ctx, u.UserName, "pw", meta, nil)
renewedAt := now.Add(IdleExpiry - time.Minute)
racing := New(renewingDS{DataStore: realDS, renew: func() {
racing := New(hookDS{DataStore: realDS, beforeDeleteIdle: func() {
Expect(realDS.Grant().Touch(ctx, issued.Grant.ID, "10.0.0.2", renewedAt, renewedAt)).To(Succeed())
}})
now = now.Add(IdleExpiry + time.Second)
@ -196,6 +172,221 @@ var _ = Describe("Service: grants", func() {
_, err = realDS.Grant().Get(ctx, issued.Grant.ID)
Expect(err).To(MatchError(model.ErrNotFound))
})
It("does not delete a kept grant when the password changed between reading the grant and the user", func() {
u := createUser(ctx, "pw", false)
issued, p := login(ctx, svc, u, "pw", nil)
racing := New(hookDS{DataStore: realDS, afterFind: func() {
Expect(svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", false)).To(Succeed())
}})
racing.SetClock(func() time.Time { return now })
_, err := racing.Authenticate(ctx, issued.Secret, "")
Expect(err).ToNot(HaveOccurred())
_, err = realDS.Grant().Get(ctx, p.GrantID)
Expect(err).ToNot(HaveOccurred())
})
It("drops admin from a grant once its user is no longer an admin", func() {
saved := KnownScopes
KnownScopes = []string{ScopeRead, ScopePassword, ScopeAdmin}
DeferCleanup(func() { KnownScopes = saved })
u := createUser(ctx, "pw", true)
issued, p := login(ctx, svc, u, "pw", nil)
Expect(p.Scopes).To(ContainElement(ScopeAdmin))
u.IsAdmin = false
Expect(realDS.User().Put(ctx, &u)).To(Succeed())
demoted, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).ToNot(HaveOccurred())
Expect(demoted.Scopes).To(Equal([]string{ScopePassword, ScopeRead}))
})
It("rejects the secret after its user is deleted, and the grant row is gone", func() {
u := createUser(ctx, "pw", false)
issued, _ := login(ctx, svc, u, "pw", nil)
Expect(realDS.User().Delete(request.WithUser(ctx, model.User{IsAdmin: true}), u.ID)).To(Succeed())
_, err := realDS.Grant().Get(ctx, issued.Grant.ID)
Expect(err).To(MatchError(model.ErrNotFound))
_, err = svc.Authenticate(ctx, issued.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("leaves a login that raced a password change with a dead grant", func() {
u := createUser(ctx, "pw", false)
reached, release := make(chan struct{}), make(chan struct{})
svc.SetCheckers(func(ds model.DataStore) []CredentialChecker {
return []CredentialChecker{pausingChecker{inner: dbChecker{ds: ds}, reached: reached, release: release}}
})
var issued *Issued
var loginErr error
done := make(chan struct{})
go func() {
defer GinkgoRecover()
defer close(done)
issued, loginErr = svc.Login(ctx, u.UserName, "pw", meta, nil)
}()
<-reached // credentials (and the old epoch) were read
u.NewPassword = "changed-meanwhile"
Expect(realDS.User().Put(ctx, &u)).To(Succeed())
close(release)
<-done
Expect(loginErr).ToNot(HaveOccurred())
_, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
})
Describe("grant management", func() {
It("lists the user's grants and marks the current one", func() {
u := createUser(ctx, "pw", false)
first, _ := login(ctx, svc, u, "pw", nil)
_, p := login(ctx, svc, u, "pw", nil)
grants, total, err := svc.ListGrants(ctx, p, 0, 10)
Expect(err).ToNot(HaveOccurred())
Expect(total).To(Equal(int64(2)))
Expect(grants).To(HaveLen(2))
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(ctx, svc, u, "pw", nil)
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, p := login(ctx, svc, u, "reset-by-admin", nil)
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, and succeeds again when the grant is already gone", func() {
u := createUser(ctx, "pw", false)
issued, p := login(ctx, svc, u, "pw", nil)
Expect(svc.Logout(ctx, p)).To(Succeed())
_, err := svc.Authenticate(ctx, issued.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
Expect(svc.Logout(ctx, p)).To(Succeed())
})
It("refuses to revoke another user's grant", func() {
alice := createUser(ctx, "pw", false)
bob := createUser(ctx, "pw", false)
aliceGrant, _ := login(ctx, svc, alice, "pw", nil)
_, bobP := login(ctx, svc, bob, "pw", nil)
Expect(svc.RevokeGrant(ctx, bobP, aliceGrant.Grant.ID)).To(MatchError(model.ErrNotFound))
_, err := svc.Authenticate(ctx, aliceGrant.Secret, "")
Expect(err).ToNot(HaveOccurred())
})
It("rejects the secret after its grant is revoked", func() {
u := createUser(ctx, "pw", false)
other, _ := login(ctx, svc, u, "pw", nil)
_, p := login(ctx, svc, u, "pw", nil)
Expect(svc.RevokeGrant(ctx, p, other.Grant.ID)).To(Succeed())
_, err := svc.Authenticate(ctx, other.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
})
Describe("ChangePassword", func() {
It("revokes other grants by default and keeps the caller's", func() {
u := createUser(ctx, "pw", false)
other, _ := login(ctx, svc, u, "pw", nil)
mine, p := login(ctx, svc, u, "pw", nil)
Expect(svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true)).To(Succeed())
_, err := svc.Authenticate(ctx, mine.Secret, "")
Expect(err).ToNot(HaveOccurred())
_, err = svc.Authenticate(ctx, other.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
_, err = svc.Login(ctx, u.UserName, "pw2", meta, nil)
Expect(err).ToNot(HaveOccurred())
})
It("keeps every grant, the caller's included, when revokeOthers is false", func() {
u := createUser(ctx, "pw", false)
other, _ := login(ctx, svc, u, "pw", nil)
mine, p := login(ctx, svc, u, "pw", nil)
Expect(svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", false)).To(Succeed())
_, err := svc.Authenticate(ctx, other.Secret, "")
Expect(err).ToNot(HaveOccurred())
_, err = svc.Authenticate(ctx, mine.Secret, "")
Expect(err).ToNot(HaveOccurred())
})
It("rejects a wrong current password without changing anything", func() {
u := createUser(ctx, "pw", false)
_, p := login(ctx, svc, u, "pw", nil)
err := svc.ChangePassword(request.WithUser(ctx, p.User), p, "wrong", "pw2", true)
Expect(err).To(MatchError(ErrCurrentPasswordMismatch))
_, err = svc.Login(ctx, u.UserName, "pw", meta, nil)
Expect(err).ToNot(HaveOccurred())
})
It("is forbidden for non-admins when user editing is off", func() {
conf.Server.EnableUserEditing = false
u := createUser(ctx, "pw", false)
_, p := login(ctx, svc, u, "pw", nil)
err := svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true)
Expect(err).To(MatchError(model.ErrNotAuthorized))
})
It("does not revive grants killed by an earlier reset when keeping grants", func() {
u := createUser(ctx, "pw", false)
killed, _ := login(ctx, svc, u, "pw", nil)
u.NewPassword = "reset-by-admin" // old-UI reset: the killed grant stays on the old epoch until presented
Expect(realDS.User().Put(ctx, &u)).To(Succeed())
_, p2 := login(ctx, svc, u, "reset-by-admin", nil)
Expect(svc.ChangePassword(request.WithUser(ctx, p2.User), p2, "reset-by-admin", "pw3", false)).To(Succeed())
_, err := svc.Authenticate(ctx, killed.Secret, "")
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("rejects a caller whose grant was revoked before the change ran", func() {
u := createUser(ctx, "pw", false)
_, p := login(ctx, svc, u, "pw", nil)
Expect(realDS.Grant().DeleteForUser(ctx, u.ID, p.GrantID)).To(Succeed())
err := svc.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true)
Expect(err).To(MatchError(model.ErrInvalidAuth))
_, err = svc.Login(ctx, u.UserName, "pw", meta, nil)
Expect(err).ToNot(HaveOccurred())
})
It("rejects a caller naming another user's grant", func() {
alice := createUser(ctx, "pw", false)
bob := createUser(ctx, "pw", false)
_, aliceP := login(ctx, svc, alice, "pw", nil)
bobGrant, _ := login(ctx, svc, bob, "pw", nil)
forged := &Principal{User: aliceP.User, GrantID: bobGrant.Grant.ID}
err := svc.ChangePassword(request.WithUser(ctx, alice), forged, "pw", "pw2", true)
Expect(err).To(MatchError(model.ErrInvalidAuth))
})
It("rolls back the password and epoch when a grant update fails", func() {
u := createUser(ctx, "pw", false)
issued, p := login(ctx, svc, u, "pw", nil)
failing := New(failingEpochDS{realDS})
failing.SetClock(func() time.Time { return now })
err := failing.ChangePassword(request.WithUser(ctx, p.User), p, "pw", "pw2", true)
Expect(err).To(MatchError(ContainSubstring("boom")))
reloaded, _ := realDS.User().Get(ctx, u.ID)
Expect(reloaded.TokenEpoch).To(Equal(u.TokenEpoch))
_, err = svc.Login(ctx, u.UserName, "pw", meta, nil)
Expect(err).ToNot(HaveOccurred())
_, err = svc.Authenticate(ctx, issued.Secret, "")
Expect(err).ToNot(HaveOccurred())
})
})
Describe("PasswordChangeable", func() {
@ -208,3 +399,65 @@ var _ = Describe("Service: grants", func() {
})
})
})
// hookDS runs its optional callbacks inside grant lookups, to land a concurrent change mid-Authenticate.
type hookDS struct {
model.DataStore
afterFind func()
beforeDeleteIdle func()
}
func (d hookDS) Grant() model.GrantRepository {
return hookGrants{GrantRepository: d.DataStore.Grant(), hooks: d}
}
type hookGrants struct {
model.GrantRepository
hooks hookDS
}
func (g hookGrants) FindBySecretHash(ctx context.Context, hash string) (*model.Grant, error) {
found, err := g.GrantRepository.FindBySecretHash(ctx, hash)
if g.hooks.afterFind != nil {
g.hooks.afterFind()
}
return found, err
}
func (g hookGrants) DeleteIdle(ctx context.Context, idleSince time.Time) (int64, error) {
if g.hooks.beforeDeleteIdle != nil {
g.hooks.beforeDeleteIdle()
}
return g.GrantRepository.DeleteIdle(ctx, idleSince)
}
type pausingChecker struct {
inner CredentialChecker
reached, release chan struct{}
}
func (c pausingChecker) Check(ctx context.Context, username, password string) (CredentialResult, error) {
res, err := c.inner.Check(ctx, username, password)
close(c.reached)
<-c.release
return res, err
}
// failingEpochDS makes SetEpoch fail inside WithTxImmediate, to prove the whole change rolls back.
type failingEpochDS struct{ model.DataStore }
func (f failingEpochDS) WithTxImmediate(block func(tx model.DataStore) error, scope ...string) error {
return f.DataStore.WithTxImmediate(func(tx model.DataStore) error {
return block(failingEpochTx{tx})
}, scope...)
}
type failingEpochTx struct{ model.DataStore }
func (f failingEpochTx) Grant() model.GrantRepository { return failingGrants{f.DataStore.Grant()} }
type failingGrants struct{ model.GrantRepository }
func (failingGrants) SetEpoch(context.Context, string, int, int, string) error {
return errors.New("boom")
}

7
server/apiv1/auth_handlers.go
View file

@ -4,7 +4,9 @@ import (
"cmp"
"context"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/utils/gg"
"github.com/navidrome/navidrome/utils/slice"
)
const defaultPageSize = 100
@ -20,10 +22,7 @@ func (rt *Router) ListGrants(ctx context.Context, req ListGrantsRequestObject) (
if err != nil {
return nil, err
}
items := make([]Grant, len(grants))
for i, g := range grants {
items[i] = toGrant(g, p.GrantID)
}
items := slice.Map(grants, func(g model.Grant) Grant { return toGrant(g, p.GrantID) })
return ListGrants200JSONResponse{Items: items, Total: int(total), Offset: offset, Limit: limit}, nil
}

23
server/apiv1/auth_test.go
View file

@ -11,6 +11,7 @@ import (
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/conf/configtest"
"github.com/navidrome/navidrome/core/auth"
"github.com/navidrome/navidrome/model"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)
@ -112,7 +113,7 @@ var _ = Describe("auth endpoints", func() {
Expect(api.call(http.MethodDelete, "/api/v1/auth/grants/"+gc.Grant.Id, narrow.Secret, nil).Code).To(Equal(http.StatusForbidden))
})
It("logs out: the secret stops at once and logoutUrl is null", func() {
It("logs out: the secret stops working and logoutUrl is null", func() {
gc := api.setup()
w := api.call(http.MethodPost, "/api/v1/auth/logout", gc.Secret, nil)
Expect(w.Code).To(Equal(http.StatusOK))
@ -123,7 +124,7 @@ var _ = Describe("auth endpoints", func() {
Expect(w.Header().Get("WWW-Authenticate")).To(Equal(`Bearer error="invalid_token"`))
})
It("revokes another grant of the caller, whose secret then stops at once", func() {
It("revokes another grant of the caller, whose secret then stops working", func() {
gc := api.setup()
other := api.login(nil)
Expect(api.call(http.MethodDelete, "/api/v1/auth/grants/"+other.Grant.Id, gc.Secret, nil).Code).To(Equal(http.StatusNoContent))
@ -133,11 +134,14 @@ var _ = Describe("auth endpoints", func() {
It("challenges with invalid_token when the grant is revoked while a password change runs", func() {
gc := api.setup()
Expect(realDS.Grant().DeleteForUser(ctx, gc.User.Id, gc.Grant.Id)).To(Succeed())
revoking := testClient{ctx: ctx, router: New(beforeTxDS{DataStore: realDS, before: func() {
Expect(realDS.Grant().DeleteForUser(ctx, gc.User.Id, gc.Grant.Id)).To(Succeed())
}})}
w := api.call(http.MethodPost, "/api/v1/auth/password", gc.Secret, map[string]any{"currentPassword": "pw", "newPassword": "pw2"})
w := revoking.call(http.MethodPost, "/api/v1/auth/password", gc.Secret, map[string]any{"currentPassword": "pw", "newPassword": "pw2"})
Expect(w.Code).To(Equal(http.StatusUnauthorized), w.Body.String())
Expect(w.Header().Get("WWW-Authenticate")).To(Equal(`Bearer error="invalid_token"`))
api.login(nil)
})
It("rejects a case-variant scopes key that would widen an explicit empty subset", func() {
@ -270,3 +274,14 @@ var _ = Describe("auth endpoints", func() {
Entry("with no declared length", func(s string) io.Reader { return io.MultiReader(strings.NewReader(s)) }),
)
})
// beforeTxDS calls before as each immediate transaction starts; authentication opens none, so it lands after the gate.
type beforeTxDS struct {
model.DataStore
before func()
}
func (d beforeTxDS) WithTxImmediate(block func(tx model.DataStore) error, scope ...string) error {
d.before()
return d.DataStore.WithTxImmediate(block, scope...)
}

63
server/apiv1/gate.go
View file

@ -31,16 +31,9 @@ type authenticator interface {
Authenticate(ctx context.Context, secret, ip string) (*apiauth.Principal, error)
}
type authKind int
const (
authPublic authKind = iota
authBearer
)
type gateOp struct {
route *routers.Route
kind authKind
public bool
scope string
limited bool
noStore bool
@ -131,17 +124,16 @@ func buildGateOp(doc *openapi3.T, path string, item *openapi3.PathItem, method s
module, _ := op.Extensions["x-module"].(string)
switch reqs := *op.Security; {
case len(reqs) == 0:
gop.kind = authPublic
gop.public = true
case len(reqs) == 1 && isScheme(reqs[0], "bearerAuth"):
gop.kind = authBearer
default:
return nil, fmt.Errorf("operation %s has a security requirement outside the allowed forms", id)
}
if gop.kind == authBearer && scope == "" && !rules.noScope[id] {
if !gop.public && scope == "" && !rules.noScope[id] {
return nil, fmt.Errorf("operation %s: bearerAuth needs x-scope", id)
}
if scope != "" {
if gop.kind != authBearer {
if gop.public {
return nil, fmt.Errorf("operation %s: x-scope needs bearerAuth", id)
}
base := cmp.Or(moduleScope[module], module)
@ -218,7 +210,7 @@ func (g *gate) handler(next http.Handler) http.Handler {
}
func (g *gate) authorize(w http.ResponseWriter, r *http.Request, op *gateOp) (*http.Request, bool) {
if op.kind == authPublic {
if op.public {
return r, true
}
secret, ok := bearerToken(r)
@ -227,13 +219,14 @@ func (g *gate) authorize(w http.ResponseWriter, r *http.Request, op *gateOp) (*h
return r, false
}
p, err := g.auth.Authenticate(r.Context(), secret, server.ClientAddr(r))
if err == nil && op.scope != "" && !apiauth.Satisfies(p.Scopes, op.scope) {
err = &scopeError{scope: op.scope}
}
if err != nil {
writeProblem(w, r, err)
return r, false
}
if op.scope != "" && !apiauth.Satisfies(p.Scopes, op.scope) {
writeProblem(w, r, &scopeError{scope: op.scope})
return r, false
}
ctx := apiauth.WithPrincipal(request.WithUser(r.Context(), p.User), p)
return r.WithContext(ctx), true
}
@ -247,7 +240,7 @@ func bearerToken(r *http.Request) (string, bool) {
return token, true
}
// SkipSettingDefaults: filling defaults re-encodes the body, which hides trailing data from jsonBodyFields.
// SkipSettingDefaults: the handlers apply defaults themselves, and the validator must not rewrite the body.
var validationOptions = &openapi3filter.Options{AuthenticationFunc: openapi3filter.NoopAuthenticationFunc, MultiError: true, SkipSettingDefaults: true}
func (g *gate) validate(w http.ResponseWriter, r *http.Request, op *gateOp, rctx *chi.Context) bool {
@ -255,9 +248,15 @@ func (g *gate) validate(w http.ResponseWriter, r *http.Request, op *gateOp, rctx
for i, k := range rctx.URLParams.Keys {
params[k] = rctx.URLParams.Values[i]
}
err := openapi3filter.ValidateRequest(r.Context(), &openapi3filter.RequestValidationInput{
Request: r, PathParams: params, Route: op.route, Options: validationOptions,
})
body, err := readBody(r, op.route.Operation)
if err == nil {
err = openapi3filter.ValidateRequest(r.Context(), &openapi3filter.RequestValidationInput{
Request: r, PathParams: params, Route: op.route, Options: validationOptions,
})
if body != nil {
r.Body = io.NopCloser(bytes.NewReader(body))
}
}
if tooLarge(err) {
writeProblem(w, r, ClientError(err, tooLargeDetail))
return false
@ -265,7 +264,7 @@ func (g *gate) validate(w http.ResponseWriter, r *http.Request, op *gateOp, rctx
var fields []ValidationError
if err != nil {
fields = sanitizeValidation(err)
} else if fields = jsonBodyFields(r, op.route.Operation); len(fields) == 0 {
} else if fields = jsonBodyFields(body, op.route.Operation); len(fields) == 0 {
return true
}
log.Debug(r.Context(), "API v1: request failed validation", "operation", op.id(), "errors", fields)
@ -273,10 +272,23 @@ func (g *gate) validate(w http.ResponseWriter, r *http.Request, op *gateOp, rctx
return false
}
// readBody reads a declared body once, so the validator, the JSON checks and the handler all see the same bytes.
func readBody(r *http.Request, op *openapi3.Operation) ([]byte, error) {
if op.RequestBody == nil || r.Body == nil || r.Body == http.NoBody {
return nil, nil
}
data, err := io.ReadAll(r.Body)
if err != nil {
return nil, err
}
r.Body = io.NopCloser(bytes.NewReader(data))
return data, nil
}
// jsonBodyFields checks what kin-openapi misses in a JSON body: data after the first value, which Go's decoder
// ignores, and keys that only case-fold to a declared property, which encoding/json decodes into that property.
func jsonBodyFields(r *http.Request, op *openapi3.Operation) []ValidationError {
if op.RequestBody == nil || op.RequestBody.Value == nil || r.Body == nil {
func jsonBodyFields(data []byte, op *openapi3.Operation) []ValidationError {
if op.RequestBody == nil || op.RequestBody.Value == nil || len(bytes.TrimSpace(data)) == 0 {
return nil
}
// Keyed on the spec, not the request's Content-Type: the handlers decode JSON whatever the header says.
@ -284,11 +296,6 @@ func jsonBodyFields(r *http.Request, op *openapi3.Operation) []ValidationError {
if media == nil || media.Schema == nil {
return nil
}
data, err := io.ReadAll(r.Body)
r.Body = io.NopCloser(bytes.NewReader(data))
if err != nil || len(bytes.TrimSpace(data)) == 0 {
return nil
}
dec := json.NewDecoder(bytes.NewReader(data))
var body any
if err := dec.Decode(&body); err != nil {

19
server/apiv1/problem.go
View file

@ -32,8 +32,7 @@ type scopeError struct {
scope string
}
func (e *scopeError) Error() string { return apiauth.ErrInsufficientScope.Error() }
func (e *scopeError) Unwrap() error { return apiauth.ErrInsufficientScope }
func (e *scopeError) Error() string { return "insufficient scope" }
const tooLargeDetail = "request body too large"
@ -60,8 +59,9 @@ func writeProblem(w http.ResponseWriter, r *http.Request, err error) {
return
}
log.Debug(r.Context(), "API v1: request failed", "path", r.URL.Path, "status", status, "code", code, err)
if code == ProblemCodeInsufficientScope {
w.Header().Set("WWW-Authenticate", scopeChallenge(err))
var se *scopeError
if errors.As(err, &se) {
w.Header().Set("WWW-Authenticate", fmt.Sprintf(`Bearer error="insufficient_scope", scope=%q`, se.scope))
}
var detail string
var ce *clientError
@ -76,20 +76,11 @@ func writeProblem(w http.ResponseWriter, r *http.Request, err error) {
writeProblemStatus(w, r, status, code, detail)
}
func scopeChallenge(err error) string {
challenge := `Bearer error="insufficient_scope"`
var se *scopeError
if errors.As(err, &se) && se.scope != "" {
challenge += fmt.Sprintf(`, scope=%q`, se.scope)
}
return challenge
}
func classifyError(err error) (int, ProblemCode) {
switch {
case tooLarge(err):
return http.StatusRequestEntityTooLarge, ProblemCodePayloadTooLarge
case errors.Is(err, apiauth.ErrInsufficientScope):
case errors.As(err, new(*scopeError)):
return http.StatusForbidden, ProblemCodeInsufficientScope
case errors.Is(err, auth.ErrSetupComplete):
return http.StatusConflict, ProblemCodeSetupComplete

2
server/apiv1/problem_test.go
View file

@ -51,7 +51,7 @@ var _ = Describe("problem", func() {
Entry("expired", model.ErrExpired, http.StatusUnauthorized, ProblemCodeUnauthorized),
Entry("validation", model.ErrValidation, http.StatusBadRequest, ProblemCodeValidation),
Entry("not available", model.ErrNotAvailable, http.StatusServiceUnavailable, ProblemCodeUnavailable),
Entry("insufficient scope", apiauth.ErrInsufficientScope, http.StatusForbidden, ProblemCodeInsufficientScope),
Entry("insufficient scope", &scopeError{scope: "read"}, http.StatusForbidden, ProblemCodeInsufficientScope),
Entry("setup complete", auth.ErrSetupComplete, http.StatusConflict, ProblemCodeSetupComplete),
Entry("password managed externally", apiauth.ErrPasswordManagedExternally, http.StatusConflict, ProblemCodePasswordManagedExternally),
Entry("unknown", errors.New("boom"), http.StatusInternalServerError, ProblemCodeInternal),