Managed Service Identity Login #5227

markcowl · 2018-01-04T21:02:01Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

twitchax

Awesome! :)

A few random comments.

twitchax · 2018-01-05T21:49:06Z

src/Common/Commands.Common.Authentication.Test/AuthenticationFactoryTests.cs

+            {
+                Id = userId,
+                Type = AzureAccount.AccountType.ManagedService
+            }; ;


Nit: extra semicolon.

twitchax · 2018-01-05T21:49:34Z

src/Common/Commands.Common.Authentication.Test/AuthenticationFactoryTests.cs

+                Type = AzureAccount.AccountType.ManagedService
+            }; ;
+            var environment = AzureEnvironment.PublicEnvironments["AzureCloud"];
+            string expectedResource = environment.ActiveDirectoryServiceEndpointResourceId;


Why not var like the others?

twitchax · 2018-01-05T21:50:11Z

src/Common/Commands.Common.Authentication.Test/AuthenticationFactoryTests.cs

+            builder.Query = string.Format("resource={0}", Uri.EscapeDataString(environment.ActiveDirectoryServiceEndpointResourceId));
+            var defaultUri = builder.Uri.ToString();
+
+            IDictionary<string, ManagedServiceTokenInfo> responses = new Dictionary<string, ManagedServiceTokenInfo>(StringComparer.OrdinalIgnoreCase)


What is our guidance on explicit vs. inferred types?

Should use type inference where it is possible. In this case, because the test class should be able to handle any usage for any type, it is required to explicitly type the input dictionary.

I mean in the declaration. It doesn't really need to be the interface type, correct?

twitchax · 2018-01-05T21:54:14Z

src/Common/Commands.Common.Authentication.Test/TestHttpOperationsFactory.cs

+
+            public IHttpOperations<T> WithHeader(string name, IEnumerable<string> value)
+            {
+                return this;


Shouldn't this throw?

No, because we simply aren't using headers in the test operations factory, but it should allow specifying headers in any code that uses it.

👍 ... maybe a comment indicating as much?

twitchax · 2018-01-05T21:56:32Z

src/Common/Commands.Common.Authentication/Authentication/ManagedServiceAccessToken.cs

+            {
+                return "ManagedService";
+            }
+        }


These can all be expression-bodied members. E.g.,

public string LoginType => "ManagedService"

These are implementing an interface

That will work, I think? 😄

An expression-bodied member is equivalent to a public get;...? Maybe?

twitchax · 2018-01-05T21:59:25Z

src/Common/Commands.Common.Authentication/Factories/AuthenticationFactory.cs

