pnp / cli-microsoft365

Manage Microsoft 365 and SharePoint Framework projects on any platform
https://aka.ms/cli-m365
MIT License
896 stars 318 forks source link

Bug report: Robustness of `m365 entra group user add` #5885

Open MartinM85 opened 5 months ago

MartinM85 commented 5 months ago

Priority

(Urgent) I can't use the CLI

Description

The current implementation of m365 entra group user add uses Graph batch requests. If one request inside one batch fails for some reason the whole command fails. It can lead to a state when several users were added to a group, but the rest not. If the user will execute the command again, it will fail, because some users have been already added.

The command doesn't return any info which users were (not) added. It makes the command unusable for the end users.

In general, the command should try to add all users to the group and return a summary of which users have been added and which have not.

Steps to reproduce

m365 entra group user add --groupId {groupId} --ids {hundredsUserIds}

One id is not a user id One user is already a member/owner of the group

Expected results

The command returns a summary

[
  {
    "userId": "...",
    "userName": "...", // optional
    "state": "added|failed",
    "error": "erroMessage" // optional
  },
  ...
]

Actual results

...

Diagnostics

No response

CLI for Microsoft 365 version

v7.6.0

nodejs version

v20.8.1

Operating system (environment)

Windows

Shell

PowerShell

cli doctor

No response

Additional Info

Priority is urgent, but it related only to this one command.

milanholemans commented 5 months ago

Hi @MartinM85, thank you for reporting this.

First of all, are you targeting a Microsoft 365 group? In that case, you can still use entra m365group user add. If not, you can always check using entra group user list which people are already members.

Regarding your proposal to provide an output, in case an API fails, we should output an error instead of just completing and returning an output. I understand that in your use case, this output could be handy, but in 95% of the cases, this will be quite verbose. Maybe we can do some research on implementing a switch parameter like we do here: https://github.com/pnp/cli-microsoft365/issues/5472#issuecomment-1937798769

MartinM85 commented 5 months ago

@milanholemans The suppressError switch or similar to https://github.com/pnp/cli-microsoft365/issues/5472#issuecomment-1937798769 can be really handy in this case. User can always call entra group user list for check before/after.

Similar switch can be handy also for #5473

milanholemans commented 5 months ago

It's important to not suppress all errors, let's check if we can differentiate a this user is already a member of the group error from another API error like this user doesn't exist. In the latter case, we should still throw an error, otherwise, people wouldn't know if everything ran successfully.

waldekmastykarz commented 4 months ago

If we get the information from the batch response about which requests failed, then we should open them. Even though it might be a lot of output, if we print only errors, it'll help users fix their config and re-try adding users to groups that failed previously.