-
Notifications
You must be signed in to change notification settings - Fork 26
fix: Merge Managed and Unmanaged Users (joining the existing_sso_users club)
#60
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
base: main
Are you sure you want to change the base?
Conversation
|
This simpler attempt should hopefully clear all checks. |
|
/do-e2e-tests |
|
End to end test has been scheduled |
|
E2E tests in progress |
1 similar comment
|
E2E tests in progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E2E test has completed with errors. If you are an external contributor, please contact the project maintainers for more information.
|
Hi @alexandertgtalbot, thanks for the PR! Were you able to do a successful run of This is the line causing the error: for group in local.sso_users[this_user].group_membershipThe error stems from the example configuration in module "aws-iam-identity-center" {
source = "../.." // local example
# source = "aws-ia/iam-identity-center/aws" // remote example
# Ensure these User/Groups already exist in your AWS account
existing_sso_groups = {
testgroup : {
group_name = "testgroup" # this must be the name of a group that already exists in your AWS account
},
}
existing_sso_users = {
testuser : {
user_name = "testuser" # this must be the name of a user that already exists in your AWS account
},
}
# Create permissions sets backed by AWS managed policies
permission_sets = {
AdministratorAccess = {
description = "Provides AWS full access permissions.",
session_duration = "PT4H", // how long until session expires - this means 4 hours. max is 12 hours
aws_managed_policies = ["arn:aws:iam::aws:policy/AdministratorAccess"]
tags = { ManagedBy = "Terraform" }
},
ViewOnlyAccess = {
description = "Provides AWS view only permissions.",
session_duration = "PT3H", // how long until session expires - this means 3 hours. max is 12 hours
aws_managed_policies = ["arn:aws:iam::aws:policy/job-function/ViewOnlyAccess"]
tags = { ManagedBy = "Terraform" }
},
}
# Assign users/groups access to accounts with the specified permissions
# Ensure these User/Groups already exist in your AWS account
account_assignments = {
testgroup : {
principal_name = "testgroup"
principal_type = "GROUP"
principal_idp = "EXTERNAL"
permission_sets = ["AdministratorAccess", "ViewOnlyAccess", ]
account_ids = [ // account(s) the user will have access to. Permissions they will have in account are above line
local.account1_account_id, // locals are used to allow for global changes to multiple account assignments
# local.account2_account_id, // if hard coding the account ids, you would need to change them in every place you want to change
# local.account3_account_id, // these are defined in a locals.tf file, example is in this directory
# local.account4_account_id,
]
},
testuser : {
principal_name = "testuser"
principal_type = "USER"
principal_idp = "EXTERNAL"
permission_sets = ["ViewOnlyAccess"]
account_ids = [ // account(s) the user will have access to. Permissions they will have in account are above line
local.account1_account_id, // locals are used to allow for global changes to multiple account assignments
# local.account2_account_id, // if hard coding the account ids, you would need to change them in every place you want to change
# local.account3_account_id, // these are defined in a locals.tf file, example is in this directory
# local.account4_account_id,
]
},
}
}In this scenario,
With that being said, I don't believe we should merge the managed ( The only use case I see for this is for existing users/groups that were manually (or otherwise) created outside of the module, but using Identity Store as the IdP. For these resource it is now desired to use these with the module. If this is the case, the way to achieve this would be the use the I haven't tested this, but if using the module "iam_identity_center" {
source = "aws-ia/iam-identity-center/aws"
version = "1.0.2"
# Define the users
sso_users = {
"john.doe" = {
user_name = "jdoe"
given_name = "John"
family_name = "Doe"
email = "[email protected]"
group_membership = ["developers", "operations"]
}
"jane.smith" = {
user_name = "jsmith
given_name = "Jane"
family_name = "Smith"
email = "[email protected]"
group_membership = ["developers", "operations"]
}
}
# Define the groups
sso_groups = {
"developers" = {
display_name = "Developers"
description = "Development team"
}
"operations" = {
display_name = "Operations"
description = "Operations team"
}
}
# ... rest of your module configuration (permission sets, assignments, etc.)
}
# Import existing users
import {
to = module.iam_identity_center.aws_identitystore_user.this["john.doe"]
id = "d-1234567890/user-id-for-john" # Format: "identity-store-id/user-id"
}
import {
to = module.iam_identity_center.aws_identitystore_user.this["jane.smith"]
id = "d-1234567890/user-id-for-jane"
}
# Import existing groups
import {
to = module.iam_identity_center.aws_identitystore_group.this["developers"]
id = "d-1234567890/group-id-for-developers" # Format: "identity-store-id/group-id"
}
import {
to = module.iam_identity_center.aws_identitystore_group.this["operations"]
id = "d-1234567890/group-id-for-operations"
}Does this seem to achieve what you're looking for? |
|
That's some great feedback @novekm, appreciate all the detail/context provided. Let me rework my attempt. |
- The input variable `existing_sso_users`, while present, was previsously unused. This includes it into a new merged collection that then updates managed as well as unmanaged user memberships.
- Add coalesce to merges so as to provide default empty collections so avoiding null-based lookup/reference errors.
|
Another attempt please @novekm . I've added |
|
Hi @alexandertgtalbot, so to confirm is this meant to merge |
|
Hi @novekm, Sorry for the War-and-Peace-level response.... The intention here is to merge both collections supporting both classes of users whilst managing RBAC through IAM IDC group assignments. Scenario 1This is my real-world experience in implementing federated auth:
to
Scenario 2
as I'm not concerned with Google we'll consider this resource: iterates over which references: leaving
which on the face of it reads like it should work but only supports direct assignment to an account, not indirect through group membership.
like the Account example above should work fine but is direct assignment instead of group assigned. Wrap-up
as the making This update thus aims to extend a warm hug to our |
|
Just following up on this @novekm, any thoughts? |
|
Okay, I accept defeat :) For any future travellers finding their way here, here's a quick unblock/fix: https://registry.terraform.io/modules/refractive-space/iam-identity-center/aws/latest |
existing_sso_users, while present, was previsously unused. This includes it into a new merged collection that then updates managed as well as unmanaged user memberships.