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

Better support for Base URI #212

Merged
2 commits merged into from
Mar 9, 2011
Merged

Better support for Base URI #212

2 commits merged into from
Mar 9, 2011

Conversation

vinibaggio
Copy link
Contributor

If an application that uses Omniauth is mounted under a sub URI (such as passenger_base_uri /sub_uri; ), OmniAuth is unable to match either callback_path or request_path.

Also, when generating the callback_url, it should be considered.

@josevalim
Copy link

It is important to notice that when passenger_base_uri is used, the passenger_base_uri is added as "SCRIPT_NAME" to rack. The fact that today Omniauth considers the SCRIPT_NAME to match the URL is wrong because this means developers need to worry about where their website will be "mounted" in production to properly configure their path_prefix.

On the other hand, SCRIPT_NAME should always be used on generation, otherwise we may link to an URL outside that sub url.

Therefore, +1 for this pull request and fix. :D

@graywh
Copy link

graywh commented Mar 4, 2011

Yes, this corrects the issues I've had, too.

@atd
Copy link

atd commented Mar 9, 2011

This also affects our project. Another +1 for this pull request

@mbleigh
Copy link
Contributor

mbleigh commented Mar 9, 2011

I had never actually looked at the SCRIPT_NAME part of Rack before...learn something new every day.

@graywh
Copy link

graywh commented Jun 14, 2012

This is still an issue for callback_path. + script_name should be moved from callback_url to callback_path.

This pull request was closed.
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

5 participants