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 support to manage access packages in identitygovernance #903

Merged
merged 17 commits into from
Apr 13, 2023

Conversation

stanleyz
Copy link
Contributor

@stanleyz stanleyz commented Oct 7, 2022

Resources added:

  1. Access package catalog (azuread_access_package_catalog)
  2. Access package (azuread_access_package)
  3. Access package assignment policy (azuread_access_package_assignment_policy)
  4. Add resources into catalog (only AadGroup tested) (azuread_access_package_resource_catalog_association)
  5. Add resource roles to package (only AadGropu tested) (azuread_access_package_resource_package_association)

Data resource added:

  1. Access package catalog (azuread_access_package_catalog)
  2. Access package (azuread_access_package)

Issues:

  1. azuread_access_package_resource_catalog_association throws a panic when there is no matched resource in the specified catalog because of a bug in the upstream library, have raised a PR manicminer/hamilton #187 to fix this.
  2. There is no valid MS graph API to delete the resource roles from an access package, hence can't be deleted programatically.
  3. Unit tests and acceptance tests passed except for the above two issues.
  4. No documentation just yet.

Minor:

  1. When testing azuread_access_package_assignment_policy against my MS developer environment, randomly some users and groups in the test might not be ready before the policy is created, this could be caused by MS API throttling, not necessarily a bug in this code
  2. Personally I haven't use this code to create resources just yet, will do next week.

Closes: #218
Closes: #547

@aleksei-aikashev
Copy link

thank you for your work, we're eagerly waiting for this PR to be merged 🙏

@kaovd
Copy link

kaovd commented Oct 10, 2022

Nice one! Feel free to take anything from #612 - encountered some of the similar issues you've mentioned. I did a fair amount of tests and seemed alright but touch and go. Good luck actually getting your SDK changes sorted as any changes to identitygov have been in limbo for a long time as I'm sure you've seen from my blockers on my PR manicminer/hamilton#156

@stanleyz
Copy link
Contributor Author

Wrote a module to manage these resource this week, fixed a few issues. However the following resources are completely not working due to the issues mentioned in manicminer/hamilton#187, one of them is recently introduced, once these are fixed, the following resources should work as expected.

  1. Add resources into catalog (only AadGroup tested) (azuread_access_package_resource_catalog_association)
  2. Add resource roles to package (only AadGropu tested) (azuread_access_package_resource_package_association)

I guess it's ready to be merged from my view.

@stanleyz
Copy link
Contributor Author

stanleyz commented Oct 14, 2022

@manicminer @katbyte able to take a look at this? Thanks.

@manicminer
Copy link
Member

@stanleyz Thanks for the contribution, and for your patience! I will make some time to review this in the next week or so.

@kaovd
Copy link

kaovd commented Nov 8, 2022

@stanleyz As this PR has more features than my older draft PR I'll drop my PR for yours and and update the issue tracker :) - As mentioned in my issue did not originally set out to do the full set as wasn't sure if a dummy delete would be accepted, or whether to implement something that recreates the resource, while hacks and workarounds could be done on the user end its not ideal, but having this in would be better than nothing. Its pretty rare to want to delete sub level resource roles anyway unless you make a mistake, as you'd would just end up deleting the whole package.
Potentially if we could trigger a force recreate for the access package on that scenario but would be very weird behaviour as the depend tree wouldn't work like that

@stanleyz
Copy link
Contributor Author

stanleyz commented Nov 8, 2022

Thanks @kaovd , pretty much same reasoning as my thinking. Additionally although I haven't tested yet regarding force recreation of new access packages, I am not sure whether that will succeed in case there are already assignments on that policy.

@aleksei-aikashev
Copy link

oh, we're so ready to start using this one 😁

thank you @stanleyz @manicminer 🙏

@tomasmota
Copy link
Contributor

Hey @manicminer, any updates on when this might be reviewed?

@greg-paynter
Copy link

@manicminer any update on this ?

@mazlumtoprak
Copy link

