Skip to content

Add Resource Server Sample #30

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

Add Resource Server Sample #30

wants to merge 5 commits into from

Conversation

ketola
Copy link
Contributor

@ketola ketola commented Apr 18, 2020 •
edited
Loading

Add Resource Server Sample
Fixes #4

@pivotal-issuemaster
Copy link

@ketola Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@ketola Thank you for signing the Contributor License Agreement!

@dfcoffin dfcoffin mentioned this pull request Apr 18, 2020
Closed
anzap
anzap reviewed Apr 20, 2020
View reviewed changes
Copy link

@anzap anzap left a comment

Choose a reason for hiding this comment

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

I see you added this under samples folder. Project does not seem to have gone the multimodule way as there is now a top-level src folder with a sample package. Not sure this should be cleaned up now or will be part of issue #10 .

samples/resource-server-sample/bin/main/application.yaml Outdated
@@ -0,0 +1,8 @@
# See https://docs.spring.io/spring-security/site/docs/current/reference/html5/#oauth2resourceserver
Copy link

Choose a reason for hiding this comment

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

Files in /bin should not be committed. PR #31 adds this folder to .gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That slipped in accidentally, it's now removed

samples/resource-server-sample/build.gradle Outdated
}

test {
useJUnitPlatform()
Copy link

Choose a reason for hiding this comment

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

Should we use JUnit4 or JUnit5 for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to use junit 5 api, as that was also used in the parent

@ketola ketola force-pushed the resource-server-sample branch from 7c5fc53 to 45abdcd Compare April 20, 2020 18:25
@ketola
Copy link
Contributor Author

ketola commented Apr 20, 2020

I see you added this under samples folder. Project does not seem to have gone the multimodule way as there is now a top-level src folder with a sample package. Not sure this should be cleaned up now or will be part of issue #10 .

I left the module as standalone on purpose as I saw that there was another issue for creating the gradle build.

@ketola ketola marked this pull request as ready for review April 20, 2020 18:30
@ketola ketola marked this pull request as draft April 22, 2020 08:35
jgrandja
jgrandja requested changes Apr 26, 2020
View reviewed changes
Copy link
Collaborator

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ketola. Please see my comments.

samples/resource-server-sample/build.gradle Outdated
@@ -0,0 +1,28 @@
plugins {
Copy link
Collaborator

@jgrandja jgrandja Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Please modify the build to follow the minimal sample, which uses the spring-build-conventions plugin (same as Spring Security).

The build file should be renamed to spring-authorization-server-samples-boot-oauth2resourceserver.gradle under the boot/oauth2resourceserver dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

samples/resource-server-sample/src/main/java/sample/SampleResource.java Outdated
import org.springframework.web.bind.annotation.RestController;

@RestController
public class SampleResource {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rename this to something more meaningful? Maybe ResourceController and resource()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

samples/resource-server-sample/src/main/resources/application.yaml Outdated
oauth2:
resourceserver:
jwt:
issuer-uri: https://accounts.google.com
Copy link
Collaborator

@jgrandja jgrandja Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Let's use the jwk-set-uri property configured with https://localhost:8090/oauth2/keys.

For the tests, you could either mock the jwkSet endpoint using MockWebServer or mock() the JwtDecoder @Bean. Take a look at OAuth2ResourceServerConfigurerTests for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this one, I will need some more time as I have not used the MockWebServer before, I'll do some research, check the example and try to finish this later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I checked out the sample from OAuth2ResourceServerConfigurerTests with the MockWebServer, but It started to feel like a lot's work to get the token validation mocked that way. Instead, I decided to mock the JwtDecoder. Let me know what you think.

@jgrandja jgrandja added status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Apr 26, 2020
@jgrandja jgrandja added this to the 0.0.1 milestone Apr 26, 2020
@jgrandja jgrandja self-assigned this Apr 26, 2020
@ketola
Copy link
Contributor Author

ketola commented Apr 28, 2020

@jgrandja, I noticed you labeled this as 'duplicate', where has this work been duplicated?

@jgrandja
Copy link
Collaborator

@ketola We mark PR's as duplicate if there is an associated issue, which there is in this case. However, we typically mark it when we merge. I'll remove the label until we merge to avoid confusion.

@jgrandja jgrandja removed the status: duplicate A duplicate of another issue label Apr 28, 2020
@jgrandja jgrandja added the status: duplicate A duplicate of another issue label May 1, 2020
@jgrandja
Copy link
Collaborator

jgrandja commented May 1, 2020

Thanks for the updates @ketola. This is now in master.

@jgrandja jgrandja closed this May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anzap anzap anzap left review comments

@jgrandja jgrandja jgrandja requested changes

Assignees

@jgrandja jgrandja

Labels
status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
0.0.1
Development

Successfully merging this pull request may close these issues.

Add Resource Server Sample
4 participants
@ketola @pivotal-issuemaster @jgrandja @anzap