Add Resources for Claims Mapping Policy Management

[computeracer]

Adds support for the claims mapping policy and claims mapping policy assignment resources so these can be
managed with Terraform. This depends on the hamilton sdk support being merged first and then this branch can be updated with the correct vendor dependencies.

Related to:

• ✨ Add ClaimsMappingPolicy manicminer/hamilton#147
• Support for managing claims mapping policies (azuread_claims_mapping_policy, azuread_claims_mapping_policy_assignment) #644
• https://docs.microsoft.com/en-us/graph/api/resources/claimsmappingpolicy?view=graph-rest-1.0
• https://docs.microsoft.com/en-us/graph/api/serviceprincipal-post-claimsmappingpolicies?view=graph-rest-1.0&tabs=http

[webframp]

Very open to redesigning the interface of this one based on feedback, we're beginning to test this already with some internal use cases

[manicminer]

Hi @computeracer, thanks again for your work on this and in the SDK. Your upstream PR is now merged, released and updated in the provider, so you can rebase this onto main and it should then be able to pass.

I've made a preliminary review, if you can take a look at the items below and rebase the branch, I'll be happy to take another look with a view to merging. Thanks!

[manicminer]

To avoid repetition, we can substitute a config here from ServicePrincipalClaimsMappingPolicy (e.g. basicClaimsMappingPolicy)

[webframp]

In principle I completely understand the need to dedupe stuff like this for future maintainability, so to make sure I understand correctly here, you want to re-use the test definition from here https://github.com/hashicorp/terraform-provider-azuread/pull/733/files#diff-673ecbaaaf4f1fe043eda4ebc250325ec41d602d5992220d3b2db8f6bf2fa1d4R42 ?

Only asking to clarify because in the basicClaimsMappingPolicy case it's used for the basic test while here in updateClaimsMappingPolicyAssignment it's used for the update test.

Should we extract out the single resource block for re-use as a constant string (or similar) or did you have some other way to re-use in mind? Do you know of other places in the tests where this has been done so we'd have an example? We want to make sure this gets updated the way you're thinking.

[computeracer]

Thank you for the good feedback. I updated the other concerns. I'll try to look at this one Monday.

[computeracer]

When working through the refactoring I found our tests were broken.
Seems they were broken even before I began to work through the suggestions.
After doing some debugging I found the odata.ID is nil
when it goes to post to msgraph.

(dlv) locals
status = 0
policy = github.com/manicminer/hamilton/msgraph.ClaimsMappingPolicy {DirectoryObject: (*"github.com/manicminer/hamilton/msgraph.DirectoryObject")(0xc000270268), Definition: *[]string nil, Description: *string nil,...+2 more}
checkPolicyAlreadyExists = github.com/manicminer/hamilton/msgraph.(*ServicePrincipalsClient).AssignClaimsMappingPolicy.func1
(dlv) print policy.ID
*"ssssssss-ssss-ssss-ssss-ssssssssss"
(dlv) ls
> github.com/manicminer/hamilton/msgraph.(*ServicePrincipalsClient).AssignClaimsMappingPolicy() ./vendor/github.com/manicminer/hamilton/msgraph/serviceprincipals.go:371 (PC: 0xbddb07)
   366:                                 return o.Error.Match(odata.ErrorAddedObjectReferencesAlreadyExist)
   367:                         }
   368:                         return false
   369:                 }
   370:
=> 371:                 body, err := json.Marshal(DirectoryObject{ODataId: policy.ODataId})
   372:                 if err != nil {
   373:                         return status, fmt.Errorf("json.Marshal(): %v", err)
   374:                 }
   375:
   376:                 _, status, _, err = c.BaseClient.Post(ctx, PostHttpRequestInput{

This field is needed according to
https://docs.microsoft.com/en-us/graph/api/serviceprincipal-post-claimsmappingpolicies?view=graph-rest-1.0&tabs=http#request-body

I wonder if we missed something when we applied this suggestion
manicminer/hamilton#147 (comment)

Any ideas on what we may have missed @manicminer?

I think we found the issue. The original code referenced in the previous discussion was building the odata.ID based on the ID manually, where the applied suggestion no longer did that. By reading the claims mapping policy in the terraform assignment create function, we get a full claims mapping policy id with the odata id properly set. Now to try the refactor 😄

[computeracer]

@manicminer I eliminated duplication with the create function for the assignment, but kept the 2nd one so simulate the update. Please let me know what you think at this point. Thank you again for your reviews.

[manicminer]

Hey @computeracer, @webframp, sorry for the delay in circling back on this. Thanks again for your work on this and for making the changes. This is mostly looking good; I've refactored it a little to split the azuread_claims_mapping_policy resource out into its own service package, and renamed the assignment resource to reflect that it's in the serviceprincipals package, along with some test fixes.

I was unable to push to your fork, so in the interest of expedience I reopened this as #766, preserving your original commits. This will go out in a release later today. Thanks again!

[webframp]

You're awesome @manicminer, thanks for the work! If we can improve anything about the way we contribute here I'd love to chat about it on slack sometime

[manicminer]

@webframp Thanks, this was a really great contribution. Feel free to open PRs early and often for feedback, and reach out any time on Slack :)

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.