Bug report: m365 cli reconsent seems not to open the browser automatically when autoOpenLinksInBrowser is true

[martinlingstuyl]

Description

I'm using autoOpenLinksInBrowser as set to true, to automatically open urls in the browser.

for m365 login, this works as expected,
for m365 cli reconsent, the correct message is shown, but the browser does not open.

No error is thrown, also when running with --debug. It's just completing the command with the message DONE.

Is this something someone can reproduce?

In the code the only difference I see is that the m365 cli reconsent command awaits the open function as a promise, while the login command does not do so:

await (this._open as typeof open)(url);

Steps to reproduce

Try the commands

Expected results

Browser is opened

Actual results

Browser did not open

Diagnostics

Executing command cli reconsent with options {"options":{"debug":true,"output":"json"}}
"Opening the following page in your browser: https://login.microsoftonline.com/common/oauth2/authorize?client_id=31359c7f-bd7e-475c-86db-fdb8c937548e&response_type=code&prompt=admin_consent"
DONE

CLI for Microsoft 365 version

6.2 (beta)

nodejs version

16.15.0

Operating system (environment)

Windows

Shell

PowerShell

cli doctor

{
"os": {
"platform": "win32",
"version": "Windows 10 Enterprise",
"release": "10.0.22621"
},
"cliVersion": "6.2.0-beta.647e291",
"nodeVersion": "v16.15.0",
"cliAadAppId": "31359c7f-bd7e-475c-86db-fdb8c937548e",
"cliAadAppTenant": "common",
"authMode": "DeviceCode",
"cliEnvironment": "",
"cliConfig": {
"showHelpOnFailure": false,
"copyDeviceCodeToClipboard": true,
"disableTelemetry": true,
"autoOpenLinksInBrowser": true
},
"roles": [],
"scopes": [
"AllSites.FullControl",
"AppCatalog.ReadWrite.All",
"ChannelMember.ReadWrite.All",
"ChannelMessage.Send",
"ChannelSettings.ReadWrite.All",
"Directory.AccessAsUser.All",
"Directory.ReadWrite.All",
"Group.ReadWrite.All",
"IdentityProvider.ReadWrite.All",
"Mail.ReadWrite",
"Mail.Send",
"Policy.Read.All",
"Reports.Read.All",
"Tasks.ReadWrite",
"Team.Create",
"TeamMember.ReadWrite.All",
"TeamsApp.ReadWrite.All",
"TeamsAppInstallation.ReadWriteForUser",
"TeamSettings.ReadWrite.All",
"TeamsTab.ReadWrite.All",
"TermStore.ReadWrite.All",
"User.Invite.All",
"User.ReadWrite.All",
"profile",
"openid",
"email"
]
}

Additional Info

No response

[waldekmastykarz]

I can't reproduce it. For me, the browser is opening just fine.

[martinlingstuyl]

You are on a Mac though, right? Any other windows users who can check this? @pnp/cli-for-microsoft-365-maintainers?

[milanholemans]

You are on a Mac though, right? Any other windows users who can check this? @pnp/cli-for-microsoft-365-maintainers?

Yes! I can confirm it doesn't open my browser.

[waldekmastykarz]

In the code the only difference I see is that the m365 cli reconsent command awaits the open function as a promise, while the login command does not do so:

Could you see if the behavior on Windows changes if you remove awaiting?

[martinlingstuyl]

I'm running in docker. @milanholemans, could you check if removing await helps?

[milanholemans]

On first sight, this doesn't seem to fix the issue. Haven't digged deeper into it yet.

[waldekmastykarz]

I'm running in docker. @milanholemans, could you check if removing await helps?

If you're running in docker, isn't this the expected behavior as the CLI tries to open the browser window in the host, which in your case is your docker container rather than your OS?

[martinlingstuyl]

No @waldekmastykarz, I mean my dev env is running in docker. I do use the CLI in PowerShell in windows.

To run a build and test it in windows I'd have to clone and build it in windows.

I knew @milanholemans doesn't work in docker, that's why I asked him

