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_registration can't be destroyed by owner if using azuread_application_owner #1335

Open
agyss opened this issue Mar 14, 2024 · 5 comments

Comments

@agyss
Copy link

agyss commented Mar 14, 2024

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

azuread 2.47.0

Affected Resource(s)

  • azuread_application_registration
  • azuread_application_owner

Terraform Configuration Files

resource "azuread_application_registration" "example" {
  display_name = "example"
}

resource "azuread_user" "jane" {
  user_principal_name = "[email protected]"
  display_name        = "Jane Fischer"
  password            = "Ch@ngeMe"
}

resource "azuread_application_owner" "example_jane" {
  application_id  = azuread_application_registration.example.id
  owner_object_id = azuread_user.jane.object_id
}

data "azurerm_client_config" "current" {}

resource "azuread_application_owner" "example_current" {
  application_id  = azuread_application_registration.example.id
  owner_object_id = data.azurerm_client_config.current.object_id, # current user

}

Expected behaviour

If the user who is performing a terraform destory on all objects above is in the owners list and does not have higher privileges (like global admin), Terraform should realize that it can't destroy the owner object as it's permissions are required to delete the application. This would result in the same behaviour as in azure cli or the azure portal.

Of course this is only true if the destroy action should delete the application_registration as well and not terraform --target=ownerobject is used.

Actual behaviour

The owner is removed from the application_registration before the application itself is deleted, leading to a 403 error as the user no longer has authorization to do so (if not assigned higher permissions like global admin).

@agyss
Copy link
Author

agyss commented Mar 14, 2024

Workaround

As I don't need the --terraform --target=ownerobject to destroy assignments (i.e., I accept I have to remove the assignments manually when necessary) I use this workaround to achieve desired functionality:

resource "terraform_data" "app_owners" {
  input = {
    app_object_id = azuread_application_registration.example.object_id
    owner_id = azuread_user.jane.object_id
  }

  provisioner "local-exec" {
    when = create
    command = "az ad app owner add --id ${self.input.app_object_id} --owner-object-id ${self.input.owner_id}"
  }
}

@manicminer
Copy link
Contributor

Thanks for reporting @agyss. Unfortunately, the wider context of the destroy operation is entirely unavailable to providers due to the way Terraform works. This means there is no way for us to know which resources are going to be destroyed. The most appropriate way t resolve these kinds of dependency issues is by manipulating the graph itself - options include the lifecycle meta argument and/or targeted destroys. Sadly because Terraform does not support different graph ordering between create and destroy operations, there is no config-based solution to this problem.

It might be possible to fudge this inside the provider, but it requires some investigation and I'm not very confident that it's possible without breaking the expected contract and negatively affecting many users. Ultimately this should probably be a feature request of Terraform Core.

@agyss
Copy link
Author

agyss commented Mar 14, 2024

I see. In this case I would propose to invent a boolean field named only_destroy_with_parent and a use_existing. The first one if set to true effectively ignores deletes while the use_existing if set to true does not throw an error if the resource already exists but "imports" it (like done with https://registry.terraform.io/providers/hashicorp/azuread/latest/docs/resources/service_principal use_existing`).

The combination of both pretty much has the same effect as the workaround suggested before.

@manicminer
Copy link
Contributor

Just a thought - do you really need the azuread_application_owner resource here? Typically when you create an app registration, using app credentials authorized via a service principal holding only the Application.ReadWrite.OwnedBy app role, the newly created application is automatically assigned the calling principal as the sole owner. I've just tested this to make sure this is still the case and it does appear to be.

This is one of the reasons the azuread_application_registration resource explicitly does not manage owners - so that the auto-assigned owner can be kept intact with the express intent of avoiding a scenario like the one you find yourself in. Have you tried just omitting the azuread_application_owner resource altogether?

@drdamour
Copy link

@manicminer in our flow we let users run terraform locally or in the pipeline. when using azuread_application we set the owner to be the pipeline SP and a set of users we let "own" that application. that group set changes and we always want to let the SP be an owner...so it's both important that we can set a SP to own and users to own in case the creator doesn't happen to be the SP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants