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

Allow passing full_host in options #3

Merged
merged 1 commit into from
Oct 14, 2020

Conversation

delongtj
Copy link

@delongtj delongtj commented Oct 5, 2020

Not sure if this will apply to anyone else, but since our application identifies the account using a subdomain, Mailchimp's upcoming changes to require a fully-qualified callback URL for OAuth required changes on our end.

Since each user's subdomain will be unique, we had to set up a reserved subdomain to route all these requests through rather than relying on the default behavior which will pass https://<account_subdomain>.kindful.com as part of the callback URL and lead to a mismatch.

We'll run on this fork for now, but this allows us to pass in our reserved subdomain as the full_host, which did not seem possible before.

@espen
Copy link

espen commented Oct 6, 2020

@espen
Copy link

espen commented Oct 6, 2020

I see this code is also discussed and used in other omniauth gems so it should be fine to merge.

omniauth/omniauth-oauth2#93

WebTheoryLLC/omniauth-twitch@3032f76

Personally I found that supporting custom subdomains is problematic when making generic oauth strategies as some providers only allow one URL so we route all callbacks through a specific app domain.

@delongtj
Copy link
Author

delongtj commented Oct 6, 2020

Yes, that is the change I'm referencing.

We will be doing what you mention in your last comment and routing all OAuth requests through a specific subdomain. Even if Mailchimp allowed multiple URLs, we wouldn't want to add one for each account we have (thousands at this point).

I wavered on whether to pass in the full callback_url or only the full_host, and it seemed a little cleaner to pass the host and allow the rest of the path to use the existing methods. For our use-case, our app will use configuration to pass in the full_host option since we will have different values for local, qa, and production servers.

@stevenkaras stevenkaras self-assigned this Oct 6, 2020
@stevenkaras
Copy link
Owner

Finally had a chance to go over this. Looks good. I'll push out a new version tonight.

@stevenkaras stevenkaras merged commit 130b26a into stevenkaras:master Oct 14, 2020
@delongtj
Copy link
Author

@stevenkaras thanks for accepting. It will help us a lot, and I'm thinking at least a few others who use the gem on apps with multiple subdomains 👍

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.

None yet

3 participants