Skip to content

MSC2285: Private read receipts#2285

Merged
turt2live merged 30 commits intoold_masterfrom
travis/msc/hidden-read-receipts
Jul 17, 2022
Merged

MSC2285: Private read receipts#2285
turt2live merged 30 commits intoold_masterfrom
travis/msc/hidden-read-receipts

Conversation

@turt2live
Copy link
Copy Markdown
Member

@turt2live turt2live commented Sep 6, 2019

@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Sep 6, 2019
@turt2live turt2live changed the title Hidden read receipts MSC2285: Hidden read receipts Sep 6, 2019
@turt2live

This comment was marked as outdated.

@ara4n ara4n self-requested a review September 6, 2019 17:13
Copy link
Copy Markdown
Member

@ara4n ara4n left a comment

Choose a reason for hiding this comment

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

LGTM

jevolk added a commit to jevolk/charybdis that referenced this pull request Sep 6, 2019
jevolk added a commit to jevolk/charybdis that referenced this pull request Sep 6, 2019
Copy link
Copy Markdown

@joepie91 joepie91 left a comment

Choose a reason for hiding this comment

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

On a HS that doesn't implement this MSC (yet), if a user were to 'disable' read receipts (ie. tell their client to make them hidden), it would silently fail to do as the user requested, because the HS would not recognize the flag.

This should probably at least be listed under the security considerations as a 'fail open' case, or ideally be resolved - maybe through a feature flag of some sort that explicitly indicates to the client that this feature is supported? Or if this will be a required part of the spec in a next version of the protocol, the spec should probably clearly indicate that this feature only exists since that particular version (so that clients know not to offer the feature on lower protocol versions).

Edit: Not really a 'nit', I suppose. Edited accordingly.

@turt2live
Copy link
Copy Markdown
Member Author

@joepie91 in general, please use line comments so conversation isn't lost. However, I hope i can respond in one comment here:

This is already partially identified in the security considerations section (if you swap out "ignore" for "unaware", at least). The expectation is that this lands in a version of the specification, which is where the MUST takes effect - servers would be non-compliant if they ignored the flag and advertised that they supported the version.

@ara4n
Copy link
Copy Markdown
Member

ara4n commented Sep 9, 2019

@mscbot fcp merge

@mscbot

This comment has been minimized.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Sep 9, 2019
@joepie91
Copy link
Copy Markdown

joepie91 commented Sep 10, 2019

@turt2live I was hoping that the 'review' feature would produce a thread like a line comment would (since this is a general comment, not tied to a specific line), but apparently not :)

Given that it's a MUST, I would propose explicitly adding a note to the spec that says "this flag did not exist before version X of the spec, and all read receipts are assumed to be visible there", so that clients which implement multiple spec versions (retroactively) can take this into account in how they design their UI.

I'm not sure whether such an "add note to the spec" point would be part of the MSC itself, or tracked separately.

@turt2live
Copy link
Copy Markdown
Member Author

It would be done at spec PR time - MSCs generally don't try and influence the exact wording of the spec given they are more informal documents than the formal spec.

@turt2live
Copy link
Copy Markdown
Member Author

@mscbot concern There should be an account data config option for this so clients can honour it always.

@turt2live
Copy link
Copy Markdown
Member Author

I'm cancelling FCP on this because I don't think it's ready for that yet, and I want to add a few more things to it:

@mscbot fcp cancel

@mscbot
Copy link
Copy Markdown
Collaborator

mscbot commented Sep 24, 2019

@turt2live proposal cancelled.

@mscbot mscbot removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Sep 24, 2019
@turt2live turt2live changed the title MSC2285: Hidden read receipts [WIP] MSC2285: Hidden read receipts Sep 24, 2019
@turt2live turt2live self-assigned this Sep 24, 2019
@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
@KitsuneRal
Copy link
Copy Markdown
Member

@mscbot resolve /receipt assumes the payload to be federatable
@mscbot resolve The paragraph in Alternatives about account data is confusing

Co-authored-by: Kévin Commaille <76261501+zecakeh@users.noreply.github.com>
@mscbot
Copy link
Copy Markdown
Collaborator

mscbot commented Jul 17, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Jul 17, 2022
@turt2live turt2live merged commit 5a75c7a into old_master Jul 17, 2022
@turt2live
Copy link
Copy Markdown
Member Author

Spec PR: matrix-org/matrix-spec#1216

@turt2live
Copy link
Copy Markdown
Member Author

Merged 🎉

@Bugsbane
Copy link
Copy Markdown

Do you know if there's any issue to watch, regarding getting this MSC supported in Elerment (Android/iOS/web)?

@SimonBrandner
Copy link
Copy Markdown
Contributor

Do you know if there's any issue to watch, regarding getting this MSC supported in Element (Android/iOS/web)?

It's already implemented in Element Web/Desktop, for Android and iOS: element-hq/element-android#2436 and element-hq/element-ios#4646 respectively

@Bugsbane
Copy link
Copy Markdown

Oh, freaking sweet! This feature was the last blocker before rolling out my Synapse server. Super happy it's already in Element, too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client-server Client-Server API kind:core MSC which is critical to the protocol's success merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.