perf(db): skip library filter when a non-admin sees all libraries (#5696)

* perf(db): skip library filter when a non-admin sees all libraries

applyLibraryFilter already short-circuits for admins and headless
contexts, but a non-admin who has been granted every library still paid
for the correlated user_library subquery, which filters out nothing yet
is the slow non-admin list/count path.

Reuse the visibility check search3 already had: skip the subquery when
the user's granted libraries cover the whole library table. The two
helpers (userSeesAllLibraries/visibleLibraryIDs) are promoted from
artist_repository to the base sqlRepository so all ~13 call sites
benefit and search3 shares the single implementation.

The skip is strictly gated on granted count >= total library count,
never on an empty/unknown library set, so access control is unchanged
for restricted users.

* refactor(db): make all-libraries skip fail closed; test cleanups

- userSeesAllLibraries: require len(visible) == total (not >=) so the
  filter skip can never over-grant if the visible set is ever inflated.
- tests: assert ToSql() returns no error; restore r.db in AfterEach to
  avoid leaking mutated state to other specs.

Signed-off-by: Deluan <deluan@navidrome.org>

---------

Signed-off-by: Deluan <deluan@navidrome.org>

persistence/artist_repository.go

@@ -668,32 +668,6 @@ func isLibraryIDFilter(filter Sqlizer) bool {
 	return ok
 }
 
-// userSeesAllLibraries reports whether the visible set already covers every library, so a search
-// needs no library filter at all.
-func (r *artistRepository) userSeesAllLibraries(visible []int) bool {
-	user := loggedUser(r.ctx)
-	if user.IsAdmin || user.ID == invalidUserId {
-		return true // visible is the whole library table
-	}
-	total, err := NewLibraryRepository(r.ctx, r.db).CountAll()
-	if err != nil || total == 0 {
-		return false
-	}
-	return int64(len(visible)) >= total
-}
-
-// visibleLibraryIDs returns the libraries the current user can see: all libraries for admin and
-// headless processes, otherwise the user's granted libraries.
-func (r *artistRepository) visibleLibraryIDs() ([]int, error) {
-	user := loggedUser(r.ctx)
-	if user.IsAdmin || user.ID == invalidUserId {
-		var ids []int
-		err := r.queryAllSlice(Select("id").From("library"), &ids)
-		return ids, err
-	}
-	return slice.Map(user.Libraries, func(lib model.Library) int { return lib.ID }), nil
-}
-
 func (r *artistRepository) Count(options ...rest.QueryOptions) (int64, error) {
 	return r.CountAll(r.parseRestOptions(r.ctx, options...))
 }

persistence/sql_base_repository.go

@@ -229,6 +229,12 @@ func (r sqlRepository) applyLibraryFilter(sq SelectBuilder, tableName ...string)
 		return sq
 	}
 
+	// A non-admin granted every library sees everything the subquery would return, so applying it is
+	// pure overhead. Skip it in that case (same fast path admins get).
+	if visible, err := r.visibleLibraryIDs(); err == nil && r.userSeesAllLibraries(visible) {
+		return sq
+	}
+
 	table := r.tableName
 	if len(tableName) > 0 {
 		table = tableName[0]
@@ -240,6 +246,32 @@ func (r sqlRepository) applyLibraryFilter(sq SelectBuilder, tableName ...string)
 		"SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)", user.ID))
 }
 