mazlumtoprak commented Nov 30, 2022

Am I the correct assumption that this PR builds upon the BETA graph endpoint and therefore does not have an option to enable "autoassignments" ? I've also not seen any options to handle "auto-assignments" via Graph API on Beta and the attribute specificAllowedTargets.

I've managed to create those dynamic rules with the V1.0 API and they seem to have a few differences. With the AllowSpecificTargets I could manage those "autoassignments". Is This provider going forward building on top of beta so we can't build it within a timely manner and without changes to the github.com/manicminer/hamilton/msgraph package? Are there any similar features I'm missing in the beta maybe?

All the other functionalities are working as expected and are awesome!

Copy link

@mazlumtoprak mazlumtoprak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo in flattenApprovalSettings

@stanleyz
Copy link
Contributor Author

stanleyz commented Nov 30, 2022

Am I the correct assumption that this PR builds upon the BETA graph endpoint and therefore does not have an option to enable "autoassignments" ? I've also not seen any options to handle "auto-assignments" via Graph API on Beta and the attribute specificAllowedTargets.

I've managed to create those dynamic rules with the V1.0 API and they seem to have a few differences. With the AllowSpecificTargets I could manage those "autoassignments". Is This provider going forward building on top of beta so we can't build it within a timely manner and without changes to the github.com/manicminer/hamilton/msgraph package? Are there any similar features I'm missing in the beta maybe?

All the other functionalities are working as expected and are awesome!

Haha, it has been some time that I have forgotten some details, but we only use Beta API for accessPackageClient as shown below, the reason of using Beta API for access package is explained here.

	accessPackageCatalogClient := msgraph.NewAccessPackageCatalogClient(o.TenantID)
 	// Use beta version because it replies more info than v1.0
 	accessPackageClient := &msgraph.AccessPackageClient{
 		BaseClient: msgraph.NewClient(msgraph.VersionBeta, o.TenantID),
 	}
 	accessPackageAssignmentPolicyClient := msgraph.NewAccessPackageAssignmentPolicyClient(o.TenantID)
 	accessPackageResourceRoleScopeClient := msgraph.NewAccessPackageResourceRoleScopeClient(o.TenantID)
 	accessPackageResourceRequestClient := msgraph.NewAccessPackageResourceRequestClient(o.TenantID)
 	accessPackageResourceClient := msgraph.NewAccessPackageResourceClient(o.TenantID)

when you say specificAllowedTargets, do you mean SpecificDirectorySubjects in the assignment policy? It's supported if yes.

@mazlumtoprak
Copy link

mazlumtoprak commented Dec 1, 2022

Am I the correct assumption that this PR builds upon the BETA graph endpoint and therefore does not have an option to enable "autoassignments" ? I've also not seen any options to handle "auto-assignments" via Graph API on Beta and the attribute specificAllowedTargets.
I've managed to create those dynamic rules with the V1.0 API and they seem to have a few differences. With the AllowSpecificTargets I could manage those "autoassignments". Is This provider going forward building on top of beta so we can't build it within a timely manner and without changes to the github.com/manicminer/hamilton/msgraph package? Are there any similar features I'm missing in the beta maybe?
All the other functionalities are working as expected and are awesome!

Haha, it has been some time that I have forgotten some details, but we only use Beta API for accessPackageClient as shown below, the reason of using Beta API for access package is explained here.

	accessPackageCatalogClient := msgraph.NewAccessPackageCatalogClient(o.TenantID)
 	// Use beta version because it replies more info than v1.0
 	accessPackageClient := &msgraph.AccessPackageClient{
 		BaseClient: msgraph.NewClient(msgraph.VersionBeta, o.TenantID),
 	}
 	accessPackageAssignmentPolicyClient := msgraph.NewAccessPackageAssignmentPolicyClient(o.TenantID)
 	accessPackageResourceRoleScopeClient := msgraph.NewAccessPackageResourceRoleScopeClient(o.TenantID)
 	accessPackageResourceRequestClient := msgraph.NewAccessPackageResourceRequestClient(o.TenantID)
 	accessPackageResourceClient := msgraph.NewAccessPackageResourceClient(o.TenantID)

