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

[Bug] ConsentHandler in MVC Partial View Produces Malformed Redirect Uri #626

Closed
2 tasks done
phil4business opened this issue Sep 30, 2020 · 6 comments
Closed
2 tasks done
Assignees
Labels
bug Something isn't working fixed P1
Milestone

Comments

@phil4business
Copy link

Which version of Microsoft Identity Web are you using?
Microsoft Identity Web 1.0.0

Where is the issue?

  • Web app
    • Sign-in users and call web APIs
  • Token cache serialization
    • Distributed caches

Is this a new or an existing app?
c. This is a new app or an experiment.

Repro

StartUp.cs

services.Configure<AuthenticationOptions>(Configuration.GetSection("AzureAd"));
string[] initialScopes = Configuration.GetSection("ServerRequestApi:Scopes").Get<string[]>();

services.AddMicrosoftIdentityWebAppAuthentication(Configuration)
    .EnableTokenAcquisitionToCallDownstreamApi(initialScopes)
    .AddDistributedTokenCaches();

services.TryAddScoped<MicrosoftIdentityConsentAndConditionalAccessHandler>();

services.AddDistributedSqlServerCache(options =>
{
    options.ConnectionString = Configuration.GetConnectionString("DefaultConnection");
    options.SchemaName = "dbo";
    options.TableName = "TokenCache";
});

services.AddHttpClient<IApiHealthService, ApiHealthService>();

services.AddHttpContextAccessor();

services.AddControllersWithViews(options =>
{
    var policy = new AuthorizationPolicyBuilder()
        .RequireAuthenticatedUser()
        .Build();
    options.Filters.Add(new AuthorizeFilter(policy));
}).AddMicrosoftIdentityUI();

ApiHealthService.cs

[AuthorizeForScopes(ScopeKeySection = "ServerRequestApi:Scopes)")]
public class ApiHealthService : IApiHealthService
{
    private readonly HttpClient _apiClient;
    private readonly ILogger<ApiHealthService> _logger;
    private readonly IConfiguration _configuration;
    private readonly ITokenAcquisition _tokenAcquisition;
    private readonly MicrosoftIdentityConsentAndConditionalAccessHandler _consentHandler;

    private readonly string _remoteServiceBaseUrl;
    private readonly string[] _scopes;

    public ApiHealthService(
        HttpClient httpClient,
        ILogger<ApiHealthService> logger,
        IConfiguration configuration,
        ITokenAcquisition tokenAcquisition,
        MicrosoftIdentityConsentAndConditionalAccessHandler consentHandler)
    {
        _logger = logger ?? throw new ArgumentNullException(nameof(logger));
        _configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
        _apiClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient));
        _tokenAcquisition = tokenAcquisition ?? throw new ArgumentNullException(nameof(tokenAcquisition));
        _consentHandler = consentHandler ?? throw new ArgumentNullException(nameof(consentHandler));

        _remoteServiceBaseUrl = _configuration.GetSection("ServerRequestApi").GetValue<string>("ApiEndpoint");
        _scopes = _configuration.GetSection("ServerRequestApi:Scopes").Get<string[]>();
            
    }

    public async Task AuthorizeApiClient()
    {
        string accessToken = await _tokenAcquisition.GetAccessTokenForUserAsync(_scopes).ConfigureAwait(false);

        _apiClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);
    }

    public async Task<IEnumerable<PipelineStatus>> GetPipelineStatus(bool showInactive)
    {
        await AuthorizeApiClient();
        List<PipelineStatus> parsedPipelineStatus = null;
        try
        {
            var jsonPipelineStatus = await _apiClient.GetStringAsync($"{_remoteServiceBaseUrl}Health/GetPipelineStatus?showInactive={showInactive}");
            parsedPipelineStatus = JsonConvert.DeserializeObject<List<PipelineStatus>>(jsonPipelineStatus);
        }
        catch (Exception ex)
        {
            _consentHandler.HandleException(ex);
        }
            
        if (null == parsedPipelineStatus)
        {
            return new List<PipelineStatus>();
        }
        parsedPipelineStatus = parsedPipelineStatus.ToList();
        return parsedPipelineStatus;
    }
}

