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

Authenticate media requests when loading avatars #2856

Merged
merged 8 commits into from
Dec 6, 2024

Conversation

Half-Shot
Copy link
Member

Fixes #2845

This introduces support for authenticated matrix media for avatars. All media requests are now made via fetch and then the result is stored as a local object url.

Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

What is the intended behaviour when EC is running as a widget?

@Half-Shot
Copy link
Member Author

Half-Shot commented Dec 2, 2024

What is the intended behaviour when EC is running as a widget?

😢 , I suspect. There isn't an API presently to fetch media over the widget API and we don't have a token to use that could do it. I wish for a world where we get scoped OIDC tokens and can just use the actual API. matrix-org/matrix-widget-api#115 would be useful.

I think for the moment we could either still support unauthenticated media for widgets while new avatars slowly replace the old ones and we lose access to them. Or we could just show placeholders for all avatars until we can properly support authenticated media.

@hughns
Copy link
Member

hughns commented Dec 2, 2024

What is the intended behaviour when EC is running as a widget?

😢 , I suspect. There isn't an API presently to fetch media over the widget API and we don't have a token to use that could do it. I wish for a world where we get scoped OIDC tokens and can just use the actual API. matrix-org/matrix-widget-api#115 would be useful.

I think for the moment we could either still support unauthenticated media for widgets while new avatars slowly replace the old ones and we lose access to them. Or we could just show placeholders for all avatars until we can properly support authenticated media.

If the PR can be updated to make it work with the widget mode fallback (with appropriate tests too), then I think we could merge this PR without having to figure out the whole widget API solution.

@hughns
Copy link
Member

hughns commented Dec 6, 2024

@fkwp as this is currently proposed user profile thumbnails will no longer show when running in widget/embedded mode. Is this acceptable? or should EC be continuing to use non-authenticated media requests when running as a widget?

Marking as draft with X-Needs-Product label until answered.

@hughns hughns marked this pull request as draft December 6, 2024 12:22
@Half-Shot
Copy link
Member Author

https://matrix.to/#/!tDLCaLXijNtYcJZEey:element.io/$rljmYPXis_Ja28l-Z7_XIqkdgC7Y3TmqFFxssHj1o4Q?via=element.io&via=matrix.org&via=half-shot.uk

So I believe we were okay to merge this now, and then fix the thumbnail in widgets later as we didn't deem it critical enough. I was going to look at expanding the widget API to do so as a next-up item.

@Half-Shot Half-Shot removed the X-Needs-Product More input needed from the Product team label Dec 6, 2024
@Half-Shot Half-Shot marked this pull request as ready for review December 6, 2024 13:46
@hughns
Copy link
Member

hughns commented Dec 6, 2024

So I believe we were okay to merge this now, and then fix the thumbnail in widgets later as we didn't deem it critical enough. I was going to look at expanding the widget API to do so as a next-up item.

👌 I made #2876 to add the feature back in.

Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

One typo. Otherwise looks good. 👍

src/Avatar.test.tsx Outdated Show resolved Hide resolved
Co-authored-by: Hugh Nimmo-Smith <[email protected]>
@Half-Shot Half-Shot merged commit 9d4cd21 into livekit Dec 6, 2024
5 checks passed
@dillfrescott
Copy link

Will this merge be in a release soon?

@hughns
Copy link
Member

hughns commented Jan 27, 2025

Will this merge be in a release soon?

There is currently a release blocker which is now being tracked at #2972

@dillfrescott
Copy link

Thank you!

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.

Support for authenticated media / profile pictures fail to load
3 participants