Fix marshalling of false value in KeycloakBoolQuoted

[vlaurin]

Resolves #492

false is currently marshalled to "" (empty string). This is incorrect: "" represents the default value for an attribute which hasn't been explicitly set.

"" could be interpreted by Keycloak as either true or false depending on what the default value is in Keycloak for the attribute considered. For example, in client scopes, both attributes include.in.token.scope and display.on.consent.screen are evaluated to true (ON) when their value is "".

As a result, "" and "false" are not equivalent and "false" must be used explicitly to accurately turn off Keycloak attributes and not rely on default Keycloak behaviours.

For the same reason, the assumption made when unmarshalling is incorrect as well, though this has a lesser negative effect: https://github.com/mrparkers/terraform-provider-keycloak/blob/master/keycloak/util.go#L24-L25

[mrparkers]

The code LGTM, thanks for looking into this.

For the same reason, the assumption made when unmarshalling is incorrect as well, though this has a lesser negative effect: https://github.com/mrparkers/terraform-provider-keycloak/blob/master/keycloak/util.go#L24-L25

Yeah, this makes sense, but I'm not sure if there's anything we can really do about that. I think the best solution would be for the Keycloak API to stop using quoted boolean values 😄 . Although if you have any suggestions I'm definitely open to them.

[vlaurin]

@mrparkers Thank for merging. Yes, the unmarshalling one is tricky. I agree that the proper fix should be on the Keycloak side, if Keycloak is assuming default values for attributes then its API should reflect these defaults rather than returning empty strings.

At the level of this provider, I think the ideal course of action would be to not map "" to either true or false but rather to something like nil though this breaks the bool type.

The risk at present is that if an attribute is set as false on a resource, but pulled as "" from Keycloak then no update will take place; if the default value assumed by Keycloak is true then that's a mismatch. To recover from it, one would have to first set the value to true to force an update to an explicit value, then switch back to false.

Instead, I think what should be expected is:

Keycloak	Terraform	Current behaviour	Expected behaviour
""	"false"	Noop	Update to "false"
""	"true"	Update to "true"	Update to "true"
"false"	"false"	Noop	Noop
"true"	"true"	Noop	Noop

This would require KeycloakBoolQuoted to have 3 states which I believe is not permitted with Go bool type?