Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2278: Deleting attachments for expired and redacted messages #2278

Open
wants to merge 12 commits into
base: old_master
Choose a base branch
from

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Sep 3, 2019

Split off from #1763

Rendered

@ara4n ara4n added proposal A matrix spec change proposal proposal-in-review labels Sep 3, 2019
@ara4n ara4n changed the title MSC for deleting expired or redacted attachments MSC2278: Deleting expired or redacted attachments Sep 3, 2019
@ara4n ara4n changed the title MSC2278: Deleting expired or redacted attachments MSC2278: Deleting attachments for expired and redacted messages Sep 3, 2019
ara4n added a commit that referenced this pull request Sep 3, 2019
@ara4n
Copy link
Member Author

ara4n commented Sep 3, 2019

FCPing this because I think it works, and because we need it for privacy work.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Sep 3, 2019

This FCP proposal has been cancelled by #2278 (comment).

Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@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 3, 2019
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

I have some concerns, but they're mostly language ones. I think the solution is largely in the right direction though.

Would be good to over-specify the errors to avoid ambiguity: 403 M_FORBIDDEN, 404 M_NOT_FOUND when deleting things that don't exist, etc

proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
The user must be authenticated via access_token or Authorization header as the
original uploader or as a server admin in order to delete the content.

Servers may wish to quarantine the deleted content for some timeframe before
Copy link
Member

Choose a reason for hiding this comment

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

we should probably spec the quarantine API (in a different MSC, in the future, eventually)

proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
proposals/2278-deleting-content.md Show resolved Hide resolved
turt2live added a commit to t2bot/matrix-media-repo that referenced this pull request Sep 5, 2019
@turt2live
Copy link
Member

I ended up writing an implementation of this (by accident, async to this MSC):

proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
proposals/2278-deleting-content.md Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Sep 9, 2019

@mscbot concern we need to spec exactly who is allowed to delete an attachment

@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
ara4n and others added 3 commits June 11, 2020 21:44
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
@ara4n
Copy link
Member Author

ara4n commented Jun 11, 2020

sorry this got stuck forever; i think i completely missed the wave of feedback at the time. i believe i've now incorporated it all.

@richvdh
Copy link
Member

richvdh commented Jul 9, 2020

I think there are still too many outstanding questions for this to come near FCP, and I don't think we yet have a complete implementation. (In particular, as it currently stands, the MSC seems to place requirements on the homeserver as well as the media repo, so I don't think @turt2live 's implementation is complete).

As such:

@mscbot fcp cancel

@mscbot
Copy link
Collaborator

mscbot commented Jul 9, 2020

This proposal is not in FCP.

@richvdh
Copy link
Member

richvdh commented Jul 9, 2020

oh, duh, it never got into FCP.

@richvdh
Copy link
Member

richvdh commented Jul 9, 2020

@mscbot resolve we need to spec exactly who is allowed to delete an attachment

1 similar comment
@richvdh
Copy link
Member

richvdh commented Jul 9, 2020

@mscbot resolve we need to spec exactly who is allowed to delete an attachment

@mscbot
Copy link
Collaborator

mscbot commented Jul 9, 2020

Unknown concern 'we need to spec exactly who is allowed to delete an attachment'.

### Encrypted rooms

There is no way for server to know what events refer to which MXC URL, so we
leave it up to the client to DELETE any MXC URLs referred to by an event after
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by this: assuming that there is more than one client in a given room, which has responsibility for making the DELETE request? I guess it has to be a client belonging to the original uploader, but what if they go away/stop watching the room/etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would have to be the original uploader, but indeed, that doesn't help if someone else redacts their event for them and they don't come back and finish it off by deleting the media.

@richvdh
Copy link
Member

richvdh commented Jan 19, 2021

@mscbot fcp cancel

there's no working impl for this yet, so it's not ready for FCP.

@mscbot mscbot added proposal-in-review and removed disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Jan 19, 2021
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
Comment on lines +68 to +72
It seems reasonable to consider the special case of clients forwarding
encrypted attachments between rooms as a 'copy by reference' - if the
original event gets deleted, the copies should too. If this isn't desired,
then the attachment should have been reencrypted and stored as a separate
instance in the media repo.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit unfortunate, but I can't say whether or not that's the right thing to do.

I quite often forward encrypted media between rooms, I also think sticker packs (and similar) could run into this instance.

Re-uploading media does seem to be the privacy-preserving approach compared to having to do some refcounting and linking each message to the media (though I wouldn't guarantee that you couldn't correlate e.g. the filesize of the sticker, etc), but I fear it may be rubbish for UX, and especially on slow/mobile connections (where we also might want to avoid reuploading to reduce overall data transmitted).

Finally, if you forward the message and then delete the forwarded copy, how could this be distinguished from deleting the original as it stands?

Deeply conflicted here because I really wish the media store wasn't a monotonically growing area on my disk (without having to resort to heuristic methods like expiring media that hasn't been accessed in a while), but privacy is of course important.


As an unrefined idea: Perhaps we could allow asking the homeserver to clone an existing piece of media to a new MXC URI? As a trade-off, my application might prefer to have the server do the copy (the actual implementation would likely be smarter than that — see hard links on a filesystem) and forward the 'new' MXC URI into a e.g. sticker event.

The rest of your proposal can still stand — if I delete a message, its MXC URI can get unlinked alongside that. I've lost some privacy potentially (though it's arguable, since if you know what sticker pack I use in my conversations a lot, the filesize probably gives it away by equal measure), but it was my choice and I'm not stopping you or anyone else from having your reencrypt-on-copy implementation when that suits the trade-off better.

### Encrypted rooms

There is no way for server to know what events refer to which MXC URL, so we
leave it up to the client to DELETE any MXC URLs referred to by an event after
Copy link
Contributor

Choose a reason for hiding this comment

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

It would have to be the original uploader, but indeed, that doesn't help if someone else redacts their event for them and they don't come back and finish it off by deleting the media.

@ara4n
Copy link
Member Author

ara4n commented Sep 3, 2024

i believe this is obsoleted by #3911 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants