Broken PR feature "[MM-25748]Allow access to managed resources #1311"

[dpanic]

I confirm (by marking "x" in the [ ] below: [x]):

• This is not a troubleshooting question. Troubleshooting questions go here: http://www.mattermost.org/troubleshoot/.
• This doesn't reproduce on web browsers (such as in Chrome). If it does, issue reports go to the Mattermost Server repository.
• I have read contributing guidelines.

---

Summary
Managed resources feature was implemented by me in June 2020. Released 2 weeks ago in Mattermost Desktop. It used to work in June. Now looks like something has changed and click on URL which used to work gives this:

Expected was that Mattermost Desktop app opens popup window, not in same window.
I can provide test credentials and test server for this.
Link to PR: #1311

Environment

• Operating System: Linux
• Mattermost Desktop App version: 4.6.1
• Mattermost Server version: 5.28.0

[Willyfrog]

now that I see the error, shouldn't the url path be managed by the proxy before going to the server? that's a server response, so it is searching for the path as if it were a team.

either that or it is navigating to it and being handled by the webapp. To test if it's one or another i'd go to a browser and see if behaviour matches (most likely), then copy the url and paste it in a new tab. If the new tab works, it means the webapp is handling it, if the result is the same, the problem lies within the server

[dpanic]

It is managed by Nginx. 1. Click on link gives error 2. Open link in new tab, works ok

…

On Thu, Oct 29, 2020 at 10:06 AM Guillermo Vayá ***@***.***> wrote: now that I see the error, shouldn't the url path be managed by the proxy before going to the server? that's a server response, so it is searching for the path as if it were a team. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1401 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACUTPKU37FQTMB6JN6UDLLSNEWALANCNFSM4TDNGOQQ> .

[Willyfrog]

@dpanic i updated my original comment as I forgot a second possibility, which seems to be what you describe

[dpanic]

@dpanic i updated my original comment as I forgot a second possibility, which seems to be what you describe

Ok so looks like now is clear where and what is the issue?

[Willyfrog]

i've created a jira ticket

[hmhealey]

Hi @dpanic. To confirm something, was the link that you clicked in a post that someone made?

[hmhealey]

Also, is this something that worked before, or is it something you want to make work?

Unfortunately, when we first started including the team name in the URL path, we didn't think to include a /teams prefix so everything directly under / outside of some special cases like /api and /plugins is considered as the name of a team, so that's what's causing the problem here.

[dpanic]

Hi @dpanic. To confirm something, was the link that you clicked in a post that someone made?

It was created by modified Jitsi plugin

[dpanic]

Also, is this something that worked before, or is it something you want to make work?

Unfortunately, when we first started including the team name in the URL path, we didn't think to include a /teams prefix so everything directly under / outside of some special cases like /api and /plugins is considered as the name of a team, so that's what's causing the problem here.

This is something @Willyfrog and I made work 3 months ago. I submited PR for this

[hmhealey]

So the reason this broke is because we changed how we detect internal vs external links in posts in the web/desktop app. Anything internal is opened in the app or browser tab, and everything external is opened outside of that. Before, we only treated a few cases as internal (like /<team>/channels/<channel>), but we kept running into issues because people would want to link to different things like the System Console or to some plugin-provided pages. We decided to change it to explicitly list things in the server that needed to be treated as external because anything unrecognized as internal because something like /foo could be part of a team named foo. If we had thought to make teams live under /teams/<team>, none of this would've happened.

So, ideally, we'd keep it this way to prevent future issues with people wanting to link to new pages within Mattermost, but obviously that screws up your case. As it stands, even if we revert the changesfor 5.29, you still won't be able to use 5.28.

@Willyfrog mentioned you're running a custom build of the desktop app for this. Are you perhaps also running a custom build of the web app? I could point you to the spot to change the internal/external link handling to add your any URLs to be treated as external.

[dpanic]

So the reason this broke is because we changed how we detect internal vs external links in posts in the web/desktop app. Anything internal is opened in the app or browser tab, and everything external is opened outside of that. Before, we only treated a few cases as internal (like /<team>/channels/<channel>), but we kept running into issues because people would want to link to different things like the System Console or to some plugin-provided pages. We decided to change it to explicitly list things in the server that needed to be treated as external because anything unrecognized as internal because something like /foo could be part of a team named foo. If we had thought to make teams live under /teams/<team>, none of this would've happened.

So, ideally, we'd keep it this way to prevent future issues with people wanting to link to new pages within Mattermost, but obviously that screws up your case. As it stands, even if we revert the changesfor 5.29, you still won't be able to use 5.28.

@Willyfrog mentioned you're running a custom build of the desktop app for this. Are you perhaps also running a custom build of the web app? I could point you to the spot to change the internal/external link handling to add your any URLs to be treated as external.

Hey, If I wanted to run custom build, than I would not submit PR to you. I will not wait 3-4 months for PR to be released :-)

I am pretty sure we can find a solution for this. As there is ONLY one url which needs to be whitelisted, /trusted
Perhaps @jespino can jump in, as he is aware of the feature mattermost/mattermost#15136

[dpanic]

Any updates on this?

[hmhealey]

Apologies. @Willyfrog and I discussed the followup for this yesterday, and I updated the Jira ticket on this, but I forgot to put those changes here. I saw you chatted about that with him though about it.

Since the web app can't get the values configured in the desktop app, we had to add a separate server setting for this that can be configured to have the web app treat those links as external when appearing in a post or anywhere else that supports Markdown. The setting takes a comma-separated list, but I tried to have it otherwise take values in the same format.

There's a few PRs involved in this, but the main one to watch for this fix is this one: mattermost/mattermost-webapp#7024

I think I understand the feature now, but just to make sure, I'm going to leave a comment on some of the unit tests in that PR if you could confirm that it behaves how you'd expect with the new setting enabled.

[dpanic]

Apologies. @Willyfrog and I discussed the followup for this yesterday, and I updated the Jira ticket on this, but I forgot to put those changes here. I saw you chatted about that with him though about it.

Since the web app can't get the values configured in the desktop app, we had to add a separate server setting for this that can be configured to have the web app treat those links as external when appearing in a post or anywhere else that supports Markdown. The setting takes a comma-separated list, but I tried to have it otherwise take values in the same format.

There's a few PRs involved in this, but the main one to watch for this fix is this one: mattermost/mattermost-webapp#7024

I think I understand the feature now, but just to make sure, I'm going to leave a comment on some of the unit tests in that PR if you could confirm that it behaves how you'd expect with the new setting enabled.

Saw it, and commented there. Thanks

[devinbinnie]

Closing as this has been fixed in the webapp.