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

WIP: Add client scope mapping of roles between clients #242

Merged
merged 6 commits into from
Mar 10, 2020

Conversation

mukuru-shaun
Copy link
Contributor

@mukuru-shaun mukuru-shaun commented Mar 3, 2020

Hey @mrparkers, thanks for creating and maintaining this provider. It's great to see such activity and collaboration.

I am planning a Keycloak deploy and am trying to provision it using terraform and this provider. A use case I have is mapping roles between clients.

This is my first time with keycloak and golang so I'm submitting a WIP just to be sure I am on the right track. Please let me know if you would prefer the implementation another way.

  • Implement create, read, delete resources (update is not needed)
  • Test coverage
  • Update docs with new resource Add example

I went with the following structure for the resource. I didn't use an array of roles as I thought that might add complexity for roles from different parent clients:

resource "keycloak_generic_client_role_mapper" "role_mapping" {
  realm_id  = "${keycloak_realm.realm.id}"
  client_id = "${keycloak_openid_client.client.id}"
  role_id   = "${keycloak_role.role.id}"
}

Any direction before I wrap up the docs and tests would be great

@mrparkers
Copy link
Owner

The code looks good so far. My only thought for you is that the provider already supports the keycloak_openid_hardcoded_role_protocol_mapper resource that allows you to map roles to clients using a protocol mapper.

I am not really sure if this solves the same problem as the resource you're suggesting, which appears to use the same API as the "Scopes" tab on the client interface.

In any case, I don't have a problem with merging this PR, I'd just like you to add some tests and some example HCL within the example/main.tf file. Adding docs would also be super helpful but I don't typically require that from new contributors.

Thanks for the PR!

@mukuru-shaun
Copy link
Contributor Author

Awesome, glad it's on the right track. The difference with the hardcoded mapper is that the role then will come back in the token for your client.
If you use the scope mapping like this proposal the user also needs to have the role assigned to them in order to get the role. So if you get the role back in the token you can be sure the user can access that role and not just that the client can access the role.

I will get the tests and docs sorted asap. Thanks again for being so active and welcoming to contributions 🙇

@mrparkers
Copy link
Owner

Thanks for the explanation, that makes perfect sense.

I'm fine with merging your PR as-is, but I'll leave it since you had mentioned that you wanted to add docs for this.

@mukuru-shaun
Copy link
Contributor Author

I don't want the lack of docs to hold this up, especially as you said you are happy to merge. I have added an example to the example/roles.tf file using the existing pet_api and pet_app clients and associated roles.

I am happy for this to be merged whenever you are.

You rock @mrparkers 🤘

Copy link
Owner

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

Perfect, thanks for the PR!

@mrparkers mrparkers merged commit dcfb7a3 into mrparkers:master Mar 10, 2020
@dmeyerholt
Copy link
Contributor

dmeyerholt commented Mar 13, 2020

Hi great work! Would it be a large effort if the possibility to map client roles would also be extended to client_scopes instead? This has been done in other resources like the keycloak_openid_user_attribute_protocol_mapper and could ease the overall amount of resources. I'm happy to help here .
Nevermind will create separate PR for that

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.

3 participants