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

feat: proxy avatars for Accounts_AvatarExternalProviderUrl #32824

Merged
merged 14 commits into from
Aug 21, 2024

Conversation

anicoa
Copy link
Contributor

@anicoa anicoa commented Jul 17, 2024

This PR adds the ability to proxy external avatars. Solves Issue #32812

Internal ticket: https://rocketchat.atlassian.net/browse/CONN-298

@anicoa anicoa requested review from a team as code owners July 17, 2024 22:08
Copy link

changeset-bot bot commented Jul 17, 2024

⚠️ No Changeset found

Latest commit: 977fc77

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@anicoa anicoa changed the title Implements proxy avatars for Accounts_AvatarExternalProviderUrl feat: proxy avatars for Accounts_AvatarExternalProviderUrl Jul 18, 2024
@casalsgh casalsgh added this to the 6.12 milestone Aug 19, 2024
Copy link
Contributor

dionisio-bot bot commented Aug 19, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@ggazzo ggazzo added the stat: QA assured Means it has been tested and approved by a company insider label Aug 19, 2024
@casalsgh casalsgh added the stat: ready to merge PR tested and approved waiting for merge label Aug 19, 2024
@dougfabris dougfabris removed stat: ready to merge PR tested and approved waiting for merge stat: QA assured Means it has been tested and approved by a company insider labels Aug 19, 2024
@ggazzo ggazzo modified the milestone: 6.12 Aug 20, 2024
@casalsgh casalsgh added the stat: QA assured Means it has been tested and approved by a company insider label Aug 20, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Aug 20, 2024
@KevLehman KevLehman added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Aug 20, 2024
@ggazzo ggazzo merged commit 1041e8c into RocketChat:develop Aug 21, 2024
43 of 45 checks passed
@anicoa
Copy link
Contributor Author

anicoa commented Sep 6, 2024

@tassoevan sorry if I get it wrong. But now - as the pr is merged now - where can i activate the proxying of the external avatar url through the rocketchat server itself? Can't find the feature I added the pr for in the admin section anymore ;)

@tassoevan
Copy link
Contributor

@anicoa We discussed internally if a setting made more sense than just always proxying all the avatars requests. As we didn't find any strong reasons for maintaining the old behavior, the setting was ditched while the new behavior was kept.

@anicoa
Copy link
Contributor Author

anicoa commented Sep 6, 2024

@tassoevan thanks for clarification and work. Found this right now in the code but could not reproduce the behaviour on a fresh updated 6.12 installation. Will try/investigate and report.

@anicoa
Copy link
Contributor Author

anicoa commented Sep 16, 2024

@tassoevan checked the functionality with one 6.12.1 and one 6.12 installation.

  1. Set Accounts_AvatarExternalProviderUrl to https://www.google.com/{username}
  2. Login
  3. Developer-Tools show request to https://www.google.com/foobar

Anything else I have to consider?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants