From 81a7ae50a4359afef2c0206dc6266e7eefb0684d Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Fri, 2 Feb 2024 11:35:05 +0530 Subject: [PATCH 01/14] feat(rumqttc/client): allow using 0 byte string and whitespace as `client_id` We now provide an additional function, `try_set_clean_session` that returns error if `client_id` is a 0 byte string and `clean_session` is not `true`. --- rumqttc/src/lib.rs | 58 +++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs index 72c2e1fb..112805d4 100644 --- a/rumqttc/src/lib.rs +++ b/rumqttc/src/lib.rs @@ -491,25 +491,14 @@ impl MqttOptions { /// # use rumqttc::MqttOptions; /// let options = MqttOptions::new("123", "localhost", 1883); /// ``` - /// NOTE: you are not allowed to use an id that starts with a whitespace or is empty. - /// for example, the following code would panic: - /// ```should_panic - /// # use rumqttc::MqttOptions; - /// let options = MqttOptions::new("", "localhost", 1883); - /// ``` pub fn new, T: Into>(id: S, host: T, port: u16) -> MqttOptions { - let id = id.into(); - if id.starts_with(' ') || id.is_empty() { - panic!("Invalid client id"); - } - MqttOptions { broker_addr: host.into(), port, transport: Transport::tcp(), keep_alive: Duration::from_secs(60), clean_session: true, - client_id: id, + client_id: id.into(), credentials: None, max_incoming_packet_size: 10 * 1024, max_outgoing_packet_size: 10 * 1024, @@ -624,11 +613,44 @@ impl MqttOptions { /// When set `false`, broker will hold the client state and performs pending /// operations on the client when reconnection with same `client_id` /// happens. Local queue state is also held to retransmit packets after reconnection. + /// + /// NOTE: It panicks if the `client_id` is a zero byte string and `clean_session` is + /// not `true`. + /// + /// ```should_panic + /// # use rumqttc::MqttOptions; + /// let mut options = MqttOptions::new("", "localhost", 1883); + /// options.set_clean_session(false); + /// ``` pub fn set_clean_session(&mut self, clean_session: bool) -> &mut Self { + if self.client_id.is_empty() { + assert!( + clean_session, + "Cannot unset clean session when client id is empty" + ); + } self.clean_session = clean_session; self } + /// Does the same thing as [`Self::set_clean_session`], however it returns an error + /// if `clean_session = false` and `client_id` is a zero byte string. + /// + /// ``` + /// # use rumqttc::{MqttOptions, OptionError}; + /// let mut options = MqttOptions::new("", "localhost", 1883); + /// let error = options.try_set_clean_session(false).unwrap_err(); + /// assert_eq!(error, OptionError::CleanSession); + /// ``` + #[cfg(feature = "url")] + pub fn try_set_clean_session(&mut self, clean_session: bool) -> Result<&mut Self, OptionError> { + if self.client_id.is_empty() && !clean_session { + return Err(OptionError::CleanSession); + } + self.clean_session = clean_session; + Ok(self) + } + /// Clean session pub fn clean_session(&self) -> bool { self.clean_session @@ -918,12 +940,6 @@ impl Debug for MqttOptions { mod test { use super::*; - #[test] - #[should_panic] - fn client_id_startswith_space() { - let _mqtt_opts = MqttOptions::new(" client_a", "127.0.0.1", 1883).set_clean_session(true); - } - #[test] #[cfg(all(feature = "use-rustls", feature = "websocket"))] fn no_scheme() { @@ -1006,10 +1022,4 @@ mod test { OptionError::Inflight ); } - - #[test] - #[should_panic] - fn no_client_id() { - let _mqtt_opts = MqttOptions::new("", "127.0.0.1", 1883).set_clean_session(true); - } } From 7237d55b26ff365e078c7886bb07b0e80796f4c6 Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Fri, 2 Feb 2024 11:51:58 +0530 Subject: [PATCH 02/14] docs(changelog): specify changes for MqttOptions --- rumqttc/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rumqttc/CHANGELOG.md b/rumqttc/CHANGELOG.md index 0705a9db..c3155d59 100644 --- a/rumqttc/CHANGELOG.md +++ b/rumqttc/CHANGELOG.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- `MqttOptions::try_set_clean_session` to check if `clean_session` is `true` when `client_id` is an empty string. - Expose `EventLoop::clean` to allow triggering shutdown and subsequent storage of pending requests - Support for all variants of TLS key formats currently supported by Rustls: `PKCS#1`, `PKCS#8`, `RFC5915`. In practice we should now support all RSA keys and ECC keys in `DER` and `SEC1` encoding. Previously only `PKCS#1` and `PKCS#8` where supported. - TLS Error variants: `NoValidClientCertInChain`, `NoValidKeyInChain`. @@ -16,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Surfaced `AsyncClient`'s `from_senders` method to the `Client` as `from_sender` ### Changed +- `MqttOptions::new` does not panic if the `client_id` is an empty string or starts with a whitespace. - Synchronous client methods take `&self` instead of `&mut self` (#646) - Removed the `Key` enum: users do not need to specify the TLS key variant in the `TlsConfiguration` anymore, this is inferred automatically. To update your code simply remove `Key::ECC()` or `Key::RSA()` from the initialization. From 104764da119566048ddc8231a44128263f5bb881 Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Fri, 2 Feb 2024 15:22:03 +0530 Subject: [PATCH 03/14] test(rumqttc/v5): fix docstring import --- rumqttc/src/v5/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rumqttc/src/v5/mod.rs b/rumqttc/src/v5/mod.rs index 4b6793b0..7e600af4 100644 --- a/rumqttc/src/v5/mod.rs +++ b/rumqttc/src/v5/mod.rs @@ -113,13 +113,13 @@ impl MqttOptions { /// - port: The port number on which broker must be listening for incoming connections /// /// ``` - /// # use rumqttc::MqttOptions; + /// # use rumqttc::v5::MqttOptions; /// let options = MqttOptions::new("123", "localhost", 1883); /// ``` /// NOTE: you are not allowed to use an id that starts with a whitespace or is empty. /// for example, the following code would panic: /// ```should_panic - /// # use rumqttc::MqttOptions; + /// # use rumqttc::v5::MqttOptions; /// let options = MqttOptions::new("", "localhost", 1883); /// ``` pub fn new, T: Into>(id: S, host: T, port: u16) -> MqttOptions { From 3e416659e9db6dfed57a8f1a8013e8c6269d1d8e Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Fri, 2 Feb 2024 15:23:49 +0530 Subject: [PATCH 04/14] test(rumqttc): avoid clippy false-positive warnings while building for testing --- rumqttc/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs index 112805d4..62f99f20 100644 --- a/rumqttc/src/lib.rs +++ b/rumqttc/src/lib.rs @@ -938,6 +938,7 @@ impl Debug for MqttOptions { #[cfg(test)] mod test { + #[allow(unused_imports)] use super::*; #[test] From 16cc313909418a85c38a259350e89eb14acb932c Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas <48434243+arunanshub@users.noreply.github.com> Date: Fri, 2 Feb 2024 15:48:38 +0530 Subject: [PATCH 05/14] docs(rumqttc): move panic info to its own section Co-authored-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com> --- rumqttc/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs index 62f99f20..0bb414de 100644 --- a/rumqttc/src/lib.rs +++ b/rumqttc/src/lib.rs @@ -614,9 +614,10 @@ impl MqttOptions { /// operations on the client when reconnection with same `client_id` /// happens. Local queue state is also held to retransmit packets after reconnection. /// - /// NOTE: It panicks if the `client_id` is a zero byte string and `clean_session` is - /// not `true`. - /// + /// # Panic + /// + /// Panics if `clean_session` is false when `client_id` is empty. + /// /// ```should_panic /// # use rumqttc::MqttOptions; /// let mut options = MqttOptions::new("", "localhost", 1883); From d785d3a44a0f54ee47f79171207681d9a04c8567 Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Fri, 2 Feb 2024 15:59:57 +0530 Subject: [PATCH 06/14] fix(rumqttc): simplify assert checks and remove unrequired feature --- rumqttc/src/lib.rs | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs index 0bb414de..29352efe 100644 --- a/rumqttc/src/lib.rs +++ b/rumqttc/src/lib.rs @@ -624,34 +624,14 @@ impl MqttOptions { /// options.set_clean_session(false); /// ``` pub fn set_clean_session(&mut self, clean_session: bool) -> &mut Self { - if self.client_id.is_empty() { - assert!( - clean_session, - "Cannot unset clean session when client id is empty" - ); - } + assert!( + self.client_id.is_empty() && clean_session, + "Cannot unset clean session when client id is empty" + ); self.clean_session = clean_session; self } - /// Does the same thing as [`Self::set_clean_session`], however it returns an error - /// if `clean_session = false` and `client_id` is a zero byte string. - /// - /// ``` - /// # use rumqttc::{MqttOptions, OptionError}; - /// let mut options = MqttOptions::new("", "localhost", 1883); - /// let error = options.try_set_clean_session(false).unwrap_err(); - /// assert_eq!(error, OptionError::CleanSession); - /// ``` - #[cfg(feature = "url")] - pub fn try_set_clean_session(&mut self, clean_session: bool) -> Result<&mut Self, OptionError> { - if self.client_id.is_empty() && !clean_session { - return Err(OptionError::CleanSession); - } - self.clean_session = clean_session; - Ok(self) - } - /// Clean session pub fn clean_session(&self) -> bool { self.clean_session From 1f242ac00947d7ca14bbac4156e8e82e256d5dc4 Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Fri, 2 Feb 2024 16:01:34 +0530 Subject: [PATCH 07/14] docs(changelog): simplify changelog description --- rumqttc/CHANGELOG.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rumqttc/CHANGELOG.md b/rumqttc/CHANGELOG.md index c3155d59..271fe2fc 100644 --- a/rumqttc/CHANGELOG.md +++ b/rumqttc/CHANGELOG.md @@ -8,7 +8,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added -- `MqttOptions::try_set_clean_session` to check if `clean_session` is `true` when `client_id` is an empty string. - Expose `EventLoop::clean` to allow triggering shutdown and subsequent storage of pending requests - Support for all variants of TLS key formats currently supported by Rustls: `PKCS#1`, `PKCS#8`, `RFC5915`. In practice we should now support all RSA keys and ECC keys in `DER` and `SEC1` encoding. Previously only `PKCS#1` and `PKCS#8` where supported. - TLS Error variants: `NoValidClientCertInChain`, `NoValidKeyInChain`. @@ -17,7 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Surfaced `AsyncClient`'s `from_senders` method to the `Client` as `from_sender` ### Changed -- `MqttOptions::new` does not panic if the `client_id` is an empty string or starts with a whitespace. +- `MqttOptions::new` now accepts empty client id. - Synchronous client methods take `&self` instead of `&mut self` (#646) - Removed the `Key` enum: users do not need to specify the TLS key variant in the `TlsConfiguration` anymore, this is inferred automatically. To update your code simply remove `Key::ECC()` or `Key::RSA()` from the initialization. From b9baeb280acf5ccf144e4a8345fada5f2cdc2c47 Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Fri, 2 Feb 2024 16:17:49 +0530 Subject: [PATCH 08/14] test(rumqttc): rework tests --- rumqttc/src/lib.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs index 29352efe..5180cb81 100644 --- a/rumqttc/src/lib.rs +++ b/rumqttc/src/lib.rs @@ -615,9 +615,9 @@ impl MqttOptions { /// happens. Local queue state is also held to retransmit packets after reconnection. /// /// # Panic - /// + /// /// Panics if `clean_session` is false when `client_id` is empty. - /// + /// /// ```should_panic /// # use rumqttc::MqttOptions; /// let mut options = MqttOptions::new("", "localhost", 1883); @@ -919,7 +919,6 @@ impl Debug for MqttOptions { #[cfg(test)] mod test { - #[allow(unused_imports)] use super::*; #[test] @@ -1004,4 +1003,14 @@ mod test { OptionError::Inflight ); } + + #[test] + fn accept_client_id_startswith_space() { + let _mqtt_opts = MqttOptions::new(" client_a", "127.0.0.1", 1883).set_clean_session(true); + } + + #[test] + fn accept_empty_client_id() { + let _mqtt_opts = MqttOptions::new("", "127.0.0.1", 1883).set_clean_session(true); + } } From 3d643959ef8f6d7a542254bbb4632a57b7792258 Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas <48434243+arunanshub@users.noreply.github.com> Date: Fri, 2 Feb 2024 11:28:21 +0000 Subject: [PATCH 09/14] fix(rumqttc): revert assert condition check --- rumqttc/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs index 5180cb81..d6f967dc 100644 --- a/rumqttc/src/lib.rs +++ b/rumqttc/src/lib.rs @@ -625,7 +625,7 @@ impl MqttOptions { /// ``` pub fn set_clean_session(&mut self, clean_session: bool) -> &mut Self { assert!( - self.client_id.is_empty() && clean_session, + !self.client_id.is_empty() || clean_session, "Cannot unset clean session when client id is empty" ); self.clean_session = clean_session; From dde4a878f9293dc3e4406a8a1dc16d031f7d4bf4 Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Sat, 3 Feb 2024 11:12:23 +0530 Subject: [PATCH 10/14] feat(rumqttc/v5): allow empty client id --- rumqttc/src/v5/mod.rs | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/rumqttc/src/v5/mod.rs b/rumqttc/src/v5/mod.rs index 7e600af4..da6c6729 100644 --- a/rumqttc/src/v5/mod.rs +++ b/rumqttc/src/v5/mod.rs @@ -116,25 +116,14 @@ impl MqttOptions { /// # use rumqttc::v5::MqttOptions; /// let options = MqttOptions::new("123", "localhost", 1883); /// ``` - /// NOTE: you are not allowed to use an id that starts with a whitespace or is empty. - /// for example, the following code would panic: - /// ```should_panic - /// # use rumqttc::v5::MqttOptions; - /// let options = MqttOptions::new("", "localhost", 1883); - /// ``` pub fn new, T: Into>(id: S, host: T, port: u16) -> MqttOptions { - let id = id.into(); - if id.starts_with(' ') || id.is_empty() { - panic!("Invalid client id"); - } - MqttOptions { broker_addr: host.into(), port, transport: Transport::tcp(), keep_alive: Duration::from_secs(60), clean_start: true, - client_id: id, + client_id: id.into(), credentials: None, request_channel_capacity: 10, max_request_batch: 0, @@ -730,12 +719,6 @@ impl Debug for MqttOptions { mod test { use super::*; - #[test] - #[should_panic] - fn client_id_startswith_space() { - let _mqtt_opts = MqttOptions::new(" client_a", "127.0.0.1", 1883).set_clean_start(true); - } - #[test] #[cfg(all(feature = "use-rustls", feature = "websocket"))] fn no_scheme() { @@ -821,8 +804,7 @@ mod test { } #[test] - #[should_panic] - fn no_client_id() { + fn allow_no_client_id() { let _mqtt_opts = MqttOptions::new("", "127.0.0.1", 1883).set_clean_start(true); } } From 71dc40548434116e0a8c6e0d390d0dc73fbe1a5b Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas <48434243+arunanshub@users.noreply.github.com> Date: Sat, 3 Feb 2024 13:13:59 +0530 Subject: [PATCH 11/14] test(rumqttc/v5): rename test Co-authored-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com> --- rumqttc/src/v5/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rumqttc/src/v5/mod.rs b/rumqttc/src/v5/mod.rs index da6c6729..04a50a4c 100644 --- a/rumqttc/src/v5/mod.rs +++ b/rumqttc/src/v5/mod.rs @@ -804,7 +804,7 @@ mod test { } #[test] - fn allow_no_client_id() { + fn allow_empty_client_id() { let _mqtt_opts = MqttOptions::new("", "127.0.0.1", 1883).set_clean_start(true); } } From 76b252dacac49d640dc1f05d3e15974177f3f32f Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Sat, 10 Feb 2024 14:55:18 +0530 Subject: [PATCH 12/14] docs(changelog): document new behavior of set_clean_session in changelog --- rumqttc/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/rumqttc/CHANGELOG.md b/rumqttc/CHANGELOG.md index 271fe2fc..b9c6f73b 100644 --- a/rumqttc/CHANGELOG.md +++ b/rumqttc/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - `MqttOptions::new` now accepts empty client id. +- `MqttOptions::set_clean_session` now panics if client ID is empty and `clean_session` flag is set to false. - Synchronous client methods take `&self` instead of `&mut self` (#646) - Removed the `Key` enum: users do not need to specify the TLS key variant in the `TlsConfiguration` anymore, this is inferred automatically. To update your code simply remove `Key::ECC()` or `Key::RSA()` from the initialization. From 301a08c90d336fc18d98cc7cf1e53ba14f95cc8f Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Sat, 10 Feb 2024 15:09:23 +0530 Subject: [PATCH 13/14] test(rumqttc): remove unrequired tests --- rumqttc/src/lib.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs index bd300e06..c4e4a7f9 100644 --- a/rumqttc/src/lib.rs +++ b/rumqttc/src/lib.rs @@ -1004,11 +1004,6 @@ mod test { ); } - #[test] - fn accept_client_id_startswith_space() { - let _mqtt_opts = MqttOptions::new(" client_a", "127.0.0.1", 1883).set_clean_session(true); - } - #[test] fn accept_empty_client_id() { let _mqtt_opts = MqttOptions::new("", "127.0.0.1", 1883).set_clean_session(true); From 566f2da818a7717aa78cea2f69d55983b42a970c Mon Sep 17 00:00:00 2001 From: Arunanshu Biswas Date: Sat, 10 Feb 2024 16:02:44 +0530 Subject: [PATCH 14/14] test(rumqttc): ensure no panic when setting `clean_session` when client ID is present --- rumqttc/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/rumqttc/src/lib.rs b/rumqttc/src/lib.rs index c4e4a7f9..ce52b452 100644 --- a/rumqttc/src/lib.rs +++ b/rumqttc/src/lib.rs @@ -1008,4 +1008,11 @@ mod test { fn accept_empty_client_id() { let _mqtt_opts = MqttOptions::new("", "127.0.0.1", 1883).set_clean_session(true); } + + #[test] + fn set_clean_session_when_client_id_present() { + let mut options = MqttOptions::new("client_id", "127.0.0.1", 1883); + options.set_clean_session(false); + options.set_clean_session(true); + } }