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

Add the ability to create roles and client-roles #102

Closed
MatteoJoliveau opened this issue Mar 8, 2019 · 22 comments · Fixed by #143
Closed

Add the ability to create roles and client-roles #102

MatteoJoliveau opened this issue Mar 8, 2019 · 22 comments · Fixed by #143

Comments

@MatteoJoliveau
Copy link

Hi, thank you for this provider!
We have a realm installation that features quite a lot of different roles and client-roles, and we would love to be able to create them through Terraform.

Is this new resource something you were considering adding?

@mrparkers
Copy link
Contributor

Hi @MatteoJoliveau, thanks for the issue!

Roles are not a feature that I personally use, but I would be happy to add support for it when I have some free time. Right now, my priority is to get more familiar with identity providers so I can leave a proper review for #92. After that, I can take a look at adding support for roles.

Alternatively, I'd also be willing to merge a PR for it if that is something you're interested in doing.

For reference, the API for this looks pretty straightforward: https://www.keycloak.org/docs-api/4.8/rest-api/index.html#_roles_resource

Let me know what your thoughts are.

@MatteoJoliveau
Copy link
Author

I'm not really that skilled in Go but I can take a shot at it. I'll see what I can put together in the next days.

@mrparkers
Copy link
Contributor

Sounds great! Feel free to ask any questions you may have in this issue.

@mrparkers
Copy link
Contributor

Hey @MatteoJoliveau, have you started working on this? I have a use case for this feature now so I would be willing to implement this resource if you weren't in the middle of working on it yourself.

@MatteoJoliveau
Copy link
Author

Hello @mrparkers! It was offloaded to a colleague which is more skilled in Go than me, but he doesn't know pretty much anything about Keycloak. I don't think he's gone very far with it ATM, so if you can start to work on it yourself it would be pretty awesome. We can provide some contributions later if you want to assign us some related tasks.

@MatteoJoliveau
Copy link
Author

Update: seems like my colleague has a first draft ready with get/update/delete implemented. I'll see if I can get the repo

@mrparkers
Copy link
Contributor

Sounds good. I'm not in any rush for this, I just wanted to make sure nobody was working on it before I started to. I can wait for a PR 😄

If your colleague wants to open a WIP PR with what they have so far, I can answer questions or offer advice.

Thanks!

@MatteoJoliveau
Copy link
Author

Sure! We uploaded his work to a public repo, we'll open the WIP PR tomorrow

@xiaoyang-connyun
Copy link
Contributor

Any updates here? Really looking forward to this feature.

@dhardiker
Copy link

We use Terraform to provision AWS account, automatically adding a SAML provider into the account, which maps up to a SAML Client in a Keycloak Realm. We then have to manually add a computed role to that client, and then assign that client role to a Keycloak group. This sounds like this feature request would cover our use case.

I don't have any Go experience and limited C++, but we can try to contribute if other efforts have stalled.

@mrparkers
Copy link
Contributor

@xiaoyang-connyun @dhardiker thanks for your interest in this feature request. I've held off on taking this on since it appeared that there was some desire to submit a PR for this.

I'll give @MatteoJoliveau a few days to chime in on the status of this, and if I don't hear anything, I'll take some time to implement this (since this is a feature I could use myself as well).

@MatteoJoliveau
Copy link
Author

Hello everyone, sorry for the delay. I got assigned another project and forgot to follow up on this.
I just checked my colleague's work and it seems it implemented support for realm roles, but it's missing tests, and client_roles are not yet implemented.

I'm very sorry to have held this issue back, we clearly had some communication issues internally. If anyone wants to pick up from there, our repository is at https://github.com/mikamai/terraform-provider-keycloak

@mrparkers
Copy link
Contributor

No problem at all @MatteoJoliveau, I just didn't want to work on it if you were still in the middle of implementing it.

I can take over this feature request unless either @xiaoyang-connyun or @dhardiker are interested in picking it up. Just let me know!

@xiaoyang-connyun
Copy link
Contributor

First of all, thanks @mrparkers , @MatteoJoliveau and your colleague for your contributions so far.
Unfortunately, I have literally zero experience in Go, although I have heard that it is a pretty appealing language. So, please go ahead, and I would be happy if you could add me as a reviewer, so I could have some feelings about it.

@mrparkers
Copy link
Contributor

Sure thing. Would you mind giving me an idea of what you think the HCL for this resource might look like?

@xiaoyang-connyun
Copy link
Contributor

Sure thing. Would you mind giving me an idea of what you think the HCL for this resource might look like?

Could I do it after July 1st? I am not available these days until July 1st.
The only thing come to my mind is how to model the user-role relation and the group-role relation, as a separate resource or a block.

@xiaoyang-connyun
Copy link
Contributor

@mrparkers

So, I come up with the proposal as follows.

resource "keycloak_role" "admin" {
    realm_id     = "${keycloak_realm.my_realm.id}"
    client_id     = "${keycloak_openid_client.my_client.id}" # Required, if it is a client role.
    name          = "admin"
    composites = ["${keycloak_role.read.id}", "${keycloak_role.write.id}"]
    description = "It is an admin."
}
resource "keycloak_group_roles" "developer" {
    realm_id = "${keycloak_realm.my_realm.id}"
    group_id = "${keycloak_group.developer.id}"
    roles       = ["${keycloak_role.admin.id}", ]
}

I think they are the minimum for me. I personally do not assign roles to the user directly, and rarely use composite roles.

Do you have any feedbacks?

@MatteoJoliveau
Copy link
Author

I like it, it's basically what we envisioned at the beginning.

I agree directly assigning roles to users is not a very good practice and can be implemented laters, while composite roles are very very useful when organizing lots of small, granular roles.

@xiaoyang-connyun
Copy link
Contributor

Yes, you are right in terms of composite roles. I rarely use it only because my application does not have that many complex role relations.

@camelpunch
Copy link
Contributor

There is a strong chance that Boclips, the company I'm working for will need this functionality. We're likely to be able to contribute this code, or continue something that has been started. Is it in progress anywhere right now, or is the mikamai fork the latest and greatest?

I'm likely to be the main contributor, and have some prior commercial experience with Go. Would be TDDing the implementation.

@mrparkers is it likely that you'll get time to do this in the next couple of months, or shall I get started?

@mrparkers
Copy link
Contributor

@xiaoyang-connyun apologies for the late reply, I think your HCL looks good. I'll base my implementation off of that unless I run into any issues.

@camelpunch thanks for your interest in contributing! I actually have a need for this resource very soon, so I'm going to pick this up within the next couple of days.

@mrparkers mrparkers mentioned this issue Aug 19, 2019
6 tasks
@fharding1
Copy link
Contributor

fharding1 commented Aug 28, 2019

Just wanted to chime in and say this feature will be immensely useful to us here at @Billups. I see there's already a WIP PR, but let me know if we can help.

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 a pull request may close this issue.

6 participants