Skip to content

RC1 Port: Add FailureReasons (#35425) #35529

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

RC1 Port: Add FailureReasons (#35425) #35529

wants to merge 5 commits into from

Conversation

@HaoK
Copy link
Member

@HaoK HaoK commented Aug 19, 2021

Backport of #35425

Customer Impact

There currently is no way to flow more detailed authorization failures from the IAuthorizationHandler to consumers that are responsible for displaying authorization errors. This change allows handlers to flow custom messages and state

Testing

Automated tests and a new sample demonstrating the usage

Risk

Low

@ghost ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Aug 19, 2021
@HaoK HaoK added the ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. label Aug 19, 2021
@HaoK
Copy link
Member Author

HaoK commented Aug 20, 2021

@wtgodbe so there is modification to unshipped APIs in this PR, but I don't think I've modified any public shipped apis, this was just a cherry pick of a main PR, so I'm not sure how to resolve the code check failure, any ideas?

@wtgodbe
Copy link
Member

wtgodbe commented Aug 20, 2021

@BrennanConroy fixed that check yesterday, it should pass now

@HaoK
Copy link
Member Author

HaoK commented Aug 20, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 20, 2021

Azure Pipelines successfully started running 2 pipeline(s).

Kahbazi
Kahbazi reviewed Aug 22, 2021
View reviewed changes
Copy link
Member

@Kahbazi Kahbazi left a comment

Choose a reason for hiding this comment

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

Is it possible to create the failure list only if it's needed?

Kahbazi
Kahbazi reviewed Aug 22, 2021
View reviewed changes
@2932

This comment has been minimized.

Sign in to view
@Tratcher Tratcher mentioned this pull request Aug 23, 2021
Closed
@Tratcher Tratcher self-assigned this Aug 23, 2021
Tratcher
Tratcher reviewed Aug 23, 2021
View reviewed changes
Kahbazi
Kahbazi reviewed Aug 23, 2021
View reviewed changes
Kahbazi
Kahbazi reviewed Aug 23, 2021
View reviewed changes
@Kahbazi
Copy link
Member

Kahbazi commented Aug 23, 2021

Ah! failed again! I hope this would fix it! @Tratcher #35529 (comment)

@Tratcher
Copy link
Member

Tratcher commented Aug 24, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 24, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@Tratcher
Copy link
Member

Tratcher commented Aug 24, 2021

@wtgodbe ready to merge.

@pranavkm pranavkm closed this Aug 24, 2021
@pranavkm
Copy link
Contributor

pranavkm commented Aug 24, 2021

Resolved as part of #35632.

@pranavkm pranavkm reopened this Aug 24, 2021
@pranavkm pranavkm closed this Aug 24, 2021
@pranavkm
Copy link
Contributor

pranavkm commented Aug 24, 2021

Do we need to apply the changes from this PR to main?

@Kahbazi
Copy link
Member

Kahbazi commented Aug 24, 2021

Do we need to apply the changes from this PR to main?

I'm on it!

@ghost
Copy link

ghost commented Aug 24, 2021

Hi @Kahbazi. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@Tratcher Tratcher deleted the haok/rc1 branch August 24, 2021 17:22
@pranavkm
Copy link
Contributor

pranavkm commented Aug 24, 2021

@Kahbazi here's the consolidated commit - aa04f8e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tratcher Tratcher Tratcher approved these changes

@blowdart blowdart blowdart approved these changes

@davidfowl davidfowl Awaiting requested review from davidfowl

+1 more reviewer

@Kahbazi Kahbazi Kahbazi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Tratcher Tratcher

Labels

area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@HaoK @wtgodbe @2932 @Kahbazi @Tratcher @pranavkm @blowdart