From a5d7f5e1fae712024b73c59be88aa3fcc1086f9f Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Sat, 25 Nov 2023 23:06:24 +0100 Subject: [PATCH] client/Client: add interface IClient This allows detangling dependencies and fixes a linker problem in test/test_translate_song.cxx. --- src/LocateUri.cxx | 6 ++--- src/LocateUri.hxx | 6 ++--- src/SongLoader.cxx | 8 ++++--- src/SongLoader.hxx | 10 ++++---- src/client/Client.hxx | 36 ++++++++-------------------- src/client/IClient.hxx | 39 ++++++++++++++++++++++++++++++ test/test_translate_song.cxx | 46 +++++++++++++++++++----------------- 7 files changed, 89 insertions(+), 62 deletions(-) create mode 100644 src/client/IClient.hxx diff --git a/src/LocateUri.cxx b/src/LocateUri.cxx index 0d01d06fd..94e811ccf 100644 --- a/src/LocateUri.cxx +++ b/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 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 diff --git a/src/LocateUri.hxx b/src/LocateUri.hxx index 3f843d5cb..cef8b140d 100644 --- a/src/LocateUri.hxx +++ b/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 diff --git a/src/SongLoader.cxx b/src/SongLoader.cxx index 303575db5..264d6b046 100644 --- a/src/SongLoader.cxx +++ b/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 - :client(&_client), db(_client.GetDatabase()), +SongLoader::SongLoader(const IClient &_client) noexcept + :client(&_client), + db(_client.GetDatabase()), storage(_client.GetStorage()) {} #endif diff --git a/src/SongLoader.hxx b/src/SongLoader.hxx index eb4d18516..e1c233c59 100644 --- a/src/SongLoader.hxx +++ b/src/SongLoader.hxx @@ -8,7 +8,7 @@ #include -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) {} diff --git a/src/client/Client.hxx b/src/client/Client.hxx index b0f6514c3..7213b10b0 100644 --- a/src/client/Client.hxx +++ b/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 -#define MPD_CLIENT_H +#pragma once +#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]] - const Storage *GetStorage() const noexcept; + // virtual methods from class IClient + 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 diff --git a/src/client/IClient.hxx b/src/client/IClient.hxx new file mode 100644 index 000000000..3a24a19fd --- /dev/null +++ b/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 +}; diff --git a/test/test_translate_song.cxx b/test/test_translate_song.cxx index 77462d18f..ebcb420d2 100644 --- a/test/test_translate_song.cxx +++ b/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,26 +108,26 @@ DetachedSong::LoadFile(Path path) return false; } -const Database * -Client::GetDatabase() const noexcept -{ - return reinterpret_cast(this); -} +class TestClient final : public IClient { +public: + // virtual methods from class IClient + void AllowFile([[maybe_unused]] Path path_fs) const override { + /* 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"}; + } -const Storage * -Client::GetStorage() const noexcept -{ - return ::storage; -} +#ifdef ENABLE_DATABASE + const Database *GetDatabase() const noexcept override { + return reinterpret_cast(this); + } -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"); -} + 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(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(1); const SongLoader secure_loader(&db, storage); - const SongLoader insecure_loader(*reinterpret_cast(1), - &db, storage); + + TestClient client; + const SongLoader insecure_loader{client, &db, storage}; /* map to music_directory */ DetachedSong song1("bar.ogg", MakeTag2b());