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

Update OwinWebApp DevApp to configure the redirect_uri parameter. #2798

Closed
wants to merge 2 commits into from

Conversation

victorm-hernandez
Copy link

Update OwinWebApp DevApp to configure the redirect_uri parameter.

  • [x ] You've read the Contributor Guide and Code of Conduct.
  • [x ] You've included unit or integration tests for your change, where applicable.
  • [x ] You've included inline docs for your change, where applicable.
  • [x ] There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

The redirect_uri parameter is appended to the authorize request while authenticating a user. It is used by the Identity Provider to redirect back to the relying party after the authentication is completed.

The current app implementation seems to suggest that the redirect_uri parameter is configured in the Startup.Auth.cs file already, as part of the ConfidentialClientApplicationOptions. However, this is not the case, this parameter is configured in OpenIdConnectAuthenticationOptions instead.

NORTHAMERICA\victorhern and others added 2 commits April 24, 2024 14:49
Current implementation seems to suggest that this parameter is configured
in the Startup.Auth.cs file, as part of the ConfidentialClientApplicationOptions
however this is not realted with the redirect_uri parameter used by the
Identity Provider to redirect back to the relying party after the authentication
is completed. This parameter is actually configured in OpenIdConnectAuthenticationOptions
parameter.
@victorm-hernandez victorm-hernandez marked this pull request as ready for review April 24, 2024 23:20
@victorm-hernandez victorm-hernandez requested a review from a team as a code owner April 24, 2024 23:20
app.AddMicrosoftIdentityWebApp(factory);
app.AddMicrosoftIdentityWebApp(factory, updateOptions: (options) =>
{
options.RedirectUri = "https://localhost:44386/";
Copy link
Author

@victorm-hernandez victorm-hernandez Apr 24, 2024

Choose a reason for hiding this comment

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

RedirectUri

This is the option which affects the redirect_uri parameter. #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed, it looked to be an app registration issue. Added https://localhost:44386/ as a redirectURI for this app in the lab. Not sure how it is even working, as this redirectURI was not registered to the app.

Copy link
Author

Choose a reason for hiding this comment

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

I found this issue with a dSTS application, this is unrelated with the configuration of the application. This shows how to affect a specific parameter called redirect_uri.

jennyf19
jennyf19 approved these changes Apr 25, 2024
using Microsoft.Identity.Client;
using Microsoft.Identity.Abstractions;
using Microsoft.Identity.Web;
Copy link
Collaborator

@jennyf19 jennyf19 Apr 25, 2024

Choose a reason for hiding this comment

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

Is this needed? #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

In reading the PR description, something doesn't seem right. Will need to look from laptop later

Copy link
Author

Choose a reason for hiding this comment

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

This is just sorting using statements alphabetically

@jennyf19 jennyf19 self-requested a review April 25, 2024 01:17
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

I don't think this PR is needed. it was an app registration issue.

@victorm-hernandez
Copy link
Author

The intent of this PR is to show the reader how to populate the redirect_uri parameter since we have multiple mentions of options with a similar name but none of them affect this specific parameter. This is independent of the application registration.


In reply to: 2026864027

@jennyf19
Copy link
Collaborator

The intent of this PR is to show the reader how to populate the redirect_uri parameter since we have multiple mentions of options with a similar name but none of them affect this specific parameter. This is independent of the application registration.

I understand that it's independent of the app registration, but the app didn't work with this change. The app registration also needed to be updated. The recommended way is for customers to use the configuration to set the corresponding settings, which is set up here.

@victorm-hernandez
Copy link
Author

Agreed, that is orthogonal to this change. This change shows how to define the desired return URL for this instance. As you know the configuration on the identity provider may hold multiple return URLs for a given application. The redirect_uri parameter defines which of those should be used for the current request.

The current sample makes the reader think they are able to configure this by using some of the properties called RedirectUri from our options class, but that is not true, the only way to do this is by using the object I introduced on the sample. Hence the reason for the PR>


In reply to: 2083123625

@victorm-hernandez
Copy link
Author

Abandoning PR since sample owner doesnt see value on the change.

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

Successfully merging this pull request may close these issues.

None yet

4 participants