refactor(invitations): enable azure_invitation for non Admins

[bufferoverflow]

Closes #885

[manicminer]

Thanks for proposing this @bufferoverflow. Unfortunately I don't think we can simply skip over 403 errors as this would mask permissions errors and we have no way of knowing whether this is desirable or not for the user.

[bufferoverflow]

A agree, however the question is why is it necessary to modify the user after creating the invitation.

According to https://learn.microsoft.com/en-us/graph/api/invitation-post?view=graph-rest-1.0&tabs=http#response checking for a 201 after creating the invite at https://github.com/hashicorp/terraform-provider-azuread/blob/main/internal/services/invitations/invitation_resource.go#L145 should be sufficient.

Regarding Destroying the resource, there is no information within the docs that a user will be deleted. I think destroying a invitation should not delete the user.
Multiple people could invite the same person, so why shall the User be deleted when doing an destroy of the invite?

[manicminer]

Ah right, I see what you mean, on closer look this is entirely a consistency check. Whilst there's no functional reason for patching the new guest user, we do it because it can take some number of minutes for the user to be created from the invitation, and we need to know the user object was created before the resource returns - otherwise if the user is referenced or managed later in the same configuration, it will fail with a 404 error.

That is also a good point regarding user deletion from this resource. The plan is actually to merge this with the azuread_user resource, or at least roll this functionality into the azuread_user resource - that way we can try to better assert for consistency. Once this has been rolled into the azuread_user resource, we can simplify the azuread_invitation resource to remove the consistency check and the user deletion. In the meantime, I'm not sure that I'm comfortable ignoring this check as this would potentially break a bunch of practitioners - we might need to look at doing this in the next major version.

We should probably update the docs to clarify that User.ReadWrite.All is required at the moment.

[bufferoverflow]

The plan is actually to merge this with the azuread_user resource, or at least roll this functionality into the azuread_user resource - that way we can try to better assert for consistency. Once this has been rolled into the azuread_user resource, we can simplify the azuread_invitation resource to remove the consistency check and the user deletion.

Do you have somehow a timeline regarding this plan? I'm asking because I might to use the fork for a while if this does not happen within a reasonable time frame.

I'm not sure that I'm comfortable ignoring this check as this would potentially break a bunch of practitioners - we might need to look at doing this in the next major version.

I would prefer and recommend removing the User deletion already from the invite as it is misleading and not obvious that a User gets deleted using the invite.

I will investigate a bit on other options to check if a user is there instead of modifying it.

[bufferoverflow]

@manicminer What about using usersClient.Get instead of usersClient.Update to check the existence of a user during the creation process? Doing so would keep the behavior the same and regarding deletion I could contribute a doc change which mentions that the destroy will only delete the User if the executed with User.ReadWrite.All permission. Would that work for you?

[manicminer]

@bufferoverflow Unfortunately, the reason why a PATCH request is being sent is that GET requests for new resources tend to be cached on creation by the API so they are returned before they are replicated.

[AzCii]

Requiring User.ReadWrite.All permissions for inviting users is just not gonna work, it's way to many permissions.

If the patch cannot be replaced by a GET or by looking for the 201 response on the invite, I would suggest just making this optional.

It might not be ideal, but for me I would a lot better than now, where inviting users are not even possible without god permissions.

[bufferoverflow]

@manicminer I added some documentation and suggested two changes to the PR. Would appreciate your feedback on this.

[manicminer]

@bufferoverflow I appreciate the updates, and whilst I would definitely like to make the azuread_invitation resource require no more than the User.Invite.All role, unfortunately we can't really do it in this way (silently skipping a failed operation if the operator does not have sufficient privilege).

Before doing this, we'll need to add the current functionality into the azuread_user resource, where we can more reasonably expect that practitioners will have elevated privileges. However, whilst that prerequisite could potentially happen without introducing any breaking changes to that resource, the changes to the azuread_invitation resource would have to wait until v3.0 of the provider as this constitutes a regression from the current behavior. Specifically, we're make a reasonable attempt to assert consistency of the new user object, which can only be achieved by attempting to patch the user (because MS Graph returns a valid user object prior to backend replication), and requiring practitioners to move to the azuread_user resource in order to keep this behavior is a breaking change.

Accordingly, I'll have to mark this PR for v3.0.0 and we'll be able to look at getting it in at that point.

[bufferoverflow]

thanks for the update @manicminer !

right now we are using the code from this MR with a delay tweak:

resource "null_resource" "delay" {
  provisioner "local-exec" {
    command = "sleep 900" # 15 min delay as per https://powerusers.microsoft.com/t5/Building-Flows/Add-a-new-guest-user-created-with-Graph-to-an-O365-security/m-p/904737#M127403
  }
  triggers = {
    always_run = "${timestamp()}"
  }
}

resource "azuread_group_member" "example" {
  group_object_id  = "3acb2135-1111-aaaa-bbbb-c67c7f6490a1"
  for_each         = local.partners
  member_object_id = azuread_invitation.invite[each.key].user_id
  depends_on       = [null_resource.delay]
}

[manicminer]

@bufferoverflow In the interest of helping with project maintenance, and avoiding stale PRs and merge conflicts down the line, I'm going to go ahead and close this PR for the time being. I've added it to the v3.0 milestone, so that we can go through and revisit former/closed PRs when we get closer to the time. Appreciate this suggestion, we'll circle back as we get closer to v3.0.