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

Retain user group app assignments #330

Merged

Conversation

Omicron7
Copy link
Contributor

We have a very distributed terraform setup where we use Okta with AWS SSO for user and permissions management. The same User and Group assignments can be made in different modules for different AWS Permission Sets by different developers. Okta user and group assignments are idempotent, meaning you can assign the same user multiple times in multiple places which only results in a single assignment in the Okta app. The flip side is that a single destroy of okta_app_user or okta_app_group_assignment completely removes the user/group from the app, even though it may still exist in multiple other terraform modules.

This adds retain_assignment arguments to both the okta_app_user and okta_app_group_assignment resources. This optional attribute allows terraform to remove the resource from state when it is destroyed, but to retain the user/group assignments on the associated Okta app. Documentation changes have been added warning that using this feature will result in the need to possibly manually remove user/group assignments in the future.

Tests have been updated and run and are passing against our instance of oktapreview.

Let me know what you think and if you'd like any changes.

@bogdanprodan-okta
Copy link
Contributor

Hi, @Omicron7! Thanks for submitting this PR! I'll review it ASAP.

@noinarisak noinarisak added the needs-investigation Needs further investigation label Feb 18, 2021
@noinarisak
Copy link
Contributor

Hi, @Omicron7! Thanks for submitting this PR.

I was chatting with @bogdanprodan-okta and I have a concern about this PR because it disrupts the TF lifecycle, ie modifying the tfstate when you have retain_assignment true and possibly manually removing user/groups on our Okta tenant. I'm not connecting the dots on the pain of what this PR will resolve for the larger community. In the end, we are trying to make sure all the PRs add value across the community by balancing out more explicit implementation than implicit ones. I hope that helps with the hesitation on this PR.

Could you detail out the scenario for us, maybe sample implementation via gist, so we can evaluate it again. Thanks again and sorry for asking for more details!

@bogdanprodan-okta bogdanprodan-okta added the waiting-response Waiting on collaborator to responde to follow on disucussion label Feb 18, 2021
@Omicron7
Copy link
Contributor Author

Omicron7 commented Feb 18, 2021

@noinarisak @bogdanprodan-okta Thanks for the responses. I understand that this somewhat disrupts the normal terraform lifecycle and hoped to warn users of that fact through the documentation updates. I'll try to create an example here that shows the issue we are running into and why this PR solves it.

We have a single Okta app using AWS SSO, we'll call it aws-sso.
We have a Terraform module (aws-okta-permissions) for adding permission sets to AWS that grant Okta users/groups access to AWS permissions. A simplified interface would be something like this (the actual module is in a private repo):

module "permissions" {
  source         = "aws-okta-permissions"
  name           = "Network Administrators"
  okta_users     = ["user.name", "user2.name2"]
  okta_groups    = ["group name"]
  iam_policy_doc = iam_policy.json
}

Internally, this module uses data sources to lookup the users/groups in Okta, assign them to the aws-sso app, and grant them the permissions in AWS. We use this module in multiple places in our larger AWS infrastructure repo, as well in other smaller application repos.

The problem we are running into is that assigning users/groups to an app in Okta is idempotent, meaning that we can assign the same user/group in multiple modules (locations in the AWS repo, and smaller repos), but it results in only one copy of the user/group assigned in the okta app. Deleting user/group from an app is not idempotent.

Here's an example using REST

POST /api/v1/apps/{appId}/users { "Id": "abc123", ...}
POST /api/v1/apps/{appId}/users { "Id": "abc123", ...}
DELETE /api/v1/apps/{appId}/users/abc123

Both POST return 2xx responses and result in the user abc123 being assigned to the app. The single DELETE removes the user. In our terraform scenario, the POSTs happen in 2 different modules, each module now "owns/manages" the user assignment to the app. If we destroy one of the modules (DELETE request), Okta removes the user assignment and de-provisions them from the app. But we still have a module that "owns" that user assignment to the app, but it is now out of sync/drift, and the user in question has lost all permissions because Okta deprovisioned them.

Here's a contrived terraform example.

In Okta, we have the following users/groups

users:
  - Tony.Stark
  - Bruce.Banner
  - Natasha.Romanoff
  - Nick.Fury
groups:
  avengers:
    - Tony.Stark
    - Bruce.Banner
    - Natasha.Romanoff

In Terraform, the following permissions are defined for AWS/Okta. They are defined in different git repos, but all use the same Okta AWS SSO app and AWS account.

# github.com/Avengers/avengers/permissions.tf
module "avengers" {
  source = "aws-okta-permissions"
  name = "The Avengers"
  okta_groups = ["avengers"]
}

# github.com/StarkIndustries/ultron/permissions.tf
module "ultron" {
  source = "aws-okta-permissions"
  name = "Ultron"
  okta_users = ["Tony.Stark", "Bruce.Banner"]
}

# github.com/StarkIndustries/insight/permissions.tf
module "project_insight" {
  source = "aws-okta-permissions"
  name = "Project Insight"
  okta_users = ["Nick.Fury", "Tony.Stark", "Natasha.Romanoff"]
}

