search

mattermost / desktop

Mattermost Desktop application for Windows, Mac and Linux
Apache License 2.0
1.99k stars 809 forks source link

[Bug]: Exec in desktop entry for generix linux seems to be buggy #2942

Open Lotbert opened 8 months ago

Lotbert commented 8 months ago

Checks before filing an issue

Mattermost Desktop Version

5.6.0

Operating System

NixOS 23.05

Mattermost Server Version

No response

Steps to reproduce

  1. Login using Gitlab SSO.
  2. Authorize Mattermost in Gitlab
  3. Allow the Browser to open in...

Expected behavior

The Browser asks to be allowed to open Mattermost. We get redirected to the Mattermost Desktop App with a logged-in user.

Observed behavior

The Browser asks us to allow it to open xdg-open (instead of Mattermost). After we allow it, the default Browser is opened with the following url in the address bar:
mattermost-dev://mattermost.example.de/login/desktop?client_token=dev-<client_token>&isDesktopDev=true&server_token=<server_token>

Log Output

I forgot to save the log outputs.

Additional Information

Two things are still wrong in the current master while writing this. I'll explain what, using two patches that fixed my local installation.

  1. The binaries packaged in the released tar archive use <mattermost-dev://> as prefix for the mattermost desktop app, a possible fix could be to build the mattermost-desktop app with the ELECTRON_IS_DEV environment variable set to 0 to get rid of the -dev (I also explain a workaround using another MimeType in the code explained below)
modified   package.json
@@ -59,7 +59,7 @@
     "package:mac-with-universal": "npm-run-all build-prod && electron-builder --mac --x64 --arm64 --universal --publish=never",
     "package:mas": "cross-env NODE_ENV=production IS_MAC_APP_STORE=true npm-run-all check-build-config build && electron-builder --mac mas --universal --publish=never",
     "package:mas-dev": "cross-env NODE_ENV=production IS_MAC_APP_STORE=true npm-run-all check-build-config build && electron-builder --mac mas-dev --universal --publish=never",
-    "package:linux": "npm-run-all package:linux-all package:linux-appImage",
+    "package:linux": "set ELECTRON_IS_DEV=0 && npm-run-all package:linux-all package:linux-appImage",
     "package:linux-appImage": "npm-run-all build-prod-upgrade package:linux-appImage-x64 package:linux-appImage-arm64",
     "package:linux-appImage-x64": "electron-builder --linux tar.gz appimage --x64 --publish=never",
     "package:linux-appImage-arm64": "cross-env CC=aarch64-linux-gnu-gcc CXX=aarch64-linux-gnu-g++ electron-builder --linux tar.gz appimage --arm64 --publish=never",
  1. The Desktop Entry Exec Attribute does not need double quotes surrounding the mattermost-desktop path. See: https://wiki.archlinux.org/title/desktop_entries#Modify_command_line_arguments
modified   src/assets/linux/create_desktop_file.sh
@@ -9,7 +9,7 @@ cat <<EOS > Mattermost.desktop
 [Desktop Entry]
 Name=Mattermost
 Comment=Mattermost Desktop application for Linux
-Exec="${FULL_PATH}/mattermost-desktop" %U
+Exec=${FULL_PATH}/mattermost-desktop %U
 Terminal=false
 Type=Application
 # I am using `MimeType=x-scheme-handler/mattermost-dev` as a workaround. 
 # The following line is still missing in v5.6.0
 MimeType=x-scheme-handler/mattermost

Also beware of the last three lines in the code above.

lesteve commented 6 months ago

This seems to affect the ArchLinux mattermost-desktop 5.6.0 package as well.

If I looked in my Firefox console I see:

Prevented navigation to “mattermost-dev://mattermost.inria.fr/login/desktop?client_token=<dev-...>&isDesktopDev=true&server_token=<...>” due to an unknown protocol.
christianhueserhzdr commented 6 months ago

We are having the same issues with the current Mattermost Desktop App in v5.6.0 with the same console message "Prevented navigation to 'mattermost://...' due to an unknown protocol". It is failing in Firefox and working in Chromium on Debian.

devinbinnie commented 6 months ago

