diff --git a/core/archiver.go b/core/archiver.go index 5d1c090cd..8c42f8f49 100644 --- a/core/archiver.go +++ b/core/archiver.go @@ -21,7 +21,7 @@ import ( type Archiver interface { ZipAlbum(ctx context.Context, id string, format string, bitrate int, w io.Writer) error ZipArtist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error - ZipShare(ctx context.Context, id string, w io.Writer) error + ZipShare(ctx context.Context, s *model.Share, w io.Writer) error ZipPlaylist(ctx context.Context, id string, format string, bitrate int, w io.Writer) error } @@ -100,16 +100,14 @@ func (a *archiver) albumFilename(mf model.MediaFile, format string, isMultiDisc return fmt.Sprintf("%s/%s", str.SanitizeFilename(mf.Album), file) } -func (a *archiver) ZipShare(ctx context.Context, id string, out io.Writer) error { - s, err := a.shares.Load(ctx, id) - if err != nil { - return err - } +// ZipShare takes an already-loaded share: Share.Load records a visit, so +// loading it again here would count every download twice. +func (a *archiver) ZipShare(ctx context.Context, s *model.Share, out io.Writer) error { if !s.Downloadable { return model.ErrNotAuthorized } log.Debug(ctx, "Zipping share", "name", s.ID, "format", s.Format, "bitrate", s.MaxBitRate, "numTracks", len(s.Tracks)) - return a.zipMediaFiles(ctx, id, s.ID, s.Format, s.MaxBitRate, out, s.Tracks, false) + return a.zipMediaFiles(ctx, s.ID, s.ID, s.Format, s.MaxBitRate, out, s.Tracks, false) } func (a *archiver) ZipPlaylist(ctx context.Context, id string, format string, bitrate int, out io.Writer) error { diff --git a/core/archiver_test.go b/core/archiver_test.go index f432139d8..2ba8f1fc0 100644 --- a/core/archiver_test.go +++ b/core/archiver_test.go @@ -130,13 +130,16 @@ var _ = Describe("Archiver", func() { Tracks: mfs, } - sh.On("Load", mock.Anything, "1").Return(share, nil) ms.On("NewStream", mock.Anything, mock.Anything, stream.Request{Format: "mp3", BitRate: 128}).Return(io.NopCloser(strings.NewReader("test")), nil).Times(2) out := new(bytes.Buffer) - err := arch.ZipShare(context.Background(), "1", out) + err := arch.ZipShare(context.Background(), share, out) Expect(err).To(BeNil()) + // Share.Load records a visit; re-loading here would double-count + // every download. + sh.AssertNotCalled(GinkgoT(), "Load", mock.Anything, mock.Anything) + zr, err := zip.NewReader(bytes.NewReader(out.Bytes()), int64(out.Len())) Expect(err).To(BeNil()) diff --git a/server/public/handle_downloads.go b/server/public/handle_downloads.go index 6aa35c341..0012c4b35 100644 --- a/server/public/handle_downloads.go +++ b/server/public/handle_downloads.go @@ -1,18 +1,41 @@ package public import ( + "cmp" + "fmt" "net/http" + "strings" + "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/req" + "github.com/navidrome/navidrome/utils/str" ) func (pub *Router) handleDownloads(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() id, err := req.Params(r).String(":id") if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - err = pub.archiver.ZipShare(r.Context(), id, w) - checkShareError(r.Context(), w, err, id) + // Load the share before streaming: once ZipShare writes its first byte the + // status is locked at 200, so errors could no longer be reported. + s, err := pub.share.Load(ctx, id) + if err != nil { + checkShareError(ctx, w, err, id) + return + } + if !s.Downloadable { + checkShareError(ctx, w, model.ErrNotAuthorized, id) + return + } + + name := str.SanitizeFilename(cmp.Or(s.Description, s.ID)) + name = strings.ReplaceAll(name, ",", "_") + w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", name+".zip")) + w.Header().Set("Content-Type", "application/zip") + + err = pub.archiver.ZipShare(ctx, s, w) + checkShareError(ctx, w, err, id) } diff --git a/server/public/handle_downloads_test.go b/server/public/handle_downloads_test.go new file mode 100644 index 000000000..1a97f4379 --- /dev/null +++ b/server/public/handle_downloads_test.go @@ -0,0 +1,134 @@ +package public + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "time" + + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +type mockArchiver struct { + called bool + err error +} + +func (m *mockArchiver) ZipAlbum(context.Context, string, string, int, io.Writer) error { + return nil +} + +func (m *mockArchiver) ZipArtist(context.Context, string, string, int, io.Writer) error { + return nil +} + +func (m *mockArchiver) ZipPlaylist(context.Context, string, string, int, io.Writer) error { + return nil +} + +func (m *mockArchiver) ZipShare(_ context.Context, _ *model.Share, w io.Writer) error { + m.called = true + if m.err != nil { + return m.err + } + _, _ = w.Write([]byte("zip-contents")) + return nil +} + +var _ = Describe("handleDownloads", func() { + var ds *tests.MockDataStore + var shareRepo *tests.MockShareRepo + var archiver *mockArchiver + var pub *Router + + BeforeEach(func() { + ds = &tests.MockDataStore{} + shareRepo = &tests.MockShareRepo{} + ds.MockedShare = shareRepo + archiver = &mockArchiver{} + pub = &Router{ds: ds, archiver: archiver, share: core.NewShare(ds)} + }) + + shareIs := func(s *model.Share) { + shareRepo.ID = s.ID + shareRepo.Entity = s + } + + makeRequest := func(id string) *httptest.ResponseRecorder { + r := httptest.NewRequest("GET", "/public/d/"+id+"?%3Aid="+id, nil) + w := httptest.NewRecorder() + pub.handleDownloads(w, r) + return w + } + + It("sets a Content-Disposition filename from the share description", func() { + shareIs(&model.Share{ID: "abc123", Description: "My Mixtape", Downloadable: true}) + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="My Mixtape.zip"`)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/zip")) + Expect(archiver.called).To(BeTrue()) + Expect(w.Body.String()).To(Equal("zip-contents")) + }) + + It("falls back to the share ID when there is no description", func() { + shareIs(&model.Share{ID: "abc123", Downloadable: true}) + + w := makeRequest("abc123") + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="abc123.zip"`)) + }) + + It("sanitizes characters that are unsafe in a filename", func() { + shareIs(&model.Share{ID: "abc123", Description: `AC/DC: Live, 1979`, Downloadable: true}) + + w := makeRequest("abc123") + + Expect(w.Header().Get("Content-Disposition")).To(Equal(`attachment; filename="AC_DC_ Live_ 1979.zip"`)) + }) + + It("returns 403 without invoking the archiver when the share is not downloadable", func() { + shareIs(&model.Share{ID: "abc123", Description: "No Download", Downloadable: false}) + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusForbidden)) + Expect(archiver.called).To(BeFalse()) + Expect(w.Header().Get("Content-Disposition")).To(BeEmpty()) + }) + + It("returns 404 when the share does not exist", func() { + shareIs(&model.Share{ID: "other", Downloadable: true}) + + w := makeRequest("missing") + + Expect(w.Code).To(Equal(http.StatusNotFound)) + Expect(archiver.called).To(BeFalse()) + }) + + It("returns 410 when the share has expired", func() { + shareIs(&model.Share{ID: "abc123", Downloadable: true, ExpiresAt: new(time.Now().Add(-time.Hour))}) + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusGone)) + Expect(archiver.called).To(BeFalse()) + }) + + It("returns 500 when the share lookup fails", func() { + shareRepo.Error = errors.New("db error") + + w := makeRequest("abc123") + + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + Expect(archiver.called).To(BeFalse()) + }) +}) diff --git a/server/subsonic/e2e/e2e_suite_test.go b/server/subsonic/e2e/e2e_suite_test.go index 8db9b621c..44098e69e 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/server/subsonic/e2e/e2e_suite_test.go @@ -330,7 +330,7 @@ func (n noopArchiver) ZipArtist(context.Context, string, string, int, io.Writer) return model.ErrNotFound } -func (n noopArchiver) ZipShare(context.Context, string, io.Writer) error { +func (n noopArchiver) ZipShare(context.Context, *model.Share, io.Writer) error { return model.ErrNotFound } diff --git a/ui/src/share/SharePlayer.jsx b/ui/src/share/SharePlayer.jsx index a3a15e50a..2384866ea 100644 --- a/ui/src/share/SharePlayer.jsx +++ b/ui/src/share/SharePlayer.jsx @@ -1,15 +1,24 @@ import ReactJkMusicPlayer from 'navidrome-music-player' +import { useCallback, useEffect, useRef, useState } from 'react' import config, { shareInfo } from '../config' import { shareCoverUrl, shareDownloadUrl, shareStreamUrl } from '../utils' import { makeStyles } from '@material-ui/core/styles' +// How long the download button stays inert after a click. The browser needs a +// moment to show its own download UI; until then the page looks unresponsive. +export const DOWNLOAD_FEEDBACK_MS = 5000 + const useStyle = makeStyles({ player: { '& .group .next-audio': { pointerEvents: (props) => props.single && 'none', opacity: (props) => props.single && 0.65, }, + '& .group.audio-download': { + pointerEvents: (props) => props.downloading && 'none', + opacity: (props) => props.downloading && 0.65, + }, '@media (min-width: 768px)': { '& .react-jinke-music-player-mobile > div': { width: 768, @@ -23,7 +32,14 @@ const useStyle = makeStyles({ }) const SharePlayer = () => { - const classes = useStyle({ single: shareInfo?.tracks.length === 1 }) + const [downloading, setDownloading] = useState(false) + const timer = useRef(null) + const classes = useStyle({ + single: shareInfo?.tracks.length === 1, + downloading, + }) + + useEffect(() => () => clearTimeout(timer.current), []) const list = shareInfo?.tracks.map((s) => { return { @@ -34,11 +50,23 @@ const SharePlayer = () => { duration: s.duration, } }) - const onBeforeAudioDownload = () => { - return Promise.resolve({ - src: shareDownloadUrl(shareInfo?.id), - }) - } + // An anchor, not a navigation: the service worker's NavigationRoute would + // intercept the streamed archive and fail it. + const customDownloader = useCallback(() => { + const link = document.createElement('a') + link.href = shareDownloadUrl(shareInfo?.id) + link.download = '' + document.body.appendChild(link) + link.click() + document.body.removeChild(link) + + setDownloading(true) + clearTimeout(timer.current) + timer.current = setTimeout( + () => setDownloading(false), + DOWNLOAD_FEEDBACK_MS, + ) + }, []) const options = { audioLists: list, mode: 'full', @@ -59,7 +87,7 @@ const SharePlayer = () => { ) } diff --git a/ui/src/share/SharePlayer.test.jsx b/ui/src/share/SharePlayer.test.jsx new file mode 100644 index 000000000..68d1eff59 --- /dev/null +++ b/ui/src/share/SharePlayer.test.jsx @@ -0,0 +1,139 @@ +import { render, act } from '@testing-library/react' +import SharePlayer, { DOWNLOAD_FEEDBACK_MS } from './SharePlayer' + +let playerProps +let renderCount + +vi.mock('navidrome-music-player', () => ({ + default: (props) => { + playerProps = props + renderCount++ + return
+ }, +})) + +vi.mock('../config', () => ({ + default: { enableDownloads: true }, + shareInfo: { + id: 'share-1', + downloadable: true, + tracks: [{ id: 't1', title: 'One', artist: 'A', duration: 100 }], + }, +})) + +vi.mock('../utils', () => ({ + shareDownloadUrl: (id) => `/share/d/${id}`, + shareStreamUrl: (id) => `/share/s/${id}`, + shareCoverUrl: (id) => `/share/img/${id}`, +})) + +describe('SharePlayer', () => { + let clickSpy + + beforeEach(() => { + vi.useFakeTimers() + playerProps = null + renderCount = 0 + // Downloading for real would navigate the jsdom window. + clickSpy = vi + .spyOn(HTMLAnchorElement.prototype, 'click') + .mockImplementation(() => {}) + }) + + afterEach(() => { + vi.runOnlyPendingTimers() + vi.useRealTimers() + vi.restoreAllMocks() + }) + + it('downloads via an anchor so the service worker does not intercept it', () => { + render() + + let anchor + clickSpy.mockImplementation(function () { + anchor = { href: this.href, download: this.download } + }) + + act(() => { + playerProps.customDownloader() + }) + + expect(clickSpy).toHaveBeenCalledTimes(1) + expect(anchor.href).toContain('/share/d/share-1') + // Empty, so the server's Content-Disposition filename wins. + expect(anchor.download).toBe('') + }) + + it('removes the anchor from the document after clicking', () => { + render() + + act(() => { + playerProps.customDownloader() + }) + + expect(document.querySelectorAll('a[download]')).toHaveLength(0) + }) + + // The inert styling itself is driven by JSS function values, which jsdom does + // not evaluate; it is verified in a browser. What is checked here is the + // state machine feeding it -- that the component re-renders on download and + // again when the window closes. + it('re-renders when the feedback window opens and closes', () => { + render() + + const beforeDownload = renderCount + act(() => { + playerProps.customDownloader() + }) + expect(renderCount).toBeGreaterThan(beforeDownload) + + const beforeExpiry = renderCount + act(() => { + vi.runAllTimers() + }) + expect(renderCount).toBeGreaterThan(beforeExpiry) + }) + + it('restarts the feedback window on a repeat download', () => { + render() + + act(() => { + playerProps.customDownloader() + }) + const elapsedBeforeRepeat = Math.floor(DOWNLOAD_FEEDBACK_MS / 2) + act(() => { + vi.advanceTimersByTime(elapsedBeforeRepeat) + }) + act(() => { + playerProps.customDownloader() + }) + + // Past the first timer's deadline, which it would have fired at had the + // repeat download not replaced it. + const beforeOriginalDeadline = renderCount + act(() => { + vi.advanceTimersByTime(DOWNLOAD_FEEDBACK_MS - elapsedBeforeRepeat + 1) + }) + expect(renderCount).toBe(beforeOriginalDeadline) + + act(() => { + vi.runAllTimers() + }) + expect(renderCount).toBeGreaterThan(beforeOriginalDeadline) + }) + + it('does not update state after unmount', () => { + const { unmount } = render() + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + act(() => { + playerProps.customDownloader() + }) + unmount() + act(() => { + vi.runAllTimers() + }) + + expect(errorSpy).not.toHaveBeenCalled() + }) +})