crossplane-contrib / provider-keycloak

Apache License 2.0
21 stars 11 forks source link

ClientRolePolicy not requiring decisionStrategy and logic fields #150

Closed a-finocchiaro closed 3 weeks ago

a-finocchiaro commented 1 month ago

Hello!

Thank you so much for maintaining this repository, this provider is awesome and extremely powerful.

Required Fields

In my playing around with it, I have noticed a small issue with the ClientRolePolicy resource. The first is that I believe the decisionStrategy and logic fields should both be required. If you exclude either of these, the keycloak server will respond with a 500 error, and in the keycloak log it will look something like this:

Caused by: com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot coerce empty String ("") to `org.keycloak.representations.idm.authorization.DecisionStrategy` value (but could if coercion was enabled using `CoercionConfi 
at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 43] (through reference chain: org.keycloak.representations.idm.authorization.RolePolicyRepresentation["decisionStrategy"])

In the example above I purposely left out decisionStrategy, but a very similar error is thrown by the keycloak server when leaving out the logic field.

My guess too is that some other ClientXPolicy resources have this same issue.

Breee commented 1 month ago

Good catch, i'll look into that. Please Provide a Minimal example and what you expect, so I can work with that 👍

a-finocchiaro commented 1 month ago

@Breee No problem! I think the CRD that gets created by upjet should include decisionStrategy and logic in the array of required fields. I'm not super familiar with how upjet works other than having just read the docs a little bit to understand how this project is built, but it seems like it might just be something like this in config/openidclient/config.go

...
        p.AddResourceConfigurator("keycloak_openid_client_role_policy", func(r *config.Resource) {
           r.ShortGroup = Group

               if s, ok := r.TerraformResource.Schema["decisionStrategy"]; ok {
                   s.Optional = false
                   s.Computed = false
               }

              if s, ok := r.TerraformResource.Schema["logic"]; ok {
                  s.Optional = false
                  s.Computed = false
              }
    })

And then that would need to also be replicated on keycloak_openid_client_client_policy, keycloak_openid_client_group_policy, and keycloak_openid_client_user_policy as well I believe.

I'm only basing this off of what I found on the upjet docs too so it's kind of just a guess at what needs to be done. If you'd like I'm more than happy to take a stab at trying to fix it and writing up a PR!

Breee commented 1 month ago

156 should fix that - i'll merge that after #155

Breee commented 3 weeks ago

merged #156

new release v1.4.0 is available