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

Adds feature to configure keycloak server with Scopes #128

Merged
merged 4 commits into from
Jan 19, 2023

Conversation

mdanish98
Copy link
Contributor

This library is indeed really useful.
I was using this library to mock keycloak authorization token behavior. But one use case it was missing like when there is an implementation for getting authorization token with normal OAuth2 request using custom getAccessToken method. The scenario is like I have a project which has a method to provide authorization and it just takes clientId, clientSecret and tokenUrl and it has its own implementation to request JWT token. Here I have to check whether it is able to successfully get the access token and then able to provide access to the secured registry, and the secured registry checks for the required scopes needed to perform certain action (Read/Write etc).
So for this there is a need to configure the realm with clientScopes while configuring the server and by not using TokenConfig.
I have added this feature to initialize and start KeyCloakMock server with clientScopes. I have also added the required test cases to cover this implementation.

Please let me know incase you need further information.

Thanks
Signed-off-by: Mohammad Ghazanfar Ali Danish [email protected]

Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
@mdanish98
Copy link
Contributor Author

Dear @ostrya

Did you had some time to check this PR ?
Actually I have another PR on my other project which is dependent on this. If this change is merged that would be easy to update the dependent PR.

Please let me know incase of any further information.

Thanks and Regards

Copy link
Collaborator

@ostrya ostrya left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. I like your change, but I have a suggestion so we can avoid manipulating static variables.

example-backend/bin/.gitignore Outdated Show resolved Hide resolved
Description:
- Removes withClientScopes method in ServerConfig
- Moves the openid scope from the TokenConfig to ServerConfig
- Now the defaultScope (openid) is added when Keycloakmock Server is
instantiated
- Removes the default value scope check from TokenConfigTest
(default_values_are_used) as now default value of scope is configured
via ServerConfig
- Modifies the TokenGeneratorTest (config_is_correctly_applied) in such
a way that it only checks the scope configured and not the defaultScope
- Adds additional test case in KeycloakMockTest to check if server is
started without using withDefaultScopes then by default it should
contain 'openid' scope

Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
Description:
- Removes .gitignore from bin folders

Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
@mdanish98 mdanish98 requested a review from ostrya October 17, 2022 06:46
@mdanish98
Copy link
Contributor Author

Dear @ostrya

Did you had a chance to check these changes?

@mdanish98
Copy link
Contributor Author

mdanish98 commented Jan 17, 2023

Dear @ostrya @mrmarbury @Achimh3011 @aaschmid ,

Actually we are dependent on this change and our project which is Eclipse Basyx an Industry 4.0 project is pending due to this pull request.

I request you to please look into this pull request and merge it so that we can use it in our project.

@ostrya
Copy link
Collaborator

ostrya commented Jan 17, 2023

Hi, sorry for the long delay. I had a really busy November and must then have lost track of this PR. I will have a look now.

- Inject this named singleton scopes inside TokenHelper
- Resets the UrlConfiguration
- Adapts to assertJ style assertions
- Updates Javadoc

Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
@mdanish98
Copy link
Contributor Author

Dear @ostrya

Many thanks for your suggestion I updated all the changes you requested.
One remark regarding the "please use a solution similar to the "resourcesToMapRolesTo" parameter:" comment:
As per your remarks I created a named singleton inside ServerModule. I tried to inject the named parameter into TokenGenerator's constructor but it was not working because the TokenGenerator is being used in SignatureComponent and due to this it was throwing error as [[Dagger/MissingBinding]](error: [Dagger/MissingBinding] @javax.inject.Named). So I thought that would be a major change because I might have to include the ServerModule.class in SignatureComponent like @component(modules = {KeyModule.class, ServerModule.class}) . And thatswhy I did the same thing you suggested in TokenHelper class instead, i.e. how "resourcesToMapRolesTo" is also implemented.

Please let me know if this is fine otherwise I will do it in TokenGenerator.

Please review it and let me know for further changes.

Thanks and Regards

@mdanish98 mdanish98 requested a review from ostrya January 18, 2023 20:38
Copy link
Collaborator

@ostrya ostrya left a comment

Choose a reason for hiding this comment

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

Yes, the approach to inject the scopes is much better. And it's fine to inject the scopes in the token helper instead.

I have just two minor remarks on the tests, otherwise it looks good now.


Set<String> scope = extractScopeFromToken(token);

assertThat(scope.size()).isEqualTo(3);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cosmetic change:

Suggested change
assertThat(scope.size()).isEqualTo(3);
assertThat(scope).hasSize(3);

@@ -47,7 +47,6 @@ void default_values_are_used() {
assertThat(config.getPreferredUsername()).isNull();
assertThat(config.getRealmAccess().getRoles()).isEmpty();
assertThat(config.getResourceAccess()).isEmpty();
assertThat(config.getScope()).isEqualTo("openid");
Copy link
Collaborator

Choose a reason for hiding this comment

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

To document the new behavior, maybe assert the scope is empty by default? I.e.

assertThat(config.getScope()).isEmpty();

@ostrya
Copy link
Collaborator

ostrya commented Jan 19, 2023

I'll just merge and adapt the tests on the main branch.

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

Successfully merging this pull request may close these issues.

2 participants