+// userSeesAllLibraries reports whether the visible set already covers every library, so a
+// library filter would exclude nothing.
+func (r sqlRepository) userSeesAllLibraries(visible []int) bool {
+	user := loggedUser(r.ctx)
+	if user.IsAdmin || user.ID == invalidUserId {
+		return true // visible is the whole library table
+	}
+	total, err := NewLibraryRepository(r.ctx, r.db).CountAll()
+	if err != nil || total == 0 {
+		return false
+	}
+	return int64(len(visible)) == total
+}
+
+// visibleLibraryIDs returns the libraries the current user can see: all libraries for admin and
+// headless processes, otherwise the user's granted libraries.
+func (r sqlRepository) visibleLibraryIDs() ([]int, error) {
+	user := loggedUser(r.ctx)
+	if user.IsAdmin || user.ID == invalidUserId {
+		var ids []int
+		err := r.queryAllSlice(Select("id").From("library"), &ids)
+		return ids, err
+	}
+	return slice.Map(user.Libraries, func(lib model.Library) int { return lib.ID }), nil
+}
+
 func (r sqlRepository) seedKey() string {
 	// Seed keys must be all lowercase, or else SQLite3 will encode it, making it not match the seed
 	// used in the query. Hashing the user ID and converting it to a hex string will do the trick

persistence/sql_base_repository_test.go

@@ -226,9 +226,21 @@ var _ = Describe("sqlRepository", func() {
 
 	Describe("applyLibraryFilter", func() {
 		var sq squirrel.SelectBuilder
+		var savedDB = r.db
 
 		BeforeEach(func() {
 			sq = squirrel.Select("*").From("test_table")
+			// Add library 2 so a user granted only library 1 is a genuine strict subset.
+			savedDB = r.db
+			r.db = GetDBXBuilder()
+			_, err := r.db.NewQuery("INSERT OR IGNORE INTO library (id, name, path) VALUES (2, 'Lib 2', '/lib2')").Execute()
+			Expect(err).ToNot(HaveOccurred())
+		})
+
+		AfterEach(func() {
+			_, err := r.db.NewQuery("DELETE FROM library WHERE id = 2").Execute()
+			Expect(err).ToNot(HaveOccurred())
+			r.db = savedDB
 		})
 
 		Context("Admin User", func() {
@@ -238,31 +250,73 @@ var _ = Describe("sqlRepository", func() {
 
 			It("should not apply library filter for admin users", func() {
 				result := r.applyLibraryFilter(sq)
-				sql, _, _ := result.ToSql()
+				sql, _, err := result.ToSql()
+				Expect(err).ToNot(HaveOccurred())
 				Expect(sql).To(Equal("SELECT * FROM test_table"))
 			})
 		})
 
-		Context("Regular User", func() {
+		Context("Regular User with a subset of libraries", func() {
 			BeforeEach(func() {
-				r.ctx = request.WithUser(context.Background(), model.User{ID: "user123", IsAdmin: false})
+				// Strict subset: granted lib 1, DB has libs 1 and 2, so the filter must apply.
+				r.ctx = request.WithUser(context.Background(), model.User{
+					ID: "user123", IsAdmin: false, Libraries: model.Libraries{{ID: 1}},
+				})
 			})
 
 			It("should apply library filter for regular users", func() {
 				result := r.applyLibraryFilter(sq)
-				sql, args, _ := result.ToSql()
+				sql, args, err := result.ToSql()
+				Expect(err).ToNot(HaveOccurred())
 				Expect(sql).To(ContainSubstring("IN (SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)"))
 				Expect(args).To(ContainElement("user123"))
 			})
 
 			It("should use custom table name when provided", func() {
 				result := r.applyLibraryFilter(sq, "custom_table")
-				sql, args, _ := result.ToSql()
+				sql, args, err := result.ToSql()
+				Expect(err).ToNot(HaveOccurred())
 				Expect(sql).To(ContainSubstring("custom_table.library_id IN"))
 				Expect(args).To(ContainElement("user123"))
 			})
 		})
 
+		Context("Regular User with no libraries", func() {
+			BeforeEach(func() {
+				r.ctx = request.WithUser(context.Background(), model.User{ID: "empty", IsAdmin: false})
+			})
+
+			It("should apply the library filter (never skip on empty)", func() {
+				result := r.applyLibraryFilter(sq)
+				sql, _, err := result.ToSql()
+				Expect(err).ToNot(HaveOccurred())
+				Expect(sql).To(ContainSubstring("IN (SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)"))
+			})
+		})
+
+		Context("Regular User who can see all libraries", func() {
+			BeforeEach(func() {
+				// Granted every library in the DB, so the filter would exclude nothing.
+				r.ctx = request.WithUser(context.Background(), model.User{
+					ID: "alllibs", IsAdmin: false, Libraries: model.Libraries{{ID: 1}, {ID: 2}},
+				})
+			})
+
+			It("should not apply the library filter (subquery would filter nothing)", func() {
+				result := r.applyLibraryFilter(sq)
+				sql, _, err := result.ToSql()
+				Expect(err).ToNot(HaveOccurred())
+				Expect(sql).To(Equal("SELECT * FROM test_table"))
+			})
+
+			It("should not apply the filter even with a custom table name", func() {
+				result := r.applyLibraryFilter(sq, "custom_table")
+				sql, _, err := result.ToSql()
+				Expect(err).ToNot(HaveOccurred())
+				Expect(sql).To(Equal("SELECT * FROM test_table"))
+			})
+		})
+
 		Context("Headless Process (No User Context)", func() {
 			BeforeEach(func() {
 				r.ctx = context.Background() // No user context
@@ -270,13 +324,15 @@ var _ = Describe("sqlRepository", func() {
 
 			It("should not apply library filter for headless processes", func() {
 				result := r.applyLibraryFilter(sq)
-				sql, _, _ := result.ToSql()
+				sql, _, err := result.ToSql()
+				Expect(err).ToNot(HaveOccurred())
 				Expect(sql).To(Equal("SELECT * FROM test_table"))
 			})
 
 			It("should not apply library filter even with custom table name", func() {
 				result := r.applyLibraryFilter(sq, "custom_table")
-				sql, _, _ := result.ToSql()
+				sql, _, err := result.ToSql()
+				Expect(err).ToNot(HaveOccurred())
 				Expect(sql).To(Equal("SELECT * FROM test_table"))
 			})
 		})