[keyserver] Prevent non-composable message types from being pinned on the keyserver
ClosedPublic
Actions

Authored by rohan on Sep 19 2023, 11:04 PM.

Details

Reviewers

atul
ginsu
ashoat

Commits

rCOMM2954159c828e: [keyserver] Prevent non-composable message types from being pinned on the…

Summary

On the client, we prevent non-composable message types (specifically robotext) messages from being pinned. This means only text and multimedia should be able to be pinned. In light of the urgent issues arising with source messages / inconsistency across keyserver and client, it'd be good to make sure we also cover this check directly on the server when a client requests to pin/unpin a message.

One thing to note is that with ENG-4981, one of the short term solutions was to just disable the pin action for sidebar source messages. If I can't resolve it and have to resort to the short-term solution, I'll do that in a different diff to keep this one just a 'unifying client and server logic' change.

Here, we throw a error if the target message that is trying to be pinned is not composable, matching the behavior on the client.

Depends on D9238

Part of ENG-4849

Test Plan

Confirmed that passing a non-composable message ID in the payload ends up in a server error

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Sep 19 2023, 11:04 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptSep 19 2023, 11:04 PM

rohan edited the summary of this revision. (Show Details)Sep 19 2023, 11:07 PM

Harbormaster completed remote builds in B22751: Diff 31294.Sep 19 2023, 11:21 PM

rohan requested review of this revision.Sep 19 2023, 11:21 PM

One thing to note is that with ENG-4981, one of the short term solutions was to just disable the pin action for sidebar source messages.

I think you linked the wrong task. Did you mean ENG-4849? ENG-4981 doesn't seem to be related to pins.

one of the short term solutions was to just disable the pin action for sidebar source messages.

I'm a little confused about this... was this a proposed solution, or an implemented solution? If proposed, can you link the exact comment that proposed it? If implemented, can you link the diff?

Here, we throw a error if the target message that is trying to be pinned is not composable, matching the behavior on the client.

That makes sense to me. Separately, I'm realizing that the set of messages from which a sidebar can be created is NOT the same as the set of messages that can be pinned. As a result, my proposed solution in this Linear comment probably is insufficient... I'm guessing we'll need at least 3 new fields in message specs, rather than 2. We can discuss it more in our 1:1 later today and come up with a more detailed plan.

This revision is now accepted and ready to land.Sep 20 2023, 6:08 AM

Closed by commit rCOMM2954159c828e: [keyserver] Prevent non-composable message types from being pinned on the… (authored by rohan). · Explain WhySep 20 2023, 9:21 AM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM2954159c828e: [keyserver] Prevent non-composable message types from being pinned on the….