[Authentication] updates to support Arlington #3734

willportnoy · 2020-04-14T04:17:43Z

No description provided.

libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs

...aries/Microsoft.Bot.Connector/Authentication/Government/MicrosoftGovernmentAppCredentials.cs

swagatmishra2007 · 2020-04-14T06:08:51Z

libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs

@@ -217,6 +221,9 @@ private bool IsAdalServiceUnavailable(Exception ex)
            return adalServiceException.ErrorCode == MsalTemporarilyUnavailable || adalServiceException.StatusCode == 429;
        }

+        private bool IsAdalServiceInvalidRequest(Exception ex)
+            => ex is AdalServiceException adal && adal.StatusCode == 400;


do we only need to care about 400's or 4xx in general or perhaps not 2xx? is there any documentation around it that you refereed for this?

I verified the 400 for Arlington bot requesting against Fairfax resource app.

I have not yet verified the status code for Fairfax bot requesting against the Arlington resource app.

I didn't see documentation, unfortunately.

swagatmishra2007 · 2020-04-14T06:10:42Z

...aries/Microsoft.Bot.Connector/Authentication/Government/MicrosoftGovernmentAppCredentials.cs

+
+                    if (OAuthScope == GovernmentAuthenticationConstants.ToChannelFromBotOAuthScope)
+                    {
+                        var optionOld = Make(GovernmentAuthenticationConstants.ToChannelFromBotLoginUrl, GovernmentAuthenticationConstants.ToChannelFromBotOAuthScope);


instead of calling them old and new, consider maintaining a list instead...who knows if there'll be a new "new" later

swagatmishra2007 · 2020-04-14T06:20:39Z

...aries/Microsoft.Bot.Connector/Authentication/Government/MicrosoftGovernmentAppCredentials.cs

+                    }
+                }
+
+                if (info != null)


slim down to info?.throw

swagatmishra2007 · 2020-04-14T06:23:54Z

...aries/Microsoft.Bot.Connector/Authentication/Government/MicrosoftGovernmentAppCredentials.cs

+                    var inner = this.inners[index];
+                    try
+                    {
+                        return await inner.GetTokenAsync(forceRefresh).ConfigureAwait(false);


suggest remembering which inner works and using that for further calls. it's just extra work to always loop through

Given this needs to be thread safe, I was opting for simplicity.

swagatmishra2007 · 2020-04-14T06:29:03Z

...aries/Microsoft.Bot.Connector/Authentication/Government/MicrosoftGovernmentAppCredentials.cs

+                        var optionOld = Make(GovernmentAuthenticationConstants.ToChannelFromBotLoginUrl, GovernmentAuthenticationConstants.ToChannelFromBotOAuthScope);
+                        var optionNew = Make("https://login.microsoftonline.us/botframework.onmicrosoft.us", "https://api.botframework.onmicrosoft.us");
+
+                        return new Authenticators(optionOld, optionNew);


keep optionNew as your first argument. almost all new bots going forward will be in that new tenant and hence trying the new one first will make sure that the for loop doesnt execute more than one iteration for most new bots

willportnoy · 2020-04-14T19:33:12Z

libraries/Microsoft.Bot.Connector/Authentication/GovernmentAuthenticationConstants.cs

+        /// <summary>
+        /// TO GOVERNMENT CHANNEL FROM BOT: Login URL V2.
+        /// </summary>
+        public const string ToChannelFromBotLoginUrlV2 = "https://login.microsoftonline.us/botframework.onmicrosoft.us";


consider https://docs.microsoft.com/en-us/azure/active-directory/fundamentals/add-custom-domain

cleemullins

There are no unit tests. here, despite quite a bit of code churn. How do we test this?

cleemullins · 2020-04-14T19:58:36Z

libraries/Microsoft.Bot.Connector/Authentication/Authenticators.cs

+{
+    internal sealed class Authenticators : IAuthenticator
+    {
+        private readonly IAuthenticator[] inners;


naming convention. "_inners"

libraries/Microsoft.Bot.Connector/Authentication/Authenticators.cs

cleemullins · 2020-04-14T19:59:39Z

libraries/Microsoft.Bot.Connector/Authentication/Authenticators.cs

+
+        public Authenticators(params IAuthenticator[] inners)
+        {
+            this.inners = inners ?? throw new ArgumentNullException(nameof(inners));


coding standard here is just "_inners = inners ?? [...]"

cleemullins · 2020-04-14T20:02:14Z

libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs

        }

+        private bool IsAdalServiceInvalidRequest(Exception ex)
+            => ex is AdalServiceException adal && adal.StatusCode == (int)HttpStatusCode.BadRequest;


I'm not at all clear on what this line of code is doing. Can you add a nice multi-line comment.

If nothing else, BadRequest and InvalidRequest are two very different things.

Also, the complexity of this if statement is staggering, as I've got to know the operator presicdence for:

is

&&

==

(int)

... and understand the more modern C# declare inline syntax.

Please turn this into a simpler method.

cleemullins · 2020-04-14T20:03:25Z

libraries/Microsoft.Bot.Connector/Authentication/Authenticators.cs

@@ -0,0 +1,44 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.


I don't understand why adding Arlington as a new auth point requires a new class called "Authenticators". CAn you elaborte with a nice long
comment on the class?

cleemullins · 2020-04-14T20:04:23Z

libraries/Microsoft.Bot.Connector/Authentication/Authenticators.cs

+        {
+            ExceptionDispatchInfo info = null;
+
+            for (int index = 0; index < this.inners.Length; ++index)


I can't follow this. I'm lost across "info, index, this.inners, and inners".

Please consider easier variable names.

cleemullins · 2020-04-14T20:05:18Z

libraries/Microsoft.Bot.Connector/Authentication/GovernmentAuthenticationConstants.cs

+        /// <summary>
+        /// TO GOVERNMENT CHANNEL FROM BOT: OAuth scope to request V2.
+        /// </summary>
+        public const string ToChannelFromBotOAuthScopeV2 = "https://api.botframework.onmicrosoft.us";


This line is really the only line I expected to see added.

It's the hardcoded tenant above (cab8a31a-1906-4287-a0d8-4eef66b95f6e - USME) that needed to be updated to a different tenant. And then

the scope (api.botframework.onmicrosoft.us) is trivially name-spaced within the resource app identifier (api.botframework.onmicrosoft.us), and

the app identifier (api.botframework.onmicrosoft.us) is name-spaced within the tenant domain (botframework.onmicrosoft.us, which we may modify to a custom domain botframework.azure.us to match our DNS name).

cleemullins · 2020-04-14T20:06:43Z

libraries/Microsoft.Bot.Connector/Authentication/MicrosoftGovernmentAppCredentials.cs

@@ -62,5 +65,34 @@ public override string OAuthEndpoint
        {
            get { return GovernmentAuthenticationConstants.ToChannelFromBotLoginUrl; }
        }
+
+        protected override Lazy<IAuthenticator> BuildIAuthenticator()


I cannot follow this method, and all of it's nested lambda functions.

Please simplify.

cleemullins · 2020-04-14T20:07:41Z

libraries/Microsoft.Bot.Connector/Authentication/MicrosoftGovernmentAppCredentials.cs

+                        return Make(OAuthEndpoint, OAuthScope);
+                    }
+                },
+                LazyThreadSafetyMode.ExecutionAndPublication);


What's this, and are we sure it's right? I have no idea what it does.

I'm following the existing style:

botbuilder-dotnet/libraries/Microsoft.Bot.Connector/Authentication/MicrosoftAppCredentials.cs

Line 144 in 55af969

LazyThreadSafetyMode.ExecutionAndPublication);

