Skip to content

Make AuthorizeFilter work in endpoint routing #9099

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

Merged
merged 7 commits into from
Apr 8, 2019

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Apr 5, 2019

Fixes #8387

@pranavkm
Copy link
Contributor Author

pranavkm commented Apr 5, 2019

I'll work on adding more tests \ but this would be a start

@pranavkm pranavkm requested review from HaoK, rynowak and JamesNK April 5, 2019 01:29
@pranavkm pranavkm force-pushed the prkrishn/authorize-identity branch from e0b7a3c to 49eeaba Compare April 5, 2019 01:42
Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

This looks a lot like what I expected!

There are a few really ugly things here, but that's not really something we can avoid.


return LoginAB(page);
}

Copy link
Member

Choose a reason for hiding this comment

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

Historically the place we see the most bugs is when there's a action that is trying to mix cookies and bearer, if its not too hard, do you think you can add one or two tests for that scenario here?

Basically something like:

[Authorize] // requires any authenticated user (aka the application cookie typically)
public class Controller {
   [Authorize(AuthenticationScheme = "Bearer")]
   public void Api();

   public void Cookie();

   [AllowAnonymous]
   public void PartyOn();

And ideally there would be a few tests that calls these actions at least twice in a few different orders (i.e. anon, bearer, cookie vs cookie, anon, bearer, etc) to make sure that we catch something like #7687

Copy link
Contributor Author

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

:shipit:

@pranavkm pranavkm force-pushed the prkrishn/authorize-identity branch from 19340a9 to 64e99f9 Compare April 7, 2019 20:18
@pranavkm pranavkm force-pushed the prkrishn/authorize-identity branch from 64e99f9 to a3a7f92 Compare April 7, 2019 20:22
rynowak and others added 2 commits April 7, 2019 13:36
(cherry picked from commit 9d32070)
@pranavkm pranavkm merged commit b93bc43 into master Apr 8, 2019
@pranavkm pranavkm deleted the prkrishn/authorize-identity branch April 8, 2019 13:03
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.

4 participants