From 100f6a0645ba80f69e7482157131d3e82effb05c Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 14 Aug 2020 10:10:17 -0400 Subject: [PATCH] Removed `engine.Users` --- cmd/wire_gen.go | 3 +- server/subsonic/api.go | 7 +- server/subsonic/engine/users.go | 63 ------------- server/subsonic/engine/users_test.go | 83 ------------------ server/subsonic/engine/wire_providers.go | 1 - server/subsonic/middlewares.go | 56 ++++++++++-- server/subsonic/middlewares_test.go | 107 +++++++++++++++++------ 7 files changed, 132 insertions(+), 188 deletions(-) delete mode 100644 server/subsonic/engine/users.go delete mode 100644 server/subsonic/engine/users_test.go diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index fe0344e25..40d864e93 100644 --- a/cmd/wire_gen.go +++ b/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 } diff --git a/server/subsonic/api.go b/server/subsonic/api.go index 412f450ad..30d46f5af 100644 --- a/server/subsonic/api.go +++ b/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 diff --git a/server/subsonic/engine/users.go b/server/subsonic/engine/users.go deleted file mode 100644 index a7fe92360..000000000 --- a/server/subsonic/engine/users.go +++ /dev/null @@ -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 -} diff --git a/server/subsonic/engine/users_test.go b/server/subsonic/engine/users_test.go deleted file mode 100644 index 307287dbe..000000000 --- a/server/subsonic/engine/users_test.go +++ /dev/null @@ -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)) - }) - }) - }) -}) diff --git a/server/subsonic/engine/wire_providers.go b/server/subsonic/engine/wire_providers.go index daec9ab74..818d45a9a 100644 --- a/server/subsonic/engine/wire_providers.go +++ b/server/subsonic/engine/wire_providers.go @@ -8,6 +8,5 @@ var Set = wire.NewSet( NewListGenerator, NewPlaylists, NewNowPlayingRepository, - NewUsers, NewPlayers, ) diff --git a/server/subsonic/middlewares.go b/server/subsonic/middlewares.go index 63aceeb46..2f265772f 100644 --- a/server/subsonic/middlewares.go +++ b/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) { diff --git a/server/subsonic/middlewares_test.go b/server/subsonic/middlewares_test.go index 5d7037897..2e596933c 100644 --- a/server/subsonic/middlewares_test.go +++ b/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() { - r := newGetRequest("u=valid", "p=password", "t=token", "s=salt", "jwt=jwt") - cp := authenticate(mockedUsers)(next) + It("passes authentication with correct credentials", func() { + r := newGetRequest("u=admin", "p=wordpass") + 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