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.

Sign up for GitHub

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

Jump to bottom

fix: allow gotrue to work with multiple custom domains #999

Merged
merged 14 commits into from
May 12, 2023
Merged

fix: allow gotrue to work with multiple custom domains #999

merged 14 commits into from
May 12, 2023

Conversation

kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented Mar 20, 2023 •
edited
Loading

What kind of change does this PR introduce?

  • Improves on feat: Support for OAuth callback proxy. #725, albeit with a slightly different approach
  • When a request is made to gotrue, gotrue will retrieve the hostname and use it in the email links / oauth callback.
  • This helps to make gotrue usable with multiple custom domains, and also allows the email links to contain the custom domain.

@kangmingtay kangmingtay self-assigned this Mar 20, 2023
@kangmingtay kangmingtay requested a review from a team as a code owner March 20, 2023 08:20
hf
hf reviewed Mar 30, 2023
View reviewed changes
Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

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

In general looks good, but how does it work with SAML? What are the SAML URLs advertized when you get the metadata?

@kangmingtay
Copy link
Member Author

@hf good catch, we can always display the current API_EXTERNAL_URL as the default, but if they use a custom domain in the metadata url, it should also go through the same check to verify that the domain being used is in the allow list

J0
J0 reviewed Apr 3, 2023
View reviewed changes
internal/mailer/mailer_test.go Outdated Show resolved Hide resolved
@J0
Copy link
Contributor

J0 commented Apr 3, 2023 •
edited
Loading

Thanks for your patience - generally LGTM as well. There's quite a bit of overlap with the PKCE PR - do. you want to get this in first? Will save quite a bit of conflict resolution

If we haven't yet we should also test this out with a proxy server

@kangmingtay
Copy link
Member Author

@J0 sounds good - lemme finish up the deployment plan first to make sure that it won't break any existing projects

If we haven't yet we should also test this out with a proxy server

I managed to test this out by editing the /etc/hosts file locally to map localhost:9999 to a custom domain. But i'll also put it on a project in staging to see if it works with the current custom domain set up.

kangmingtay
kangmingtay commented May 8, 2023
View reviewed changes
internal/mailer/mailer.go Outdated Show resolved Hide resolved
hf
hf reviewed May 9, 2023
View reviewed changes
Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

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

Looks OK, I'd just reconsider the hostname allow list as I don't think it does much.

internal/conf/configuration.go Outdated Show resolved Hide resolved
internal/mailer/template.go Outdated Show resolved Hide resolved
internal/mailer/template.go Outdated Show resolved Hide resolved
hf
hf reviewed May 10, 2023
View reviewed changes
internal/api/mail_test.go Outdated Show resolved Hide resolved
hf
hf approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@hf hf 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!

@kangmingtay kangmingtay merged commit 91a82ed into master May 12, 2023
1 check passed
@kangmingtay kangmingtay deleted the km/fix-custom-domain branch May 12, 2023 17:04
@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.67.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@hf hf mentioned this pull request May 22, 2023
Merged
hf added a commit that referenced this pull request May 23, 2023
@hf
fix: use configured redirect URL for external providers (#1114)
42bb1e0 
With #999 custom domains were introduced, however for OAuth, the
redirect URLs should in fact be the ones specified in the config and not
ones interpreted from the `X-Forwarded-Host` header.
@rmvh rmvh mentioned this pull request Sep 22, 2023
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hf hf hf approved these changes

@J0 J0 J0 approved these changes

Assignees

@kangmingtay kangmingtay

Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kangmingtay @J0 @hf