cleemullins · 2020-04-14T20:09:03Z

libraries/Microsoft.Bot.Connector/Authentication/MicrosoftGovernmentAppCredentials.cs

+                    }
+                    else
+                    {
+                        return Make(OAuthEndpoint, OAuthScope);


I don't get how this branch of the "IF" statement returns an "Make()" function result, while the other returns a new set of authenticators.

... yet the parent function needs the lazy.

There's got to be a more clear way to express this to simple minds like mine.

@carlosscastro can you give some insight into the need for the two methods (with Lazy AdalAuthenticator and Lazy IAuthenticator)? I tried to copy the existing code pattern from MicrosoftAppCredentials.

tomlm · 2020-04-15T16:39:29Z

libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs

@@ -225,7 +251,7 @@ private RetryParams ComputeAdalRetry(Exception ex)

                // When the Service Token Server (STS) is too busy because of “too many requests”,
                // it returns an HTTP error 429 with a hint about when you can try again (Retry-After response field) as a delay in seconds
-                if (adalServiceException.ErrorCode == MsalTemporarilyUnavailable || adalServiceException.StatusCode == 429)
+                if (adalServiceException.ErrorCode == MsalTemporarilyUnavailable || adalServiceException.StatusCode == HttpTooManyRequests)


HttpTooManyRequests [](start = 119, length = 19)

(int)HttpStatusCode.TooManyRequests.

tomlm · 2020-04-15T16:39:39Z

libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs

@@ -214,7 +220,27 @@ private bool IsAdalServiceUnavailable(Exception ex)

            // When the Service Token Server (STS) is too busy because of “too many requests”,
            // it returns an HTTP error 429
-            return adalServiceException.ErrorCode == MsalTemporarilyUnavailable || adalServiceException.StatusCode == 429;
+            return adalServiceException.ErrorCode == MsalTemporarilyUnavailable || adalServiceException.StatusCode == HttpTooManyRequests;


HttpTooManyRequests [](start = 118, length = 19)

(int)HttpStatusCode.TooManyRequests

libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs

tomlm

cleemullins · 2020-04-29T00:45:06Z

libraries/Microsoft.Bot.Connector/Authentication/GovernmentAuthenticationConstants.cs

@@ -16,7 +16,7 @@ public static class GovernmentAuthenticationConstants
        /// <summary>
        /// TO GOVERNMENT CHANNEL FROM BOT: Login URL.
        /// </summary>
-        public const string ToChannelFromBotLoginUrl = "https://login.microsoftonline.us/cab8a31a-1906-4287-a0d8-4eef66b95f6e";
+        public const string ToChannelFromBotLoginUrl = "https://login.microsoftonline.us/MicrosoftServices.onmicrosoft.us";


does casing matter here? URI Segments are normally not case sensitive, but I'm worried the casing here might cause comparison issues.

[Authentication] updates to support Arlington

3183abd

willportnoy added the R9 Release 9 - May 15th, 2020 label Apr 14, 2020

willportnoy requested review from swagatmishra2007 and carlosscastro April 14, 2020 04:17

carlosscastro reviewed Apr 14, 2020

View reviewed changes

libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs Outdated Show resolved Hide resolved

carlosscastro reviewed Apr 14, 2020

View reviewed changes

...aries/Microsoft.Bot.Connector/Authentication/Government/MicrosoftGovernmentAppCredentials.cs Outdated Show resolved Hide resolved

swagatmishra2007 reviewed Apr 14, 2020

View reviewed changes

willportnoy added 5 commits April 14, 2020 09:57

use HttpStatusCode constants

1a8565a

new government authentication constants

a7bd087

prefer the newer government authenticator

1fa1cdb

undo file move changes to minimize diffs

a69818f

move Authenticators class to separate file

d4791a8

willportnoy commented Apr 14, 2020

View reviewed changes

cleemullins suggested changes Apr 14, 2020

View reviewed changes

willportnoy added 2 commits April 14, 2020 13:57

code review feedback

44ec5f8

simplify IAuthenticator factory

d6b2ce1

tomlm reviewed Apr 15, 2020

View reviewed changes

libraries/Microsoft.Bot.Connector/Authentication/AdalAuthenticator.cs Show resolved Hide resolved

tomlm approved these changes Apr 15, 2020

View reviewed changes

tomlm added the Draft PR label Apr 15, 2020

willportnoy added 5 commits April 15, 2020 14:50

HttpStatusCode.TooManyRequests available in netstandard2.1

cb195c2

add unit tests for authenticators

5cf3915

Merge branch 'master' into wportnoy/Arlington

fef14b4

Merge remote-tracking branch 'origin/master' into wportnoy/Arlington

bef8101

update to Microsoft Services tenant

8d614c8

carlosscastro approved these changes Apr 22, 2020

View reviewed changes

update unit test

2b634aa

willportnoy marked this pull request as ready for review April 29, 2020 00:33

willportnoy requested review from johnataylor and mrivera-ms as code owners April 29, 2020 00:33

willportnoy removed the Draft PR label Apr 29, 2020

cleemullins approved these changes Apr 29, 2020

View reviewed changes

tomlm merged commit e5d193c into master Apr 29, 2020

tomlm deleted the wportnoy/Arlington branch April 29, 2020 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Authentication] updates to support Arlington #3734

