diff --git a/core/apiauth/cache.go b/core/apiauth/cache.go index 136b5c336..d19d53dd5 100644 --- a/core/apiauth/cache.go +++ b/core/apiauth/cache.go @@ -55,6 +55,13 @@ func (c *livenessCache) put(id string, e livenessEntry, now time.Time, started u if len(c.entries) >= maxLivenessEntries { maps.DeleteFunc(c.entries, func(_ string, v livenessEntry) bool { return !now.Before(v.expires) }) } + if _, refresh := c.entries[id]; !refresh && len(c.entries) >= maxLivenessEntries { + // Dropping an arbitrary live entry only costs that grant one extra DB read. + for k := range c.entries { + delete(c.entries, k) + break + } + } if len(c.evicted) >= maxLivenessEntries { clear(c.evicted) c.floor = c.gen diff --git a/core/apiauth/cache_test.go b/core/apiauth/cache_test.go index 999f3e24a..a3429897d 100644 --- a/core/apiauth/cache_test.go +++ b/core/apiauth/cache_test.go @@ -81,4 +81,17 @@ var _ = Describe("livenessCache", func() { c.put("fresh", livenessEntry{}, t0.Add(time.Minute), c.begin()) Expect(c.len()).To(Equal(1)) }) + + It("never grows past its cap, even when every entry is live", func() { + for i := range maxLivenessEntries { + c.put(fmt.Sprint(i), livenessEntry{}, t0, c.begin()) + } + c.put("fresh", livenessEntry{}, t0, c.begin()) + Expect(c.len()).To(Equal(maxLivenessEntries)) + _, ok := c.get("fresh", t0) + Expect(ok).To(BeTrue()) + + c.put("fresh", livenessEntry{userID: "u1"}, t0, c.begin()) + Expect(c.len()).To(Equal(maxLivenessEntries)) + }) }) diff --git a/core/apiauth/scopes.go b/core/apiauth/scopes.go index 62c840f1f..367c19519 100644 --- a/core/apiauth/scopes.go +++ b/core/apiauth/scopes.go @@ -19,6 +19,10 @@ func known(s string) bool { return slices.Contains(KnownScopes, s) } +func grantable(s string, isAdmin bool) bool { + return known(s) && (s != ScopeAdmin || isAdmin) +} + func normalize(in []string) []string { out := slices.Clone(in) slices.Sort(out) @@ -32,9 +36,7 @@ func Entitled(requested []string, isAdmin bool) []string { } var out []string for _, s := range requested { - switch { - case s == ScopeAdmin && !isAdmin: - case s == ScopeAll || known(s): + if s == ScopeAll || grantable(s, isAdmin) { out = append(out, s) } } @@ -56,9 +58,7 @@ func Expand(granted []string, isAdmin bool) []string { // 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 !known(s) || (s == ScopeAdmin && !isAdmin) - }) + out := slices.DeleteFunc(slices.Clone(scopes), func(s string) bool { return !grantable(s, isAdmin) }) return normalize(out) } diff --git a/core/apiauth/service.go b/core/apiauth/service.go index 0bd3a22bc..4cbf25eea 100644 --- a/core/apiauth/service.go +++ b/core/apiauth/service.go @@ -7,6 +7,7 @@ import ( "fmt" "slices" "sync" + "sync/atomic" "time" "github.com/navidrome/navidrome/conf" @@ -60,7 +61,7 @@ type Service struct { cache *livenessCache now func() time.Time signerMu sync.Mutex - sg *signer + sg atomic.Pointer[signer] } func New(ds model.DataStore) *Service { @@ -77,16 +78,19 @@ func New(ds model.DataStore) *Service { // signer loads the key on first use, so building the router never touches the database; only a success is kept. func (s *Service) signer() (*signer, error) { + if sg := s.sg.Load(); sg != nil { + return sg, nil + } s.signerMu.Lock() defer s.signerMu.Unlock() - if s.sg != nil { - return s.sg, nil + if sg := s.sg.Load(); sg != nil { + return sg, nil } sg, err := loadSigner(context.Background(), s.ds, func() time.Time { return s.now() }) if err != nil { return nil, err } - s.sg = sg + s.sg.Store(sg) return sg, nil } @@ -111,15 +115,15 @@ func (s *Service) Login(ctx context.Context, username, password string, meta Cli func (s *Service) Setup(ctx context.Context, username, password string, meta ClientMeta, scopes []string) (*Issued, error) { var issued *Issued - err := s.ds.WithTxImmediate(func(tx model.DataStore) error { - u, err := auth.CreateFirstAdmin(ctx, tx, username, password) - if err != nil { - return err - } + _, err := auth.CreateFirstAdmin(ctx, s.ds, username, password, func(tx model.DataStore, u *model.User) error { + var err error issued, err = s.issue(ctx, tx, *u, "setup", meta, scopes) return err }) - return issued, err + if err != nil { + return nil, err + } + return issued, nil } // issue stores a grant bound to the epoch read with the user, so a racing password change leaves it dead. diff --git a/core/apiauth/service_test.go b/core/apiauth/service_test.go index 06d716faa..fb51ac6a2 100644 --- a/core/apiauth/service_test.go +++ b/core/apiauth/service_test.go @@ -30,12 +30,12 @@ type flakyProps struct { ds *flakyPropsDS } -func (p *flakyProps) PutIfAbsent(ctx context.Context, id, value string) error { +func (p *flakyProps) Get(ctx context.Context, id string) (string, error) { if p.ds.failures > 0 { p.ds.failures-- - return errFlakyProps + return "", errFlakyProps } - return p.PropertyRepository.PutIfAbsent(ctx, id, value) + return p.PropertyRepository.Get(ctx, id) } var meta = ClientMeta{Name: "Living room", Client: "TestApp", ClientVersion: "1.0"} diff --git a/core/apiauth/signer.go b/core/apiauth/signer.go index 6290b7130..055f6a69e 100644 --- a/core/apiauth/signer.go +++ b/core/apiauth/signer.go @@ -58,14 +58,10 @@ func loadSigner(ctx context.Context, ds model.DataStore, now func() time.Time) ( } func loadKey(ctx context.Context, ds model.DataStore) (string, error) { - enc, err := utils.Encrypt(ctx, auth.EncryptionKey(), newKey()) - if err != nil { - return "", fmt.Errorf("encrypting API v1 key: %w", err) - } - if err := ds.Property().PutIfAbsent(ctx, consts.JWTAPIv1SecretKey, enc); err != nil { - return "", fmt.Errorf("storing API v1 key: %w", err) - } stored, err := ds.Property().Get(ctx, consts.JWTAPIv1SecretKey) + if errors.Is(err, model.ErrNotFound) { + stored, err = createKey(ctx, ds) + } if err != nil { return "", fmt.Errorf("reading API v1 key: %w", err) } @@ -85,11 +81,15 @@ func loadKey(ctx context.Context, ds model.DataStore) (string, error) { return nil } log.Warn(ctx, "Could not decrypt API v1 key, replacing it") + k, enc, err := newEncryptedKey(ctx) + if err != nil { + return err + } if err := tx.Property().Put(ctx, consts.JWTAPIv1SecretKey, enc); err != nil { return err } - key, err = utils.Decrypt(ctx, auth.EncryptionKey(), enc) - return err + key = k + return nil }) if err != nil { return "", fmt.Errorf("replacing API v1 key: %w", err) @@ -97,6 +97,26 @@ func loadKey(ctx context.Context, ds model.DataStore) (string, error) { return key, nil } +// createKey re-reads after the insert-if-absent, so nodes racing to create the key agree on the winner. +func createKey(ctx context.Context, ds model.DataStore) (string, error) { + _, enc, err := newEncryptedKey(ctx) + if err != nil { + return "", err + } + if err := ds.Property().PutIfAbsent(ctx, consts.JWTAPIv1SecretKey, enc); err != nil { + return "", fmt.Errorf("storing API v1 key: %w", err) + } + return ds.Property().Get(ctx, consts.JWTAPIv1SecretKey) +} + +func newEncryptedKey(ctx context.Context) (key, enc string, err error) { + key = newKey() + if enc, err = utils.Encrypt(ctx, auth.EncryptionKey(), key); err != nil { + return "", "", fmt.Errorf("encrypting API v1 key: %w", err) + } + return key, enc, nil +} + // newKey returns 256 random bits, the minimum RFC 7518 asks of an HS256 key. func newKey() string { b := make([]byte, 32) diff --git a/core/auth/first_admin.go b/core/auth/first_admin.go index 319b325eb..a082e6222 100644 --- a/core/auth/first_admin.go +++ b/core/auth/first_admin.go @@ -15,26 +15,40 @@ import ( var ErrSetupComplete = errors.New("setup already complete") -// CreateFirstAdmin must run inside ds.WithTxImmediate, so the count and the insert cannot interleave. -func CreateFirstAdmin(ctx context.Context, tx model.DataStore, username, password string) (*model.User, error) { - count, err := tx.User().CountAll(ctx) +// CreateFirstAdmin counts and inserts in one locked transaction, so racing setups cannot both win. +// then, if not nil, runs in that same transaction with the new user. +func CreateFirstAdmin(ctx context.Context, ds model.DataStore, username, password string, then func(tx model.DataStore, u *model.User) error) (*model.User, error) { + var created *model.User + err := ds.WithTxImmediate(func(tx model.DataStore) error { + count, err := tx.User().CountAll(ctx) + if err != nil { + return fmt.Errorf("counting users: %w", err) + } + if count > 0 { + return ErrSetupComplete + } + log.Warn(ctx, "Creating initial user", "user", username) + u := model.User{ + ID: id.NewRandom(), + UserName: username, + Name: cases.Title(language.Und).String(username), + NewPassword: password, + IsAdmin: true, + LastLoginAt: new(time.Now()), + } + if err := tx.User().Put(ctx, &u); err != nil { + return fmt.Errorf("creating initial user: %w", err) + } + if created, err = tx.User().Get(ctx, u.ID); err != nil { + return err + } + if then != nil { + return then(tx, created) + } + return nil + }) if err != nil { - return nil, fmt.Errorf("counting users: %w", err) + return nil, err } - if count > 0 { - return nil, ErrSetupComplete - } - log.Warn(ctx, "Creating initial user", "user", username) - u := model.User{ - ID: id.NewRandom(), - UserName: username, - Name: cases.Title(language.Und).String(username), - NewPassword: password, - IsAdmin: true, - LastLoginAt: new(time.Now()), - } - if err := tx.User().Put(ctx, &u); err != nil { - return nil, fmt.Errorf("creating initial user: %w", err) - } - return tx.User().Get(ctx, u.ID) + return created, nil } diff --git a/core/auth/first_admin_test.go b/core/auth/first_admin_test.go index d22cea2f2..0cbbcf575 100644 --- a/core/auth/first_admin_test.go +++ b/core/auth/first_admin_test.go @@ -2,6 +2,7 @@ package auth_test import ( "context" + "errors" "path/filepath" "sync" "time" @@ -33,17 +34,8 @@ var _ = Describe("CreateFirstAdmin", Ordered, func() { Expect(err).ToNot(HaveOccurred()) }) - createWith := func(name string, wrap func(model.DataStore) model.DataStore) (*model.User, error) { - var u *model.User - err := ds.WithTxImmediate(func(tx model.DataStore) error { - var err error - u, err = auth.CreateFirstAdmin(ctx, wrap(tx), name, "secret") - return err - }) - return u, err - } create := func(name string) (*model.User, error) { - return createWith(name, func(tx model.DataStore) model.DataStore { return tx }) + return auth.CreateFirstAdmin(ctx, ds, name, "secret", nil) } It("creates an admin with a title-cased name and returns it with its id", func() { @@ -65,6 +57,19 @@ var _ = Describe("CreateFirstAdmin", Ordered, func() { Expect(err).To(MatchError(auth.ErrSetupComplete)) }) + It("runs then in the same transaction, rolling the user back when it fails", func() { + boom := errors.New("boom") + var seen string + _, err := auth.CreateFirstAdmin(ctx, ds, "john", "secret", func(tx model.DataStore, u *model.User) error { + seen = u.ID + Expect(tx.User().CountAll(ctx)).To(Equal(int64(1))) + return boom + }) + Expect(err).To(MatchError(boom)) + Expect(seen).ToNot(BeEmpty()) + Expect(ds.User().CountAll(ctx)).To(BeZero()) + }) + It("lets exactly one of two concurrent setups win", func() { var wg sync.WaitGroup errs := make([]error, 2) @@ -73,7 +78,7 @@ var _ = Describe("CreateFirstAdmin", Ordered, func() { go func() { defer GinkgoRecover() defer wg.Done() - _, errs[i] = createWith(name, func(tx model.DataStore) model.DataStore { return slowCountDS{tx} }) + _, errs[i] = auth.CreateFirstAdmin(ctx, slowCountDS{ds}, name, "secret", nil) }() } wg.Wait() @@ -87,6 +92,10 @@ type slowCountDS struct{ model.DataStore } func (d slowCountDS) User() model.UserRepository { return slowCountUsers{d.DataStore.User()} } +func (d slowCountDS) WithTxImmediate(block func(tx model.DataStore) error, scope ...string) error { + return d.DataStore.WithTxImmediate(func(tx model.DataStore) error { return block(slowCountDS{tx}) }, scope...) +} + type slowCountUsers struct{ model.UserRepository } // Holds the transaction open after counting, so an unlocked count would interleave with the other racer. diff --git a/server/apiv1/auth_test.go b/server/apiv1/auth_test.go index 89edd3ed2..f1b3e7c0c 100644 --- a/server/apiv1/auth_test.go +++ b/server/apiv1/auth_test.go @@ -13,7 +13,6 @@ 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" ) @@ -82,10 +81,7 @@ var _ = Describe("auth endpoints", func() { go func() { defer GinkgoRecover() defer wg.Done() - v0Err = realDS.WithTxImmediate(func(tx model.DataStore) error { // what v0 /auth/createAdmin runs - _, err := auth.CreateFirstAdmin(ctx, tx, "v0admin", "pw") - return err - }) + _, v0Err = auth.CreateFirstAdmin(ctx, realDS, "v0admin", "pw", nil) // what v0 /auth/createAdmin runs }() wg.Wait() Expect(realDS.User().CountAll(ctx)).To(Equal(int64(1))) diff --git a/server/auth.go b/server/auth.go index dc70c5048..e772c1da2 100644 --- a/server/auth.go +++ b/server/auth.go @@ -124,10 +124,7 @@ func createAdmin(ds model.DataStore) func(w http.ResponseWriter, r *http.Request _ = rest.RespondWithError(w, http.StatusUnprocessableEntity, err.Error()) return } - err = ds.WithTxImmediate(func(tx model.DataStore) error { - _, err := auth.CreateFirstAdmin(r.Context(), tx, username, password) - return err - }) + _, err = auth.CreateFirstAdmin(r.Context(), ds, username, password, nil) if errors.Is(err, auth.ErrSetupComplete) { _ = rest.RespondWithError(w, http.StatusForbidden, "Cannot create another first admin") return diff --git a/server/auth_test.go b/server/auth_test.go index e016de5e1..b7db3a881 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -77,10 +77,7 @@ var _ = Describe("Auth", func() { Describe("CreateFirstAdmin", func() { It("returns the error when the user cannot be saved", func() { failing := dsWithFailingPut(errors.New("db is down")) - err := failing.WithTxImmediate(func(tx model.DataStore) error { - _, err := auth.CreateFirstAdmin(ctx, tx, "johndoe", "secret") - return err - }) + _, err := auth.CreateFirstAdmin(ctx, failing, "johndoe", "secret", nil) Expect(err).To(MatchError(ContainSubstring("db is down"))) }) })