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

Update MSC4039 #1

Merged

Conversation

HarHarLinks
Copy link

@HarHarLinks HarHarLinks commented Aug 7, 2024

updates matrix-org#4039

changelog:

  • added download API to handle MSC3916 "Authentication for media"
  • added e2ee aware variants for both upload and download

policy stuff:

  • All your commits have a Signed-off-by line in the message (more info).

@HarHarLinks HarHarLinks force-pushed the nic/feat/widgetapi-content-repo branch 3 times, most recently from 096ef1c to f58482e Compare August 7, 2024 16:47
- added download API to handle MSC3916 "Authentication for media"
- added e2ee aware variants for both upload and download

Signed-off-by: Kim Brose <kim.brose@nordeck.net>
@HarHarLinks HarHarLinks force-pushed the nic/feat/widgetapi-content-repo branch from f58482e to 4784869 Compare August 7, 2024 16:56
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
@HarHarLinks
Copy link
Author

HarHarLinks commented Aug 13, 2024

TODO: review wrt. actual current implementation

TODO: deprecate matrix_base_url widget URL param https://github.com/matrix-org/matrix-widget-api/blob/3c9543cbe4255869953d1b79d513de2eaf87033e/src/templating/url-template.ts#L48-L49

Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
@HarHarLinks HarHarLinks force-pushed the nic/feat/widgetapi-content-repo branch from 3c2aa8c to 08d4b7f Compare August 13, 2024 16:31
@HarHarLinks HarHarLinks marked this pull request as ready for review August 13, 2024 16:32
Copy link

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Looks good overall. 👍 Some smaller remarks

XMLHttpRequestBodyInit can also be form data or URL search params. Maybe we should use a binary format instead (array or blob…)?

unauthenticated download endpoint, provided they acquired the URL of the homeserver, the widget API does not currently
contain a way to authenticate with the upload and new download endpoints nor does it otherwise expose these functions
from the client hosting the widget to the widget itself. This would be useful to post images or attachments, but also to
upload larger data that can't easily be stored in a room event, like a whiteboard's content that can easily grow larger

Choose a reason for hiding this comment

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

I would keep this more general, something like

„but also to upload larger data that can't easily be stored in a room event exceeding the 64kb event size limit“

Copy link
Author

Choose a reason for hiding this comment

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

this is a leftover from the original MSC; I think illustrating the point preempts questions such as "whatever could be so big!" but am also fine with changing it 🤷

proposals/4039-widget-api-media.md Show resolved Hide resolved
Signed-off-by: Kim Brose <kim.brose@nordeck.net>
@HarHarLinks
Copy link
Author

HarHarLinks commented Aug 14, 2024

XMLHttpRequestBodyInit can also be form data or URL search params. Maybe we should use a binary format instead (array or blob…)?

while possibly true, I deliberately did not touch this (which you didn't see because the diff hides unchanged parts - see the upload direction) as it describes the existing implementation of the MSC prior to this PR.

looking into the link you provided

typedef (Blob or BufferSource or FormData or URLSearchParams or USVString) XMLHttpRequestBodyInit;

sounds like XMLHttpRequestBodyInit can also be a Blob. Sounds then like it could be easy to change if we are the only ones using this API and already using Blob.

An alternative could be to create a new MSC and hence a new unstable endpoint, and having to care less about backwards compatibility.

This appears to be the original reasoning for the genericness of the interface: https://github.com/matrix-org/matrix-widget-api/pull/86/files#diff-67e6f0676307a87283ca4533526de9b3ac4ff3bafdd0d016947e40e2b3574757R693-R694 🤷

@weeman1337
Copy link

Should we directly update the MSC pull request or first implement it to see if it works?

@HarHarLinks
Copy link
Author

We should start implementing (at least the authed media parts) right away regardless, but I would also suggest to get some SCT (and maybe Element) eyes on it asap, particularly in terms of Authed Media and possible support/alignment with that.

@HarHarLinks HarHarLinks merged commit 514e8be into nic/feat/widgetapi-upload-files Aug 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants