Details

Reviewers

ginsu
tomek
atul

Commits

rCOMM31b49cdf0e97: [keyserver] Add a function to toggle the pin status of a message

Summary

Create a function that toggles the pin status of a message provided the request of the type ToggleMessagePinRequest. We check the permissions for the requester to ensure they have the correct permissions to manage message pins.

Linear: https://linear.app/comm/issue/ENG-3184/add-a-function-to-toggle-the-pin-status-of-a-message

To not block review while I figure out how to alert the client of a message pin, I've created a follow-up task to decide what to replace Promise<void> with: https://linear.app/comm/issue/ENG-3392/alert-the-client-of-a-message-pin-unpin

Depends on D6928

(Note: I put this in thread-updaters since I plan to return some kind of robotext and it seems like convention that it's handled here (i.e. removeMembers, joinThread, etc). Feel free to correct me if I've not scoped the conventions correctly though).

Test Plan

The test plan for this function will be combined into the test plan for the subsequent diff, since the endpoints and responders will be set up there.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Mar 2 2023, 12:49 PM

Herald added a subscriber: ashoat. · View Herald TranscriptMar 2 2023, 12:49 PM

rohan edited the summary of this revision. (Show Details)Mar 2 2023, 12:50 PM

Harbormaster completed remote builds in B17061: Diff 23363.Mar 2 2023, 1:02 PM

rohan requested review of this revision.Mar 2 2023, 1:02 PM

Flip the isPinned condition

Harbormaster completed remote builds in B17063: Diff 23365.Mar 2 2023, 1:21 PM

tomek added inline comments.Mar 8 2023, 5:04 AM

keyserver/src/updaters/thread-updaters.js
848 ↗	(On Diff #23365)	How do we plan to inform other clients about message being pinned? Shouldn't we send some kind of update?
lib/types/thread-types.js
477 ↗	(On Diff #23365)	This might be a little surprising for a user of this api to send a request with `true` here which means that the message will be not pinned. Maybe we should rename this to simply `pin` or even `newPinState`, or something else, but less confusing?

Are we still waiting on D6924 before the rest of the stack is ready for review? Back to your queue for now, feel free to re-request review as-is if I should be reviewing it.

lib/types/thread-types.js
477 ↗	(On Diff #23365)	I agree, I think we could instead replace `isPinned` with `+action: 'pin' \| 'unpin'`. Thoughts?

This revision now requires changes to proceed.Mar 10 2023, 3:42 PM

rohan added inline comments.Mar 20 2023, 12:40 PM

keyserver/src/updaters/thread-updaters.js
848 ↗	(On Diff #23365)	Yeah I plan to handle that in https://linear.app/comm/issue/ENG-3190/include-robotext-for-the-chat-when-a-message-is-pinned-and-unpinned
lib/types/thread-types.js
477 ↗	(On Diff #23365)	Good point, I'll change `isPinned`

Update ToggleMessagePinRequest

Harbormaster completed remote builds in B17429: Diff 23883.Mar 20 2023, 1:35 PM

tomek added inline comments.Mar 21 2023, 1:44 AM

keyserver/src/updaters/thread-updaters.js
848 ↗	(On Diff #23365)	A robotext message is only one part of this. But we have to make sure that clients get the info that a message is pinned so that they can display it as pinned. Do we plan to introduce a new update type? Or is the plan different?

rohan added inline comments.Mar 23 2023, 3:54 PM

keyserver/src/updaters/thread-updaters.js
848 ↗	(On Diff #23365)	Between our conversation on D6924 and here, I've created a task to handle this to not block review here. https://linear.app/comm/issue/ENG-3392/alert-the-client-of-a-message-pin-unpin

rohan edited the summary of this revision. (Show Details)Mar 23 2023, 3:58 PM

tomek accepted this revision.Mar 24 2023, 9:19 AM

tomek added inline comments.

keyserver/src/updaters/thread-updaters.js
849–855 ↗	(On Diff #23883)	Is it a good idea to trust a client that that a message if from a thread with `threadID`? For example, a client could send a `threadID` of a thread where pinning messages is allowed, and `messageID` from a thread where it isn't. It should be a lot safer to only receive a `messageID` and fetch `threadID` from the db.
861 ↗	(On Diff #23883)	Is this comment still relevant? We're not checking in the query what is the pin status - we're only setting the status based on what we received in a request. Also, the whole function might be renamed, as we're not toggling.
863–875 ↗	(On Diff #23883)	Maybe it will be a little more maintainable if the query was defined in one place and only the values were computed conditionally?

rohan added inline comments.Mar 24 2023, 10:40 AM

keyserver/src/updaters/thread-updaters.js
849–855 ↗	(On Diff #23883)	From a security perspective, if we're worried about a client who has pin / unpin privileges in one thread making a request with a message from another thread, maybe an easier solution would be to update the query to UPDATE messages SET pinned = 1, pin_time = ${Date.now()} WHERE id = ${messageID} AND thread = ${threadID} would be a solution to prevent another query? From my thinking, this validates 1) permissions between thread and viewer, and 2) ensures that the requested message is part of the thread that the viewer has permission to pin in. If I'm missing something though, I can also fetch the `threadID` from the `messageID`.
861 ↗	(On Diff #23883)	Yeah I have the same opinion in my comment here - if we change it there I'll change it here
863–875 ↗	(On Diff #23883)	Yeah that's fair I'll update to do this instead

atul resigned from this revision.Mar 24 2023, 10:54 AM

This revision is now accepted and ready to land.Mar 24 2023, 10:54 AM

tomek added inline comments.Mar 24 2023, 11:31 AM

keyserver/src/updaters/thread-updaters.js
849–855 ↗	(On Diff #23883)	It also makes sense. But the downside is that we won't inform a client that setting pin failed. Still, adding a condition on thread is beneficial.

ashoat added inline comments.Mar 24 2023, 1:52 PM

keyserver/src/updaters/thread-updaters.js
849–855 ↗	(On Diff #23883)	Arguably we could just fetch the message and use the `threadID` from that

Fetch the threadID via the messageID, remove threadID from ToggleMessagePinRequest and consolidate the two togglePinQuery queries into one concrete query (all addressing feedback)

Harbormaster completed remote builds in B17608: Diff 24102.Mar 24 2023, 4:03 PM

tomek accepted this revision.Mar 27 2023, 1:35 AM

Rebase

Harbormaster completed remote builds in B17877: Diff 24492.Mar 31 2023, 4:31 AM

Rebase

Harbormaster completed remote builds in B18105: Diff 24805.Apr 7 2023, 9:08 AM

Rebase before landing

Harbormaster completed remote builds in B18145: Diff 24847.Apr 7 2023, 2:41 PM

Rebase before landing

Harbormaster completed remote builds in B18239: Diff 24982.Apr 11 2023, 9:00 AM

Closed by commit rCOMM31b49cdf0e97: [keyserver] Add a function to toggle the pin status of a message (authored by rohan, committed by RohanK6 <rohan.khanderia@gmail.com>). · Explain WhyApr 11 2023, 9:00 AM

This revision was automatically updated to reflect the committed changes.