[martinlingstuyl]

On first sight, this doesn't seem to fix the issue. Haven't digged deeper into it yet.

Ok, too bad... more research necessary in that case..

[Adam-it]

hi. I had a quick check on this one and seems not to be working for me as well.
I tried to investigate maybe why but unfortunately did not have any luck 🤔

[waldekmastykarz]

I'll try to repro it on Windows

[MathijsVerbeeck]

I just noticed the same behavior for command m365 cli issue --type command . Also using Windows though. Wanted to reuse this functionallity for #4037 , but can't seem to get it working. When placing a try catch around it, it will also not go into the catch statement. Have also tried both awaiting and not awaiting, but nothing changes. Also specified the browser to use using await open('https://pnp.github.io/cli-microsoft365/', {app: {name: 'firefox'}}); for example, but without success.

[martinlingstuyl]

Thanks for the research @MathijsVerbeeck!

[MathijsVerbeeck]

I have also tested this on my macbook and can confirm that it works there. I've tried a lot of things, but the only way I can make it work in other commands beside m365 login --authType browser or m365 login --authType devicecode with setting autoOpenLinksInBrowser set to true is by adding { wait: true } to the open statement, which makes no sense at all 😄

I will research further I guess 😄.

[MathijsVerbeeck]

After some research from me and @milanholemans we noticed that when we added a delay after calling open('https://google.be') by for example simply adding a for loop that would iterate 100.000 items, the browser would open after approximately 3 seconds. So it seems that the method open is not properly awaited by the module that we are using. I have logged an issue at the repository which can be found at the following url: sindresorhus/open#298

Adding the option wait: true is also not an option to resolve this issue, as it will then wait for macbook users until the user has explicitly closed the entire application, which would be very annoying. I'm wondering if this functionality has worked in the past. Are there any old Windows-users who maybe have experience using this and remembering that it has indeed worked?

[martinlingstuyl]

Great research @MathijsVerbeeck and @milanholemans!

[martinlingstuyl]

So have you discovered why it does work on the login command? Is the flow there somehow different?

[MathijsVerbeeck]

So have you discovered why it does work on the login command? Is the flow there somehow different?

Yes, because the login command will wait until the credentials have been entered / device code has been entered. The command will only be finished when the user either exits the command or the Terminal has received a response from let's say the devicecode, leaving all the time for the browser to open (as I specified in my previous comment after like two - three seconds).

Does that make sense? 😄

[martinlingstuyl]

Ah, that certainly makes sense. 🤙

[waldekmastykarz]

What if we'd add artificial and arbitrary delay, so that we give the browser sufficient time to open, but by the time the person goes through the consent, the command will stop its execution?

[MathijsVerbeeck]

That would solve it for this command, but we have more than just this command that use the open functionality such as m365 cli issue --type command and m365 app open. The issue also only occurs on users using Windows. What we could do is capture the response of the open method. This response contains a PID. We could then add like some sort of a own-made await that would only trigger for Windows users that would resolve when a process with the specific PID is found. I'm not sure if that would actually work, but it's something that comes to mind.

[martinlingstuyl]

The downside of using an arbitrary wait is that it might take longer than 3 seconds. On my end it sometimes takes 5 or 6.

[martinlingstuyl]

Should we implement a prompt saying something like "Opening the browser, press any key to continue..."?

[MathijsVerbeeck]

Should we implement a prompt saying something like "Opening the browser, press any key to continue..."?

I do not believe that is a good idea, I believe that writing our own await/resolve method would be better then. Also we've logged an issue in the repository that we are currently using, so we may get an answer from them too 😄

[milanholemans]

Does it take 5 or 6 for the window to show up or does our code need that time or the browser won't launch? I'm not worried if it's the former but if it's the latter then it would be odd indeed.

That's hard to say. My guess is that we really have to wait and don't resolve our command promise until the window opens.

[martinlingstuyl]

Also we've logged an issue in the repository that we are currently using, so we may get an answer from them too

Which could be a reason to not dive too deeply and implement a workaround.

[milanholemans]

