simi / omniauth-facebook

Facebook OAuth2 Strategy for OmniAuth
https://simi.github.io/omniauth-facebook/
1.26k stars 403 forks source link

Allow users to dynamically set callback url #310

Closed DaiDuongPixta closed 4 years ago

DaiDuongPixta commented 6 years ago

@mkdynamic I create this pull request to let users dynamically set callback url by putting redirect_uri=#{some_url} in request. Currently users can only set callback_url using options in initializers/devise.rb or initializers/ominauth.rb. But with this method, there is only one callback path. So if developers want to have customized callback URLs for each kind of request, they may need to use constraints. If we can dynamically set callback path, it is easier to have multiple omniauth request/callback pairs for different purpose. For example:

  1. For sign up: request = /users/auth/facebook?redirect_uri=signup_path
  2. For deactivate account: request = /users/auth/facebook?redirect_uri=deactivate_account_path

FYI: I learn this approach from google_oauth2 : https://github.com/omniauth/omniauth/issues/593#issuecomment-29777604 (I had tried it, and it worked on my own app).

simi commented 6 years ago

Isn't there security risk with this approach by providing custom crafted url /users/auth/facebook?redirect_uri=malicious_path to user? I don't remember now if FB is checking at least domain when redirecting back.

DaiDuongPixta commented 6 years ago

Isn't there security risk with this approach by providing custom crafted url /users/auth/facebook?redirect_uri=malicious_path to user? I don't remember now if FB is checking at least domain when redirecting back.

FB (and other providers) has a whitelist of valid redirect URLs that app owners can edit. Developers can put reliable callback URLs in this list. So FB won't make a callback to malicious_path (of course if malicious_path is NOT in the whitelist). And I think approach is just a more flexible way of customizing redirect URL. Without this approach, developers are still able to customize it, but by putting the customized URL in initializers/devise.rb. (But can create just one callback URL only).

milushov commented 6 years ago

Useful feature 👍

DaiDuongPixta commented 6 years ago

@simi @mkdynamic Do you have any other concerns?

foloinfo commented 5 years ago

Hi, I wanted to have this feature so I tried, but it didn't work for me.

When you override callback url, it will return request.env['omniauth.auth'] as nil, because omniauth gem checks if it's on callback path

I also tried to overcome this problem by allowing options[:callback_path] to accept Array, and callback phase was actually initiated, but failed due to omniauth: (facebook) Authentication failure! invalid_credentials: OAuth2::Error, {"message"=>"Error validating verification code. Please make sure your redirect_uri is identical to the one you used in the OAuth dialog request error.

Here is the code I put in initializer to monkeypatch omniauth strategy.

module OmniAuth
  module Strategies
    class Facebook < OmniAuth::Strategies::OAuth2
      def callback_url
        if @authorization_code_from_signed_request_in_cookie
          ''
        else
          # from this pull-req and modified to allow array of callback_path
          request.params['redirect_uri'] || options[:callback_url] || (full_host + script_name + callback_path.first)
        end
      end

      def on_callback_path?
        if(callback_path.is_a?(Array))
          callback_path.map{ |path| on_path?(path) }.any?
        else
          on_path?(callback_path)
        end
      end

      def callback_path
        if options[:callback_path].is_a?(Array)
          @callback_path = options[:callback_path]
        else
          @callback_path ||= begin
            path = options[:callback_path] if options[:callback_path].is_a?(String)
            path ||= current_path if options[:callback_path].respond_to?(:call) && options[:callback_path].call(env)
            path ||= custom_path(:request_path)
            path ||= "#{path_prefix}/#{name}/callback"
            path
          end
        end
      end

    end
  end
end

Does anybody have a solution for this? It will be really nice if I could dynamically set redirect_uri for callbacks.

github-actions[bot] commented 4 years ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.