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 TokenIssuancePolicy #215

Merged
merged 7 commits into from
Apr 3, 2023

Conversation

kenchan0130
Copy link
Contributor

This adds initial support for the TokenIssuancePolicy resource.

Represents the policy to specify the characteristics of SAML tokens issued by Azure AD. You can use token issuance policies to:

  • Set signing options
  • Set signing algorithm
  • Set SAML token version

I believe it will also be useful for adding resources to azure ad terraform in the future, as it will be used to configure the SAML signature algorithm, etc.

https://learn.microsoft.com/en-us/graph/api/resources/tokenissuancepolicy?view=graph-rest-1.0

Copy link
Owner

@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.

Hi @kenchan0130, thanks for contributing to this SDK! Overall this is looking great. I have just one comment about a method signature/behavior which is mainly about trying to be consistent between client methods. If you can take a look at this, this should be good to merge. Thanks!

msgraph/applications.go Outdated Show resolved Hide resolved
@manicminer manicminer added enhancement New feature or request package/msgraph labels Feb 13, 2023
@manicminer manicminer modified the milestones: v0.57.0, v0.58.0 Feb 21, 2023
@@ -778,3 +778,135 @@ func (c *ServicePrincipalsClient) AssignAppRoleForResource(ctx context.Context,

return &appRoleAssignment, status, nil
}

// AssignTokenIssuancePolicy assigns tokenIssuancePolicies to a service principal
func (c *ServicePrincipalsClient) AssignTokenIssuancePolicy(ctx context.Context, servicePrincipal *ServicePrincipal) (int, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@manicminer
As you have said in #215 (comment), I have made the interface like AssignClaimsMappingPolicy.

However, I would like to be sure that my current changes are not a problem for the overall design policy of ServicePrincipalClient, as it differs from your opinion.

Copy link
Owner

Choose a reason for hiding this comment

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

@kenchan0130 Thanks for updating. As per my earlier comment, I think it would be best to use a function signature like:

func (c *ServicePrincipalsClient) AssignTokenIssuancePolicy(ctx context.Context, servicePrincipalId string, TokenIssuancePolicy) (int, error) {
  // ...
}

or, if multiple policies can be assigned in one operation:

func (c *ServicePrincipalsClient) AssignTokenIssuancePolicy(ctx context.Context, servicePrincipalId string, []TokenIssuancePolicy) (int, error) {
  // ...
}

The reason we differ from this in the case of Owners, Members etc is that these are relational rather than child objects and as such they require the full OData ID to construct a link field. For an example of child objects have a look at the ApplicationsClient{}.AddPassword() method.

Secondly, for the question of whether this operation should happen for applications or service principals, I note that the docs only seem to cover the /applications/{id}/tokenIssuancePolicies endpoint - could it be that these are applicable to both /applications/{id}/tokenIssuancePolicies and /servicePrincipals/{id}/tokenIssuancePolicies? If so, it would be nice to add methods to both clients here to offers users the choice. But if this is a case of the docs being wrong, I am happy to defer to your experience here.

Copy link
Contributor Author

@kenchan0130 kenchan0130 Mar 2, 2023

Choose a reason for hiding this comment

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

@manicminer

Thanks for updating. As per my earlier comment, I think it would be best to use a function signature like

OK, I understand.
I changed interfaces bd28117.

Secondly, for the question of whether this operation should happen for applications or service principals, I note that the docs only seem to cover the /applications/{id}/tokenIssuancePolicies endpoint - could it be that these are applicable to both /applications/{id}/tokenIssuancePolicies and /servicePrincipals/{id}/tokenIssuancePolicies?

I have tried to apply a policy to the application as follows and found that this request results in an error.

curl 'https://graph.microsoft.com/v1.0/applications/a191c3d6-9371-446a-a4aa-647226be2f1b/tokenIssuancePolicies/$ref' \
  -H 'Authorization: Bearer xxxxx' \
  --data-raw '{"@odata.id":"https://graph.microsoft.com/v1.0/policies/tokenIssuancePolicies/8efb6001-1369-4c68-be2d-04182c8edd76"}'
{
    "error": {
        "code": "Request_BadRequest",
        "message": "Policy operations on v2 application are disabled.",
        "innerError": {
            "date": "2023-03-02T14:55:42",
            "request-id": "3ce11a1a-d63a-4100-9bf8-c83ca1666fbd",
            "client-request-id": "663c72ad-ccc3-2eef-38aa-3ea17b11a44b"
        }
    }
}

Therefore, I chose not to implement it for the application.

@kenchan0130
Copy link
Contributor Author

The fail in this test does not appear to be related to any change in my code.

=== RUN   TestConnectedOrganizationClient
    connectedorganization_test.go:68: ConnectedOrganizationClient.Create(): ConnectedOrganizationClient.BaseClient.Post(): unexpected status 400 with OData error: ConnectedOrganizationAlreadyExists: The connected organziation already exists.
--- FAIL: TestConnectedOrganizationClient (0.45s)

@kenchan0130 kenchan0130 force-pushed the patch-token-issuance-policy branch 2 times, most recently from 537c1b1 to e42673e Compare February 23, 2023 16:43
@kenchan0130
Copy link
Contributor Author

I rebased it to remove the conflict with the main branch.

@kenchan0130
Copy link
Contributor Author

@manicminer
I have responded to your change request.
Please review my code.

@kenchan0130
Copy link
Contributor Author

kenchan0130 commented Mar 22, 2023

@manicminer
How has the progress been coming along?
If you need any additional corrections, please let me know.

@manicminer manicminer self-requested a review March 23, 2023 15:01
Copy link
Owner

@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.

@kenchan0130 Many thanks for clarifying and updating. Sorry for the delay, this LGTM 🚀

@manicminer manicminer merged commit 0d9f379 into manicminer:main Apr 3, 2023
manicminer added a commit that referenced this pull request Apr 3, 2023
@kenchan0130 kenchan0130 deleted the patch-token-issuance-policy branch April 9, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request package/msgraph
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants