Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sample with JWT #68

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Sample with JWT #68

wants to merge 10 commits into from

Conversation

patilsnr
Copy link
Contributor

@patilsnr patilsnr commented Sep 21, 2023

Adds sample with JWT authentication tied to a managed identity

@patilsnr patilsnr marked this pull request as ready for review September 25, 2023 19:37
@patilsnr
Copy link
Contributor Author

/azp run

Copy link
Contributor

@rido-min rido-min left a comment

Choose a reason for hiding this comment

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

I'd like to include JWT support in the MqttConnectionSettings, at least we should use .env files to specify the hostname

My machine takes 5+ secs to acquire the token, there might be some optimizations we can make, and also test with a ManagedIdentity

Review how to refresh the token

Let's wait until we have the builtin roles

@andyk-ms
Copy link

andyk-ms commented Oct 4, 2023

Please note additional improvements in the PBI as TODO.

@rido-min
Copy link
Contributor

rido-min commented Oct 4, 2023

I'd like to do a quick test pass with the new builtin roles before merging.

@varunpuranik
Copy link

I'd like to include JWT support in the MqttConnectionSettings, at least we should use .env files to specify the hostname

My machine takes 5+ secs to acquire the token, there might be some optimizations we can make, and also test with a ManagedIdentity

Review how to refresh the token

Let's wait until we have the builtin roles

Do we need to block on this? In the spirit of iterative improvements on these samples, i would suggest we check it in, and put another item on our backlog to improve it based on the steps above.
Any concerns @rido-min ?
Thoughts @patilsnr ?

@patilsnr
Copy link
Contributor Author

patilsnr commented Nov 1, 2023

I'd like to include JWT support in the MqttConnectionSettings, at least we should use .env files to specify the hostname
My machine takes 5+ secs to acquire the token, there might be some optimizations we can make, and also test with a ManagedIdentity
Review how to refresh the token
Let's wait until we have the builtin roles

Do we need to block on this? In the spirit of iterative improvements on these samples, i would suggest we check it in, and put another item on our backlog to improve it based on the steps above. Any concerns @rido-min ? Thoughts @patilsnr ?

I'm pro merging it in, and have been for some time. There are readme instructions on how to use this -- the main improvement to be had is using the built-in roles that have been recently rolled out but don't currently work (as opposed to creating a custom role).

@rido-min
Copy link
Contributor

this PR is blocked by dotnet/MQTTnet#1881

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.

4 participants