fix BadRequest error when trying to remove AADUser group memberships

[snunezMSFT]

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

[snunezMSFT]

There is an issue in the code with $user.id on the function Set-TargetResource,

$user is created in line 425 as a result of calling Get-TargetResource without Id

Depending on the if in line 505, $user gets updated on line 561. In this case, if updated it will have the Id property

When changing group membership, the operation will fail if the Id property is not updated.

[ykuijs]

Hi @snunezMSFT, thanks for submitting this PR. Could you please also add an entry to the changelog, so we can keep track of all changes in a version?

[snunezMSFT]

Thanks Yorik, I have updated the changelog adding one line describing the issue fixed.

[ricmestre]

The problem here is the missing ' in the filter when calling Get-MgGroup, why are you making additional requests to Graph to call Get-MgUser?

New-MgUser is supposed to return the Id of the just created object, if that's not the case and the reason behind why you call Get-MgUser then that it's an additional issue to the one that you originally reported and should be addressed separately or at least documented.

[ricmestre]

I see, that works when it's a new user it works but not for other cases.

It's better if you instead return the Id of the user in Get-TargetResource and add it as parameter, that way you don't have to make additional requests to Graph to find out the Id.

[snunezMSFT]

Thanks @ricmestre, yes, as you stated the issue is that the code works when there is a new user, but not for existing users.

I have added the Get-MgUser as I see it later in the code for managing RoleMemberships around line 679.
Adding the Id of the user in Get-TargetResource is another option, but I am not sure about the impact of changing it, maybe it is going to affect other Get-TargetResource calls. This function is called in Test-TargetReource and Export-TargetResource, adding the Id (that is tenant dependant) can affect these scenarios.

Calling Get-MgUser everytime is less efficient, but it has less impact in the existing behavior. One improvement is to call it earlier in the code and save the value in $userId. So if it is a new user, the $userId will be $user.Id, when it is an existing user, then $userId is the result for calling one time Get-MgUser

[snunezMSFT]

To avoid multiple calls to Get-MgUser as @ricmestre has suggested, I have changed the code for using $userId for User Id when changing role and group membership in Set-TargetResource function. Role changes already had the call for Get-MgUser (now redundant). Group changes were missing the Id if the user is not new