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.

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

Resolve Log authorization detail failure message by override ToString in each Requirement #10822

Closed
wants to merge 9 commits into from
Closed

Resolve Log authorization detail failure message by override ToString in each Requirement #10822

wants to merge 9 commits into from

Conversation

wu-yafeng
Copy link
Contributor

Log authorization detail failure message by override ToString in each Requirement

  • Add AuthorizationFailure parameter to internal LoggingExtensions.UserAuthorizationFailed
  • Override each Requirement ToString method

Failed Authorization Policy Details in Logs #7789

@wu-yafeng wu-yafeng changed the title Master Resolve Log authorization detail failure message by override ToString in each Requirement Jun 4, 2019
@Eilon Eilon added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 4, 2019
@analogrelay analogrelay added this to the 3.0.0-preview7 milestone Jun 4, 2019
@analogrelay analogrelay requested a review from HaoK June 4, 2019 16:33
@HaoK
Copy link
Member

HaoK commented Jun 4, 2019

@wu-yafeng this looks like a reasonable start, do you think you could add some tests to demonstrate what the log output would look like?

@wu-yafeng
Copy link
Contributor Author

@wu-yafeng this looks like a reasonable start, do you think you could add some tests to demonstrate what the log output would look like?

sure

亚锋 吴 added 2 commits June 5, 2019 11:39
Add tests to demonstrate the log output.
ff8e3bd 
- Fix requirement ToString format
- Add requirement ToString tests
- Add Authorization log output tests
Fix CI Failure
0e00d2b
@analogrelay
Copy link
Contributor

@HaoK if we can get this reviewed an in for preview8, awesome. Otherwise it will go to 3.1

HaoK
HaoK requested changes Jul 11, 2019
View reviewed changes
Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

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

Looks good except for some minor text changes

@wu-yafeng
Copy link
Contributor Author

Poor english 😢 ,I'll fix it

Fix requirement text
38f1aca 
- Fix `AssertRequirement.ToString` text and test case.
- Fix `DenyAnonymous.ToString` misspell and test case.
- Fix `ClaimsAuthorizationRequirement` text and tes case
- Update DefaultAuthorizationServiceTests
- Update LoggingExtensions log message when authorize fail
@pranavkm pranavkm modified the milestones: 3.0.0-preview8, 5.0.0-alpha1 Aug 13, 2019
@analogrelay analogrelay removed this from the blazor-wasm milestone Oct 3, 2019
@analogrelay
Copy link
Contributor

@HaoK let's try to close this out now that we're in to the free-for-all 5.0 timeframe ;)

@analogrelay analogrelay added this to the 5.0.0-preview1 milestone Oct 17, 2019
@HaoK HaoK mentioned this pull request Oct 23, 2019
Merged
HaoK added a commit that referenced this pull request Oct 25, 2019
@HaoK
Merge AuthZ ToString PR (#15350)
c376e83 
Rebased verison of #10822
@HaoK
Copy link
Member

HaoK commented Oct 25, 2019

Thanks @wu-yafeng I had to clean up some minor things and rebased, and merged these changes in PR #15350

@HaoK HaoK closed this Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HaoK HaoK HaoK requested changes

@analogrelay analogrelay Awaiting requested review from analogrelay

@Tratcher Tratcher Awaiting requested review from Tratcher

Assignees

@HaoK HaoK

Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Milestone
5.0.0-preview1
Development

Successfully merging this pull request may close these issues.
5 participants
@wu-yafeng @HaoK @analogrelay @pranavkm @Eilon