_Layout.cshtml

@inject IApiHealthService HealthSvc
@inject Microsoft.Identity.Web.MicrosoftIdentityConsentAndConditionalAccessHandler ConsentHandler
<...>
<div class="container body-content">
        @{
            try
            {
                var pipelineStatus = await HealthSvc.GetPipelineStatus(false);
                if (pipelineStatus.Any())
                {
                    foreach (var message in pipelineStatus)
                    {
                        <div class="alert alert-warning">
                            @message.Timestamp: @message.Message
                        </div>
                    }
                }
            }
            catch (Exception ex)
            {
                ConsentHandler.HandleException(ex);
            }
        }
        @RenderBody()
    </div>

Expected behavior
The user is redirected to the expected page.

Actual behavior
The redirect Uri returned by MicrosoftIdentityConsentAndConditionalAccessHandler.HandleException (non-Blazor) is malformed, containing and extra /. E.g.: Attempts to navigate to https://localhost:37289 redirects to https://localhost:37289//, navigating to https://localhost:37289/VMRequests redirects to https://localhost:37289//VMRequests/

To recreate:

  1. log in to the app normally (navigate to https://localhost:37289)
  2. delete the token from the cache
  3. force a refresh while still on the main page (https://localhost:37289)
  4. be redirected to https://localhost:37289// (you can see the request in the debug console. E.g.: https://localhost:37289/MicrosoftIdentity/Account/Challenge?redirectUri=https://localhost:37289//&scope=...)

This also works with an InMemoryTokenCache if you leave the browser open but restart the app.

Possible solution
Modify MicrosoftIdentityConsentAndConditionalAccessHandler.HandleException or MicrosoftIdentityConsentAndConditionalAccessHandler.CreateBaseUri to not add in extra /s if they were not present in the originating Uri.

@jmprieur
Copy link
Collaborator

@phil4business, what did you register in your app registration? (https://localhost:37289/ ?)
What do you have in your launchsettings.json ?

@jmprieur jmprieur added bug Something isn't working P1 labels Sep 30, 2020
@jmprieur
Copy link
Collaborator

Taking this as a P1 to be more robust.

@phil4business
Copy link
Author

phil4business commented Sep 30, 2020

app registration

"replyUrlsWithType": [
		{
			"url": "https://localhost:37289/signin-oidc",
			"type": "Web"
		},
		{
			"url": "http://localhost:37289",
			"type": "Web"
		},
		{
			"url": "https://localhost:37289",
			"type": "Web"
		}
	],

launchSettings.json

"profiles": {
    "ServerRequest": {
      "commandName": "Project",
      "launchBrowser": true,
      "environmentVariables": {
        "ASPNETCORE_ENVIRONMENT": "localhost"
      },
      "applicationUrl": "https://localhost:37289/"
    }
  }

@phil4business
Copy link
Author

Adjusting launchSettings to have "applicationUrl": "https://localhost:37289", and changing the app registration to only contain "url": "https://localhost:37289/signin-oidc" have no effect.

@robi26
Copy link
Contributor

robi26 commented Oct 5, 2020

We have the same problem with malformed redirect URLs when hosting the blazor (server-side) web app in a directory / virtual path:
The wrong URL looks like this:
https://localhost:44398//sampleMicrosoftIdentity/Account/Challenge?redirectUri=https://localhost:44398//sample&scope=user.read%20openid%20offline_access%20profile&loginHint=&domainHint=&claims=&policy=

@jennyf19 jennyf19 added the fixed label Oct 5, 2020
@jennyf19 jennyf19 added this to the 1.1.0 milestone Oct 5, 2020
@jennyf19
Copy link
Collaborator

jennyf19 commented Oct 6, 2020

Included in 1.1.0 release.

@jennyf19 jennyf19 closed this as completed Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed P1
Projects
None yet
Development

No branches or pull requests

4 participants