Skip to content

No-op Authorization middleware for Razor Pages #7028

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 2 commits into from
Jan 25, 2019
Merged

Conversation

pranavkm
Copy link
Contributor

Workaround for #7011

@pranavkm pranavkm requested a review from Tratcher as a code owner January 25, 2019 18:35
@pranavkm pranavkm requested review from JamesNK and rynowak January 25, 2019 18:35
await _next(context);
return;
}

Copy link
Member

@rynowak rynowak Jan 25, 2019

Choose a reason for hiding this comment

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

I guess what I expected to see was for AuthorizeFilter (MVC) to ignore AuthorizationMiddlewareInvokedKey - so that we always run authorization twice. This should be a little bit slower, but it's is close to what we shipped in preview1 that didn't have this bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Summing up offline conversation: Authenticate \ Authorize has side effects and I wanted to avoid running it twice if we could help it. This does work correctly for the particular scenario that is broken. I've added an additional test with controllers to make sure that works correctly too so we've covered all the bases.

@natemcmaster
Copy link
Contributor

If this is ready to go, let's merge ASAP so we can rebuild the rest of the stack for P2.

@JamesNK
Copy link
Member

JamesNK commented Jan 25, 2019

I'm confused about the contents of this PR. I can see test changes but no product code fix.

@natemcmaster

This comment has been minimized.

@pranavkm
Copy link
Contributor Author

🆙 📅 sorry about the confusion

@mkArtakMSFT
Copy link
Contributor

Merging this as flaky tests are blocking this for third round now.

@mkArtakMSFT mkArtakMSFT merged commit 1aa50fa into master Jan 25, 2019
@pranavkm pranavkm deleted the prkrishn/7011 branch April 22, 2019 16: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.

5 participants