Add credential support to ASP.NET Core integration layer #6805

pakrym · 2019-07-02T22:52:06Z

Adding support for multiple features:

Loading client secret and certificate credentials from IConfiguration
Having a global default credential
Having a global client options configuration
Extension methods for Azure.KeyVault.Secrets

In extensions methods, the credential parameter should be made optional. The default value means that a global default credential would be used.

New extension type name pattern SomethingClientBuilderExtensions, method name AddSomethingClient

pakrym · 2019-07-03T21:27:01Z

eng/Packages.Data.props

@@ -38,14 +39,14 @@
    <PackageReference Update="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="2.6.1" />
    <PackageReference Update="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.6.2" />
    <PackageReference Update="Microsoft.CodeAnalysis" Version="2.3.0" />
-    <PackageReference Update="Microsoft.Extensions.Configuration.Abstractions" Version="2.2.0" />
-    <PackageReference Update="Microsoft.Extensions.Configuration.Binder" Version="2.2.0" />
+    <PackageReference Update="Microsoft.Extensions.Configuration.Abstractions" Version="2.1.0" />


I've downgraded versions of dependencies to the current LTS version. Azure.Core.Extensions is the only project using them

pakrym · 2019-07-03T21:40:52Z

sdk/identity/Azure.Identity/src/ClientCertificateCredential.cs

-        private string _clientId;
-        private X509Certificate2 _clientCertificate;
-        private AadIdentityClient _client;
+        public string TenantId { get; }


cc @schaabs

I've exposed the properties to enable testing.

pakrym · 2019-07-03T23:08:02Z

Hm, pipeline tries to pack my sample project for some reason.

jsquire

Looks good to me, but I'm not overly familiar with some of the context; I'd recommend not using me as the only approval.

jsquire · 2019-07-16T17:28:42Z

sdk/core/Azure.Core.Extensions/samples/Azure.Core.Extensions.Samples.csproj

+    <WarnOnPackingNonPackableProject>false</WarnOnPackingNonPackableProject>
+  </PropertyGroup>
+  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.App" IsImplicitlyDefined="true" />


Orthogonal to these changes: I'm not familiar with IsImplicitlyDefined and a quick search didn't turn up anything. I'd be grateful for a quick pointer to a reference or explanation of what it is for. :)

Sooooo, you are not supposed to have package version for Microsoft.AspNetCore.App package, SDK would pick the highest patch for you based on the target framework.

Unfortunately, this conflicts with our central version management strategy that produces a warning when something outside of Packages.Data.props set a package version. IsImplicitlyDefined suppresses this warning.

sdk/core/Azure.Core.Extensions/samples/CustomPolicy.cs

sdk/core/Azure.Core.Extensions/samples/appsettings.json

jsquire · 2019-07-16T17:33:12Z

sdk/core/Azure.Core.Extensions/src/Azure.Core.Extensions.csproj

@@ -29,6 +29,7 @@
  </ItemGroup>

  <ItemGroup>
+    <ProjectReference Include="..\..\..\identity\Azure.Identity\src\Azure.Identity.csproj" />


Should this use a package reference rather than a project reference?

I'll keep it a package reference until preview2 so we can iterate them in parallel.

sdk/core/Azure.Core.Extensions/src/Internal/ConfigurationClientFactory.cs

tg-msft

I haven't used ASP.NET in five years and don't recognize any of this... so feel free to disregard most of my comments/questions if you're following standard patterns I'm oblivious to.

The only real issue I see is specializing everything to TokenCredential when we have other types for different services. I'm okay with it if you and @schaabs decide that's the plan, but think it'll feel weird if users are trying to setup a default StorageSharedKeyCredential to use with all their BlobClients.

tg-msft · 2019-07-16T17:59:02Z

eng/Directory.Build.Data.props

@@ -39,9 +39,10 @@

  <PropertyGroup>
    <IsTestProject Condition="$(MSBuildProjectName.EndsWith('.Tests'))">true</IsTestProject>
+    <IsSamplesProject Condition="$(MSBuildProjectName.EndsWith('.Samples'))">true</IsSamplesProject>


Nit - any reason not to follow our existing .Samples.Tests approach for testable samples?

I really want to give folks outside our team an easy way to play with new APIs in ASP.NET Core app

We can also consider checking for /samples/ in the path.

...nfiguration/Azure.ApplicationModel.Configuration/src/ConfigurationClientBuilderExtensions.cs

tg-msft · 2019-07-16T18:06:02Z

sdk/core/Azure.Core.Extensions/samples/Startup.cs

