Add RedirectUri to MicrosoftIdentityOptions #144

jennyf19 · 2020-05-05T17:31:42Z

add tests

fix for #115

jmprieur

Looks awesome to me

I've proposed a more verbose XML comment
and also you might want to remove the /* test only */ comment

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs

src/Microsoft.Identity.Web/TokenAcquisition.cs

bgavrilMS · 2020-05-05T18:23:53Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

@@ -371,7 +364,7 @@ private async Task<IConfidentialClientApplication> BuildConfidentialClientApplic
                    authority = $"{ _applicationOptions.Instance}tfp/{_microsoftIdentityOptions.Domain}/{_microsoftIdentityOptions.DefaultUserFlow}";
                    app = ConfidentialClientApplicationBuilder
                        .CreateWithApplicationOptions(_applicationOptions)
-                        .WithRedirectUri(currentUri)


nit: You can combine the 2 builders to avoid repeated code. #Resolved

good idea.

In reply to: 420316439 [](ancestors = 420316439)

bgavrilMS · 2020-05-05T18:25:59Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

@@ -570,11 +563,34 @@ private IAccount GetAccountByUserFlow(IEnumerable<IAccount> accounts, string use
            foreach (var account in accounts)
            {
                string accountIdentifier = account.HomeAccountId.ObjectId.Split('.')[0];
-                if (accountIdentifier.EndsWith(userFlow.ToLower()))
+                if (accountIdentifier.EndsWith(userFlow.ToLower(CultureInfo.CurrentCulture), StringComparison.InvariantCulture))


Please use InvariantCulture in ToLower(). #Resolved

yes thanks.

In reply to: 420317657 [](ancestors = 420317657)

bgavrilMS · 2020-05-05T18:30:22Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

+                {
+                    return _microsoftIdentityOptions.RedirectUri;
+                }
+                else


else is not needed because you return at line 579 #Resolved

bgavrilMS · 2020-05-05T18:30:47Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

+                }
+                else
+                {
+                    _logger.LogInformation("MicrosoftIdentityOptions RedirectUri value must have a Uri Scheme " +


Wouldn't you want to throw an exception here? #Resolved

no because we can still figure out the redirect URI based on the httpContext. But logging the information. I guess this is more of question for JM>

In reply to: 420320742 [](ancestors = 420320742)

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs

bgavrilMS · 2020-05-05T18:32:36Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

+
+            var request = CurrentHttpContext.Request;
+            return UriHelper.BuildAbsolute(
+                request.Scheme,


I don't really understand the value here, but I think you've discussed this and you're ok with it. #Resolved

Imagine that you run locally on http;//localhost:1234
then you deploy to azure (bg.azurewebapps.net)

if you don't use redirectUri, you don't need to change your config file.
Now if you add a reverse proxy that tweaks your URI, you might want to specify it (there are cases where that's needed. See [Bug] Redirect URI is set to http instead of https when deploying to Azure App Service for Docker container (Linux) #115 #Resolved

what JM said. :)

In reply to: 420358960 [](ancestors = 420358960)

Thx for the explanation!

bgavrilMS · 2020-05-05T18:34:02Z

tests/Microsoft.Identity.Web.Test.Integration/AcquireTokenForAppIntegrationTests.cs

+        [InlineData("http://cats.b2clogin.com/signout-callback-oidc", true)]
+        public void ValidateRedirectUriFromMicrosoftIdentityOptions(
+            string redirectUri,
+            bool isTrue)


I think you can come up with a better name here :) #Resolved

expectConfiguredUri? #Resolved

expected?
#Resolved

lol...yes true...this was a quick thing for the test.

In reply to: 420322679 [](ancestors = 420322679)

bgavrilMS · 2020-05-05T18:35:16Z

tests/Microsoft.Identity.Web.Test.Integration/AcquireTokenForAppIntegrationTests.cs

+            string redirectUri,
+            bool isTrue)
+        {
+            string httpContextRedirectUri = "https://IdentityDotNetSDKAutomation/";       


How does this value make into _tokenAcquisition ? #Resolved

through the MicrosoftIdentityOptions, which are injected by DI in the TokenAcquisition constructor #Resolved

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs

jmprieur

More suggestions based on Bogdan's feedback

- add tests

jennyf19 · 2020-05-05T22:19:13Z

thanks for the suggestions. i took them all much appreciated (and thanks for responding to Bogdan's questions as well.)

In reply to: 406100408 [](ancestors = 406100408)

pmaytak

👍

pmaytak · 2020-05-06T08:11:01Z

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs

+        /// In a Web app, gets or sets the RedirectUri (URI where the token will be sent back by
+        /// Azure Active Directory or Azure Active Directory B2C)
+        /// This property is exclusive with <see cref="CallbackPath"/> which should be used preferably if you don't want
+        /// to have a different deployed configuration from your developper configuration.


developer.

pmaytak · 2020-05-06T08:13:10Z

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs

+        /// This property is exclusive with <see cref="CallbackPath"/> which should be used preferably if you don't want
+        /// to have a different deployed configuration from your developper configuration.
+        /// There are cases where RedirectUri is needed, for instance when you use a reverse proxy that transforms https
+        /// URLs (external world) to http URLs (inside the protected area). This can also be useful for Web apps running


pmaytak · 2020-05-06T08:14:20Z

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs

+        /// Azure Active Directory or Azure Active Directory B2C)
+        /// This property is exclusive with <see cref="CallbackPath"/> which should be used preferably if you don't want
+        /// to have a different deployed configuration from your developper configuration.
+        /// There are cases where RedirectUri is needed, for instance when you use a reverse proxy that transforms https


pmaytak · 2020-05-06T08:18:41Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

+                {
+                    return _microsoftIdentityOptions.RedirectUri;
+                }
+                _logger.LogInformation("MicrosoftIdentityOptions RedirectUri value must have a Uri Scheme " +


URI scheme, HTTP, HTTPS.

jmprieur · 2020-05-06T10:30:38Z

@pmaytak : please propose a PR with your suggestions.
cc: @jennyf19

jmprieur approved these changes May 5, 2020

View reviewed changes

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs Outdated Show resolved Hide resolved

src/Microsoft.Identity.Web/TokenAcquisition.cs Show resolved Hide resolved

jmprieur requested a review from bgavrilMS May 5, 2020 17:53

bgavrilMS reviewed May 5, 2020

View reviewed changes

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs Show resolved Hide resolved

bgavrilMS reviewed May 5, 2020

View reviewed changes

jmprieur reviewed May 5, 2020

View reviewed changes

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs Outdated Show resolved Hide resolved

jmprieur reviewed May 5, 2020

View reviewed changes

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs Outdated Show resolved Hide resolved

jmprieur reviewed May 5, 2020

View reviewed changes

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs Show resolved Hide resolved

jmprieur reviewed May 5, 2020

View reviewed changes

jennyf19 added 2 commits May 5, 2020 15:16

Add RedirectUri to MicrosoftIdentityOptions

53e50c1

- add tests

PR comments

e0c6d58

jennyf19 force-pushed the jennyf/redirect branch from 2104009 to e0c6d58 Compare May 5, 2020 22:16

bgavrilMS approved these changes May 5, 2020

View reviewed changes

jennyf19 merged commit 6396e13 into master May 5, 2020

jennyf19 deleted the jennyf/redirect branch May 5, 2020 22:23

pmaytak reviewed May 6, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RedirectUri to MicrosoftIdentityOptions #144

Add RedirectUri to MicrosoftIdentityOptions #144

jennyf19 commented May 5, 2020

jmprieur left a comment

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jennyf19 May 5, 2020

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jennyf19 May 5, 2020

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jennyf19 May 5, 2020

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jmprieur May 5, 2020 •

edited by jennyf19

Loading

jennyf19 May 5, 2020

bgavrilMS May 5, 2020

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jmprieur May 5, 2020 •

edited by jennyf19

Loading

jennyf19 May 5, 2020

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jmprieur May 5, 2020 •

edited by jennyf19

Loading

jmprieur left a comment

jennyf19 commented May 5, 2020

pmaytak left a comment

pmaytak May 6, 2020

pmaytak May 6, 2020

pmaytak May 6, 2020

pmaytak May 6, 2020

jmprieur commented May 6, 2020

Add RedirectUri to MicrosoftIdentityOptions #144

Add RedirectUri to MicrosoftIdentityOptions #144

Conversation

jennyf19 commented May 5, 2020

jmprieur left a comment

Choose a reason for hiding this comment

bgavrilMS May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

bgavrilMS May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

jmprieur May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

bgavrilMS May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

jmprieur May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

jmprieur May 5, 2020 • edited by jennyf19 Loading

Choose a reason for hiding this comment

jmprieur left a comment

Choose a reason for hiding this comment

jennyf19 commented May 5, 2020

pmaytak 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

jmprieur commented May 6, 2020

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jmprieur May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jmprieur May 5, 2020 •

edited by jennyf19

Loading

bgavrilMS May 5, 2020 •

edited by jennyf19

Loading

jmprieur May 5, 2020 •

edited by jennyf19

Loading