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

Spring Boot Security: OAuth 2.0 and OpenID Connect #443

Merged
merged 36 commits into from
Mar 4, 2022
Merged

Spring Boot Security: OAuth 2.0 and OpenID Connect #443

merged 36 commits into from
Mar 4, 2022

Conversation

pblanchardie
Copy link
Contributor

@pblanchardie pblanchardie commented Dec 30, 2021 •
edited
Loading

fixes #270

  • Add OAuth2 Client config with defaults and custom issuer URI
  • Default oauth2Login -> it works out of the box, without authority mapping
  • src/main/docker/keycloak.yml
  • patch ExceptionTransaltorIT with MockUser (same as JWT)
  • move to mvc/security
  • Add CSRF handling in test (.csrf())
  • Add SecurityConfiguration, CORS, etc.
  • User Account (very close to the standalone JWT version)
  • Add JWT claim converter for authority mapping (as in existing JHipster)
  • Add JWT Resource Server (required for web services)

@codecov
Copy link

codecov bot commented Dec 30, 2021 •
edited
Loading

Codecov Report

Merging #443 (c5b5ad7) into main (681cdca) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                main      #443    +/-   ##
============================================
  Coverage     100.00%   100.00%            
- Complexity      1279      1309    +30     
============================================
  Files            240       247     +7     
  Lines           3988      4114   +126     
  Branches          71        71            
============================================
+ Hits            3988      4114   +126
Impacted Files Coverage Δ
...r/lite/generator/project/domain/DefaultConfig.java 100.00% <ø> (ø)
...pster/lite/generator/project/domain/Constants.java 100.00% <100.00%> (ø)
...ity/common/domain/CommonSecurityDomainService.java 100.00% <100.00%> (ø)
...ucture/config/CommonSecurityBeanConfiguration.java 100.00% <100.00%> (ø)
.../application/OAuth2SecurityApplicationService.java 100.00% <100.00%> (ø)
...oot/mvc/security/oauth2/domain/OAuth2Security.java 100.00% <100.00%> (ø)
...ity/oauth2/domain/OAuth2SecurityDomainService.java 100.00% <100.00%> (ø)
...ucture/config/OAuth2SecurityBeanConfiguration.java 100.00% <100.00%> (ø)
...structure/primary/rest/OAuth2SecurityResource.java 100.00% <100.00%> (ø)
...boot/database/mysql/domain/MySQLDomainService.java 100.00% <0.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 681cdca...c5b5ad7. Read the comment docs.

@pascalgrimaud
Copy link
Member

@pblanchardie : my suggestion would be to focus on 1 use case per PR.

I mean:

  • 1st PR for OAuth2:
    • structure in JH Lite
    • default files like Keycloak, dependency, config, etc
    • 1 single API for 1 option (you can choose the one you want)
  • then, in another PR, add another option (JWT, Opaque Token or something else)

It's better for reviews and simpler to implement I think

@pblanchardie
Copy link
Contributor Author

@pascalgrimaud yes it will be simpler for both of us, I updated the description accordingly and will remove the code for these options.

@pblanchardie
Copy link
Contributor Author

pblanchardie commented Dec 31, 2021 •
edited
Loading

addClient simply adds dependencies and configuration for a provider, and you can add multiple ones (eg. facebook, github, google on a public app). Not sure you want to keep this this API. I can also remove it and only keep addDefault.

it works well as is, so should we proceed with SecurityConfiguration and Account related stuff in another PR?

@pblanchardie pblanchardie marked this pull request as ready for review December 31, 2021 22:16
@pascalgrimaud
Copy link
Member

I'd like to know how it can be tested manually ?
Do I need to generate the front part with generator-jhipster to test the redirect to Keycloak ?

@pblanchardie
Copy link
Contributor Author

Simply add an hello world restcontroller with 1 requestmapping which returns hello and it works like a charm :)

@pascalgrimaud
Copy link
Member

Maybe missing Keycloak Docker Compose ?

@pblanchardie
Copy link
Contributor Author

yes, will add Keycloak Docker Compose.

@pblanchardie pblanchardie marked this pull request as draft January 2, 2022 10:35
@pblanchardie pblanchardie force-pushed the 270-add-spring-security-oauth2 branch 2 times, most recently from 8c23cc4 to c931df6 Compare January 2, 2022 20:53
@pblanchardie pblanchardie marked this pull request as ready for review January 2, 2022 21:15
@pblanchardie
Copy link
Contributor Author

@pascalgrimaud still some informative TODOs

@pascalgrimaud
Copy link
Member

Agree with your TODO, excepting:

  • Refactor with Spring Boot Security Commons -> another PR?

As the security part is complex, I'd prefer to keep all code completely separated.
The more common part we have, the more it's annoying to maintain both options at the same time.

@pblanchardie pblanchardie force-pushed the 270-add-spring-security-oauth2 branch from c931df6 to 029e17e Compare January 3, 2022 09:15
@pblanchardie
Copy link
Contributor Author

@pascalgrimaud weird build error!

@pascalgrimaud
Copy link
Member

@pblanchardie : related to #468 -> can you update your branch plz?

pblanchardie
pblanchardie commented Jan 17, 2022
View reviewed changes
...erator/server/springboot/mvc/security/oauth2/infrastructure/rest/OAuth2SecurityResource.java Outdated

@Operation(summary = "Add Spring Security default login with OAuth2")
@ApiResponse(responseCode = "500", description = "An error occurred while adding Spring Security default login with OAuth2")
@PostMapping("/oauth2/default")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"default" => "full" means Client + Login (for frontend) + Server (for service).

Which options to provide?
eg.

  • full
  • login-only (for a monolith that does not expose its API)
  • service-only (for web services, microservice)

@pascalgrimaud
Copy link
Member

I didn't forget this PR.
I'll ping you as soon as I work again on this

@pascalgrimaud
Copy link
Member

@pblanchardie : I'll work on this branch today. I'll refresh it and start adding OAuth2 files

@pblanchardie
Copy link
Contributor Author

Hi @pascalgrimaud ! Good news thx for the update. Tell me if you need anything.

@pascalgrimaud pascalgrimaud marked this pull request as ready for review March 4, 2022 17:14
@pascalgrimaud
Copy link
Member

It's ready for review.
If you want to have a look plz @Bolo89 @pblanchardie

About account, I'll do this work in another PR

@pascalgrimaud pascalgrimaud merged commit 8d9eb4d into jhipster:main Mar 4, 2022
@pascalgrimaud pascalgrimaud mentioned this pull request Mar 4, 2022
Closed
@pascalgrimaud
Copy link
Member

thanks for the review @Bolo89

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

@MarlonLuan MarlonLuan MarlonLuan left review comments

@Bolo89 Bolo89 Bolo89 left review comments

Assignees
No one assigned
Labels
area: feature request 💡 server: spring boot theme: security
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spring Boot Security: OAuth2
4 participants
@pblanchardie @pascalgrimaud @MarlonLuan @Bolo89