Add Keycloak_openid_client_permission for admin_fine_grained_authz #364
Conversation
jermarchand
force-pushed
the
master
branch
3 times, most recently
from
b71513d to
7ad056d
Compare
|
@tomrutsaert could you take a look at this when you have a chance? I'm very unfamiliar with the permissions / policy features within Keycloak. |
There was a problem hiding this comment.
Looks good.
Better than what I created: resourceKeycloakIdentityProviderTokenExchangeScopePermission
By looking at this, mine does to many things automatically.
The policy should indeed be created in its own separate resource (and not implictly)
I do not recall why I did it like this....
Remarks:
- I would call the resource in plural: keycloak_openid_client_permissions as that is what it is called in the keycloak UI + it handles more then 1 permission
- I would remove the permission toggle on the keycloak openid client resource and let the fact you have a keycloak_openid_client_permission resource handle the enable/disable call.
- Perhaps add an extra test, to see that the policy was in fact set on the correct permission.
@jermarchand what do you think?
| "github.com/mrparkers/terraform-provider-keycloak/keycloak" | ||
| ) | ||
|
|
||
| func resourceKeycloakOpenidClientPermission() *schema.Resource { |
There was a problem hiding this comment.
Should this not plural resourceKeycloakOpenidClientPermissions instead of resourceKeycloakOpenidClientPermission?
There was a problem hiding this comment.
Yep, I'll rename this resource.
| Type: schema.TypeBool, | ||
| Optional: true, | ||
| Default: false, | ||
| }, |
There was a problem hiding this comment.
Is having a resourceKeycloakOpenidClientPermissions which refers the openid client not make this obsolete?
The resourceKeycloakOpenidClientPermissions can never exist when this is false...
I would let the resourceKeycloakOpenidClientPermissions handle the enable of disable of the permissions on the client, just by the fact it exist. You only have to be clearfull that you take this into account when destroying this resource....
Even in keycloak I think the permissions are located in the wrong location. It is really a resource in it own right
What do you think?
There was a problem hiding this comment.
I added permissions_enabled in resourceKeycloakOpenidClient to be able to enable / disable permissions independent of the permissions configuration themselves.
We can indeed consider that the presence of resourceKeycloakOpenidClientPermissions activates the permissions on the client.
To be able to manage its deactivation, I will keep the enabled parameter in resourceKeycloakOpenidClientPermissions which with the value false will delete the resource
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If you agree this would be a part of resourceKeycloakOpenidClientPermissions create method
| Type: schema.TypeBool, | ||
| Optional: true, | ||
| Default: true, | ||
| }, |
There was a problem hiding this comment.
Does this not conflict with permission_enabled parameter on the keycloak_openid_client?
Perhaps both are even obsolete. Because this resource does not exist if it is disabled.
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testKeycloakOpenidClientPermission_basic(realmName, clientId), | ||
| Check: testAccCheckKeycloakOpenidClientPermissionExists("keycloak_openid_client_permission.my_permission"), |
There was a problem hiding this comment.
Could you add a test that checks that the policy is actually set on the permission?
|
@tomrutsaert could you take a look at my updates ? |
|
Looks good to me. |
…eycloak#364) Co-authored-by: Jerome Marchand <prestataire.0121541@ouest-france.fr>
Add on
keycloak_openid_clientthe optional optionpermissions_enabledto enable Fine grained permissions on the client.Add resource
keycloak_openid_client_permissionto manage the policies to apply to each scopes.