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: validate values for app_role and oauth2_permissions to screen for duplicates #287

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

manicminer
Copy link
Contributor

Roles (appRoles) and scopes (oauth2Permissions) exposed by an application share a common namespace where the values cannot overlap. This change adds some local validation to screen for these before PATCHing an application.

Changes the resulting error message from this:

Error: patching Azure AD Application with ID "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee": graphrbac.ApplicationsClient#Patch: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Request_BadRequest","date":"2020-06-09T10:06:21","message":{"lang":"en","value":"Request contains a property with duplicate values."},"requestId":"dab8185c-239f-44dd-8ca3-8bd13e3e67b3","values":[{"item":"PropertyName","value":"oauth2Permissions"},{"item":"PropertyErrorCode","value":"DuplicateValue"}]}}]

to this:

Error: validation failed: duplicate app_role / oauth2_permissions value found: "administer"

Closes: #269

@manicminer manicminer added this to the v0.11.0 milestone Jun 26, 2020
@manicminer manicminer requested a review from a team June 26, 2020 03:08
@ghost ghost added the size/M label Jun 26, 2020
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the pr @manicminer, overall looks good, in addition to the one comment i've left inline i think it might also warrant a note in the docs? other then that LGTM 👍

azuread/resource_application.go Outdated Show resolved Hide resolved
@manicminer manicminer force-pushed the app/roles-scopes-dup-validation branch from d66aac3 to 6b7f698 Compare June 30, 2020 01:36
@ghost ghost added the documentation label Jun 30, 2020
@manicminer manicminer force-pushed the app/roles-scopes-dup-validation branch from d4fee77 to beb62b3 Compare June 30, 2020 01:51
@manicminer
Copy link
Contributor Author

@katbyte Thanks! Note added to resource documentation.

@manicminer
Copy link
Contributor Author

Test results:

Screenshot 2020-06-30 02 55 11

@manicminer manicminer merged commit 9b56e0c into master Jun 30, 2020
manicminer added a commit that referenced this pull request Jun 30, 2020
@manicminer manicminer deleted the app/roles-scopes-dup-validation branch July 1, 2020 22:57
@ghost
Copy link

ghost commented Jul 9, 2020

This has been released in version 0.11.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azuread" {
    version = "~> 0.11.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jul 30, 2020

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Jul 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azuread_application: DuplicateValue
2 participants