client/Client: add interface IClient

This allows detangling dependencies and fixes a linker problem in
test/test_translate_song.cxx.

src/LocateUri.cxx

@@ -3,7 +3,7 @@
 
 #include "config.h"
 #include "LocateUri.hxx"
-#include "client/Client.hxx"
+#include "client/IClient.hxx"
 #include "fs/AllocatedPath.hxx"
 #include "ls.hxx"
 #include "storage/Registry.hxx"
@@ -17,7 +17,7 @@
 #include <stdexcept>
 
 static LocatedUri
-LocateFileUri(const char *uri, const Client *client
+LocateFileUri(const char *uri, const IClient *client
 #ifdef ENABLE_DATABASE
 	      , const Storage *storage
 #endif
@@ -86,7 +86,7 @@ LocateAbsoluteUri(UriPluginKind kind, const char *uri
 
 LocatedUri
 LocateUri(UriPluginKind kind,
-	  const char *uri, const Client *client
+	  const char *uri, const IClient *client
 #ifdef ENABLE_DATABASE
 	  , const Storage *storage
 #endif

src/LocateUri.hxx

@@ -18,7 +18,7 @@
 #endif
 #endif
 
-class Client;
+class IClient;
 
 #ifdef ENABLE_DATABASE
 class Storage;
@@ -65,7 +65,7 @@ struct LocatedUri {
  *
  * Throws #std::runtime_error on error.
  *
- * @param client the #Client that is used to determine whether a local
+ * @param client the #IClient that is used to determine whether a local
  * file is allowed; nullptr disables the check and allows all local
  * files
  * @param storage a #Storage instance which may be used to convert
@@ -74,7 +74,7 @@ struct LocatedUri {
  */
 LocatedUri
 LocateUri(UriPluginKind kind,
-	  const char *uri, const Client *client
+	  const char *uri, const IClient *client
 #ifdef ENABLE_DATABASE
 	  , const Storage *storage
 #endif

src/SongLoader.cxx

@@ -3,7 +3,8 @@
 
 #include "SongLoader.hxx"
 #include "LocateUri.hxx"
-#include "client/Client.hxx"
+#include "Partition.hxx"
+#include "client/IClient.hxx"
 #include "db/DatabaseSong.hxx"
 #include "storage/StorageInterface.hxx"
 #include "song/DetachedSong.hxx"
@@ -14,8 +15,9 @@
 
 #ifdef ENABLE_DATABASE
 
-SongLoader::SongLoader(const Client &_client) noexcept
+SongLoader::SongLoader(const IClient &_client) noexcept
-	:client(&_client), db(_client.GetDatabase()),
+	:client(&_client),
+	 db(_client.GetDatabase()),
 	 storage(_client.GetStorage()) {}
 
 #endif

src/SongLoader.hxx

@@ -8,7 +8,7 @@
 
 #include <cstddef>
 
-class Client;
+class IClient;
 class Database;
 class Storage;
 class DetachedSong;
@@ -22,7 +22,7 @@ struct LocatedUri;
  * is assumed that all local files are allowed.
  */
 class SongLoader {
-	const Client *const client;
+	const IClient *const client;
 
 #ifdef ENABLE_DATABASE
 	const Database *const db;
@@ -31,14 +31,14 @@ class SongLoader {
 
 public:
 #ifdef ENABLE_DATABASE
-	explicit SongLoader(const Client &_client) noexcept;
+	explicit SongLoader(const IClient &_client) noexcept;
 	SongLoader(const Database *_db, const Storage *_storage) noexcept
 		:client(nullptr), db(_db), storage(_storage) {}
-	SongLoader(const Client &_client, const Database *_db,
+	SongLoader(const IClient &_client, const Database *_db,
 		   const Storage *_storage) noexcept
 		:client(&_client), db(_db), storage(_storage) {}
 #else
-	explicit SongLoader(const Client &_client) noexcept
+	explicit SongLoader(const IClient &_client) noexcept
 		:client(&_client) {}
 	explicit SongLoader(std::nullptr_t, std::nullptr_t) noexcept
 		:client(nullptr) {}

src/client/Client.hxx

@@ -1,9 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 // Copyright The Music Player Daemon Project
 
-#ifndef MPD_CLIENT_H
+#pragma once
-#define MPD_CLIENT_H
 
+#include "IClient.hxx"
 #include "Message.hxx"
 #include "command/CommandResult.hxx"
 #include "command/CommandListBuilder.hxx"
@@ -32,7 +32,7 @@ class Storage;
 class BackgroundCommand;
 
 class Client final
-	: FullyBufferedSocket
+	: public IClient, FullyBufferedSocket
 {
 	friend struct ClientPerPartitionListHook;
 	friend class ClientList;
@@ -223,19 +223,6 @@ public:
 		}
 	}
 
-	/**
-	 * Is this client allowed to use the specified local file?
-	 *
-	 * Note that this function is vulnerable to timing/symlink attacks.
-	 * We cannot fix this as long as there are plugins that open a file by
-	 * its name, and not by file descriptor / callbacks.
-	 *
-	 * Throws #std::runtime_error on error.
-	 *
-	 * @param path_fs the absolute path name in filesystem encoding
-	 */
-	void AllowFile(Path path_fs) const;
-
 	Partition &GetPartition() const noexcept {
 		return *partition;
 	}
@@ -251,19 +238,18 @@ public:
 	[[gnu::pure]]
 	PlayerControl &GetPlayerControl() const noexcept;
 
-	/**
-	 * Wrapper for Instance::GetDatabase().
-	 */
-	[[gnu::pure]]
-	const Database *GetDatabase() const noexcept;
-
 	/**
 	 * Wrapper for Instance::GetDatabaseOrThrow().
 	 */
 	const Database &GetDatabaseOrThrow() const;
 
-	[[gnu::pure]]
+	// virtual methods from class IClient
-	const Storage *GetStorage() const noexcept;
+	void AllowFile(Path path_fs) const override;
+
+#ifdef ENABLE_DATABASE
+	const Database *GetDatabase() const noexcept override;
+	const Storage *GetStorage() const noexcept override;
+#endif // ENABLE_DATABASE
 
 private:
 	CommandResult ProcessCommandList(bool list_ok,
@@ -287,5 +273,3 @@ void
 client_new(EventLoop &loop, Partition &partition,
 	   UniqueSocketDescriptor fd, SocketAddress address, int uid,
 	   unsigned permission) noexcept;
-
-#endif

src/client/IClient.hxx

@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright The Music Player Daemon Project
+
+#pragma once
+
+#include "config.h"
+
+class Path;
+class Database;
+class Storage;
+struct Partition;
+
+/**
+ * An abstract interface for #Client which can be used for unit tests
+ * instead of the full #Client class.
+ */
+class IClient {
+public:
+	/**
+	 * Is this client allowed to use the specified local file?
+	 *
+	 * Note that this function is vulnerable to timing/symlink attacks.
+	 * We cannot fix this as long as there are plugins that open a file by
+	 * its name, and not by file descriptor / callbacks.
+	 *
+	 * Throws #std::runtime_error on error.
+	 *
+	 * @param path_fs the absolute path name in filesystem encoding
+	 */
+	virtual void AllowFile(Path path_fs) const = 0;
+
+#ifdef ENABLE_DATABASE
+	[[gnu::pure]]
+	virtual const Database *GetDatabase() const noexcept = 0;
+
+	[[gnu::pure]]
+	virtual const Storage *GetStorage() const noexcept = 0;
+#endif // ENABLE_DATABASE
+};

test/test_translate_song.cxx

@@ -6,7 +6,7 @@
 #include "playlist/PlaylistSong.hxx"
 #include "song/DetachedSong.hxx"
 #include "SongLoader.hxx"
-#include "client/Client.hxx"
+#include "client/IClient.hxx"
 #include "tag/Builder.hxx"
 #include "tag/Names.hxx"
 #include "tag/Tag.hxx"
@@ -108,27 +108,27 @@ DetachedSong::LoadFile(Path path)
 	return false;
 }
 
-const Database *
+class TestClient final : public IClient {
-Client::GetDatabase() const noexcept
+public:
-{
+	// virtual methods from class IClient
-	return reinterpret_cast<const Database *>(this);
+	void AllowFile([[maybe_unused]] Path path_fs) const override {
-}
-
-const Storage *
-Client::GetStorage() const noexcept
-{
-	return ::storage;
-}
-
-void
-Client::AllowFile([[maybe_unused]] Path path_fs) const
-{
 		/* always fail, so a SongLoader with a non-nullptr
 		   Client pointer will be regarded "insecure", while one with
 		   client==nullptr will allow all files */
-	throw std::runtime_error("foo");
+		throw std::runtime_error{"foo"};
 	}
 
+#ifdef ENABLE_DATABASE
+	const Database *GetDatabase() const noexcept override {
+		return reinterpret_cast<const Database *>(this);
+	}
+
+	const Storage *GetStorage() const noexcept override {
+		return ::storage;
+	}
+#endif // ENABLE_DATABASE
+};
+
 static std::string
 ToString(const Tag &tag)
 {
@@ -201,7 +201,8 @@ TEST_F(TranslateSongTest, Insecure)
 {
 	/* illegal because secure=false */
 	DetachedSong song1 (uri1);
-	const SongLoader loader(*reinterpret_cast<const Client *>(1));
+	TestClient client;
+	const SongLoader loader{client};
 	EXPECT_FALSE(playlist_check_translate_song(song1, {},
 						   loader));
 }
@@ -243,8 +244,9 @@ TEST_F(TranslateSongTest, Relative)
 {
 	const Database &db = *reinterpret_cast<const Database *>(1);
 	const SongLoader secure_loader(&db, storage);
-	const SongLoader insecure_loader(*reinterpret_cast<const Client *>(1),
+
-					 &db, storage);
+	TestClient client;
+	const SongLoader insecure_loader{client, &db, storage};
 
 	/* map to music_directory */
 	DetachedSong song1("bar.ogg", MakeTag2b());