Skip to content

Adds external auth capabilities.#761

Merged
swanandx merged 12 commits intobytebeamio:mainfrom
nathansizemore:dynamic-auth
Dec 5, 2023
Merged

Adds external auth capabilities.#761
swanandx merged 12 commits intobytebeamio:mainfrom
nathansizemore:dynamic-auth

Conversation

@nathansizemore
Copy link
Contributor

@nathansizemore nathansizemore commented Dec 2, 2023

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

Notes

  • Breaks config API by renaming auth to static_auth.

This solves two problems we currently have with rumqttd:

  • The ability to get client id on new connections, when a new connection happens.
  • Allow hooks into our external auth service.

Copy link
Contributor

@swanandx swanandx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the PR. This should fix #584 !

added some comments, please have a look and feel free to comment :)

@swanandx
Copy link
Contributor

swanandx commented Dec 5, 2023

PR LGTM :)!

just last question, instead of making external_auth public, we can impl helper fn to set external auth ( like we do for reload filters & consolesettings ).:

@@ -132,11 +132,21 @@ pub struct ConnectionSettings {
     pub max_inflight_count: usize,
     pub auth: Option<HashMap<String, String>>,
     #[serde(skip)]
-    pub external_auth: Option<AuthHandler>,
+    external_auth: Option<AuthHandler>,
     #[serde(default)]
     pub dynamic_filters: bool,
 }

+impl ConnectionSettings {
+    /// Sepcify a function to be used for authentication.
+    ///
+    /// This will take precedence and auth specified in config will
+    /// be ignored.
+    pub fn set_external_auth(&mut self, auth: AuthHandler) {
+        self.external_auth.replace(auth);
+    }
+}
+

though, as you are the one of the users who will be using this, lmk which way seems better, Thank you!

Otherwise, we can have both the derive & make it pub 😅

@nathansizemore
Copy link
Contributor Author

just last question, instead of making external_auth public, we can impl helper fn to set external auth ( like we do for reload filters & consolesettings ).:

If the method doesn't do anything else besides setting the prop, doesn't seem worth the extra code to me. But if it's some style preference or whatever, feel free to change it up.

@swanandx
Copy link
Contributor

swanandx commented Dec 5, 2023

If the method doesn't do anything else besides setting the prop, doesn't seem worth the extra code to me

you are right, we can tackle it when we add better way to construct configs without TOML file. 😄

@swanandx swanandx merged commit aba0456 into bytebeamio:main Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants