[FIX] Quoted messages from message links when user has no permission #20815
Conversation
…annels in which they are in
sampaiodiego
changed the title
app/oembed/server/jumpToMessage.js
Outdated
|
|
||
| // validates if user can see the message | ||
| // user has to belong to the room the message was first wrote in | ||
| const canAccessRoom = Meteor.call('canAccessRoom', jumpToMessage.rid, Meteor.userId()); |
There was a problem hiding this comment.
please try to avoid using Meteor methods, specially on server code. always check if there is a service that does the same and if you can't used async functions (like here), look for alternative functions or use Promise.await.
to check if a user has permission to access a room you can use canAccessRoom
Co-authored-by: Diego Sampaio <[email protected]>
app/oembed/server/jumpToMessage.js
Outdated
|
|
||
| // validates if user can see the message | ||
| // user has to belong to the room the message was first wrote in | ||
| const canAccessRoomForUser = canAccessRoom({ _id: jumpToMessage.rid }, Meteor.user()); |
There was a problem hiding this comment.
I think you'll need to send a complete Room object to canAccessRoom..
I'd also recommend to store Meteor.user() in a variable instead of calling it inside the loop.
There was a problem hiding this comment.
Regarding this one, I noticed that in other places they were retrieving Rooms.findById(...., { _id: 1 }). That will exclude other fields and returning just the ID as an object. This mimics the same behavior to avoid calling the database n times. (And it looks like its working haha)
Regarding the Meteor.user I didn't know it was an expensive operation, I'll move it to the top.
app/oembed/server/jumpToMessage.js
Outdated
| // validates if user can see the message | ||
| // user has to belong to the room the message was first wrote in | ||
| const canAccessRoomForUser = canAccessRoom({ _id: jumpToMessage.rid }, Meteor.user()); | ||
| if (jumpToMessage && canAccessRoomForUser) { |
There was a problem hiding this comment.
why not having a shortcut as others here? 😬
| if (jumpToMessage && canAccessRoomForUser) { | |
| if (!canAccessRoomForUser) { | |
| return; | |
| } |
There was a problem hiding this comment.
Ah, yes 🤔 since we are checking for jumpToMessage some lines above, it makes sense to use the shortcircuit here. Nice catch!
sampaiodiego
changed the title
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments