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.

Sign up for GitHub

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

Jump to bottom

Decouple Web lib from UI #1452

Merged
merged 8 commits into from
Oct 1, 2021
Merged

Decouple Web lib from UI #1452

merged 8 commits into from
Oct 1, 2021

Conversation

Ponant
Copy link
Contributor

@Ponant Ponant commented Sep 20, 2021

Fixes #1034

@ghost
Copy link

ghost commented Sep 20, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

jmprieur
jmprieur approved these changes Sep 22, 2021
View reviewed changes
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks @Ponant

Great improvement!

src/Microsoft.Identity.Web/AzureADB2COpenIDConnectEventHandlers.cs
@@ -92,7 +92,7 @@ public Task OnRemoteFailure(RemoteFailureContext context)
{
_errorAccessor.SetMessage(context.HttpContext, message);

context.Response.Redirect($"{context.Request.PathBase}/MicrosoftIdentity/Account/Error");
context.Response.Redirect($"{context.Request.PathBase}{Options.ErrorPath}");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing.

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs
@@ -133,5 +134,19 @@ internal bool HasClientCredentials
/// See https://docs.microsoft.com/azure/active-directory/managed-identities-azure-resources/how-to-manage-ua-identity-portal.
/// </summary>
public string? UserAssignedManagedIdentityClientId { get; set; }

/// <summary>
/// Sets the ResetPassword route path.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Sets the ResetPassword route path.
/// ResetPassword path within the application's base path.

(proposing to remove the 'Sets' as it's set/get. Adding precision on what the path is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ponant are you able to update the comment as per @jmprieur 's suggestion? thx.

src/Microsoft.Identity.Web/MicrosoftIdentityOptions.cs Show resolved Hide resolved
@Ponant
Copy link
Contributor Author

Ponant commented Sep 22, 2021

You are welcome @jmprieur.
Notice that the only thing I did not attempt to change is the

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

Which lives in the Web library. So I believe it will be great to find a way to have MicrosoftIdentity/Account/Challenge set by the user as part of the configuration, so any suggestion for a future PR are welcomed.

@Ponant
Copy link
Contributor Author

Ponant commented Sep 22, 2021

For BlazorChallengeUri, can we inject IOptionsMonitor<MicrosoftIdentityOptions> into the ctor of
MicrosoftIdentityConsentAndConditionalAccessHandler? How to get the scheme? That would solve the problem, I believe, and will make the lib 100% independent from the UI lib.

@Ponant @jmprieur
Apply suggestions from code review
af3a37a 
Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
@Ponant
Copy link
Contributor Author

Ponant commented Sep 24, 2021

@jennyf19 , done

@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 1, 2021

@Ponant can you remove the Microsoft.Identity.Web.xml file from the PR? I'll then merge to another branch so i can pull it locally and test.

@Ponant
Copy link
Contributor Author

Ponant commented Oct 1, 2021

@jennyf19 , is it OK now?

@jennyf19 jennyf19 changed the base branch from master to jennyf/ponantPR October 1, 2021 19:09
@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 1, 2021

sorry, i meant just not push it up, so there is no conflict. sorry about the confusion. i'll resolve it.

@jennyf19 jennyf19 merged commit 713056c into AzureAD:jennyf/ponantPR Oct 1, 2021
@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 4, 2021

@Ponant in MicrosoftIdentityConsentAndConditionalAccessHandler you can get the IOptionsMonitor and the Scheme from the ServiceProvider, which is passed into the constructor already, and then do something like this.

do you want to propose a new PR?

@Ponant
Copy link
Contributor Author

Ponant commented Oct 4, 2021 •
edited
Loading

@jennyf19 , I can look into that when I have time, just give it some time. How is this url triggered? I guess I need a blazor wasm to invoke this api, but at which stage? Obvisouly, I did not dig into that but I can look at it during weekends, I do not expect it to be difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennyf19 jennyf19 jennyf19 left review comments

@jmprieur jmprieur jmprieur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Microsoft.Identity.Web independent of the UI strings
3 participants
@Ponant @jennyf19 @jmprieur