when you say specificAllowedTargets, do you mean SpecificDirectorySubjects in the assignment policy? It's supported if yes.

Thanks for the explanation and the link. I mean specificAllowedTargets, which is available in the v1.0 API of https://graph.microsoft.com/v1.0/identityGovernance/entitlementManagement/assignmentPolicies. In the Entra Portal it's this option, that can be set through the V1.0 API (it has not the same endpoint name though):
image

As the schema of V1.0 and Beta are quite different, I also don't think that it's applicable to just change the client.

@manicminer manicminer modified the milestones: v2.31.0, v2.32.0 Dec 1, 2022
@tomasmota
Copy link
Contributor

Not sure how these are meant to be handled, but I have had problems doing the following:

  • Adding a new ad group as a resource in a catalog, complains that group does not exist. This only happens sometimes
  • Removing group as resource in catalog, right after deleting access package containing that group. Complains that there are active entitlements. This seems to happen almost every time.

For both of these errors, re-running terraform apply fixes the issue. Below is an example of an error when removing the group from the catalog:
image

@stanleyz
Copy link
Contributor Author

stanleyz commented Jan 9, 2023

  • Removing group as resource in catalog, right after deleting access package containing that group. Complains that there are active entitlements. This seems to happen almost every time.

Yes, this requires running Terraform destroy twice to delete the association because there is MS API to delete the association between the access package and group, so it relies on the access package to deleted first, then the group can be removed from a catalog.

Adding a new ad group as a resource in a catalog, complains that group does not exist. This only happens sometimes

This is a bit weird, I never had this issue in my use, might need take a look further with detailed logs.

@stanleyz
Copy link
Contributor Author

stanleyz commented Jan 9, 2023

Thanks for the explanation and the link. I mean specificAllowedTargets, which is available in the v1.0 API of https://graph.microsoft.com/v1.0/identityGovernance/entitlementManagement/assignmentPolicies. In the Entra Portal it's this option, that can be set through the V1.0 API (it has not the same endpoint name though): image

Right, it doesn't seem the upstream SDK supports this yet, see below code.
https://github.com/manicminer/hamilton/blob/f4755d0ebb4a31635e33557b2e65db8e12cc5d97/msgraph/models.go#L30

As the schema of V1.0 and Beta are quite different, I also don't think that it's applicable to just change the client.

As said, the policy uses v1.0 API, only the access package resource uses beta.

@manicminer manicminer modified the milestones: v2.32.0, v2.33.0 Jan 12, 2023
@sikksakk
Copy link

Pretty please 😊

@alexwilcox9
Copy link
Contributor

As commenting on the thread emails everyone who is subscribed to the topic can we keep comments on topic rather than asking for status updates?

It's a pain having to wait but asking won't make it come quicker

(Putting a thumbs up on the PR itself is a good way to show your interest in the feature)

@manicminer
Copy link
Member

Test results

Screenshot 2023-04-13 at 17 04 37

Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @stanleyz for working on and submitting this - this addition is highly appreciated especially with working out how best to map these into Terraform resources. I'm happy with the design and the high quality of the implementation meant that I did not have to rework anything.

Apologies to all subscribers for the delay in getting to this, I had hoped to get this in awhile back. I've worked on this the last couple days to get it over the line, the tests look good and it's now ready to go.

Many thanks @kaovd, @mazlumtoprak, @tomasmota, @arsatiki, @ccadruvi, @cedrox for testing and reviewing. And @aleksei-aikashev for the encouragement.

@manicminer manicminer merged commit 6f4c690 into hashicorp:main Apr 13, 2023
9 checks passed
manicminer added a commit that referenced this pull request Apr 13, 2023
@github-actions
Copy link

This functionality has been released in v2.37.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet