Skip to content

OAuth2ResourceServerSpec should allow its ServerAuthenticationConverter to be configurable #6190

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 1 commit into from
Nov 29, 2018

Conversation

edeandrea
Copy link
Contributor

@edeandrea edeandrea commented Nov 29, 2018

Fixes gh-6186

@edeandrea edeandrea changed the title OAuth2ResourceServerSpec should allow its ServerAuthenticationConvert… OAuth2ResourceServerSpec should allow its ServerAuthenticationConverter to be configurable Nov 29, 2018
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

@edeandrea This looks awesome! I left just one ask in your unit test.

Could you please also format your commit message so that the title is a bit shorter?

// @formatter:off
http
.authorizeExchange()
.anyExchange().access(authorizationManager())
Copy link
Contributor

@jzheaux jzheaux Nov 29, 2018

Choose a reason for hiding this comment

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

Is there a reason you need this extra instrumentation, as opposed to something like

Suggested change
.anyExchange().access(authorizationManager())
.anyExchange().hasAuthority("SCOPE_message:read")

If we can, it's nice to be able to reduce the number of moving parts in a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct I can switch this test. I will issue a new commit that changes this.

@edeandrea
Copy link
Contributor Author

@jzheaux I pushed a new commit and rebased so the commit message is better. Sorry about that!

@edeandrea
Copy link
Contributor Author

Also - any chance of getting this in pre 5.2?

@jgrandja
Copy link
Contributor

@edeandrea This can only get into 5.2 since bearerTokenConverter(...) is new

@edeandrea
Copy link
Contributor Author

@jgrandja bearerTokenConverter(...) is what I added in the PR. There aren't any new classes or anything added in this PR - just a change to the configuration DSL.

@jgrandja
Copy link
Contributor

@edeandrea The new method in the DSL is a new symbol so it cannot be added in a patch release - only a minor or major release. Makes sense? See versioning

@edeandrea
Copy link
Contributor Author

Got it - thanks for the explanation @jgrandja.

@jzheaux jzheaux merged commit be423de into spring-projects:master Nov 29, 2018
@jzheaux
Copy link
Contributor

jzheaux commented Nov 29, 2018

Thanks for the PR, @edeandrea! This is now merged into master.

@edeandrea edeandrea deleted the gh-6186 branch November 29, 2018 21:43
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.

3 participants