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

Add client secret authentication for Service Bus #359

Merged
merged 4 commits into from
Feb 18, 2021

Conversation

nikhgup
Copy link
Contributor

@nikhgup nikhgup commented Feb 11, 2021

Adding support for client secret based authentication
upgrading servicebus library
adding Microsoft.Identity.client nuet package

Tracked by: https://github.com/NuGet/Engineering/issues/3650

@dnfgituser
Copy link

dnfgituser commented Feb 11, 2021

CLA assistant check
All CLA requirements met.

@@ -16,6 +18,25 @@ public TopicClientWrapper(string connectionString, string path)
_client = TopicClient.CreateFromConnectionString(connectionString, path);
}

public TopicClientWrapper(String clientId, string clientSecret, string tenantId, string serviceBusUrl, string path)
Copy link
Contributor

@loic-sharma loic-sharma Feb 11, 2021

Choose a reason for hiding this comment

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

Please make sure to use the string keyword:

Suggested change
public TopicClientWrapper(String clientId, string clientSecret, string tenantId, string serviceBusUrl, string path)
public TopicClientWrapper(string clientId, string clientSecret, string tenantId, string serviceBusUrl, string path)

For more information why, see: https://blog.paranoidcoding.com/2019/04/08/string-vs-String-is-not-about-style.html

@@ -12,11 +12,12 @@
<PackageReference Include="Microsoft.Extensions.Logging">
<Version>2.2.0</Version>
</PackageReference>
<PackageReference Include="Microsoft.Identity.Client" Version="4.25.0" />
Copy link
Contributor

@loic-sharma loic-sharma Feb 11, 2021

Choose a reason for hiding this comment

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

Please follow the style of the other PackageReferences in this file. (To be honest, I personally prefer your style but we should stay consistent.)

Suggested change
<PackageReference Include="Microsoft.Identity.Client" Version="4.25.0" />
<PackageReference Include="Microsoft.Identity.Client">
<Version>4.25.0</Version>
</PackageReference>

@loic-sharma loic-sharma changed the title adding authentication using client secret Add client secret authentication for Service Bus Feb 11, 2021
Copy link
Contributor

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

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

Your changes look good! I also tested that we can send and receive messages successfully after your Service Bus changes

nikhgup and others added 2 commits February 15, 2021 13:48
<PackageReference Include="Newtonsoft.Json">
<Version>10.0.3</Version>
</PackageReference>
<PackageReference Include="WindowsAzure.ServiceBus">
<Version>4.1.3</Version>
<Version>6.0.3</Version>
Copy link
Member

Choose a reason for hiding this comment

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

This is a little spooky. Have we tested all of our Service Bus jobs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I tested 1 receiver scenario (Orchestrator) and two sender scenarios (Gallery and Orchestrator). Our jobs all use the same underlying framework to send/receive Service Bus messages, so I think this should cover all our jobs.

@@ -16,6 +18,25 @@ public TopicClientWrapper(string connectionString, string path)
_client = TopicClient.CreateFromConnectionString(connectionString, path);
}

public TopicClientWrapper(string clientId, string clientSecret, string tenantId, string serviceBusUrl, string path)
{
AzureActiveDirectoryTokenProvider.AuthenticationCallback authCallback = async (audience, authority, state) =>
Copy link
Member

Choose a reason for hiding this comment

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

Beautiful. Nice work 🥇 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants