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

PolicyEvaluator doesn't have correct logic #56656

Open
1 task done
slowjams opened this issue Jul 6, 2024 · 3 comments
Open
1 task done

PolicyEvaluator doesn't have correct logic #56656

slowjams opened this issue Jul 6, 2024 · 3 comments
Labels
area-security Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. Status: No Recent Activity

Comments

@slowjams
Copy link

slowjams commented Jul 6, 2024

Is there an existing issue for this?

  • I have searched the existing issues

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
Copy link
Contributor

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
Copy link
Member

Thanks for contacting us.

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

@mkArtakMSFT mkArtakMSFT added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Jul 8, 2024
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-security Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. Status: No Recent Activity
Projects
None yet
Development

No branches or pull requests

3 participants