Merge branch 'master' into artwork-blurhash

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 {

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())
 

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)
 }

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())
+	})
+})

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
 }
 

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 = () => {
     <ReactJkMusicPlayer
       {...options}
       className={classes.player}
-      onBeforeAudioDownload={onBeforeAudioDownload}
+      customDownloader={customDownloader}
     />
   )
 }

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 <div data-testid="player" />
+  },
+}))
+
+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(<SharePlayer />)
+
+    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(<SharePlayer />)
+
+    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(<SharePlayer />)
+
+    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(<SharePlayer />)
+
+    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(<SharePlayer />)
+    const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+
+    act(() => {
+      playerProps.customDownloader()
+    })
+    unmount()
+    act(() => {
+      vi.runAllTimers()
+    })
+
+    expect(errorSpy).not.toHaveBeenCalled()
+  })
+})