diff --git a/server/auth.go b/server/auth.go index fdeb8c455..d9ade7b29 100644 --- a/server/auth.go +++ b/server/auth.go @@ -218,14 +218,14 @@ func UsernameFromExtAuthHeader(r *http.Request) string { log.Error("ExtAuth enabled but no proxy IP found in request context. Please report this error.") return "" } - if !validateIPAgainstList(reverseProxyIp, conf.Server.ExtAuth.TrustedSources) { - log.Warn(r.Context(), "IP is not whitelisted for external authentication", "proxy-ip", reverseProxyIp, "client-ip", r.RemoteAddr) - return "" - } username := r.Header.Get(conf.Server.ExtAuth.UserHeader) if username == "" { return "" } + if !validateIPAgainstList(reverseProxyIp, conf.Server.ExtAuth.TrustedSources) { + log.Warn(r.Context(), "IP is not whitelisted for external authentication", "proxy-ip", reverseProxyIp, "client-ip", r.RemoteAddr) + return "" + } log.Trace(r, "Found username in ExtAuth.UserHeader", "username", username) return username } diff --git a/server/auth_test.go b/server/auth_test.go index af9ffcaeb..a4d592c51 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -16,12 +16,15 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/id" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/sirupsen/logrus" + "github.com/sirupsen/logrus/hooks/test" ) var _ = Describe("Auth", func() { @@ -234,6 +237,56 @@ var _ = Describe("Auth", func() { }) }) + Describe("UsernameFromExtAuthHeader", func() { + var hook *test.Hook + var r *http.Request + + BeforeEach(func() { + conf.Server.ExtAuth.TrustedSources = "192.168.0.0/16" + prevLevel := log.CurrentLevel() + l, h := test.NewNullLogger() + hook = h + prevLogger := log.SetDefaultLogger(l) + log.SetLevel(log.LevelWarn) + DeferCleanup(func() { + log.SetDefaultLogger(prevLogger) + log.SetLevel(prevLevel) + }) + r = httptest.NewRequest("GET", "/", nil) + }) + + warnings := func() []*logrus.Entry { + var ws []*logrus.Entry + for _, e := range hook.AllEntries() { + if e.Level == logrus.WarnLevel { + ws = append(ws, e) + } + } + return ws + } + + It("returns the username from a trusted source", func() { + r.Header.Set("Remote-User", "janedoe") + r = r.WithContext(request.WithReverseProxyIp(r.Context(), "192.168.0.42")) + Expect(UsernameFromExtAuthHeader(r)).To(Equal("janedoe")) + Expect(warnings()).To(BeEmpty()) + }) + + It("does not warn when an untrusted source sends no user header", func() { + r = r.WithContext(request.WithReverseProxyIp(r.Context(), "8.8.8.8")) + Expect(UsernameFromExtAuthHeader(r)).To(BeEmpty()) + Expect(warnings()).To(BeEmpty()) + }) + + It("warns when an untrusted source sends the user header", func() { + r.Header.Set("Remote-User", "janedoe") + r = r.WithContext(request.WithReverseProxyIp(r.Context(), "8.8.8.8")) + Expect(UsernameFromExtAuthHeader(r)).To(BeEmpty()) + Expect(warnings()).To(HaveLen(1)) + Expect(warnings()[0].Message).To(Equal("IP is not whitelisted for external authentication")) + }) + }) + Describe("tokenFromHeader", func() { It("returns the token when the Authorization header is set correctly", func() { req := httptest.NewRequest("GET", "/", nil) diff --git a/ui/src/authProvider.js b/ui/src/authProvider.js index 18badbc9c..f93114ded 100644 --- a/ui/src/authProvider.js +++ b/ui/src/authProvider.js @@ -64,7 +64,8 @@ const authProvider = { logout: () => { removeItems() - if (config.extAuthLogoutURL) { + // Only proxy-authenticated sessions go to the IdP; others (e.g. direct LAN access) get the login form + if (config.extAuthLogoutURL && config.auth) { window.location.href = config.extAuthLogoutURL return Promise.resolve(false) } diff --git a/ui/src/authProvider.test.js b/ui/src/authProvider.test.js new file mode 100644 index 000000000..b16a39815 --- /dev/null +++ b/ui/src/authProvider.test.js @@ -0,0 +1,47 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import config from './config' +import authProvider from './authProvider' + +vi.mock('./config', () => ({ default: {} })) + +describe('authProvider.logout', () => { + const logoutURL = 'https://auth.example.com/signout' + + beforeEach(() => { + vi.stubGlobal('location', { href: '' }) + localStorage.setItem('is-authenticated', 'true') + localStorage.setItem('token', 'abc') + }) + + afterEach(() => { + vi.unstubAllGlobals() + localStorage.clear() + delete config.extAuthLogoutURL + delete config.auth + }) + + it('clears the stored session', async () => { + await authProvider.logout() + expect(localStorage.getItem('is-authenticated')).toBeNull() + expect(localStorage.getItem('token')).toBeNull() + }) + + it('does not redirect when no logout URL is configured', async () => { + config.auth = { id: '1' } + await expect(authProvider.logout()).resolves.toBeUndefined() + expect(window.location.href).toBe('') + }) + + it('redirects to the logout URL when the page was authenticated by the proxy', async () => { + config.extAuthLogoutURL = logoutURL + config.auth = { id: '1' } + await expect(authProvider.logout()).resolves.toBe(false) + expect(window.location.href).toBe(logoutURL) + }) + + it('does not redirect when the page was not authenticated by the proxy', async () => { + config.extAuthLogoutURL = logoutURL + await expect(authProvider.logout()).resolves.toBeUndefined() + expect(window.location.href).toBe('') + }) +}) diff --git a/ui/src/setupTests.js b/ui/src/setupTests.js index ddb999f3c..7cb46e09c 100644 --- a/ui/src/setupTests.js +++ b/ui/src/setupTests.js @@ -14,6 +14,9 @@ const localStorageMock = (function () { setItem: function (key, value) { store[key] = value.toString() }, + removeItem: function (key) { + delete store[key] + }, clear: function () { store = {} },