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.

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

Enable OAuth2 refresh token. #15424

Merged
merged 7 commits into from
Jun 28, 2021
Merged

Conversation

mshima
Copy link
Member

@mshima mshima commented Jun 21, 2021

  • Implement OAuth2{Reactive}RefreshTokensWebFilter.java and instantiate dependencies.
  • Inject OAuth token/user into required services for tests.
  • Fix angular requiring api/account twice, causing problem with user synchronization.

Fixes #15069.


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@mshima mshima marked this pull request as draft June 21, 2021 21:41
@mshima mshima changed the title Enable OAuth2 refresh token for non gateway applications. Enable OAuth2 refresh token. Jun 26, 2021
@Tcharl
Copy link
Contributor

Tcharl commented Jun 26, 2021

Right, using filter is way better than what we did.

@mshima mshima marked this pull request as ready for review June 26, 2021 13:44
@@ -1165,6 +1165,35 @@ const serverFiles = {
templates: ['META-INF/services/reactor.blockhound.integration.BlockHoundIntegration'],
},
],
springBootOauth2: [
{
condition: generator => generator.authenticationType === 'oauth2' && generator.applicationType === 'monolith',
Copy link
Contributor

Choose a reason for hiding this comment

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

constants?

@@ -999,7 +999,16 @@ public class UserService {
})
<%_ } _%>
.collect(Collectors.toSet()));
return <% if (databaseType !== 'no' && !reactive) { %>new <%= asDto('AdminUser') %>(syncUserWithIdP(attributes, user))<% } else if (!reactive) { %>user<% } %><% if (databaseType === 'no' && reactive) { %>Mono.just(user)<% } else if (reactive) { %>syncUserWithIdP(attributes, user).flatMap(u -> Mono.just(new <%= asDto('AdminUser') %>(u)))<% } %>;

<%_ if (databaseType === 'no') { _%>
Copy link
Contributor

Choose a reason for hiding this comment

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

constants?

@Tcharl
Copy link
Contributor

Tcharl commented Jun 26, 2021

Everything looks good now: do you want I merge it?

@mshima
Copy link
Member Author

mshima commented Jun 26, 2021

Everything looks good now: do you want I merge it?

If it looks ok, then yes 😄.

Copy link
Contributor

@mraible mraible left a comment

Choose a reason for hiding this comment

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

We should add offline_access as a scope and configure Keycloak to return a refresh token by default.

private <%= reactivePrefix %>OAuth2AuthorizedClientService authorizedClientService;

@Autowired
private ClientRegistration clientRegistration;
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent 4 spaces

Copy link
Contributor

@mraible mraible left a comment

Choose a reason for hiding this comment

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

😃🤩✌️

@mshima
Copy link
Member Author

mshima commented Jun 27, 2021

We should add offline_access as a scope and configure Keycloak to return a refresh token by default.

@mraible in my tests offline_access is not required to enable refresh tokens, it's already enabled by default.

At least our angular client, uses cookies, which expires and I think this makes offline_access useless.
And we don't provide a mobile app, so IMO offline_access is not a sensible default.

@mraible
Copy link
Contributor

mraible commented Jun 27, 2021 via email

@Tcharl
Copy link
Contributor

Tcharl commented Jun 28, 2021

@mraible offline access is achieved at Keycloak level, not spring-sec one right? Also, the claim is already configured on keycloak side.
Merging it, from what I've seen & tested everything is ok and much better than before

@Tcharl Tcharl merged commit c566b8a into jhipster:main Jun 28, 2021
@mraible
Copy link
Contributor

mraible commented Jun 28, 2021

The client (Spring Security in this case) needs to pass in offline_access as a scope in order to get a refresh token. If one is returned without this scope, that seems strange.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate and fix refresh tokens for OAuth with Keycloak and Okta
6 participants