dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
34.88k stars 9.85k forks source link

PolicyEvaluator doesn't have correct logic #56656

Open slowjams opened 1 week ago

slowjams commented 1 week ago

Is there an existing issue for this?

Describe the bug

https://source.dot.net/#Microsoft.AspNetCore.Authorization.Policy/PolicyEvaluator.cs,99

I believe the logic is wrong here, so if authorization pass while authentication doesn't pass, the response is 200 not 401.

e.g an endpoint accessed by an unauthenticated user below

[HttpGet]
[Authorize(Policy = "AtLeast18")]
public IEnumerable<string> GetBeer()
       => new[] { "Tsingtao", "Carlton"};
builder.Services.AddAuthorization(options =>
{
    options.AddPolicy("AtLeast18", policyBuider => policyBuider.Requirements.Add(new MinimumAgeRequirement(18)));
});
public class MinimumAgeHandler : AuthorizationHandler<MinimumAgeRequirement>
{
    protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, MinimumAgeRequirement requirement)
    {
        context.Succeed(requirement);  // a junior developer make a mistake in checking user's age and make it equivalent as everyone is over 18

        return Task.CompletedTask;
    }
}

despite of the mistake made by the junior developer, and I would still expect the response to be 401 rather than 200

Expected Behavior

he response to be 401 rather than 200

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

.NET 8

Anything else?

No response

joegoldman2 commented 1 week ago

The requirement DenyAnonymousAuthorizationRequirement is missing from your policy. You can call RequireAuthenticatedUser to add it:

builder.Services.AddAuthorization(options =>
{
    options.AddPolicy("AtLeast18",
        policyBuider =>
        {
            policyBuider.RequireAuthenticatedUser();
            policyBuider.Requirements.Add(new MinimumAgeRequirement(18));
        });
});
mkArtakMSFT commented 1 week ago

Thanks for contacting us.

Did you get chance to validate @joegoldman2 's suggestion above? That seems reasonable.

dotnet-policy-service[bot] commented 2 days ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

See our Issue Management Policies for more information.