Removed engine.Users

cmd/wire_gen.go

@@ -44,14 +44,13 @@ func CreateSubsonicAPIRouter() (*subsonic.Router, error) {
 	artwork := core.NewArtwork(dataStore, artworkCache)
 	nowPlayingRepository := engine.NewNowPlayingRepository()
 	listGenerator := engine.NewListGenerator(dataStore, nowPlayingRepository)
-	users := engine.NewUsers(dataStore)
 	playlists := engine.NewPlaylists(dataStore)
 	transcoderTranscoder := transcoder.New()
 	transcodingCache := core.NewTranscodingCache()
 	mediaStreamer := core.NewMediaStreamer(dataStore, transcoderTranscoder, transcodingCache)
 	archiver := core.NewArchiver(dataStore)
 	players := engine.NewPlayers(dataStore)
-	router := subsonic.New(artwork, listGenerator, users, playlists, mediaStreamer, archiver, players, dataStore)
+	router := subsonic.New(artwork, listGenerator, playlists, mediaStreamer, archiver, players, dataStore)
 	return router, nil
 }
 

server/subsonic/api.go

@@ -26,7 +26,6 @@ type Router struct {
 	Artwork       core.Artwork
 	ListGenerator engine.ListGenerator
 	Playlists     engine.Playlists
-	Users         engine.Users
 	Streamer      core.MediaStreamer
 	Archiver      core.Archiver
 	Players       engine.Players
@@ -35,11 +34,11 @@ type Router struct {
 	mux http.Handler
 }
 
-func New(artwork core.Artwork, listGenerator engine.ListGenerator, users engine.Users,
+func New(artwork core.Artwork, listGenerator engine.ListGenerator,
 	playlists engine.Playlists, streamer core.MediaStreamer,
 	archiver core.Archiver, players engine.Players, ds model.DataStore) *Router {
 	r := &Router{Artwork: artwork, ListGenerator: listGenerator, Playlists: playlists,
-		Users: users, Streamer: streamer, Archiver: archiver, Players: players, DataStore: ds}
+		Streamer: streamer, Archiver: archiver, Players: players, DataStore: ds}
 	r.mux = r.routes()
 	return r
 }
@@ -55,7 +54,7 @@ func (api *Router) routes() http.Handler {
 
 	r.Use(postFormToQueryParams)
 	r.Use(checkRequiredParameters)
-	r.Use(authenticate(api.Users))
+	r.Use(authenticate(api.DataStore))
 	// TODO Validate version
 
 	// Subsonic endpoints, grouped by controller

server/subsonic/engine/users.go

@@ -1,63 +0,0 @@
-package engine
-
-import (
-	"context"
-	"crypto/md5"
-	"encoding/hex"
-	"fmt"
-	"strings"
-
-	"github.com/deluan/navidrome/core/auth"
-	"github.com/deluan/navidrome/model"
-)
-
-type Users interface {
-	Authenticate(ctx context.Context, username, password, token, salt, jwt string) (*model.User, error)
-}
-
-func NewUsers(ds model.DataStore) Users {
-	return &users{ds}
-}
-
-type users struct {
-	ds model.DataStore
-}
-
-func (u *users) Authenticate(ctx context.Context, username, pass, token, salt, jwt string) (*model.User, error) {
-	user, err := u.ds.User(ctx).FindByUsername(username)
-	if err == model.ErrNotFound {
-		return nil, model.ErrInvalidAuth
-	}
-	if err != nil {
-		return nil, err
-	}
-	valid := false
-
-	switch {
-	case jwt != "":
-		claims, err := auth.Validate(jwt)
-		valid = err == nil && claims["sub"] == username
-	case pass != "":
-		if strings.HasPrefix(pass, "enc:") {
-			if dec, err := hex.DecodeString(pass[4:]); err == nil {
-				pass = string(dec)
-			}
-		}
-		valid = pass == user.Password
-	case token != "":
-		t := fmt.Sprintf("%x", md5.Sum([]byte(user.Password+salt)))
-		valid = t == token
-	}
-
-	if !valid {
-		return nil, model.ErrInvalidAuth
-	}
-	// TODO: Find a way to update LastAccessAt without causing too much retention in the DB
-	//go func() {
-	//	err := u.ds.User(ctx).UpdateLastAccessAt(user.ID)
-	//	if err != nil {
-	//		log.Error(ctx, "Could not update user's lastAccessAt", "user", user.UserName)
-	//	}
-	//}()
-	return user, nil
-}

server/subsonic/engine/users_test.go

@@ -1,83 +0,0 @@
-package engine
-
-import (
-	"context"
-
-	"github.com/deluan/navidrome/core/auth"
-	"github.com/deluan/navidrome/model"
-	"github.com/deluan/navidrome/persistence"
-	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
-)
-
-var _ = Describe("Users", func() {
-	Describe("Authenticate", func() {
-		var users Users
-		BeforeEach(func() {
-			ds := &persistence.MockDataStore{}
-			users = NewUsers(ds)
-		})
-
-		Context("Plaintext password", func() {
-			It("authenticates with plaintext password ", func() {
-				usr, err := users.Authenticate(context.TODO(), "admin", "wordpass", "", "", "")
-				Expect(err).NotTo(HaveOccurred())
-				Expect(usr).To(Equal(&model.User{UserName: "admin", Password: "wordpass"}))
-			})
-
-			It("fails authentication with wrong password", func() {
-				_, err := users.Authenticate(context.TODO(), "admin", "INVALID", "", "", "")
-				Expect(err).To(MatchError(model.ErrInvalidAuth))
-			})
-		})
-
-		Context("Encoded password", func() {
-			It("authenticates with simple encoded password ", func() {
-				usr, err := users.Authenticate(context.TODO(), "admin", "enc:776f726470617373", "", "", "")
-				Expect(err).NotTo(HaveOccurred())
-				Expect(usr).To(Equal(&model.User{UserName: "admin", Password: "wordpass"}))
-			})
-		})
-
-		Context("Token based authentication", func() {
-			It("authenticates with token based authentication", func() {
-				usr, err := users.Authenticate(context.TODO(), "admin", "", "23b342970e25c7928831c3317edd0b67", "retnlmjetrymazgkt", "")
-				Expect(err).NotTo(HaveOccurred())
-				Expect(usr).To(Equal(&model.User{UserName: "admin", Password: "wordpass"}))
-			})
-
-			It("fails if salt is missing", func() {
-				_, err := users.Authenticate(context.TODO(), "admin", "", "23b342970e25c7928831c3317edd0b67", "", "")
-				Expect(err).To(MatchError(model.ErrInvalidAuth))
-			})
-		})
-
-		Context("JWT based authentication", func() {
-			var validToken string
-			BeforeEach(func() {
-				u := &model.User{UserName: "admin"}
-				var err error
-				validToken, err = auth.CreateToken(u)
-				if err != nil {
-					panic(err)
-				}
-			})
-			It("authenticates with JWT token based authentication", func() {
-				usr, err := users.Authenticate(context.TODO(), "admin", "", "", "", validToken)
-
-				Expect(err).NotTo(HaveOccurred())
-				Expect(usr).To(Equal(&model.User{UserName: "admin", Password: "wordpass"}))
-			})
-
-			It("fails if JWT token is invalid", func() {
-				_, err := users.Authenticate(context.TODO(), "admin", "", "", "", "invalid.token")
-				Expect(err).To(MatchError(model.ErrInvalidAuth))
-			})
-
-			It("fails if JWT token sub is different than username", func() {
-				_, err := users.Authenticate(context.TODO(), "not_admin", "", "", "", validToken)
-				Expect(err).To(MatchError(model.ErrInvalidAuth))
-			})
-		})
-	})
-})

server/subsonic/engine/wire_providers.go

@@ -8,6 +8,5 @@ var Set = wire.NewSet(
 	NewListGenerator,
 	NewPlaylists,
 	NewNowPlayingRepository,
-	NewUsers,
 	NewPlayers,
 )

server/subsonic/middlewares.go

@@ -1,12 +1,16 @@
 package subsonic
 
 import (
+	"context"
+	"crypto/md5"
+	"encoding/hex"
 	"fmt"
 	"net"
 	"net/http"
 	"net/url"
 	"strings"
 
+	"github.com/deluan/navidrome/core/auth"
 	"github.com/deluan/navidrome/log"
 	"github.com/deluan/navidrome/model"
 	"github.com/deluan/navidrome/model/request"
@@ -64,29 +68,37 @@ func checkRequiredParameters(next http.Handler) http.Handler {
 	})
 }
 
-func authenticate(users engine.Users) func(next http.Handler) http.Handler {
+func authenticate(ds model.DataStore) func(next http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			ctx := r.Context()
 			username := utils.ParamString(r, "u")
+
 			pass := utils.ParamString(r, "p")
 			token := utils.ParamString(r, "t")
 			salt := utils.ParamString(r, "s")
 			jwt := utils.ParamString(r, "jwt")
 
-			usr, err := users.Authenticate(r.Context(), username, pass, token, salt, jwt)
+			usr, err := validateUser(ctx, ds, username, pass, token, salt, jwt)
 			if err == model.ErrInvalidAuth {
-				log.Warn(r, "Invalid login", "username", username, err)
+				log.Warn(ctx, "Invalid login", "username", username, err)
 			} else if err != nil {
-				log.Error(r, "Error authenticating username", "username", username, err)
+				log.Error(ctx, "Error authenticating username", "username", username, err)
 			}
 
 			if err != nil {
-				log.Warn(r, "Invalid login", "username", username)
 				SendError(w, r, newError(responses.ErrorAuthenticationFail))
 				return
 			}
 
-			ctx := r.Context()
+			// TODO: Find a way to update LastAccessAt without causing too much retention in the DB
+			//go func() {
+			//	err := ds.User(ctx).UpdateLastAccessAt(usr.ID)
+			//	if err != nil {
+			//		log.Error(ctx, "Could not update user's lastAccessAt", "user", usr.UserName)
+			//	}
+			//}()
+
 			ctx = request.WithUser(ctx, *usr)
 			r = r.WithContext(ctx)
 
@@ -95,6 +107,38 @@ func authenticate(users engine.Users) func(next http.Handler) http.Handler {
 	}
 }
 
+func validateUser(ctx context.Context, ds model.DataStore, username, pass, token, salt, jwt string) (*model.User, error) {
+	user, err := ds.User(ctx).FindByUsername(username)
+	if err == model.ErrNotFound {
+		return nil, model.ErrInvalidAuth
+	}
+	if err != nil {
+		return nil, err
+	}
+	valid := false
+
+	switch {
+	case jwt != "":
+		claims, err := auth.Validate(jwt)
+		valid = err == nil && claims["sub"] == username
+	case pass != "":
+		if strings.HasPrefix(pass, "enc:") {
+			if dec, err := hex.DecodeString(pass[4:]); err == nil {
+				pass = string(dec)
+			}
+		}
+		valid = pass == user.Password
+	case token != "":
+		t := fmt.Sprintf("%x", md5.Sum([]byte(user.Password+salt)))
+		valid = t == token
+	}
+
+	if !valid {
+		return nil, model.ErrInvalidAuth
+	}
+	return user, nil
+}
+
 func getPlayer(players engine.Players) func(next http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

server/subsonic/middlewares_test.go

@@ -7,9 +7,11 @@ import (
 	"net/http/httptest"
 	"strings"
 
+	"github.com/deluan/navidrome/core/auth"
 	"github.com/deluan/navidrome/log"
 	"github.com/deluan/navidrome/model"
 	"github.com/deluan/navidrome/model/request"
+	"github.com/deluan/navidrome/persistence"
 	"github.com/deluan/navidrome/server/subsonic/engine"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -113,29 +115,24 @@ var _ = Describe("Middlewares", func() {
 	})
 
 	Describe("Authenticate", func() {
-		var mockedUsers *mockUsers
+		var ds model.DataStore
 		BeforeEach(func() {
-			mockedUsers = &mockUsers{}
+			ds = &persistence.MockDataStore{}
 		})
 
-		It("passes all parameters to users.Authenticate ", func() {
+		It("passes authentication with correct credentials", func() {
-			r := newGetRequest("u=valid", "p=password", "t=token", "s=salt", "jwt=jwt")
+			r := newGetRequest("u=admin", "p=wordpass")
-			cp := authenticate(mockedUsers)(next)
+			cp := authenticate(ds)(next)
 			cp.ServeHTTP(w, r)
 
-			Expect(mockedUsers.username).To(Equal("valid"))
-			Expect(mockedUsers.password).To(Equal("password"))
-			Expect(mockedUsers.token).To(Equal("token"))
-			Expect(mockedUsers.salt).To(Equal("salt"))
-			Expect(mockedUsers.jwt).To(Equal("jwt"))
 			Expect(next.called).To(BeTrue())
 			user, _ := request.UserFrom(next.req.Context())
-			Expect(user.UserName).To(Equal("valid"))
+			Expect(user.UserName).To(Equal("admin"))
 		})
 
 		It("fails authentication with wrong password", func() {
 			r := newGetRequest("u=invalid", "", "", "")
-			cp := authenticate(mockedUsers)(next)
+			cp := authenticate(ds)(next)
 			cp.ServeHTTP(w, r)
 
 			Expect(w.Body.String()).To(ContainSubstring(`code="40"`))
@@ -221,6 +218,75 @@ var _ = Describe("Middlewares", func() {
 			})
 		})
 	})
+
+	Describe("validateUser", func() {
+		var ds model.DataStore
+		BeforeEach(func() {
+			ds = &persistence.MockDataStore{}
+		})
+
+		Context("Plaintext password", func() {
+			It("authenticates with plaintext password ", func() {
+				usr, err := validateUser(context.TODO(), ds, "admin", "wordpass", "", "", "")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(usr).To(Equal(&model.User{UserName: "admin", Password: "wordpass"}))
+			})
+
+			It("fails authentication with wrong password", func() {
+				_, err := validateUser(context.TODO(), ds, "admin", "INVALID", "", "", "")
+				Expect(err).To(MatchError(model.ErrInvalidAuth))
+			})
+		})
+
+		Context("Encoded password", func() {
+			It("authenticates with simple encoded password ", func() {
+				usr, err := validateUser(context.TODO(), ds, "admin", "enc:776f726470617373", "", "", "")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(usr).To(Equal(&model.User{UserName: "admin", Password: "wordpass"}))
+			})
+		})
+
+		Context("Token based authentication", func() {
+			It("authenticates with token based authentication", func() {
+				usr, err := validateUser(context.TODO(), ds, "admin", "", "23b342970e25c7928831c3317edd0b67", "retnlmjetrymazgkt", "")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(usr).To(Equal(&model.User{UserName: "admin", Password: "wordpass"}))
+			})
+
+			It("fails if salt is missing", func() {
+				_, err := validateUser(context.TODO(), ds, "admin", "", "23b342970e25c7928831c3317edd0b67", "", "")
+				Expect(err).To(MatchError(model.ErrInvalidAuth))
+			})
+		})
+
+		Context("JWT based authentication", func() {
+			var validToken string
+			BeforeEach(func() {
+				u := &model.User{UserName: "admin"}
+				var err error
+				validToken, err = auth.CreateToken(u)
+				if err != nil {
+					panic(err)
+				}
+			})
+			It("authenticates with JWT token based authentication", func() {
+				usr, err := validateUser(context.TODO(), ds, "admin", "", "", "", validToken)
+
+				Expect(err).NotTo(HaveOccurred())
+				Expect(usr).To(Equal(&model.User{UserName: "admin", Password: "wordpass"}))
+			})
+
+			It("fails if JWT token is invalid", func() {
+				_, err := validateUser(context.TODO(), ds, "admin", "", "", "", "invalid.token")
+				Expect(err).To(MatchError(model.ErrInvalidAuth))
+			})
+
+			It("fails if JWT token sub is different than username", func() {
+				_, err := validateUser(context.TODO(), ds, "not_admin", "", "", "", validToken)
+				Expect(err).To(MatchError(model.ErrInvalidAuth))
+			})
+		})
+	})
 })
 
 type mockHandler struct {
@@ -233,23 +299,6 @@ func (mh *mockHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	mh.called = true
 }
 
-type mockUsers struct {
-	engine.Users
-	username, password, token, salt, jwt string
-}
-
-func (m *mockUsers) Authenticate(ctx context.Context, username, password, token, salt, jwt string) (*model.User, error) {
-	m.username = username
-	m.password = password
-	m.token = token
-	m.salt = salt
-	m.jwt = jwt
-	if username == "valid" {
-		return &model.User{UserName: username, Password: password}, nil
-	}
-	return nil, model.ErrInvalidAuth
-}
-
 type mockPlayers struct {
 	engine.Players
 	transcoding *model.Transcoding