pnp
Bug report: `m365 spfx project permissions grant` does not show a decent error message
Priority
(Medium) I'm annoyed but I'll live
Description
If something goes wrong in the m365 spfx project permissions grant command, the only error message you get (also with --verbose --debug is Error: [object: Object]
Steps to reproduce
- Start with an SPFx project where you require some Graph permissions
- Run
m365 loginagainst a non-English SharePoint tenant with a user that does not have English as a primary language on that tenant - Run
m365 spfx project permissions grantfor the first time, the permissions are being added - Run
m365 spfx project permissions grantfor the second time and notice that you get an error
Expected results
I would expect a warning that the permissions already exist (as you get on an English tenant)
Or at least a decent error message.
Actual results
You get an error on the first request that fails and subsequent permission do not seem to be added:
Diagnostics
Executing command spfx project permissions grant with options {"options":{"verbose":true,"debug":true,"output":"text"}}
Granting API permissions defined in the current SPFx project
Timings:
api: 3224.8341ms
core: 363.0921ms
command: 3309.6939ms
options: 0.2048ms
total: 3676.5476ms
validation: 0.3937ms
Error: [object Object]
CLI for Microsoft 365 version
7.6.0
nodejs version
v18.18.2
Operating system (environment)
Windows
Shell
PowerShell
cli doctor
authMode : Browser
cliAadAppId : 31359c7f-bd7e-475c-86db-fdb8c937548e
cliAadAppTenant: common
cliConfig : {"autoOpenLinksInBrowser":true,"copyDeviceCodeToClipboard":true,"output":"text","printErrorsAsPlainText":true,"prompt":true,"showHelpOnFailure":false,"showSpinner":true,"helpMode":"options","authType":"browser"}
cliEnvironment :
cliVersion : 7.6.0
nodeVersion : v18.18.2
os : {"platform":"win32","version":"Windows 10 Enterprise","release":"10.0.26058"}
roles : []
scopes : {"https://graph.microsoft.com":["AllSites.FullControl","AppCatalog.ReadWrite.All","AuditLog.Read.All","Bookings.Read.All","Calendars.Read","ChannelMember.ReadWrite.All","ChannelMessage.Read.All","ChannelMessage.Send","ChannelSettings.ReadWrite.All","Chat.Read","Chat.ReadWrite","Directory.AccessAsUser.All","Directory.ReadWrite.All","ExternalConnection.ReadWrite.All","Group.ReadWrite.All","IdentityProvider.ReadWrite.All","InformationProtectionPolicy.Read","Mail.Read.Shared","Mail.ReadWrite","Mail.Send","Notes.Read.All","OnlineMeetings.Read","OnlineMeetingTranscript.Read.All","Place.Read.All","Policy.Read.All","RecordsManagement.ReadWrite.All","Reports.Read.All","SecurityEvents.Read.All","ServiceHealth.Read.All","ServiceMessage.Read.All","ServiceMessageViewpoint.Write","Tasks.ReadWrite","Team.Create","TeamMember.ReadWrite.All","TeamsAppInstallation.ReadWriteForUser","TeamSettings.ReadWrite.All","TeamsTab.ReadWrite.All","TermStore.ReadWrite.All","User.Invite.All","User.ReadWrite.All","profile","openid","email"],"https://boxxacc.sharepoint.com":["AllSites.FullControl","AppCatalog.ReadWrite.All","AuditLog.Read.All","Bookings.Read.All","Calendars.Read","ChannelMember.ReadWrite.All","ChannelMessage.Read.All","ChannelMessage.Send","ChannelSettings.ReadWrite.All","Chat.Read","Chat.ReadWrite","Directory.AccessAsUser.All","Directory.ReadWrite.All","ExternalConnection.ReadWrite.All","Group.ReadWrite.All","IdentityProvider.ReadWrite.All","InformationProtectionPolicy.Read","Mail.Read.Shared","Mail.ReadWrite","Mail.Send","Notes.Read.All","OnlineMeetings.Read","OnlineMeetingTranscript.Read.All","Place.Read.All","Policy.Read.All","RecordsManagement.ReadWrite.All","Reports.Read.All","SecurityEvents.Read.All","ServiceHealth.Read.All","ServiceMessage.Read.All","ServiceMessageViewpoint.Write","Tasks.ReadWrite","Team.Create","TeamMember.ReadWrite.All","TeamsAppInstallation.ReadWriteForUser","TeamSettings.ReadWrite.All","TeamsTab.ReadWrite.All","TermStore.ReadWrite.All","User.Invite.All","User.ReadWrite.All"]}
Additional Info
In https://github.com/pnp/cli-microsoft365/blob/c74fe4b3252f13ca679f70e8402daccef952ef1a/src/m365/spfx/commands/project/project-permissions-grant.ts#L52
is explicitly checked for the English language in the error message:
if (err.error && err.error.message.indexOf('already exists') > -1) {
await this.warn(logger, err.error.message);
continue;
}
In Dutch you get an error like: Er bestaat al een OAuth-machtiging met de resource Microsoft Graph en het bereik Files.Read.All., so that will return false in the if and therefore the error to be thrown as an Exception instead of being logged as a warning.
Thank you for reporting this issue @bjdekker! That's indeed annoying. Let's get this fixed.
We'd need to discuss the route to fix it @pnp/cli-for-microsoft-365-maintainers,
- The command should not break on an "already exists" error, but it should break on all other errors.
- The command logic should not be based on a localized exception message.
I'm not sure if the CSOM call that's currently done returns an error code that we can use. Maybe it does. We could also refactor the command to use the Microsoft Graph and see if that returns a correct error code. That would be a better solution in the long run in terms of maintaining old VS new tech.
You're right on both points @martinlingstuyl. What's more, we should print the error message. Seems that our error handling logic is missing one more error shape, so we should definitely update it to properly handle this exception as well.
I looked at the comments and as a suggestion I would suggest implementing an error recording system and performance metrics: Create an error recording system that captures and records details about any failures or problems encountered during the execution of the command. Additionally, incorporate performance metrics to monitor command execution time and resource consumption, helping to identify potential bottlenecks or areas for improvement. This will allow for deeper analysis of command performance and help identify error patterns that may arise in different usage scenarios.
You're right on both points @martinlingstuyl. What's more, we should print the error message. Seems that our error handling logic is missing one more error shape, so we should definitely update it to properly handle this exception as well.
After investigating this issue, I don't believe there's a problem with the handling logic of the CLI. It seems more likely that the issue lies in the continued use of cli.executeCommandWithOutput, which returns the output in its entirety, including stderr as a text value.
A couple of years ago, I worked on refactoring commands in the CLI refactor project. Where I also refactored this command : Refactor-spfx-project-permissions-grant.
I tried the refactored command, which resulted in a readable error being returned:
EDIT: How coincidental - it's been exactly one year since the comment I'm responding to! 😅
@nicodecleyre, thanks for taking an interest in this one.
I was wondering if you would like to take this issue on and maybe somehow refresh your branch and open a PR for it.
I remember that change and I think the biggest challenge there was that we created a utility function in spo.ts that was very similar to the same logic present in spoServicePrincipalGrantAddCommand so in order not to introduce duplicated logic we would need to refactor that command to also use that util as well.
@nicodecleyre, thanks for taking an interest in this one. I was wondering if you would like to take this issue on and maybe somehow refresh your branch and open a PR for it. I remember that change and I think the biggest challenge there was that we created a utility function in
spo.tsthat was very similar to the same logic present inspoServicePrincipalGrantAddCommandso in order not to introduce duplicated logic we would need to refactor that command to also use that util as well.
Sure thing, always happy to help! I'll refresh my branch and create a pull request later on this week