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

changing callback_path to callback_url to account for relative root url #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

angelkbrown
Copy link

When an app is deployed to a subdirectory of the webroot, so that the root url is like http://host/subdirectory, the authentication callback fails, since callback_path is passed as the action to the form, which causes the user to be directed to http://host/auth/ldap/callback.

This fix passes callback_url as the action to the form so that the form action is the full url: http://host/subdirectory/auth/ldap/callback for applications deployed to a subdirectory, or http://host/auth/ldap/callback for applications deployed in the host's webroot.

@pyu10055
Copy link
Collaborator

I believe you can set custom :callback_path option to /subdirectory/auth/ldap/callback

:callback_path and :request_path are part of omniauth base strategy, which ldap has included

@aldanor
Copy link

aldanor commented Mar 10, 2015

Has anyone figured how to resolve this?

Including the sub URI in the :callback_path seems wrong, because then on_callback_path? would fail (as it matches against it). So the correct solution would indeed be to use callback_url.

@aldanor
Copy link

aldanor commented Mar 10, 2015

TL;DR: this PR does look like a correct solution, are there are any reasons not to merge it in?

@jmccann
Copy link

jmccann commented Mar 18, 2015

I just ran into this. Tried to work around it by forcing the callback_path but env['omniauth.auth'] is nil. Seems like this is a known issue when you set :callback_path as is suggested as the workaround for this issue.

omniauth/omniauth#767

@aldanor
Copy link

aldanor commented Mar 18, 2015

@jmccann I've ran into the same issue -- the omniauth.auth is empty because is_on_*_path methods start behaving wrong (they are just matching substrings).

I was able to make it work by doing something like this:

module OmniAuthLDAPExt
    def request_phase
        @callback_path = nil  # latest version caches callback path
        path = options[:callback_path]
        options[:callback_path] = callback_url
        form = super
        options[:callback_path] = path
        form
    end
end

module OmniAuth
    module Strategies
        class LDAP
            prepend OmniAuthLDAPExt
        end
    end
end

@jmccann
Copy link

jmccann commented Mar 18, 2015

@aldanor Thanks! I'm testing monkeypatching in the fix right now doing the following as part of my setup:

module OmniAuth
  module Strategies
    class LDAP
      def request_phase
        OmniAuth::LDAP::Adaptor.validate @options
        f = OmniAuth::Form.new(:title => (options[:title] || "LDAP Authentication"), :url => callback_url)
        f.text_field 'Login', 'username'
        f.password_field 'Password', 'password'
        f.button "Sign In"
        f.to_response
      end
    end
  end
end

@aldanor
Copy link

aldanor commented Mar 18, 2015

@jmccann it's pretty much the same, I just wanted to avoid copying/pasting all the form code, hence using prepend / super.

@opensorceror
Copy link

This issue still persists...looks like this PR is not going to get merged anytime soon?

@opensorceror
Copy link

I was able to solve it using the following Nginx rewrite rule as suggested by @lud0vicb here:

rewrite ^/auth/ldap/(.*)$ /forum/auth/ldap/$1 last

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