From 68840dda958475a1ad5ca9b0bbef39e5b965262d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Korina=20=C5=A0imi=C4=8Devi=C4=87?= Date: Thu, 2 May 2024 13:04:21 +0200 Subject: [PATCH] Add missing topic alias name validation Summary: related to T13767 - allow empty topic name + topic alias in props - add mqtt features tests Reviewers: ivica Reviewed By: ivica Subscribers: miljen, iljazovic Differential Revision: https://repo.mireo.local/D29445 --- .../async_mqtt5/detail/topic_validation.hpp | 14 +- include/async_mqtt5/impl/publish_send_op.hpp | 7 +- include/async_mqtt5/impl/subscribe_op.hpp | 2 +- .../integration/{coroutine.cpp => client.cpp} | 12 +- test/integration/mqtt_features.cpp | 181 ++++++++++++++++++ test/unit/publish_send_op.cpp | 2 +- test/unit/string_validation.cpp | 19 ++ 7 files changed, 223 insertions(+), 14 deletions(-) rename test/integration/{coroutine.cpp => client.cpp} (95%) create mode 100644 test/integration/mqtt_features.cpp diff --git a/include/async_mqtt5/detail/topic_validation.hpp b/include/async_mqtt5/detail/topic_validation.hpp index 78697df..7576110 100644 --- a/include/async_mqtt5/detail/topic_validation.hpp +++ b/include/async_mqtt5/detail/topic_validation.hpp @@ -10,7 +10,7 @@ namespace async_mqtt5::detail { static constexpr uint32_t min_subscription_identifier = 1; static constexpr uint32_t max_subscription_identifier = 268'435'455; -static constexpr std::string_view shared_sub_id = "$share/"; +static constexpr std::string_view shared_sub_prefix = "$share/"; inline bool is_utf8_no_wildcard(validation_result result) { return result == validation_result::valid; @@ -28,7 +28,11 @@ inline validation_result validate_topic_name(std::string_view str) { return validate_impl(str, is_valid_topic_size, is_utf8_no_wildcard); } -inline validation_result validate_share_name(std::string_view str) { +inline validation_result validate_topic_alias_name(std::string_view str) { + return validate_impl(str, is_valid_string_size, is_utf8_no_wildcard); +} + +inline validation_result validate_shared_topic_name(std::string_view str) { return validate_impl(str, is_not_empty, is_utf8_no_wildcard); } @@ -80,17 +84,17 @@ inline validation_result validate_shared_topic_filter( if (!is_valid_topic_size(str.size())) return validation_result::invalid; - if (str.compare(0, shared_sub_id.size(), shared_sub_id) != 0) + if (str.compare(0, shared_sub_prefix.size(), shared_sub_prefix) != 0) return validation_result::invalid; - str.remove_prefix(shared_sub_id.size()); + str.remove_prefix(shared_sub_prefix.size()); size_t share_name_end = str.find_first_of('/'); if (share_name_end == std::string::npos) return validation_result::invalid; validation_result result; - result = validate_share_name(str.substr(0, share_name_end)); + result = validate_shared_topic_name(str.substr(0, share_name_end)); if (result != validation_result::valid) return validation_result::invalid; diff --git a/include/async_mqtt5/impl/publish_send_op.hpp b/include/async_mqtt5/impl/publish_send_op.hpp index 4818b67..487f69f 100644 --- a/include/async_mqtt5/impl/publish_send_op.hpp +++ b/include/async_mqtt5/impl/publish_send_op.hpp @@ -332,7 +332,12 @@ private: constexpr uint8_t default_maximum_qos = 2; constexpr uint8_t default_payload_format_ind = 0; - if (validate_topic_name(topic) != validation_result::valid) + auto topic_name_valid = props[prop::topic_alias].has_value() ? + validate_topic_alias_name(topic) == validation_result::valid : + validate_topic_name(topic) == validation_result::valid + ; + + if (!topic_name_valid) return client::error::invalid_topic; auto max_qos = _svc_ptr->connack_property(prop::maximum_qos) diff --git a/include/async_mqtt5/impl/subscribe_op.hpp b/include/async_mqtt5/impl/subscribe_op.hpp index 1130cc0..27194ef 100644 --- a/include/async_mqtt5/impl/subscribe_op.hpp +++ b/include/async_mqtt5/impl/subscribe_op.hpp @@ -200,7 +200,7 @@ private: validation_result result = validation_result::valid; if ( - topic_filter.compare(0, shared_sub_id.size(), shared_sub_id) == 0 + topic_filter.compare(0, shared_sub_prefix.size(), shared_sub_prefix) == 0 ) { if (!shared_available) return client::error::shared_subscription_not_available; diff --git a/test/integration/coroutine.cpp b/test/integration/client.cpp similarity index 95% rename from test/integration/coroutine.cpp rename to test/integration/client.cpp index eed675f..763c01c 100644 --- a/test/integration/coroutine.cpp +++ b/test/integration/client.cpp @@ -46,7 +46,7 @@ void assign_tls_sni( } // end namespace async_mqtt5 -BOOST_AUTO_TEST_SUITE(coroutine/*, *boost::unit_test::disabled()*/) +BOOST_AUTO_TEST_SUITE(client/*, *boost::unit_test::disabled()*/) using namespace async_mqtt5; namespace asio = boost::asio; @@ -54,7 +54,7 @@ namespace asio = boost::asio; constexpr auto use_nothrow_awaitable = asio::as_tuple(asio::use_awaitable); template -asio::awaitable sanity_check(mqtt_client& c) { +asio::awaitable test_client(mqtt_client& c) { // Note: Older versions of GCC compilers may not handle temporaries // correctly in co_await expressions. // (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98401) @@ -133,7 +133,7 @@ BOOST_AUTO_TEST_CASE(tcp_client_check) { co_spawn(ioc, [&]() -> asio::awaitable { - co_await sanity_check(c); + co_await test_client(c); timer.cancel(); }, asio::detached @@ -167,7 +167,7 @@ BOOST_AUTO_TEST_CASE(websocket_tcp_client_check) { co_spawn(ioc, [&]() -> asio::awaitable { - co_await sanity_check(c); + co_await test_client(c); timer.cancel(); }, asio::detached @@ -201,7 +201,7 @@ BOOST_AUTO_TEST_CASE(openssl_tls_client_check) { co_spawn(ioc, [&]() -> asio::awaitable { - co_await sanity_check(c); + co_await test_client(c); timer.cancel(); }, asio::detached @@ -237,7 +237,7 @@ BOOST_AUTO_TEST_CASE(websocket_tls_client_check) { co_spawn(ioc, [&]() -> asio::awaitable { - co_await sanity_check(c); + co_await test_client(c); timer.cancel(); }, asio::detached diff --git a/test/integration/mqtt_features.cpp b/test/integration/mqtt_features.cpp new file mode 100644 index 0000000..fa8205d --- /dev/null +++ b/test/integration/mqtt_features.cpp @@ -0,0 +1,181 @@ +#include + +#include +#ifdef BOOST_ASIO_HAS_CO_AWAIT + +#include +#include +#include +#include +#include + +#include + +#include + +#include + +BOOST_AUTO_TEST_SUITE(mqtt_features/*, *boost::unit_test::disabled()*/) + +using namespace async_mqtt5; +namespace asio = boost::asio; + +constexpr auto use_nothrow_awaitable = asio::as_tuple(asio::use_awaitable); + +constexpr auto test_duration = std::chrono::seconds(5); + +using stream_type = asio::ip::tcp::socket; + +constexpr auto broker = "broker.hivemq.com"; +constexpr auto connect_wait_dur = std::chrono::milliseconds(200); +constexpr auto topic = "async-mqtt5/test"; +constexpr auto share_topic = "$share/sharename/async-mqtt5/test"; +constexpr auto payload = "hello from async-mqtt5"; + +template +void run_test(TestCase&& test_case) { + using namespace asio::experimental::awaitable_operators; + + asio::io_context ioc; + co_spawn( + ioc, + [&ioc, test_case = std::forward(test_case)]() -> asio::awaitable { + asio::steady_timer test_timer(ioc, test_duration); + co_await(test_case() || test_timer.async_wait(use_nothrow_awaitable)); + }, + asio::detached + ); + ioc.run(); +} + +asio::awaitable test_manual_use_topic_alias() { + auto ex = co_await asio::this_coro::executor; + + mqtt_client client(ex); + client.brokers(broker) + .connect_property(prop::topic_alias_maximum, uint16_t(10)) + .async_run(asio::detached); + + asio::steady_timer connect_timer(ex, connect_wait_dur); + co_await connect_timer.async_wait(use_nothrow_awaitable); + + uint16_t topic_alias = 1; + publish_props pprops; + pprops[prop::topic_alias] = topic_alias; + + auto&& [ec_1, rc_1, _] = co_await client.async_publish( + topic, payload, retain_e::no, pprops, use_nothrow_awaitable + ); + BOOST_TEST_WARN(!ec_1); + BOOST_TEST_WARN(!rc_1); + + auto&& [ec_2, rc_2, __] = co_await client.async_publish( + "", payload, retain_e::no, pprops, use_nothrow_awaitable + ); + BOOST_TEST_WARN(!ec_2); + BOOST_TEST_WARN(!rc_2); +} + +BOOST_AUTO_TEST_CASE(manual_use_topic_alias) { + run_test(test_manual_use_topic_alias); +} + +asio::awaitable test_subscription_identifiers() { + auto ex = co_await asio::this_coro::executor; + + mqtt_client client(ex); + client.brokers(broker) + .async_run(asio::detached); + + auto&& [ec_1, rc_1, _] = co_await client.async_publish( + topic, payload, retain_e::yes, publish_props {}, use_nothrow_awaitable + ); + BOOST_TEST_WARN(!ec_1); + BOOST_TEST_WARN(!rc_1); + + int32_t sub_id = 123; + subscribe_props sprops; + sprops[prop::subscription_identifier] = sub_id; + + subscribe_options sub_opts = { .no_local = no_local_e::no }; + + auto&& [ec_2, rcs, __] = co_await client.async_subscribe( + { topic, sub_opts }, sprops, use_nothrow_awaitable + ); + BOOST_TEST_WARN(!ec_2); + BOOST_TEST_WARN(!rcs[0]); + + auto&& [ec_3, rec_topic, rec_payload, rec_props] = + co_await client.async_receive(use_nothrow_awaitable); + BOOST_TEST_WARN(!ec_3); + BOOST_TEST_WARN(rec_topic == topic); + BOOST_TEST_WARN(rec_payload == payload); + const auto& sub_ids = rec_props[prop::subscription_identifier]; + BOOST_TEST_WARN(!sub_ids.empty()); + if (!sub_ids.empty()) + BOOST_TEST_WARN(sub_ids[0] == sub_id); +} + +BOOST_AUTO_TEST_CASE(subscription_identifiers) { + run_test(test_subscription_identifiers); +} + +asio::awaitable test_shared_subscription() { + auto ex = co_await asio::this_coro::executor; + + mqtt_client client(ex); + client.brokers(broker) + .async_run(asio::detached); + + subscribe_options sub_opts = { .no_local = no_local_e::no }; + + auto&& [ec_1, rcs, __] = co_await client.async_subscribe( + { share_topic, sub_opts}, subscribe_props {}, use_nothrow_awaitable + ); + BOOST_TEST_WARN(!ec_1); + BOOST_TEST_WARN(!rcs[0]); + + // shared subscriptions do not send Retained Messages on first subscribe + auto&& [ec_2, rc_2, _] = co_await client.async_publish( + topic, payload, retain_e::no, publish_props{}, use_nothrow_awaitable + ); + BOOST_TEST_WARN(!ec_2); + BOOST_TEST_WARN(!rc_2); + + auto&& [ec_3, rec_topic, rec_payload, ___] = + co_await client.async_receive(use_nothrow_awaitable); + BOOST_TEST_WARN(!ec_3); + BOOST_TEST_WARN(rec_topic == topic); + BOOST_TEST_WARN(rec_payload == payload); +} + +BOOST_AUTO_TEST_CASE(shared_subscription) { + run_test(test_shared_subscription); +} + +asio::awaitable test_user_property() { + auto ex = co_await asio::this_coro::executor; + + mqtt_client client(ex); + client.brokers(broker) + .async_run(asio::detached); + + publish_props pprops; + pprops[prop::user_property].push_back({ "key_1", "value_1" }); + pprops[prop::user_property].push_back({ "key_2", "value_2" }); + pprops[prop::user_property].push_back({ "key_3", "value_3" }); + + auto&& [ec_1, rc_1, _] = co_await client.async_publish( + topic, payload, retain_e::no, pprops, use_nothrow_awaitable + ); + BOOST_TEST_WARN(!ec_1); + BOOST_TEST_WARN(!rc_1); +} + +BOOST_AUTO_TEST_CASE(user_property) { + run_test(test_user_property); +} + +BOOST_AUTO_TEST_SUITE_END() + +#endif diff --git a/test/unit/publish_send_op.cpp b/test/unit/publish_send_op.cpp index 9ff4e09..7f30532 100644 --- a/test/unit/publish_send_op.cpp +++ b/test/unit/publish_send_op.cpp @@ -61,7 +61,7 @@ void run_test( detail::publish_send_op< client_service_type, decltype(handler), qos_e::at_least_once > { svc_ptr, std::move(handler) } - .perform(topic_name, payload, retain_e::yes, pprops); + .perform(topic_name, payload, retain_e::yes, pprops); ioc.run_for(std::chrono::milliseconds(500)); BOOST_TEST(handlers_called == expected_handlers_called); diff --git a/test/unit/string_validation.cpp b/test/unit/string_validation.cpp index 5f1cf06..817407e 100644 --- a/test/unit/string_validation.cpp +++ b/test/unit/string_validation.cpp @@ -104,6 +104,25 @@ BOOST_AUTO_TEST_CASE(topic_name_validation) { BOOST_CHECK(validate_topic_name("+/tennis/#") == validation_result::has_wildcard_character); } +BOOST_AUTO_TEST_CASE(topic_alias_name_validation) { + using namespace async_mqtt5::detail; + + BOOST_CHECK(validate_topic_alias_name("") == validation_result::valid); + BOOST_CHECK(validate_topic_alias_name("topic") == validation_result::valid); + BOOST_CHECK(validate_topic_alias_name("topic/subtopic") == validation_result::valid); + + BOOST_CHECK(validate_topic_alias_name("#") == validation_result::has_wildcard_character); + BOOST_CHECK(validate_topic_alias_name("sport#") == validation_result::has_wildcard_character); + BOOST_CHECK(validate_topic_alias_name("sport/#") == validation_result::has_wildcard_character); + + BOOST_CHECK(validate_topic_alias_name("+") == validation_result::has_wildcard_character); + BOOST_CHECK(validate_topic_alias_name("+sport") == validation_result::has_wildcard_character); + BOOST_CHECK(validate_topic_alias_name("sport+") == validation_result::has_wildcard_character); + BOOST_CHECK(validate_topic_alias_name("sport/+/player1") == validation_result::has_wildcard_character); + + BOOST_CHECK(validate_topic_alias_name("+/tennis/#") == validation_result::has_wildcard_character); +} + BOOST_AUTO_TEST_CASE(shared_topic_filter_validation) { using namespace async_mqtt5::detail;