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

azuread_application bug: oauth2_permission_scope & app_role duplicate id #963

Closed
tsanton opened this issue Jan 6, 2023 · 9 comments · Fixed by #971
Closed

azuread_application bug: oauth2_permission_scope & app_role duplicate id #963

tsanton opened this issue Jan 6, 2023 · 9 comments · Fixed by #971

Comments

@tsanton
Copy link
Contributor

tsanton commented Jan 6, 2023

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritise this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritise the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform (and AzureAD Provider) Version

Terraform v1.3.2
on linux_amd64
+ provider registry.terraform.io/hashicorp/azuread v2.31.0

Affected Resource(s)

  • azuread_application

Debug Output

$ terraform apply

Error: checking for duplicate app roles / OAuth2.0 permission scopes: validation failed: duplicate ID found: "c21c916a-31e1-3a42-d62a-21fc26652602"

Expected Behavior

Terraform should be able to create/update an app registration to contain both and app_role (ar) and a oauth2_permission_scope (ops) given that:

  • ops.admin_consent_description == ar.description
  • ops.admin_consent_display_name == ar.display_name
  • ops.enabled == ar.enabled
  • ops.origin == ar.enabled (abstracted away, but defaults to "Application"(?))
  • ops.value == ar.value

Actual Behavior

Terraform errors out with duplicate id warning.

Important Factoids

Below you'll find a redacted version of my application manifest. I manually altered and added the app_role with displayName == "Read access", enabling both delegated and application auth flows to access my API with the correct permission (scp & roles).

The root cause of the problem seem to come from applicationValidateRolesScopes:

It's not enough to validate unique id's; we must at minimum allow overlapping id & value pairs in both slices.
Ideally(?) we should validate that all attributes are equal between scope and role on matching id & value. Don't have docs on this (and haven't tested different display names and description values) as I blindly trusted this SO answer (and it worked).

Further we must update the documentation for the azuread_application. It claims that:

In Azure Active Directory, application roles (app_role) and permission scopes (oauth2_permission_scope) exported by an application share the same namespace and cannot contain duplicate values

This is not the case. Given the condition that all common fields in both objects match they can indeed overlap.

{
	"id": "xxxxxxxx-7c2a-4943-8cd7-xxxxxxxxxxxx",
	"acceptMappedClaims": true,
	"accessTokenAcceptedVersion": 2,
	"addIns": [],
	"allowPublicClient": false,
	"appId": "xxxxxxxx-7c59-48ee-a2a3-xxxxxxxxxxxx",
	"appRoles": [
		{
			"allowedMemberTypes": [
				"Application",
				"User"
			],
			"description": "API read access",
			"displayName": "Read access",
			"id": "c21c916a-31e1-3a42-d62a-21fc26652602",
			"isEnabled": true,
			"lang": null,
			"origin": "Application",
			"value": "read"
		}
	],
	"oauth2AllowUrlPathMatching": false,
	"oauth2AllowIdTokenImplicitFlow": false,
	"oauth2AllowImplicitFlow": false,
	"oauth2Permissions": [
		{
			"adminConsentDescription": "API read access",
			"adminConsentDisplayName": "Read access",
			"id": "c21c916a-31e1-3a42-d62a-21fc26652602",
			"isEnabled": true,
			"lang": null,
			"origin": "Application",
			"type": "User",
			"userConsentDescription": "Allow the application to read from API on your behalf.",
			"userConsentDisplayName": "API Read access",
			"value": "read"
		}
	],
	"oauth2RequirePostResponse": false,
	"preAuthorizedApplications": [
		{
			"appId": "xxxxxxxx-96c5-4f38-8533-xxxxxxxx",
			"permissionIds": [
				"c21c916a-31e1-3a42-d62a-21fc26652602"
			]
		}
	],
	"replyUrlsWithType": [
		{
			"url": "https://XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX.com/fed/login",
			"type": "Web"
		}
	]
}
@manicminer
Copy link
Contributor

@tsanton Thanks for looking into this, and for the PR. Sometimes a StackOverflow and self-experimentation is all we have to go on. I agree the provider isn't quite right here and the probable correct approach is to compare the IDs and values, since as you say at a minimum these are allowed to be the same across a role/scope combination.

@tsanton
Copy link
Contributor Author

tsanton commented Jan 11, 2023

Hi @manicminer!

I discovered that the assumption was wrong when testing the provider fix by creating an application without all matching fields.
The error message (400 response) I received from the API was as follows:

The following values must match for the 'oauth2Permissions' and 'appRoles' properties with identifier '<UUID>': (description, adminConsentDescription), (displayName, adminConsentDisplayName),(isEnabled,isEnabled),(origin, origin),(value, value). Ensure that you are intending to have entries with the same identifier, and if so, are updating them together

Therefor I expanded the validation to compare these values -> I also added the 400 response as the error message in the provider.

@manicminer
Copy link
Contributor

@tsanton Ah ok, that's great thanks for updating it. I'll review the PR shortly 👍

@manicminer manicminer added this to the v2.32.0 milestone Jan 11, 2023
@manicminer
Copy link
Contributor

@tsanton Just checking whether you were able to update the PR to validate across all fields? Thanks.

@tsanton
Copy link
Contributor Author

tsanton commented Jan 12, 2023

@manicminer I implemented the initial PR with validation across (almost) all fields.

Here is the struct that I'm utilising for validation.

type appPermission struct {
		id          string
		displayName string
		description string
		enabled     bool
		value       string
	}

I've left out the scope.origin == role.origin because of:

Specifies if the app role is defined on the application] object or on the servicePrincipal entity. Must not be included in any POST or PATCH requests. Read-only.

docs: https://learn.microsoft.com/en-us/graph/api/resources/approle?view=graph-rest-1.0#properties

Would you like me to add the origin field to the validation?

@manicminer
Copy link
Contributor

I re-read the diff and I see now, I missed the DeepEqual comparison :)

We can ignore the origin field for validation purposes.

@tsanton
Copy link
Contributor Author

tsanton commented Jan 12, 2023

Just checked: the origin field is not part of neither the appRoles []interface{} nor the oauth2Permissions []interface{} validation input.

With other words: the PR is complete :)

@github-actions
Copy link

This functionality has been released in v2.32.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!

@github-actions
Copy link

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants