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

Properties should reference scope not scopes #6510

Closed
wants to merge 1 commit into from
Closed

Properties should reference scope not scopes #6510

wants to merge 1 commit into from

Conversation

sdoxsee
Copy link
Contributor

@sdoxsee sdoxsee commented Feb 7, 2019

OAuth2ClientProperties.Registration has a member scope but not scopes. This is the easy fix for the sample but i think it should be scopes

@jgrandja
Copy link
Contributor

jgrandja commented Feb 7, 2019

Thanks for the PR @sdoxsee. Would you be able to update the same in oauth2webclient-webflux?

@jgrandja jgrandja self-assigned this Feb 7, 2019
@sdoxsee
Copy link
Contributor Author

sdoxsee commented Feb 7, 2019

done. Thanks!

@rwinch
Copy link
Member

rwinch commented Feb 7, 2019

Thanks @sdoxsee It looks like there are few additional places. Can you update the PR to get those as well? Here are all the places I found with a quick search:

$ rg "scopes: "
samples/boot/oauth2webclient-webflux/README.adoc
43:            scopes: read:user,public_repo

samples/boot/oauth2webclient/README.adoc
43:            scopes: read:user,public_repo

samples/boot/oauth2webclient/src/main/resources/application.yml
19:            scopes: read:user,public_repo

samples/boot/oauth2webclient-webflux/src/main/resources/application.yml
19:            scopes: read:user,public_repo

docs/manual/src/docs/asciidoc/_includes/reactive/oauth2/access-token.adoc
17:            scopes: read:user,public_repo

@sdoxsee
Copy link
Contributor Author

sdoxsee commented Feb 7, 2019

Thanks @rwinch. I've got those now as well. Would it make more sense to change the OAuth2ClientProperties.Registration.scope member to scopes rather than change other places to use scope?

@jgrandja
Copy link
Contributor

jgrandja commented Feb 7, 2019

@sdoxsee In regards to...

Would it make more sense to change the OAuth2ClientProperties.Registration.scope member to scopes

The name scope is intentional to align with the spec. You than may be wondering why the plural name Set<String> ClientRegistration.getScopes()? It's because it returns a Set. This is the naming convention we adopted a ways back.

The changes look great in this PR and it's ready to get merged. Can you please squash to 1 commit and ensure the commit message follows this format and includes Fixes gh-* at the end.
Thanks!

@sdoxsee
Client OAuth2 properties to use scope not scopes
2507049 
OAuth2ClientProperties.Registration (which captures .properties and
.yml for OAuth2 Client) has a member `scope` but not `scopes`. Samples
and documentation were using `scopes` and have now been updated to use
`scope`.

Fixes spring-projectsgh-6510
@jgrandja jgrandja added in: docs An issue in Documentation or samples Samples type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Feb 8, 2019
@jgrandja jgrandja added this to the 5.2.0.M2 milestone Feb 8, 2019
@jgrandja jgrandja closed this in a7a9271 Feb 8, 2019
@jgrandja
Copy link
Contributor

jgrandja commented Feb 8, 2019

Thanks for the PR @sdoxsee! This is now in master.

@jgrandja jgrandja mentioned this pull request Feb 8, 2019
Closed
jgrandja pushed a commit that referenced this pull request Feb 8, 2019
@sdoxsee @jgrandja
Client OAuth2 properties to use scope not scopes
7633fbf 
OAuth2ClientProperties.Registration (which captures .properties and
.yml for OAuth2 Client) has a member `scope` but not `scopes`. Samples
and documentation were using `scopes` and have now been updated to use
`scope`.

Fixes gh-6510
@sdoxsee
Copy link
Contributor Author

sdoxsee commented Feb 8, 2019

Awesome. Thanks @jgrandja

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

@jgrandja jgrandja

Labels
in: docs An issue in Documentation or samples in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.M2
Development

Successfully merging this pull request may close these issues.
3 participants
@sdoxsee @jgrandja @rwinch