Add OIDC backchannel initiators #1045
Conversation
| @@ -41,7 +41,7 @@ func expandClient(data *schema.ResourceData) (*management.Client, error) { | |||
| InitiateLoginURI: value.String(config.GetAttr("initiate_login_uri")), | |||
| EncryptionKey: value.MapOfStrings(config.GetAttr("encryption_key")), | |||
| IsTokenEndpointIPHeaderTrusted: value.Bool(config.GetAttr("is_token_endpoint_ip_header_trusted")), | |||
| OIDCBackchannelLogout: expandOIDCBackchannelLogout(data), | |||
There was a problem hiding this comment.
Perhaps it would be better to create a nested OIDCLogout structure instead of the current approach, as oidc_backchannel_logout_urls already contains other values. Instead of removing oidc_backchannel_logout_urls, we can deprecate it and introduce OIDCLogout, with nested properties such as oidc_backchannel_logout_urls and BackChannelLogoutInitiators, similar to the implementation in the SDK. This approach ensures backward compatibility, as it's not going to be a major release.
There was a problem hiding this comment.
The concern I had with that approach is you introduce the potential of setting the back channel logout urls in two different places, via the depreciated property and via the new nested properties. I imagine the Auth0 API will reject if you try that but I wasn't sure?
I also was trying to follow the proposal in this issue #1030
Generally I do prefer flat json to nested properties.
It seems the main reason for this change is your comment about backwards compatibility. These properties exist on the API and are set even though you can't control them via terraform currently. I guess the concern is someone gets a provider update and then it will try to set them back to the default values if they don't add their current values to their terraform? I can see that would be an issue. I just hate resorting to nested JSON but I suppose there isn't a better option.
| @@ -557,7 +557,9 @@ func flattenClient(data *schema.ResourceData, client *management.Client) error { | |||
| data.Set("initiate_login_uri", client.GetInitiateLoginURI()), | |||
| data.Set("signing_keys", client.SigningKeys), | |||
| data.Set("client_metadata", client.GetClientMetadata()), | |||
| data.Set("oidc_backchannel_logout_urls", client.GetOIDCBackchannelLogout().GetBackChannelLogoutURLs()), | |||
There was a problem hiding this comment.
Same here don't remove this just deprecate this.
internal/auth0/client/resource.go
Outdated
| "oidc_backchannel_logout_initiators_mode": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| ValidateFunc: validation.StringInSlice([]string{ | ||
| "all", "custom", | ||
| }, false), | ||
| Description: "Which initiators should receive a logout token. Possible values are `all` indicating all " + | ||
| "initiators or `custom` meaning only those selected via oidc_backchannel_logout_initiators", | ||
| }, | ||
| "oidc_backchannel_logout_initiators": { | ||
| Type: schema.TypeSet, | ||
| Elem: &schema.Schema{ | ||
| Type: schema.TypeString, | ||
| ValidateFunc: validation.StringInSlice([]string{ | ||
| "rp-logout", "idp-logout", "password-changed", "session-expired", "session-revoked", | ||
| "account-deleted", "email-identifier-changed", "mfa-phone-unenrolled", "account-deactivated", | ||
| }, false), | ||
| }, | ||
| Optional: true, | ||
| Description: "Which specific initiators should receive a logout token. Possible values are " + | ||
| "rp-logout, idp-logout, password-changed, session-expired, session-revoked, account-deleted, email-identifier-changed, mfa-phone-unenrolled, account-deactivated", | ||
| }, |
There was a problem hiding this comment.
Perhaps it would be better to create a nested OIDCLogout structure instead of the current approach, as oidc_backchannel_logout_urls already contains other values. We can deprecate oidc_backchannel_logout_urls and introduce OIDCLogout, with nested properties such as oidc_backchannel_logout_urls and BackChannelLogoutInitiators, similar to the implementation in the SDK.
|
@developerkunal I just updated with your suggestion of using a oidc_logout block with nested attributes. There are still some failing tests when I run E2E in my test tenant not related to features we don't have enabled. The problem appears to be that the API returns the logout urls both in the OIDCBackchannelLogout object and the OIDCLogout object. I've tried a couple different approaches but rolled them back because they all had unintended side effects. The problem is we don't necessarily know which approach in the flatten to take, ignore the old setting or ignore the new one. The paths I've gone down have all caused another test to fail. I'm going to step away and then come back at it fresh. If you have any suggestions for handling it I'd appreciate them. |
| @@ -62,6 +62,7 @@ data "auth0_client" "some-client-by-id" { | |||
| - `native_social_login` (List of Object) Configuration settings to toggle native social login for mobile native applications. Once this is set it must stay set, with both resources set to `false` in order to change the `app_type`. (see [below for nested schema](#nestedatt--native_social_login)) | |||
| - `oidc_backchannel_logout_urls` (Set of String) Set of URLs that are valid to call back from Auth0 for OIDC backchannel logout. Currently only one URL is allowed. | |||
There was a problem hiding this comment.
Should we mark this one as deprecated as well?
There was a problem hiding this comment.
Do we want to change the data source to pull back the data differently? I didn't change the data source to add the new nested structure and this is auto generated from that so that is why it isn't marked as depreciated.
There was a problem hiding this comment.
yeah, make sense, for client retrieval it is probably not deprecated.
@bryanroute If I understand you correctly, I think, it's better to ignore old setting |
|
@Seha16 I'm not sure I understand your suggestion. If I don't set the old oidc_backchannel_logout_urls then won't that generate a unexpected plan diff for someone using the old property and not implementing the new block? I'm not sure if I'm understanding your suggestion. |
|
@bryanroute I just wanted to reply on your message
And I want to say that I like you current implementation and agree with you to ignore func flattenOIDCBackchannelURLs(backchannelLogout *management.OIDCBackchannelLogout, logout *management.OIDCLogout) []string {
if logout != nil {
return nil
} else {
return backchannelLogout.GetBackChannelLogoutURLs()
}
}
...
data.Set("oidc_backchannel_logout_urls", flattenOIDCBackchannelURLs(client.GetOIDCBackchannelLogout(), client.GetOIDCLogout())),
data.Set("oidc_logout", flattenOIDCLogout(client.GetOIDCLogout())),There was silence in this PR and no replies on your comments for a while. So I thought voting for your solution may bring some attention of the project contributors. |
|
Ah @Seha16 I thought you were from Auth0. I got a message on my support ticket tracking the missing functionality. They are supposed to be reviewing my PR soon and figured that you were the one doing so. Appreciate drawing attention to it. I'm hoping I hear back from the team soon so we can get this functionality. |
🔧 Changes
This adds two new properties to the auth0_client resource:
oidc_backchannel_logout_initiators_mode to set the mode for which initiators should get a logout token. Valid values are "all" or "custom"
oidc_backchannel_logout_initiators to set which specific initiators should get a logout token if the mode is set to "custom"
This allows all the configuration around the OIDC backchannel logout to be set via terraform where as currently you can only set the URL and nothing else.
Worth calling out this is my first PR working with a terraform provider so if I'm missing something about the terraform SDK and how it is supposed to work I apologize in advance. I just really want this functionality added and this seems to do it.
📚 References
Addresses this open issue with the proposed design
#1030
Also addresses this support ticket: https://support.auth0.com/tickets/02368461
🔬 Testing
Test checks were added to the existing tests however http recordings need to be regenerated for the tests to pass and it appears from what I could figure that has to be done by someone that has access to the test tenant
terraform-provider-auth0-dev.eu.auth0.comI did run the e2e tests against my own test tenant and everything passed except for some tests related to features not enabled in my test tenant.
This was manually tested and verified by creating a client with none of the back channel properties set, one with the mode all and one with the mode custom. Everything appears to function as I expected.
I do have an open question whether the approach to handling the default mode/initiators is the correct one however I'm not sure of a better approach and the behavior seems to mirror similar behavior I've observed with other terraform providers I've worked with.
📝 Checklist