+
+            services.AddAzureClients(builder => {
+
+                builder.AddSecretClient(Configuration.GetSection("KeyVault"))


When I see it written like this, I wonder if we should be calling it AddKeyVaultSecretClient.

We've been back and forth on this couple of times with @KrzysztofCwalina, I'm open to any naming schema but would prefer to follow up on this later.

sdk/core/Azure.Core.Extensions/samples/Startup.cs

sdk/core/Azure.Core.Extensions/src/Internal/AzureClientsGlobalOptions.cs

tg-msft · 2019-07-16T18:48:15Z

sdk/core/Azure.Core.Extensions/src/Internal/ConfigurationClientFactory.cs

+                return credential;
+            }
+
+            // TODO: More logging


Should we throw an exception if they don't provide enough detail? Like say they forgot the clientId, would it be helpful to tell them that here?

We should, something similar to constructor not found exception.

sdk/core/Azure.Core.Extensions/src/Internal/ConfigureClientOptions.cs

sdk/core/Azure.Core/src/Extensions/IAzureClientsBuilder.cs

fixed next link in backup (#6805)

pakrym added 8 commits July 2, 2019 15:40

Add credential support to ASP.NET Core integration layer

fb7334a

Ctrl+s

9326eee

Some feedback

7935f60

Fluent

7bb6bc0

Use core

54420da

Public

753a6a7

Properties

7764830

CTRL+S

cd3d5b0

pakrym commented Jul 3, 2019

View reviewed changes

*nix cert fixes

6f511b7

pakrym commented Jul 3, 2019

View reviewed changes

Adjust names

4129fd1

pakrym marked this pull request as ready for review July 3, 2019 23:00

pakrym added 4 commits July 5, 2019 09:54

WarnOnPackingNonPackableProject

58d2d78

IsImplicitlyDefined

97dde25

Merge remote-tracking branch 'azure/master' into pakrym/creds

aca1122

merge

e2fc779

pakrym requested review from schaabs, jsquire, maririos and tg-msft and removed request for schaabs July 15, 2019 21:40

Merge remote-tracking branch 'azure/master' into pakrym/creds

02cd1c8

jsquire approved these changes Jul 16, 2019

View reviewed changes

pakrym added 2 commits July 16, 2019 11:04

Merge remote-tracking branch 'azure/master' into pakrym/creds

d46c1f9

Feedback and merge fix

2e340f3

pakrym requested review from chidozieononiwu and danieljurek as code owners July 16, 2019 18:30

pakrym requested review from mitchdenny and weshaggard as code owners July 16, 2019 18:30

tg-msft approved these changes Jul 16, 2019

View reviewed changes

pakrym added 4 commits July 17, 2019 09:32

PR feedback

8c127eb

Merge remote-tracking branch 'azure/master' into pakrym/creds

211b0c0

More renames

1baa8bb

Fix the build

e85c0e8

pakrym merged commit 336bc87 into Azure:master Jul 17, 2019

pakrym deleted the pakrym/creds branch July 17, 2019 18:44

openapi-bot-tih-dev bot referenced this pull request in AzureSDKAutomation/azure-sdk-for-net Aug 7, 2019

Generated from ad759850354ddb7b4ffea0a49b70243fd9ec5a42

02f4cb4

fixed next link in backup (#6805)

openapi-bot-tih-dev bot referenced this pull request in AzureSDKAutomation/azure-sdk-for-net Aug 7, 2019

Generated from ad759850354ddb7b4ffea0a49b70243fd9ec5a42

25ff3c8

fixed next link in backup (#6805)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add credential support to ASP.NET Core integration layer #6805

Add credential support to ASP.NET Core integration layer #6805

pakrym commented Jul 2, 2019 •

edited

Loading

pakrym Jul 3, 2019

pakrym Jul 3, 2019

pakrym commented Jul 3, 2019

jsquire left a comment

jsquire Jul 16, 2019

pakrym Jul 16, 2019

jsquire Jul 16, 2019

pakrym Jul 17, 2019 •

edited

Loading

tg-msft left a comment

tg-msft Jul 16, 2019

pakrym Jul 16, 2019

weshaggard Jul 16, 2019

tg-msft Jul 16, 2019

pakrym Jul 17, 2019 •

edited

Loading

tg-msft Jul 16, 2019

pakrym Jul 17, 2019


		services.AddAzureClients(builder => {

		builder.AddSecretClient(Configuration.GetSection("KeyVault"))

Add credential support to ASP.NET Core integration layer #6805

Add credential support to ASP.NET Core integration layer #6805

Conversation

pakrym commented Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Jul 3, 2019

jsquire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

tg-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Jul 2, 2019 •

edited

Loading

pakrym Jul 17, 2019 •

edited

Loading

pakrym Jul 17, 2019 •

edited

Loading