[Authentication] updates to support Arlington #3734

willportnoy commented Apr 14, 2020

swagatmishra2007 Apr 14, 2020

willportnoy Apr 14, 2020

swagatmishra2007 Apr 14, 2020

swagatmishra2007 Apr 14, 2020

swagatmishra2007 Apr 14, 2020

willportnoy Apr 14, 2020

swagatmishra2007 Apr 14, 2020

willportnoy Apr 14, 2020

cleemullins left a comment

cleemullins Apr 14, 2020

cleemullins Apr 14, 2020

cleemullins Apr 14, 2020

cleemullins Apr 14, 2020

cleemullins Apr 14, 2020

cleemullins Apr 14, 2020

willportnoy Apr 14, 2020

cleemullins Apr 14, 2020

cleemullins Apr 14, 2020

willportnoy Apr 14, 2020

cleemullins Apr 14, 2020

willportnoy Apr 14, 2020 •

edited

tomlm Apr 15, 2020 •

edited

tomlm Apr 15, 2020 •

edited

tomlm left a comment

cleemullins Apr 29, 2020

		@@ -0,0 +1,44 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

[Authentication] updates to support Arlington #3734

[Authentication] updates to support Arlington #3734

Conversation

willportnoy commented Apr 14, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cleemullins 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willportnoy Apr 14, 2020 • edited

Choose a reason for hiding this comment

tomlm Apr 15, 2020 • edited

Choose a reason for hiding this comment

tomlm Apr 15, 2020 • edited

Choose a reason for hiding this comment

tomlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willportnoy Apr 14, 2020 •

edited

tomlm Apr 15, 2020 •

edited

tomlm Apr 15, 2020 •

edited