+
+                if (string.IsNullOrWhiteSpace(tenant))
+                {
+                    tenant = environment.AdTenant?? "Common";


Nit: the null-coalesce operator is typically spaced on both sides like the ternary (?:) operator.

twitchax · 2018-01-05T22:00:46Z

src/Common/Commands.Common.Authentication/HttpClientOperationsFactory.cs

+
+            public async Task DeleteAsync(string requestUri, CancellationToken token)
+            {
+                await SafeSendRequestAsync(new HttpRequestMessage(HttpMethod.Delete, requestUri), token);


This would be more succinct as:

public Task Func() { return ThingyThatReturnsATaskAsync(); }

twitchax · 2018-01-05T22:01:35Z

src/Common/Commands.Common.Authentication/HttpClientOperationsFactory.cs

+                {
+                    var exception = new HttpRequestException(string.Format("Unexpected response status code '{0}' received for request '{{{1} {2}}} Body: {{{3}}}", 
+                        response.StatusCode, request.Method, request.RequestUri, response.Content.ReadAsStringAsync().GetAwaiter().GetResult()));
+                    ServiceClientTracing.Error(invocationId, exception);


Any reason to not use string interpolation?

No, but this should probably be a string resource, anyway

twitchax · 2018-01-05T22:02:40Z

src/Common/Commands.Common/Utilities/GeneralUtilities.cs

@@ -257,7 +258,7 @@ public static string GetHttpResponseLog(string statusCode, IDictionary<string, I
            httpResponseLog.AppendLine(string.Format("============================ HTTP RESPONSE ============================{0}", Environment.NewLine));
            httpResponseLog.AppendLine(string.Format("Status Code:{0}{1}{0}", Environment.NewLine, statusCode));
            httpResponseLog.AppendLine(string.Format("Headers:{0}{1}", Environment.NewLine, MessageHeadersToString(headers)));
-            httpResponseLog.AppendLine(string.Format("Body:{0}{1}{0}", Environment.NewLine, body));
+            httpResponseLog.AppendLine(string.Format("Body:{0}{1}{0}", Environment.NewLine, TransformBody(body)));


Same here for string interpolation.

maddieclayton

A few small comments. Otherwise it all looks reasonable to me.

maddieclayton · 2018-01-10T17:02:06Z

src/Common/Commands.Common.Authentication/Authentication/ManagedServiceAccessToken.cs

+                factory = HttpClientOperationsFactory.Create();
+            }
+
+            _tokenGetter = factory.GetHttpOperations<ManagedServiceTokenInfo>().WithHeader("Metadata", new[] { "true" });


What happens here if the if statement above evaluates to false? Do you need a null check?

In this case, it uses the default implementation of HttpOperationsFactory, which just makes calls using HttpClient

maddieclayton · 2018-01-10T17:07:34Z

src/Common/Commands.Common.Authentication/Factories/AuthenticationFactory.cs

@@ -26,7 +26,7 @@ namespace Microsoft.Azure.Commands.Common.Authentication.Factories
 {
    public class AuthenticationFactory : IAuthenticationFactory
    {
-        public const string CommonAdTenant = "Common";
+        public const string CommonAdTenant = "Common", DefaultMSILoginUri = "http://localhost:50342/oauth2/token";


Is the default really a localhost? Just curious, I don't entirely understand how MSI works.

@maddieclayton Yes, the default is always localhost - it is hosted on a VM at the loopback address.

maddieclayton · 2018-01-10T17:22:06Z

src/ResourceManager/Profile/Commands.Profile/help/Add-AzureRmAccount.md

+ [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>]
+```
+
+### ManagedServiceLogin


Please add an example for the new login

maddieclayton · 2018-01-17T02:43:21Z

on demand run here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/357/

maddieclayton · 2018-01-17T17:12:28Z

@azuresdkci Test this please

maddieclayton · 2018-01-17T18:55:25Z

@azuresdkci Test this please

markcowl · 2018-01-19T20:20:37Z

on demand run here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/367/

azuresdkci added the needs-review label Jan 4, 2018

azuresdkci assigned cormacpayne Jan 4, 2018

maddieclayton added needs-revision and removed needs-review labels Jan 5, 2018

maddieclayton assigned markcowl and unassigned cormacpayne Jan 5, 2018

twitchax previously approved these changes Jan 5, 2018

View reviewed changes

markcowl dismissed twitchax’s stale review via 9386b54 January 5, 2018 22:57

markcowl force-pushed the msilogin branch from ccf3d7c to e514cd1 Compare January 5, 2018 23:02

markcowl added needs-review and removed needs-revision labels Jan 6, 2018

markcowl removed their assignment Jan 6, 2018

twitchax previously approved these changes Jan 6, 2018

View reviewed changes

maddieclayton reviewed Jan 10, 2018

View reviewed changes

maddieclayton assigned markcowl Jan 10, 2018

maddieclayton added needs-revision and removed needs-review labels Jan 10, 2018

markcowl dismissed twitchax’s stale review via 359805f January 17, 2018 02:40

maddieclayton previously approved these changes Jan 17, 2018

View reviewed changes

markcowl removed their assignment Jan 17, 2018

markcowl added Waiting for On Demand and removed needs-revision labels Jan 17, 2018

markcowl dismissed maddieclayton’s stale review via 00b2779 January 18, 2018 05:01

maddieclayton added needs-revision and removed Waiting for On Demand labels Jan 18, 2018

maddieclayton assigned markcowl Jan 18, 2018

Support ManagedServiceLogin in Add-AzureRmAccount

bfcefb5

markcowl force-pushed the msilogin branch from dc11872 to bfcefb5 Compare January 19, 2018 18:40

markcowl added needs-review and removed needs-revision labels Jan 19, 2018

markcowl assigned maddieclayton and unassigned markcowl Jan 19, 2018

maddieclayton approved these changes Jan 19, 2018

View reviewed changes

maddieclayton removed their assignment Jan 19, 2018

maddieclayton removed the needs-review label Jan 19, 2018

maddieclayton merged commit 03f08dd into Azure:preview Jan 19, 2018

Managed Service Identity Login #5227

Managed Service Identity Login #5227

Uh oh!

Conversation

markcowl commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

twitchax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maddieclayton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maddieclayton commented Jan 17, 2018

Uh oh!

maddieclayton commented Jan 17, 2018

Uh oh!

maddieclayton commented Jan 17, 2018

Uh oh!

markcowl commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

markcowl commented Jan 4, 2018 •

edited

Loading

markcowl commented Jan 19, 2018 •

edited

Loading