-
Notifications
You must be signed in to change notification settings - Fork 27
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
Feature/Added support for client_credentials grant type #85
Feature/Added support for client_credentials grant type #85
Conversation
7b55b79
to
379488b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your proposal and your PR (and sorry that I did not find time to look at it earlier). In general, I like your idea and would like to integrate it.
However, as this is not a fully functional Keycloak replacement but only a means to simplify tests, I try to keep the code as simple as possible and not worry too much about not being compliant with the specification. So I would like you to check if you can re-use more of the resource owner password grant flow logic and still get the solution to work for your problem.
And one more thing: please add an integration test similar to KeycloakMockIntegrationTest#mock_server_login_with_resource_owner_password_credentials_flow_works so that the full flow is validated once for a happy case.
mock/src/main/java/com/tngtech/keycloakmock/api/ServerConfig.java
Outdated
Show resolved
Hide resolved
|
||
private ServerConfig(@Nonnull final Builder builder) { | ||
this.port = builder.port; | ||
this.protocol = builder.protocol; | ||
this.defaultHostname = builder.defaultHostname; | ||
this.defaultRealm = builder.defaultRealm; | ||
this.resourcesToMapRolesTo = builder.resourcesToMapRolesTo; | ||
this.rolesForResourcesServiceAccounts = builder.rolesForResourcesServiceAccounts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if you read the roles you want to assign from the client credentials (i.e. password), similar to how it is done in all other cases.
@@ -114,4 +144,13 @@ private String toTokenResponse(String token, String sessionId) { | |||
.put(SESSION_STATE, sessionId) | |||
.encode(); | |||
} | |||
|
|||
private String toTokenResponse(String token) { | |||
// With client credentials flow, there is no refresh token and no session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that this is what the specification says, but unless there are actual client side issues, I'd like to keep the code as simple as possible, and just return the same response everywhere.
public class BasicTokenHelper { | ||
|
||
@Nonnull | ||
public Map<String, String> parseToken(@Nonnull String token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please return an actual business object (and throw some Exception if the token is not valid).
@@ -53,6 +57,25 @@ public String getToken(Session session, UrlConfiguration requestConfiguration) { | |||
return tokenGenerator.getToken(builder.build(), requestConfiguration); | |||
} | |||
|
|||
@Nullable | |||
public String getServiceAccountToken(String clientId, UrlConfiguration requestConfiguration) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the same reasoning as above, I'd argue that it is better in my opinion to keep the code simple and stupid and actually create an AdHocSession even for the client credentials grant, rather than build special logic for this case.
doReturn(NONCE).when(session).getNonce(); | ||
doReturn(USER).when(session).getUsername(); | ||
doReturn(ROLES).when(session).getRoles(); | ||
lenient().doReturn(CLIENT_ID).when(session).getClientId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general: rather than making these lenient, I'd prefer if you moved this into a more specific method and call that in every test case that needs it.
In this specific case, as I said before, I would like to re-use the session logic even for the client credentials flow.
lenient().doReturn(SESSION_ID).when(session).getSessionId(); | ||
lenient().doReturn(NONCE).when(session).getNonce(); | ||
lenient().doReturn(USER).when(session).getUsername(); | ||
lenient().doReturn(ROLES).when(session).getRoles(); | ||
doReturn(TOKEN).when(tokenGenerator).getToken(configCaptor.capture(), same(urlConfiguration)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not register captors during mock setup, but use them only during verification (see https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#15).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that was my own fault.
@ostrya Thank you for your feedback ! I will make the changes as soon as possible |
One more hint: I have just created #88 where I already added support for basic auth credentials within the token route for password grant flow. It may be best if you rebase to that branch to avoid merge conflicts. You should then be able to access username and password from the routing context using (omitting null checks) |
@ostrya Thank you! I try to fix your reviews this week! |
…oak mock to test service account Integrates the grant type client_credentials to allow to use keycloak-mock to test a service account integration. Service accounts are often used to authenticate (micro)services between them. Signed-off-by: Alexis SEGURA <alex.segura06@gmail.com>
e393f97
to
5bea409
Compare
Hi @alexisgra, I rebased your changes to the latest master because I want to avoid merge commits. Please check that all required changes are still present and pull the changes on your side. |
return; | ||
} | ||
|
||
final Map<String, String> clientCredentials = new BasicTokenHelper().parseToken(basicAuthToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please re-use the mechanism that the password flow uses (i.e. take the username and password from the routingContext.user()).
Hi @alexisgra , are you making progress on the review comments? If not, I can also address the open points myself, as I'd like to merge this and release a new version today. |
Hi @ostrya, |
It would be great if you could finish the change this weekend. In this case I will wait with the release. Take your time and push the changes when you are ready :) |
@ostrya Thanks! I do this asap in the weekend |
Signed-off-by: Alexis SEGURA <alex.segura06@gmail.com>
2a64e43
to
0d85ee2
Compare
Hi @ostrya, I pushed my changes. Let me know if its good, I hope I haven't forgotten anything :) |
Signed-off-by: Alexis SEGURA <alex.segura06@gmail.com>
Looks good now, thanks for your effort :) |
Hi!
This PR integrates the grant type client_credentials to allow to use keycloak-mock to test a service account integration. Service accounts are often used to authenticate (micro)services between them.
Typical use case :
I have several services that communicate with each other. Each service is a Keycloak client, I authenticate requests between services via service accounts. I want to test this integration with keycloak-mock.
Keycloak service accounts doc : https://www.keycloak.org/docs/latest/server_admin/index.html#_service_accounts
According to the documentation, there is no session created with the grant type client_credentials, nor refresh token returned.
I can add an example in example-backend projet if you want.
This is my first open source contribution, I tried to do my best, it's with pleasure that I will take into account your feedback!
Thank you for this super useful lib!