Due to the lack of response on the open repo, I've been experimenting with it and might be onto something.
The open command returns a process object which contains the process ID (pid). I found out (at least on Windows) that the process is being started, but we should wait for it to terminate before executing the rest of the code. If we don't wait for it to terminate, the browser will not open.

There are 2 solutions, either we take the pid and poll every few milliseconds if the process is still active. When the process doesn't exist anymore, we can execute the rest of our code.

In pseudo code:

const process = await open(url, { wait: false });

let processActive = true;
while (processActive) {
  sleep(100);
  processActive = await processExists(process.pid);
}

// Rest of the code

But a better way to me, in the process object, we can add listeners to this process. This enables us to add a listener when the process terminates.

In code this looks like this:

private async openBrowser(link: string): Promise<void> {
  return new Promise<void>(async (res) => {
    const process = await open(link, { wait: false });
    process.addListener('exit', () => res());
  });
}

Tried this function locally and seems to be working for me 100% of the time. The only question is, will this also work for Mac and other OSs? My guess is that it probably won't work there. But worst case we can perform a check on which OS we are before executing this piece of code.

While writing this I realize it could be easier to just do something like the code below which will do probably the same as my code.

const runningOnWindows = process.platform === "win32"

await open(url, { wait: runningOnWindows });

[waldekmastykarz]

If I read the code correctly, are you suggesting that we wait until the process closes? Is this necessary? I thought that we only had to wait until the process starts not until it exits. If we wait until the exit, if you choose to open a link in the browser, the command won't stop its execution until you close the browser which I don't think is desirable.

[milanholemans]

If I read the code correctly, are you suggesting that we wait until the process closes? Is this necessary? I thought that we only had to wait until the process starts not until it exits. If we wait until the exit, if you choose to open a link in the browser, the command won't stop its execution until you close the browser which I don't think is desirable.

Correct, except on Windows apparently 🤣
I did a test where I'm constantly polling the process state. The moment my browser pops open, the process dies, I'm unable to retrieve it anymore.

That's why the wait: true option on the open function fixes this issue on Windows, but blocks execution on Mac and other OSs.
Therefore I'm afraid it's inevitable that we have to check for which OS the user is using.

[waldekmastykarz]

Gotcha. If that's needed for win, then let's use it and also pay attention when we update open if the behavior is still correct.

[milanholemans]

Gotcha. If that's needed for win, then let's use it and also pay attention when we update open if the behavior is still correct.

Indeed, because I think this behavior is not really intentional.

I suggest that we create a util function to open URLs in browsers so we don't have to do the OS check in all these different commands.

[Adam-it]

@milanholemans awesome find 👍. All in for the util method 👍
I think it seems like a solid plan and we could open it up right?

[MathijsVerbeeck]

@milanholemans asked me to verify on OS X. Will do this tomorrow!

[MathijsVerbeeck]

Can confirm that this works properly 😄

[milanholemans]

The execution of the command is not blocked when you leave the website open?

[MathijsVerbeeck]

The execution of the command is not blocked when you leave the website open?

Since we did this together, and you saw it on my screen, you too can confirm was not 😆

[milanholemans]

Just wanted to be sure!

@pnp/cli-for-microsoft-365-maintainers, seems like we have 2 options.

either we do

const runningOnWindows = process.platform === "win32"

await open(url, { wait: runningOnWindows });

or

private async openBrowser(link: string): Promise<void> {
  return new Promise<void>(async (res) => {
    const process = await open(link, { wait: false });
    process.addListener('exit', () => res());
  });
}

[waldekmastykarz]

I vote for checking for win32. Looks simpler, and easier to revert when it's no longer needed.

[Jwaegebaert]

I'm also more of a fan of checking win32 platform

[martinlingstuyl]

Me as well 👍

[milanholemans]

Thank you for your opinions. Let's use that approach.
Let's create a util function to open links in the browser using this approach.

Ready to open it up!

[MathijsVerbeeck]

As I need this to fix #4037 , I would love to pick this one up.

[milanholemans]

All yours!