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

Role Scopes: Allow specifying client scope instead of client #253

Merged

Conversation

dmeyerholt
Copy link
Contributor

@dmeyerholt dmeyerholt commented Mar 16, 2020

My first shot at extending this great provider.
Following #242 In addition to clients, I want to extend the keycloak_generic_client_role_mapper resource to also manage client roles for client scopes. This allows consolidation of client roles in dedicated client scopes.
Im no go developer and still need to extend the

  • validation
  • test cases
  • documentation

@mrparkers
Copy link
Owner

Hey @dmeyerholt, thanks for opening this PR. Your changes look good so far.

I could be wrong, but I don't think you need to add any more validation on top of what you already have - all I would have expected to see was the ConflictsWith on the client id and client scope id attributes.

For the test cases, you should be able to copy/paste a lot of code in the resource_keycloak_generic_client_role_mapper_test.go file and replace the client_id attribute with client_scope_id in the HCL snippets, and add new assertions to make sure the correct value is set after tf apply runs. I'd also appreciate it if you could add an example HCL configuration within example/main.tf.

Let me know if you need any assistance with this.

@dmeyerholt
Copy link
Contributor Author

dmeyerholt commented Mar 19, 2020

Thanks @mrparkers,

  • validation using ConflictsWith should be sufficient, right
  • will provide more testcases
  • additional HCL example coming up as well
    Hoping to finish in the next days :) and looking forward in providing more client scope related PRs as we use them a lot and they come in very handy when dealing with a lot of resources.

@dmeyerholt dmeyerholt force-pushed the client_roles_in_client_scopes branch 5 times, most recently from e279b01 to 72867f8 Compare March 20, 2020 13:33
@dmeyerholt
Copy link
Contributor Author

dmeyerholt commented Mar 20, 2020

Hey @mrparkers I think it's done so far. for documentation I extended the pet examples in roles.tf. For that to work properly those changes were done

  • on the app client: disabled full scope (so explicit roles have to be mapped into the client)
  • Now map all pet_api client roles into the app client (realm role missing though, maybe another pr ;) )
  • new optional client scope makes it possible to map the (new) client role read-pet-with-details into the tokens only, when that client scope is requested in the token request.

@dmeyerholt dmeyerholt marked this pull request as ready for review March 20, 2020 13:43
@dmeyerholt dmeyerholt force-pushed the client_roles_in_client_scopes branch from 72867f8 to e41051c Compare March 20, 2020 13:44
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.

This looks great, thanks for the contribution!

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