Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add acs_endpoints to SAML app (okta_app_saml) definition #226

Conversation

pranjalranjan
Copy link
Contributor

SAML app resource (okta_app_saml) in terraform okta provider is missing the ability to add multiple acs endpoints, which are needed at times when multiple Requestable SSO URLs are needed. Proposed the following changes:

  1. Added "acs_endpoints" argument to SAML app resource definition.
  2. Made corresponding changes to saml app data source and syncSamlSettings.

SAML app resource in terraform okta provider is missing the ability to add multiple acs endpoints.

1. Added "acs_endpoints" argument to SAML app resource definition.
2. Made corresponding changes to saml app data source and syncSamlSettings.
SAML app resource in terraform okta provider is missing the ability to add multiple acs endpoints.

1. Added "acs_endpoints" argument to SAML app resource definition.
2. Made corresponding changes to saml app data source and syncSamlSettings.
@bogdanprodan-okta
Copy link
Contributor

Hi @pranjalranjan ! Thanks for submitting this PR! I will try to review it shortly.

@pranjalranjan
Copy link
Contributor Author

Thanks @bogdanprodan-okta! Just to add, most probably this argument was missing earlier as the golang-sdk was on a very old version and "AcsEndpoint" struct has been added to the sdk in October 2020 release only. Added to it , now the provider is on the latest version.

@bogdanprodan-okta bogdanprodan-okta added the enhancement Asking for new behavior or feature label Dec 10, 2020
@bogdanprodan-okta
Copy link
Contributor

Hi @pranjalranjan ! I've added a few things in, however, Okta API doesn't work as expected and doesn't add access endpoints. I will investigate the issue and once it's fixed I'll merge this PR.

@pranjalranjan
Copy link
Contributor Author

Thanks @bogdanprodan-okta . There is some inconsistency with the API I have observed. If the request payload during app creation (POST) doesn't have the parameter "app{}" (which maps to "app_settings_json" in the terraform arguments case), the ACS endpoints are not added for some reason, in a new SAML application. I haven't found any documentation around this, which is why I am not sure if this is a bug or it is intended to be like that. This is only the case with a new app creation, and not with the update scenario.

I assume here that since in this case, "app{}" is an 'omitempty' parameter in the golang sdk, it is not being sent along with the payload if "app_settings_json" is empty, which is why even after sending "acs_endpoints", they are not being updated. Is it possible to validate this?

@bogdanprodan-okta
Copy link
Contributor

Thanks @bogdanprodan-okta . There is some inconsistency with the API I have observed. If the request payload during app creation (POST) doesn't have the parameter "app{}" (which maps to "app_settings_json" in the terraform arguments case), the ACS endpoints are not added for some reason, in a new SAML application. I haven't found any documentation around this, which is why I am not sure if this is a bug or it is intended to be like that. This is only the case with a new app creation, and not with the update scenario.

I assume here that since in this case, "app{}" is an 'omitempty' parameter in the golang sdk, it is not being sent along with the payload if "app_settings_json" is empty, which is why even after sending "acs_endpoints", they are not being updated. Is it possible to validate this?

Wow, that's interesting. I've added a fix in my last commit that will forcibly create app{}.

@pranjalranjan
Copy link
Contributor Author

Thanks @bogdanprodan-okta.. That ( force creating app{} ) seems to have worked out for acs_endpoints.

@bogdanprodan-okta
Copy link
Contributor

Thanks @bogdanprodan-okta.. That ( force creating app{} ) seems to have worked out for acs_endpoints.

Cool, I'm merging this PR and will publish it with 3.7.0 version.

@bogdanprodan-okta bogdanprodan-okta merged commit effede9 into okta:master Dec 15, 2020
@summersonne
Copy link

acs_endpoints = ["https://example.com", "https://okta.com"]

An argument named "acs_endpoints" is not expected here.

From example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Asking for new behavior or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants