feat: support role IDs in default role resource

[horus]

Currently only the realm roles are allowed to be added, however some of the client roles could be found in a fresh installation, taking this as an example:

The last 2 roles are unmanageable with the following configuration:

resource "keycloak_default_roles" "default_roles" {
    realm_id      = keycloak_realm.realm.id
    default_roles = ["uma_authorization", "offline_access", "view-profile", "manage-account"]
}

This PR, if accepted, will add a new optional attribute called default_role_ids which is a list of UUIDs of the specific roles you want to have in your default role:

resource "keycloak_default_roles" "default_roles" {
   realm_id      = keycloak_realm.realm.id
   default_roles = ["uma_authorization", "offline_access" ]
   default_role_ids = [
     "8550facd-b29e-4da9-beca-ca2cd5673b20", # view-profile
     "8696cdda-8403-4ab7-bdc9-a1131f02e3ee", # manage-account
   ]
 }

We could obtain the original behavior of the resource when omitted (as if it's an empty list).

[mrparkers]

Overall the code LGTM. I don't have a problem with adding this attribute, but I wonder if it would be easier for users of this resource to instead allow you to specify client roles using the format ${client_id}/${role_name} instead of using IDs. This would prevent you from having to use a data source to retrieve the ID for a role that Keycloak creates automatically

resource "keycloak_default_roles" "default_roles" {
   realm_id      = keycloak_realm.realm.id
   default_roles = ["uma_authorization", "offline_access", "account/view-profile", "account/manage-account" ]
 }

What do you think about this?

[horus]

To be honest this was exactly what I did in my first implementation, with a different separator. :-)

I wanted a new attribute to serve as a backdoor that I could supply client roles. Instead of change the behavior of the original default_roles, I added the optional default_role_ids. It emphasizes the use of UUIDs: the users are forced to check the resources or referring via data sources instead.

The real problem: default_roles is string-typed, although it turned out working perfectly fine, looks like a design fault to me. Strings sometimes are ambiguous and error-prone. Since we're using Terraform here, we could manage all the resource in one place, and make them connected with attributes:

resource "keycloak_default_roles" "default_roles" {
   realm_id      = keycloak_realm.realm.id
   default_roles = [ keycloak_role.view-profile.id, keycloak_role.manage-account.id ]
 }

...as the example showed here.

I could switch to ${client_id}/${role_name} as it won't cost much on my side. However, it's my first time contributing to this project, so having a discussion would definitely help.

Thank you!

[zifeo]

@mrparkers @horus is there something blocking this to move forward? as far as I understand, there is no way to add the account deletion role by default for now?

[horus]

@zifeo

Actually, I'm still awaiting the project owner's input on how we should resolve this issue.
Originally I thought that ${client_id}/${role_name} would be more intuitive, but UUIDs are more unambiguous.
I'm fine with either solution.

Please note that I'm responding to a 1-year-old PR, so circumstances may have evolved.

[zifeo]

@horus On my end, I would push ${client_id}/${role_name} as it is more explicit and similar to the existing options. WDYT @mrparkers? Happy to help where I can to get this shipped :)

[KiwiKilian]

As I'm also in need to access the default roles, it seems more straightforward to use ${client_id}/${role_name} instead of defining the default_role_ids.

[horus]

The code has been updated to use ${client_id}/${role_name} instead of UUID according to the feedback.
Please let me know if you have any questions or suggestions. Thank you!

[robson90]

Would love to see this !
@horus Tests on 20.0.5 are failing.

[horus]

@robson90

Thank you for pointing this out.
I can't find any reason related to this PR that would cause the test to fail.
Could you help me to investigate the issue together?
After merging and rerunning the tests, the error is gone.

[sventorben]

🚀 Need this feature for our platform development! Great work, can't wait to see it merged.

[robson90]

@mrparkers would you be so kind, to review and possibly merge this PR?

[TehreemNisa]

Can this feature be merged? We are looking forward to it as we need it in our platform.

Thanks,

[jazzlyn]

any updates on this?

[kominoshja]

Tested on KC 25.0.2 and works as expected. @mrparkers i know you're working on the migration of #964, but this seems like a quick merge

[kominoshja]

Edit: nvm, getting this when creating a realm with default_roles:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xb466d8]