Skip to content

[FIX] mention-links not being always resolved#11745

Merged
ggazzo merged 12 commits intoRocketChat:developfrom
assistify:core/fix-channel-mentions
Mar 1, 2019
Merged

[FIX] mention-links not being always resolved#11745
ggazzo merged 12 commits intoRocketChat:developfrom
assistify:core/fix-channel-mentions

Conversation

@mrsimpson
Copy link
Contributor

@mrsimpson mrsimpson commented Aug 10, 2018

  • If a mention of a channel refers to a non-public channel, the link was calculated wrongly (hard-coded to /channel/... )
  • If a target room mentioned was renamed, the link was not adapted. This can now be resolved by passing the room's id

This fixes an issue when navigating to a private thread

- If a mention of a channel refers to a non-public channel, the link was calculated wrongly (hard-coded to /channel/... )
- If a target room mentioned was renamed, the link was not adapted. This can now be resolved by passing the room's id
@mrsimpson mrsimpson requested a review from a team August 10, 2018 21:52
}

if (room.t !== 'c' || RocketChat.authz.hasPermission(Meteor.userId(), 'view-c-room') !== true) {
if (RocketChat.authz.hasPermission(Meteor.userId(), `view-${ room.t }-room`) !== true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you cannot do this. the previous check works because everyone can access a public channel. but the same doesn't apply for private groups.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not everyone has the right to access public channels, only those with the proper view-c permission.
The change will still work for public channels, but in addition return Ids of non-public-channels - if the user has permission to view them.
Is there a reason why a user should be allowed to get the Ids of channels for which he’s not authorized if he’s authorized to see public channels? 🧐

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I have totally understood your question 😅

but so far we do not show to users a list of private channels/groups, people also cannot "guess" a private group's name, we don't tell them if that group exists or not.. if someone tries to guess a private group name from the URL, he receives a message like "No private group with name "channel-name" was found!"

So we need to be consistent here and not give them the private group's _id

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to flag I also have a pending PR for related behaviour. #11703

My commit detailed here.

@sampaiodiego Would be great if you can look at that to find a solution in balance with this one.

The changes to the getRoomIdByNameOrId method knocked out a bunch of people's bots, because the bot can no longer get the ID for a private room, even when the bot is in the room.

See #11552

@mrsimpson
Copy link
Contributor Author

@sampaiodiego I have seen you merged tim's proposal. I merged develop.
Kindly check the rest.

@mrsimpson
Copy link
Contributor Author

@sampaiodiego this is really a pity: FlowRouter.goToRoomById internally uses the ChatRoom collection - which still seems to be cached. And in this cache, only subscribed rooms are included - different to what the name suggests. At least, public channels should be included there.
Since this would be quite some change, I implemented a workaround.
This allows to refer to channels in the mention via its renaming-friedly _id and is less intrusive - but more hacky.
Would you suggest which route to go?

@sampaiodiego
Copy link
Member

wow, I'm sry. I have completely lost track of this.

I'm afraid I'm not seeing value in this, like: renamed rooms will sill have broken links and private groups doesn't have links, so ... what about we fix those issues? start linking to joined private groups and updating links on room names?

@mrsimpson
Copy link
Contributor Author

mrsimpson commented Sep 13, 2018

@sampaiodiego ah! I seem to have forgot the part with the channel mention being stored with its id internally. We do this on our fork with threading. I’ll have to adapt this to the creation of the normal channel mention. I’ll do this ASAP.
Still: the change in the resolution onclick is only a workaround, imho.
What do you think about the ChatRooms collection to be independent of the subscriptions?

Sent with GitHawk

@mrsimpson
Copy link
Contributor Author

@sampaiodiego I now added the way the mention links are being created. And I changed the behavior of FlowRouter.goToRoomById. Earlier it resolved only subscriptions which was kind of weird with respect to the name of the function. Also, I considered this to be the smallest modification with respect to security and side-effects.
Now, mentions of public, renamed, public not subscribed and private and subscribed rooms are being rendered and resolved. Links to private, not subscribed rooms are rendered but not resolved.
Wdyt?

Sent with GitHawk

@zdumitru
Copy link
Contributor

zdumitru commented Jan 8, 2019

@sampaiodiego Can this please be escalated, as it currently blocks #11803 ? Thanks.

@zdumitru
Copy link
Contributor

@mrsimpson Can you, please, merge develop branch into this PR? Otherwise merging is blocked. Thanks.

@mrsimpson
Copy link
Contributor Author

mrsimpson commented Feb 20, 2019

@ggazzo bringing this to your attention: If you want to have a stable navigation between the parent channel and thread, there needs to be some enhancement.

Shamelessly assigned yourself since you're most likely taking care of this anyway ;)

@mrsimpson mrsimpson assigned mrsimpson and ggazzo and unassigned mrsimpson Feb 20, 2019
@tassoevan tassoevan requested a review from ggazzo February 20, 2019 11:17
@ggazzo ggazzo merged commit 8f40051 into RocketChat:develop Mar 1, 2019
@rodrigok rodrigok mentioned this pull request Apr 28, 2019
@mrsimpson mrsimpson deleted the core/fix-channel-mentions branch December 7, 2020 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments