Skip to content

Ensure schemes aren't used on components authorization. Fixes #10570 #12239

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

Merged
merged 2 commits into from
Jul 17, 2019
Merged

Ensure schemes aren't used on components authorization. Fixes #10570 #12239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f693617
Ensure schemes aren't used on components authorization. Fixes #10570
SteveSandersonMS Jul 16, 2019
d3eece5
Rename method
SteveSandersonMS Jul 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Components/Components/src/Auth/AuthorizeViewCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,27 @@ protected override async Task OnParametersSetAsync()
private async Task<bool> IsAuthorizedAsync(ClaimsPrincipal user)
{
var authorizeData = GetAuthorizeData();
EnsureNoAuthenticationSchemeSpecified(authorizeData);

var policy = await AuthorizationPolicy.CombineAsync(
AuthorizationPolicyProvider, authorizeData);
var result = await AuthorizationService.AuthorizeAsync(user, Resource, policy);
return result.Succeeded;
}

private static void EnsureNoAuthenticationSchemeSpecified(IAuthorizeData[] authorizeData)
{
// It's not meaningful to specify a nonempty scheme, since by the time Components
// authorization runs, we already have a specific ClaimsPrincipal (we're stateful).
// To avoid any confusion, ensure the developer isn't trying to specify a scheme.
for (var i = 0; i < authorizeData.Length; i++)
{
var entry = authorizeData[i];
if (!string.IsNullOrEmpty(entry.AuthenticationSchemes))
{
throw new NotSupportedException($"The authorization data specifies an authentication scheme with value '{entry.AuthenticationSchemes}'. Authentication schemes cannot be specified for components.");
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

But you could have multiple schemes and have the scheme act as a filter (that's how I recall it works today). Don't we want to be consistent with the rest of the platform?

Copy link
Member

Choose a reason for hiding this comment

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

The schemes are passed in to the authentication system when used with server-side auth: https://github.com/aspnet/AspNetCore/blob/master/src/Security/Authorization/Policy/src/PolicyEvaluator.cs#L35

Given that server-side auth with have already happened at this point, there isn't really anything we can do here that would be consistent.

yes, you could filter but that not going to work with a passive scheme like bearer.

Since we can't be consistent, and we don't have a clear use case, it seems better to just block it.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's not supported at the IAuthenticationStateProvider layer, as that only has an API to get the "current authentication state" which is a ClaimsPrincipal. It doesn't have a way to get different principals for different schemes.

In the future we could choose to make a more advanced version of IAuthenticationStateProvider (e.g., ISchemeAuthenticationStateProvider) that has APIs for getting different authentication states for different schemes. If we do that then we could relax this limitation. But since we're not planning that for 3.0, this "fail hard" rule is to help people not get confused about the feature set.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can do this in the future without breaking changes.

}
}
24 changes: 24 additions & 0 deletions src/Components/Components/test/Auth/AuthorizeViewTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,24 @@ public void IncludesResourceInAuthorizeCall()
});
}

[Fact]
public void RejectsNonemptyScheme()
{
// Arrange
var authorizationService = new TestAuthorizationService();
var renderer = CreateTestRenderer(authorizationService);
var rootComponent = new TestAuthStateProviderComponent(builder =>
{
builder.OpenComponent<AuthorizeViewCoreWithScheme>(0);
builder.CloseComponent();
});
renderer.AssignRootComponentId(rootComponent);

// Act/Assert
var ex = Assert.Throws<NotSupportedException>(rootComponent.TriggerRender);
Assert.Equal("The authorization data specifies an authentication scheme with value 'test scheme'. Authentication schemes cannot be specified for components.", ex.Message);
}

private static TestAuthStateProviderComponent WrapInAuthorizeView(
RenderFragment<AuthenticationState> childContent = null,
RenderFragment<AuthenticationState> authorizedContent = null,
Expand Down Expand Up @@ -557,5 +575,11 @@ public class TestPolicyRequirement : IAuthorizationRequirement
{
public string PolicyName { get; set; }
}

public class AuthorizeViewCoreWithScheme : AuthorizeViewCore
{
protected override IAuthorizeData[] GetAuthorizeData()
=> new[] { new AuthorizeAttribute { AuthenticationSchemes = "test scheme" } };
}
}
}