search

pnp / cli-microsoft365

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

Bug report: Some examples for the 'login' command do not mention the other necessary options you need: tenant and appId #3725

Open martinlingstuyl opened 1 year ago

martinlingstuyl commented 1 year ago

Description

Check out this example:

m365 login --authType secret --secret topSeCr3t@007

That's not a good example, as it would not log you in successfully. You need the --appId value as well. And if your app registration is configured as single tenant, you will need the --tenant option as well. Otherwise you get an error. An example should be able to be run as it is (with the right values), so we should add the options here.

All the examples for logging in with a certificate, have the same deficiency and should be fixed as well.

appieschot commented 1 year ago

We should also mention that if you use the login with a custom appId you need to specify the tenant if your app is not multi tenant. Feels like we can pick that up in this same issue.

martinlingstuyl commented 1 year ago

Updated it

waldekmastykarz commented 1 year ago

That's not a good example, as it would not log you in successfully. You need the --appId value as well. And if your app registration is configured as single tenant, you will need the --tenant option as well. Otherwise you get an error.

I think it would work if the values are set in an environment variable, but I do agree that it's not clear, so we should either expand the sample's code or mention it specifically in the sample's description that precedes the code.

martinlingstuyl commented 1 year ago

I see what you mean. I'm in favor of expanding the samples. Using env variables, while a possibility is not the primary mode to work, I think.

We could add an extra note about the possibility to use env variables...

waldekmastykarz commented 1 year ago

Using env variables, while a possibility is not the primary mode to work, I think.

I think it all depends on the use case. I think you're unlikely to login interactively with a secret or cert, because these options are typically used in the context of automation with app-only access, where you might define app- and tenant ID in env vars. There's no harm in adding an extra explanation to the existing sample saying that it assumes specific values are set in env vars, and then adding another version where app- and tenant ID are specified through args.