microsoftgraph / microsoft-graph-devx-content

Content used by the DevX API to enhance clients and tooling.
MIT License
22 stars 44 forks source link

Undocumented permissions for using AdditionalData to add group members #362

Open erikly opened 2 years ago

erikly commented 2 years ago

When adding users in bulk as in Example 2 (using AdditionalData) using Delegated Permissions, I get the following error

Code: Authorization_RequestDenied
Message: Insufficient privileges to complete the operation. 

unless I grant the app Directory.AccessAsUser.All (Delegated). However, this is not mentioned in the documentation. I only tried it because it was mentioned in the answer to this question on StackOverflow. Adding users as in Example 1 works without this permission.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

github-actions[bot] commented 2 years ago

This issue has been assigned to you, @Jordanndahl. You are listed as the author for the document associated with this issue. If this is not correct, please take the following actions.

FaithOmbongi commented 2 years ago

Thank you @erikly for raising this issue. What group type were you adding members to and that's the value of its isAssignableToRole property?

erikly commented 2 years ago

The group type is Unified, and IsAssignableToRole is null. Here's the full request (strings replaced with dummy values) used to create the group through the .NET Graph Client:

    var groupToCreate = new Group
    {
        Description = "A description"
        DisplayName = "A display name",
        GroupTypes = new[] { "Unified" },
        MailEnabled = false,
        MailNickname = "mailnickname",
        SecurityEnabled = true,
        Visibility = "private"
    };
     await _graphServiceClient.Groups.Request().AddAsync(groupToCreate);
FaithOmbongi commented 2 years ago

Thank you for the feedback @erikly. This seems to affect only the SDK - I can't replicate it through the REST API.

Escalating to @andrueastman who understands more about SDKs to assist. Andrew, can you assist here?

erikly commented 2 years ago

Here is the call to add the users. batch is an IEnumerable of user IDs retrieved through the same client (the syntax differs from the example, see microsoftgraph/microsoft-graph-devx-api#1006). The batch size is max 20 as the docs specify, but the call fails even for a single user (the same user works with example 1).

var updateGroup = new Group
{
    AdditionalData = new Dictionary<string, object>
    {
        {
            "members@odata.bind",
            batch.Select(id => $"https://graph.microsoft.com/v1.0/directoryObjects/{id}").ToArray()
        }
    }
};

await _graphServiceClient.Groups[groupId]
    .Request()
    .UpdateAsync(updateGroup);
andrueastman commented 2 years ago

Hey @erikly,

Thanks for raising this.

Do you have an issues if you change the sample on example 2 to this?


GraphServiceClient graphClient = new GraphServiceClient( authProvider );

var group = new Group
{
    AdditionalData = new Dictionary<string, object>()
    {
        {"members@odata.bind", JsonDocument.Parse("[\"https://graph.microsoft.com/v1.0/directoryObjects/{id}\",\"https://graph.microsoft.com/v1.0/directoryObjects/{id}\",\"https://graph.microsoft.com/v1.0/directoryObjects/{id}\"]")}
    }
};

await graphClient.Groups["{group-id}"]
    .Request()
    .UpdateAsync(group);
erikly commented 2 years ago

Hi,

Yes, I get the same error. I am using the latest client (4.19) on .NET 6, if that's relevant.

andrueastman commented 2 years ago

Thank you for the feedback @erikly. This seems to affect only the SDK - I can't replicate it through the REST API.

Escalating to @andrueastman who understands more about SDKs to assist. Andrew, can you assist here?

Hey @FaithOmbongi,

Just to confirm, do you also need to add the Directory.AccessAsUser.All permission? Or is it already available in you permissions?

FaithOmbongi commented 2 years ago

Hey @FaithOmbongi,

Just to confirm, do you also need to add the Directory.AccessAsUser.All permission? Or is it already available in you permissions?

Hi @andrueastman, not in this instance. I revoked the previously consented permissions and tested with only Group.ReadWrite.All which worked.

FaithOmbongi commented 2 years ago

Bumping this @andrueastman. Kindly advise if this is still an issue.

Recap summary - Bulk add members through the API should work with Group.ReadWrite.All only but requires the most privileged Directory.AccessAsUser.All permission when using the .NET SDK.

FaithOmbongi commented 2 years ago

Update: Updated the issue title for better search and visibility.

jasonjoh commented 2 years ago

Transferring to microsoft-graph-devx-api repository.