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

Make Microsoft.Identity.Web independent of the UI strings #1034

Closed
Ponant opened this issue Mar 1, 2021 · 9 comments · Fixed by #1452
Closed

Make Microsoft.Identity.Web independent of the UI strings #1034

Ponant opened this issue Mar 1, 2021 · 9 comments · Fixed by #1452
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@Ponant
Copy link
Contributor

Ponant commented Mar 1, 2021

Hello,
Currently, it seems that the Microsoft.Identity.Web is somewhat dependent on a hard coded path for the error, such as here (I did not check elsewhere yet)
https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web/AzureADB2COpenIDConnectEventHandlers.cs

This makes it ok when used in conjunction with the UI package, but not immediate to customize otherwise. I was hitting this issue on Azure B2C by triggering errors on purpose, and I was not using the Microsoft.Identity.Web.UI package.

I think it will be great to move this kind of string out of the class to a configuration that can be set in AddMicrosoftIdentityWebApp.

Does this makes sense?

@jmprieur jmprieur added the enhancement New feature or request label Mar 2, 2021
@jmprieur
Copy link
Collaborator

jmprieur commented Mar 2, 2021

Would you think that this could go in MicrosoftIdentityOptions, @Ponant ?

@Ponant
Copy link
Contributor Author

Ponant commented Mar 2, 2021

I am not familiar enough with this package but I just checked out MicrosoftIdentityOptions and it seems this is where developers would enter the azure b2c configuration, which for instance will end up in appsettings.json. Why not. I was imagining something à la .Net Core, such as when we configure the cookie.Name, e.g. cookieOptions.LogoutPath = new PathString("/logout"), you would have then

options.ErrorPath = new PathString("/error");

with a default value for "/MicrosoftIdentity/Account/Error"
and the same for other strings such as the resetpassword path.

In any case I think it will be clean to do so, currently there is a sort of implicit assumption that you will use the UI package along. I saw a few issues around this where it was suggested to trigger the OnRemoteFailure events or to override the RCL pages by creating the page structure in the project. Overriding the rcl might create issues in the future if the overriding behavior is changed in razor pages (I don't expect it to be the case, but who knows). So, the general strategy is to let the user say where he would like errors and resets path to be and the library would just do that, otherwise would default to the paths in the UI lib.

I am happy to test it if need be.

@Ponant
Copy link
Contributor Author

Ponant commented Sep 16, 2021

Hi @jmprieur , I come back to you to see if you are interested in a PR on this issue. Looking at the code and your suggestion, it seems that putting the strings in MicrosoftIdentityOptions is good option.
If you are OK I could PR this.
it will be tested in web app Razor Pages I have on Azure B2C.
Let me know

@Ponant
Copy link
Contributor Author

Ponant commented Sep 17, 2021

There is also this dependency on the UI route which could/should be fixed

internal const string BlazorChallengeUri = "MicrosoftIdentity/Account/Challenge?redirectUri=";

@Ponant
Copy link
Contributor Author

Ponant commented Sep 18, 2021

@jmprieur , in addition to the above, what about

var callbackUrl = Url.Page(_optionsMonitor.Get(scheme).SignedOutRedirectUri,
pageHandler: null, values: null, protocol: Request.Scheme);

and set SignedOutRedirectUri="/Account/SignedOut" in the ctor of MicrosoftIdentityOptions,

instead of the hardcoded string for the SignOut in the Account controller

var callbackUrl = Url.Page("/Account/SignedOut", pageHandler: null, values: null, protocol: Request.Scheme);

@Ponant
Copy link
Contributor Author

Ponant commented Sep 20, 2021

@jmprieur , I am done with the changes. As a reminder, the idea is to make the Web library independent from the UI one.
Changes are fairly straightforward:

In MicrosoftIdentityOptions (3 changes)

   /// <summary>
        /// Initializes a new instance of the <see cref="MicrosoftIdentityOptions"/> class.
        /// </summary>
        public MicrosoftIdentityOptions()
        {
            SignedOutRedirectUri = "/Account/SignedOut";
        }


        /// <summary>
        /// Sets the ResetPassword route path.
        /// Defaults to /MicrosoftIdentity/Account/ResetPassword,
        /// which is the value used by Microsoft.Identity.Web.UI.
        /// </summary>
        public PathString ResetPasswordPath { get; set; } = new PathString("/MicrosoftIdentity/Account/ResetPassword");

        /// <summary>
        /// Sets the Error route path.
        /// Defaults to the value /MicrosoftIdentity/Account/Error,
        /// which is the value used by Microsoft.Identity.Web.UI.
        /// </summary>
        public PathString ErrorPath { get; set; } = new PathString("/MicrosoftIdentity/Account/Error");

In UpdateMergedOptionsFromMicrosoftIdentityOptions method

        internal static void UpdateMergedOptionsFromMicrosoftIdentityOptions(
             MicrosoftIdentityOptions microsoftIdentityOptions, MergedOptions mergedOptions)
        {
            mergedOptions.ResetPasswordPath = microsoftIdentityOptions.ResetPasswordPath;
            mergedOptions.ErrorPath = microsoftIdentityOptions.ErrorPath;

In AzureADB2COpenIDConnectEventHandlers

                context.Response.Redirect($"{context.Request.PathBase}{Options.ResetPasswordPath}/{SchemeName}");
                context.Response.Redirect($"{context.Request.PathBase}{Options.ErrorPath}");

In SignOut action

                var callbackUrl = Url.Page(_optionsMonitor.Get(scheme)
                      .SignedOutRedirectUri, pageHandler: null, values: null, protocol: Request.Scheme);

I can push and PR that.

@Ponant
Copy link
Contributor Author

Ponant commented Oct 4, 2021

Done

@Ponant Ponant closed this as completed Oct 4, 2021
@jennyf19 jennyf19 added this to the 1.18.0 milestone Oct 6, 2021
@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 6, 2021

Included in 1.18.0 release thanks for the great contribution @Ponant

@Ponant
Copy link
Contributor Author

Ponant commented Oct 6, 2021

you are most welcome @jennyf19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants