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

Don't call /directoryObjects for applications or service principals #713

Merged
merged 2 commits into from
Jan 14, 2022

Conversation

manicminer
Copy link
Member

@manicminer manicminer commented Jan 10, 2022

No longer retrieve owner principals when managing applications or service principals. Since we no longer use the @odata.id returned by the API, it's not necessary to retrieve owner principals via the /directoryObjects endpoint. This can sometimes require additional permissions, which we did not document, and which is undesirable in any case.

Additionally, document additional potentially required permissions for groups as we can't get around this when managing groups, due to necessarily needing to distinguish between different types of owner principals.

Fixes: #703

Test results

Screenshot 2022-01-13 at 13 05 57

Screenshot 2022-01-13 at 12 51 05

…ssions

Since we no longer use the @odata.id returned by the API, it's not
necessary to retrieve owner principals via the /directoryObjects
endpoint. This can sometimes require additional permissions, which we
did not document, and which is undesirable in any case.
@tsologub
Copy link

I do not know yet why, but this doesn't work for me. I receive the same:

Error: Could not create application
│
│   with azuread_application.app_taras-playground,
│   on playground.tf line 16, in resource "azuread_application" "app_taras-playground":
│   16: resource "azuread_application" "app_taras-playground" {
│
│ ApplicationsClient.BaseClient.Post(): unexpected status 403 with OData error: Authorization_RequestDenied: Insufficient privileges to complete the operation.

Testing with the following declaration:

terraform {
  required_providers {
    azuread = {
      source = "registry.terraform.io/hashicorp/azuread"
    }
  }
}

# Has the following permissions: Application.ReadWrite.OwnedBy, User.Read
provider "azuread" {
  client_id     = "id"
  client_secret = "secret"
  tenant_id     = "id"
}

resource "azuread_application" "app_taras-playground" {
  display_name            = "taras-playground"
  owners                  = [
    "4ad0e09f-a951-4946-90a2-dacddea52079", # object-id of the SPN from above
    "3c422d16-cebe-4f1f-a282-2163a8ca3dcf"  # my object-id
  ]
  prevent_duplicate_names = true
  web {
    redirect_uris = ["https://taras-playground/"]
  }
}

resource "azuread_service_principal" "spn_taras-playground" {
  application_id               = azuread_application.app_taras-playground.application_id
  app_role_assignment_required = true
  owners                       = [
    "4ad0e09f-a951-4946-90a2-dacddea52079", # object-id of the SPN from above
    "3c422d16-cebe-4f1f-a282-2163a8ca3dcf"  # my object-id
  ]
}

resource "azuread_service_principal_password" "taras-playground" {
  service_principal_id = azuread_service_principal.spn_taras-playground.id
}

@tsologub
Copy link

Correct me if I am wrong, when we perform create application and pass owners = [...] explicitly, the MSFT Graph API will perform add owner behind the scenes for every owner from our owners array. And according to the Add Owner permissions section Required Application.ReadWrite.OwnedBy and Directory.Read.All. Note: Application.ReadWrite.OwnedBy will not be sufficient to add another owner.

@manicminer
Copy link
Member Author

manicminer commented Jan 13, 2022

@tsologub There are two ways the provider assigns owners - when creating it will include up to 20 owners in the POST to /applications to create the app, and when updating (or when there are >20 owners to assign at creation) it will POST to the /applications/GUID/owners endpoint.

When you get the 401 error, does Terraform prefix the error output with "Could not create application", or "Could not add owners to application"? That will indicate which of the two calls are failing.

The docs state that Application.ReadWrite.All is required to create applications too, but this isn't always the case. So IMO it's not clear whether that same role is needed to append owners or not, since many operations on applications do work with Application.ReadWrite.OwnedBy. The effective permissions are also very different when authenticating as a user versus as a service principal - I'm assuming you're doing the latter as we're talking about app roles?

@tsologub
Copy link

When you get the 401 error, does Terraform prefix the error output with "Could not create application", or "Could not add owners to application"?

In my case, I have two owners included. So the 2022-01-11T14:43:15.918+0100 [ERROR] vertex "azuread_application.app_taras-playground" error: Could not create application first one is failing.

The effective permissions are also very different when authenticating as a user versus as a service principal - I'm assuming you're doing the latter as we're talking about app roles?

Yes, I am authenticating with the service principal.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@manicminer manicminer merged commit 6406b23 into main Jan 14, 2022
@manicminer manicminer deleted the bugfix/user-permissions-for-app-sp-resources branch January 14, 2022 00:16
manicminer added a commit that referenced this pull request Jan 14, 2022
@github-actions
Copy link

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

@manicminer manicminer restored the bugfix/user-permissions-for-app-sp-resources branch January 17, 2022 12:40
@manicminer manicminer deleted the bugfix/user-permissions-for-app-sp-resources branch January 17, 2022 12:40
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2022
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.

DirectoryObjects.BaseClient.Get() 403 during assigning the owners for azuread_application
3 participants