Each instance of the aws-okta-permissions internally looks up the users/groups (data source) from Okta and assigns them (okta_app_user, okta_app_group_assignment respectively) to the aws-sso app. If for any reason, we want to destroy one of these modules, say ultron, it will result in destroying the associated okta_app_user for both Tony.Stark and Bruce.Banner. This will cause Okta to deprovision those users from the AWS SSO app. Those user will no longer be able to login to AWS even though they have permissions and okta_app_user resources defined in other places.

By using the retain_assignment attribute, we can tell terraform NOT to DELETE the okta_app_user or okta_app_group_assignment from Okta when they are destroyed in terraform. With those attributes enabled, Tony.Stark and Bruce.Banner would still be able to login to AWS and access their other assigned permissions. They could not access ultron any more as those associated AWS permissions were destroyed.

In contrast, If we destroyed project_insight with retain_assignment enabled, Nick.Fury, Tony.Stark, and Natasha.Romanoff's AWS permissions would be removed for that project, but all 3 would still be able to log in. Tony.Stark and Natasha.Romanoff would still have permissions from the avengers module, but Nick.Fury, even though he could still log in, would have no permissions in AWS.

Conclusion

I hope this helps to clarify the issues we are encountering any why this PR solves it. I thought it could be useful for others, and so made a PR rather than just using a forked provider for ourselves. I'm open to any changes in wording or documentation to let others know this is an advanced feature that disrupts the standard terraform lifecycle and that they should be aware of that before enabling it. I'm positive I've seen this pattern in some other providers that also had idempotent resources, but haven't been able to find them again.

Let me know if you have any more questions.

@Omicron7
Copy link
Contributor Author

Omicron7 commented Feb 18, 2021

Here's another Terraform resource that does something similar:
google_project_service

You can set the disable_on_destroy attribute to false to cause terraform to remove the resource from state, but not issue the DELETE to the google project.

@bogdanprodan-okta
Copy link
Contributor

Hi, @Omicron7! I think we can merge your PR since it doesn't have any breaking changes to the existing infrastructure.

