From 8963eeb95945bc7b519db2ee3421e60ab61ab425 Mon Sep 17 00:00:00 2001 From: Firehawk Date: Sun, 12 Jul 2026 04:26:51 +0930 Subject: [PATCH] Fix LDAP auth review feedback --- core/ldapauth/ldapauth_test.go | 22 ++++++------ .../20260711130000_add_user_auth_source.sql | 2 ++ server/subsonic/middlewares.go | 35 ++++++++++--------- 3 files changed, 32 insertions(+), 27 deletions(-) diff --git a/core/ldapauth/ldapauth_test.go b/core/ldapauth/ldapauth_test.go index da831ff1c..76ef52e74 100644 --- a/core/ldapauth/ldapauth_test.go +++ b/core/ldapauth/ldapauth_test.go @@ -12,8 +12,8 @@ func TestLoginUserFilter(t *testing.T) { t.Run("defaults to username attribute equality", func(t *testing.T) { t.Parallel() - got := loginUserFilter(Source{UserNameAttribute: "uid"}, "firehawk") - want := "(uid=firehawk)" + got := loginUserFilter(Source{UserNameAttribute: "uid"}, "directory-user") + want := "(uid=directory-user)" if got != want { t.Fatalf("loginUserFilter() = %q, want %q", got, want) } @@ -21,8 +21,8 @@ func TestLoginUserFilter(t *testing.T) { t.Run("preserves explicit placeholder filters", func(t *testing.T) { t.Parallel() - got := loginUserFilter(Source{UserNameAttribute: "uid", UserFilter: "(&(objectClass=person)(%s=%s))"}, "firehawk") - want := "(&(objectClass=person)(uid=firehawk))" + got := loginUserFilter(Source{UserNameAttribute: "uid", UserFilter: "(&(objectClass=person)(%s=%s))"}, "directory-user") + want := "(&(objectClass=person)(uid=directory-user))" if got != want { t.Fatalf("loginUserFilter() = %q, want %q", got, want) } @@ -30,8 +30,8 @@ func TestLoginUserFilter(t *testing.T) { t.Run("supports one-placeholder username filters", func(t *testing.T) { t.Parallel() - got := loginUserFilter(Source{UserNameAttribute: "uid", UserFilter: "(uid=%s)"}, "firehawk") - want := "(uid=firehawk)" + got := loginUserFilter(Source{UserNameAttribute: "uid", UserFilter: "(uid=%s)"}, "directory-user") + want := "(uid=directory-user)" if got != want { t.Fatalf("loginUserFilter() = %q, want %q", got, want) } @@ -39,8 +39,8 @@ func TestLoginUserFilter(t *testing.T) { t.Run("adds username assertion to discovery filters", func(t *testing.T) { t.Parallel() - got := loginUserFilter(Source{UserNameAttribute: "uid", UserFilter: "(objectClass=person)"}, "firehawk") - want := "(&(objectClass=person)(uid=firehawk))" + got := loginUserFilter(Source{UserNameAttribute: "uid", UserFilter: "(objectClass=person)"}, "directory-user") + want := "(&(objectClass=person)(uid=directory-user))" if got != want { t.Fatalf("loginUserFilter() = %q, want %q", got, want) } @@ -65,10 +65,10 @@ func TestAuthInternalSkipsExternalUsersForFallback(t *testing.T) { t.Parallel() repo := tests.CreateMockUserRepo() - repo.Data["firehawk"] = &model.User{ID: "1", UserName: "firehawk", Password: "generated", AuthSource: "ldap", AuthSourceID: "freeipa"} + repo.Data["directory-user"] = &model.User{ID: "1", UserName: "directory-user", Password: "generated", AuthSource: "ldap", AuthSourceID: "freeipa"} ds := &tests.MockDataStore{MockedUser: repo} - user, found, err := authInternal(t.Context(), ds, "firehawk", "ldap-password", true) + user, found, err := authInternal(t.Context(), ds, "directory-user", "ldap-password", true) if err != nil { t.Fatalf("authInternal() unexpected error: %v", err) } @@ -76,7 +76,7 @@ func TestAuthInternalSkipsExternalUsersForFallback(t *testing.T) { t.Fatalf("authInternal() found=%v user=%#v, want external user to be skipped for fallback", found, user) } - user, found, err = authInternal(t.Context(), ds, "firehawk", "ldap-password", false) + user, found, err = authInternal(t.Context(), ds, "directory-user", "ldap-password", false) if err != nil { t.Fatalf("authInternal() unexpected error: %v", err) } diff --git a/db/migrations/20260711130000_add_user_auth_source.sql b/db/migrations/20260711130000_add_user_auth_source.sql index 13f2b730a..164034f20 100644 --- a/db/migrations/20260711130000_add_user_auth_source.sql +++ b/db/migrations/20260711130000_add_user_auth_source.sql @@ -1,10 +1,12 @@ -- +goose Up +-- Add LDAP ownership metadata to users. -- +goose StatementBegin alter table user add column auth_source varchar(32) default '' not null; alter table user add column auth_source_id varchar(255) default '' not null; -- +goose StatementEnd -- +goose Down +-- Roll back the LDAP ownership metadata added in the Up migration. -- +goose StatementBegin alter table user drop column auth_source; alter table user drop column auth_source_id; diff --git a/server/subsonic/middlewares.go b/server/subsonic/middlewares.go index 1d21fdbcc..cb8f969a1 100644 --- a/server/subsonic/middlewares.go +++ b/server/subsonic/middlewares.go @@ -127,25 +127,28 @@ func authenticate(ds model.DataStore) func(next http.Handler) http.Handler { salt, _ := p.String("s") jwt, _ := p.String("jwt") - usr, err = ds.User(ctx).FindByUsernameWithPassword(username) - if errors.Is(err, context.Canceled) { - log.Debug(ctx, "API: Request canceled when authenticating", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) - return - } - switch { - case errors.Is(err, model.ErrNotFound): - if pass != "" && token == "" && jwt == "" { - usr, err = ldapauth.Authenticate(ctx, ds, "", username, pass) - } else { + if pass != "" && token == "" && jwt == "" { + usr, err = ldapauth.Authenticate(ctx, ds, "", username, pass) + if usr == nil && err == nil { err = model.ErrInvalidAuth } - case err != nil: - log.Error(ctx, "API: Error authenticating username", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) - default: - err = validateCredentials(usr, pass, token, salt, jwt) - if err != nil { - log.Warn(ctx, "API: Invalid login", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) + } else { + usr, err = ds.User(ctx).FindByUsernameWithPassword(username) + if errors.Is(err, context.Canceled) { + log.Debug(ctx, "API: Request canceled when authenticating", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) + return } + switch { + case errors.Is(err, model.ErrNotFound): + err = model.ErrInvalidAuth + case err != nil: + log.Error(ctx, "API: Error authenticating username", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) + default: + err = validateCredentials(usr, pass, token, salt, jwt) + } + } + if err != nil { + log.Warn(ctx, "API: Invalid login", "auth", "subsonic", "username", username, "remoteAddr", r.RemoteAddr, err) } }