So it looks like the Arch Linux build for some reason is being built in development mode, despite the fact that they are setting NODE_ENV=production. The mattermost-dev: protocol should only be necessary if you're developing the app so all official releases should be using the mattermost: protocol, which is baked into the .desktop file.

I tried to reproduce their build process but I wasn't able to reproduce the issue, looks like it builds in prod mode for me.

Sounds like this is what's causing this issue for most people, if you're seeing mattermost-dev: that's likely the app misreporting that it's in dev mode.

Lotbert commented 6 months ago

@devinbinnie Correct, it is build in dev mode for some reason. Did you use the package:linux script from the package.json to build the app?

devinbinnie commented 6 months ago

@devinbinnie Correct, it is build in dev mode for some reason. Did you use the package:linux script from the package.json to build the app?

@Lotbert I'm running build and then package:linux-all-x64 just as the Arch script does. Did not build in dev mode for me.

Lotbert commented 6 months ago

Ok. I think somehow the isDev in the following line:

const protocol = isDev ? 'mattermost-dev' : mainProtocol;

evaluates to true. But why?

Digging a bit deeper i had a look into electron-is-dev and found the following line

const isDev = isEnvSet ? getFromEnv : !electron.app.isPackaged;

Since i didn't find a file containing ELECTRON_IS_DEV maybe electron.app.isPackaged is a falsy value in the environment executing the app that produces the mentioned bug(s).

I just wanted to leave this here...

Lotbert commented 6 months ago

@devinbinnie isn't the ci pipeline executing package:linux here:
https://github.com/mattermost/desktop/blob/v5.6.0/.github/workflows/release.yaml#L58

Maybe i am getting it wrong...

devinbinnie commented 6 months ago

@devinbinnie isn't the ci pipeline executing package:linux here: https://github.com/mattermost/desktop/blob/v5.6.0/.github/workflows/release.yaml#L58

Maybe i am getting it wrong...

Yes the pipeline is, which basically executes the same thing: package:linux will run build-prod and package:linux-all-x64 build-prod just runs NODE_ENV=production npm run build

To the above point it's possible there's a bug in that package. Grabbing the .tar.gz file from the GitHub release, it's running in Production Mode as well. The only place I can see this reproduced is from the Arch package from pacman.

enticedwanderer commented 6 months ago

The reason for this I think is the behavior of electron.app.isPackaged. This post summarizes the behavior. But the gist is that at least for the Arch linux, the package for electron names the executable as electron27 while the isPackaged flag behavior explicitly expects it to be exactly electron (see arch linux package). Since it isn't it assumes it isn't packaged and thus it behaves like it is in dev mode.

Likely when you build it yourself you end up running it against your normal electron package with matching binary name. I'm not sure what's the best way to go about fixing this, but the easiest seems to be to just set ELECTRON_FORCE_IS_PACKAGED env in the build script.

Lotbert commented 6 months ago

IMHO using set ELECTRON_IS_DEV=0 should also work and has clearer semantics if one only looks at the name of the env var. That's also what I proposed to try in the first of the patches I posted originally.

Lotbert commented 5 months ago

IMHO using set ELECTRON_IS_DEV=0 should also work and has clearer semantics if one only looks at the name of the env var. That's also what I proposed to try in the first of the patches I posted originally.

This works @lzszt and me just did that in a PR for the respective nixpkg and it fixed the bug.

See https://github.com/NixOS/nixpkgs/pull/279645#discussion_r1555581090

Lotbert commented 1 month ago

Any news here @devinbinnie . In the meantime the nixos cummunity fixed the set ELECTRON_IS_DEV=0 part here, but the Exec line in the Desktop entry file is still buggy and uses double quotes, which results my browser opening the link after the sso login instead of mattermost-desktop.

This is 2. in my original post above.

Here is the spec for the Exec key in Desktop entries

devinbinnie commented 1 month ago

Been on vacation for the past few weeks, I will say this isn't likely to be worked on anytime soon, so I'm open to have someone else look at it and I can try and guide where I can.

Lotbert commented 1 month ago

Been on vacation for the past few weeks, I will say this isn't likely to be worked on anytime soon, so I'm open to have someone else look at it and I can try and guide where I can.

I'd be happy to fix this in a PR coming soon.

Currently i am working around it with a custom desktop entry, that applies the fixes to the desktop entry from my original posting above.