@bogdanprodan-okta bogdanprodan-okta merged commit e465901 into okta:master Feb 23, 2021
eatplaysleep added a commit to eatplaysleep/terraform-provider-mgm_okta that referenced this pull request Mar 8, 2021
* Custom swa app (okta#328)

* Set AUTO_LOGIN as sign_on mode for predefined apps

* Force 'okta_app_user_schema' resource recreate when changing scope (okta#331)

* Fix 'terraform plan' in case delete_when_absent is set to 'false' (okta#332)

* Fixed validation for 'login_mode' and 'login_scopes' (okta#333)

* Add 5 second wait after create

* Added okta_auth_server_scopes datasource (okta#336)

* Added social IdP data source (okta#337)

* Fixed error handling (okta#338)

* Moved validation for okta_app_oauth out of CustomizeDiff (okta#340)

* Fixed group role when removing all the items from target_group_list (okta#341)

Fixed group role when removing all the items from target_group_list

* Added retry for role re-assignment

* Formating

* Improved retry logic

* Build fix

* Added extra code

* Added changelog

* Remove extra space

* Fixed okta_idp_oidc subject_match_attribute value setup

* use loop to perform multiple retries of find

* Use backoff lib per bogdanprodan-okta

* Update error messages

* Only set ID at end once search for user passes

* Bump actions/stale from v3.0.16 to v3.0.17

Bumps [actions/stale](https://github.com/actions/stale) from v3.0.16 to v3.0.17.
- [Release notes](https://github.com/actions/stale/releases)
- [Commits](actions/stale@v3.0.16...996798e)

Signed-off-by: dependabot[bot] <support@github.com>

* Retain user group app assignments (okta#330)

Add option to retain `okta_app_group_assignment` and `okta_app_user` on destroy.

* Fixed validation for several resources (okta#348)

* Added 'target_app_list' to the 'okta_group_role' resource (okta#349)

Added 'target_app_list' to the 'okta_group_role' resource

* Added 'OVERRIDE' master property (okta#351)

* Fixed setup for default scope (okta#352)

* Fixed setup for deafult scope

* Fixed vet

* Updated CHANGELOG (okta#354)

Updated CHANGELOG

* Changelog (okta#355)

Updated CHANGELOG

* Bump github.com/hashicorp/terraform-plugin-sdk/v2 from 2.4.3 to 2.4.4 (okta#357)

Bumps [github.com/hashicorp/terraform-plugin-sdk/v2](https://github.com/hashicorp/terraform-plugin-sdk) from 2.4.3 to 2.4.4.
- [Release notes](https://github.com/hashicorp/terraform-plugin-sdk/releases)
- [Changelog](https://github.com/hashicorp/terraform-plugin-sdk/blob/master/CHANGELOG.md)
- [Commits](hashicorp/terraform-plugin-sdk@v2.4.3...v2.4.4)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Removed redundant method (okta#358)

Removed redundant method

* Added new formatting tool, remove unused dep (okta#360)

* Add api scopes (okta#356)

Add resource okta_app_oauth_api_scope

* Removed 'ForceNew' in case policy name changes (okta#362)

Removed 'ForceNew' in case policy name changes

* Added hotp factor to the Okta MFA policy (okta#363)

Added hotp factor to the okta mfa policy

* Fixed error handler (okta#366)

* Fixed error handler

* Removed validation for 'single_logout_issuer'

* Bump actions/stale from v3.0.17 to v3.0.18 (okta#371)

Bumps [actions/stale](https://github.com/actions/stale) from v3.0.17 to v3.0.18.
- [Release notes](https://github.com/actions/stale/releases)
- [Commits](actions/stale@v3.0.17...3b3c3f0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Co-authored-by: bogdanprodan-okta <71279414+bogdanprodan-okta@users.noreply.github.com>
Co-authored-by: Tom Goodsell <ymylei@users.noreply.github.com>
Co-authored-by: Bogdan Prodan <bogdan.prodan@okta.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Brian Zoetewey <brian.zoetewey@cru.org>
Co-authored-by: Marius Sturm <marius@graylog.com>
eatplaysleep added a commit to eatplaysleep/terraform-provider-mgm_okta that referenced this pull request Mar 8, 2021
* Custom swa app (okta#328)

* Set AUTO_LOGIN as sign_on mode for predefined apps

* Force 'okta_app_user_schema' resource recreate when changing scope (okta#331)

* Fix 'terraform plan' in case delete_when_absent is set to 'false' (okta#332)

* Fixed validation for 'login_mode' and 'login_scopes' (okta#333)

* Add 5 second wait after create

* Added okta_auth_server_scopes datasource (okta#336)

* Added social IdP data source (okta#337)

* Fixed error handling (okta#338)

* Moved validation for okta_app_oauth out of CustomizeDiff (okta#340)

* Fixed group role when removing all the items from target_group_list (okta#341)

Fixed group role when removing all the items from target_group_list

* Added retry for role re-assignment

* Formating

* Improved retry logic

* Build fix

* Added extra code

* Added changelog

* Remove extra space

* Fixed okta_idp_oidc subject_match_attribute value setup

* use loop to perform multiple retries of find

* Use backoff lib per bogdanprodan-okta

* Update error messages

* Only set ID at end once search for user passes

* Bump actions/stale from v3.0.16 to v3.0.17

Bumps [actions/stale](https://github.com/actions/stale) from v3.0.16 to v3.0.17.
- [Release notes](https://github.com/actions/stale/releases)
- [Commits](actions/stale@v3.0.16...996798e)

Signed-off-by: dependabot[bot] <support@github.com>

* Retain user group app assignments (okta#330)

Add option to retain `okta_app_group_assignment` and `okta_app_user` on destroy.

* Fixed validation for several resources (okta#348)

* Added 'target_app_list' to the 'okta_group_role' resource (okta#349)

Added 'target_app_list' to the 'okta_group_role' resource

* Added 'OVERRIDE' master property (okta#351)

* Fixed setup for default scope (okta#352)

* Fixed setup for deafult scope

* Fixed vet

* Updated CHANGELOG (okta#354)

Updated CHANGELOG

* Changelog (okta#355)

Updated CHANGELOG

* Bump github.com/hashicorp/terraform-plugin-sdk/v2 from 2.4.3 to 2.4.4 (okta#357)

Bumps [github.com/hashicorp/terraform-plugin-sdk/v2](https://github.com/hashicorp/terraform-plugin-sdk) from 2.4.3 to 2.4.4.
- [Release notes](https://github.com/hashicorp/terraform-plugin-sdk/releases)
- [Changelog](https://github.com/hashicorp/terraform-plugin-sdk/blob/master/CHANGELOG.md)
- [Commits](hashicorp/terraform-plugin-sdk@v2.4.3...v2.4.4)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Removed redundant method (okta#358)

Removed redundant method

* Added new formatting tool, remove unused dep (okta#360)

* Add api scopes (okta#356)

Add resource okta_app_oauth_api_scope

* Removed 'ForceNew' in case policy name changes (okta#362)

Removed 'ForceNew' in case policy name changes

* Added hotp factor to the Okta MFA policy (okta#363)

Added hotp factor to the okta mfa policy

* Fixed error handler (okta#366)

* Fixed error handler

* Removed validation for 'single_logout_issuer'

* Bump actions/stale from v3.0.17 to v3.0.18 (okta#371)

Bumps [actions/stale](https://github.com/actions/stale) from v3.0.17 to v3.0.18.
- [Release notes](https://github.com/actions/stale/releases)
- [Commits](actions/stale@v3.0.17...3b3c3f0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Co-authored-by: bogdanprodan-okta <71279414+bogdanprodan-okta@users.noreply.github.com>
Co-authored-by: Tom Goodsell <ymylei@users.noreply.github.com>
Co-authored-by: Bogdan Prodan <bogdan.prodan@okta.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Brian Zoetewey <brian.zoetewey@cru.org>
Co-authored-by: Marius Sturm <marius@graylog.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-investigation Needs further investigation waiting-response Waiting on collaborator to responde to follow on disucussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants