From 136713de4fe9a7c144760b25e7b2a97aad6c8148 Mon Sep 17 00:00:00 2001 From: George Claghorn Date: Tue, 28 Aug 2018 05:04:19 -0400 Subject: [PATCH] Implement the server-side OAuth 2.0 flow --- Gemfile.lock | 15 +++- Rakefile | 2 +- .../authorizations_controller.rb | 17 +++++ .../google_sign_in/base_controller.rb | 15 ++++ .../google_sign_in/callbacks_controller.rb | 24 ++++++ config/routes.rb | 4 + google_sign_in.gemspec | 1 + lib/google_sign_in.rb | 8 ++ lib/google_sign_in/engine.rb | 17 +++++ lib/google_sign_in/helper.rb | 73 +------------------ lib/google_sign_in/identity.rb | 6 +- lib/google_sign_in/redirect_protector.rb | 25 +++++++ test/{ => models}/identity_test.rb | 3 +- test/models/redirect_protector_test.rb | 34 +++++++++ test/test_helper.rb | 1 + 15 files changed, 168 insertions(+), 77 deletions(-) create mode 100644 app/controllers/google_sign_in/authorizations_controller.rb create mode 100644 app/controllers/google_sign_in/base_controller.rb create mode 100644 app/controllers/google_sign_in/callbacks_controller.rb create mode 100644 config/routes.rb create mode 100644 lib/google_sign_in/redirect_protector.rb rename test/{ => models}/identity_test.rb (81%) create mode 100644 test/models/redirect_protector_test.rb diff --git a/Gemfile.lock b/Gemfile.lock index 4f1d09c..f11b47c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -4,6 +4,7 @@ PATH google_sign_in (0.1.4) activesupport (>= 5.1) google-id-token (>= 1.4.0) + oauth2 (>= 1.4.0) GEM remote: https://rubygems.org/ @@ -15,12 +16,24 @@ GEM tzinfo (~> 1.1) byebug (9.1.0) concurrent-ruby (1.0.5) + faraday (0.12.2) + multipart-post (>= 1.2, < 3) google-id-token (1.4.2) jwt (>= 1) i18n (1.1.0) concurrent-ruby (~> 1.0) - jwt (2.1.0) + jwt (1.5.6) minitest (5.11.3) + multi_json (1.13.1) + multi_xml (0.6.0) + multipart-post (2.0.0) + oauth2 (1.4.0) + faraday (>= 0.8, < 0.13) + jwt (~> 1.0) + multi_json (~> 1.3) + multi_xml (~> 0.5) + rack (>= 1.2, < 3) + rack (2.0.5) rake (12.0.0) thread_safe (0.3.6) tzinfo (1.2.5) diff --git a/Rakefile b/Rakefile index a61ad18..bd1e2f4 100644 --- a/Rakefile +++ b/Rakefile @@ -4,7 +4,7 @@ require "rake/testtask" Rake::TestTask.new do |test| test.libs << "test" - test.test_files = FileList["test/*_test.rb"] + test.test_files = FileList["test/**/*_test.rb"] end task default: :test diff --git a/app/controllers/google_sign_in/authorizations_controller.rb b/app/controllers/google_sign_in/authorizations_controller.rb new file mode 100644 index 0000000..777f7e3 --- /dev/null +++ b/app/controllers/google_sign_in/authorizations_controller.rb @@ -0,0 +1,17 @@ +require 'securerandom' + +class GoogleSignIn::AuthorizationsController < GoogleSignIn::BaseController + def create + redirect_to login_url(scope: 'openid profile email', state: state), + flash: { proceed_to: params.require(:proceed_to), state: state } + end + + private + def login_url(**params) + client.auth_code.authorize_url(prompt: 'login', **params) + end + + def state + @state ||= SecureRandom.base64(64) + end +end diff --git a/app/controllers/google_sign_in/base_controller.rb b/app/controllers/google_sign_in/base_controller.rb new file mode 100644 index 0000000..340997c --- /dev/null +++ b/app/controllers/google_sign_in/base_controller.rb @@ -0,0 +1,15 @@ +require 'oauth2' + +class GoogleSignIn::BaseController < ActionController::Base + protect_from_forgery with: :exception + + private + def client + @client ||= OAuth2::Client.new \ + GoogleSignIn.client_id, + GoogleSignIn.client_secret, + authorize_url: 'https://accounts.google.com/o/oauth2/auth', + token_url: 'https://www.googleapis.com/oauth2/v3/token', + redirect_uri: callback_url + end +end diff --git a/app/controllers/google_sign_in/callbacks_controller.rb b/app/controllers/google_sign_in/callbacks_controller.rb new file mode 100644 index 0000000..d6d1387 --- /dev/null +++ b/app/controllers/google_sign_in/callbacks_controller.rb @@ -0,0 +1,24 @@ +require_dependency 'google_sign_in/redirect_protector' + +class GoogleSignIn::CallbacksController < GoogleSignIn::BaseController + def show + if valid_request? + redirect_to proceed_to_url, flash: { google_sign_in_token: id_token } + else + head :unprocessable_entity + end + end + + private + def valid_request? + flash[:state].present? && params.require(:state) == flash[:state] + end + + def proceed_to_url + flash[:proceed_to].tap { |url| GoogleSignIn::RedirectProtector.ensure_same_origin(url, request.url) } + end + + def id_token + client.auth_code.get_token(params.require(:code))['id_token'] + end +end diff --git a/config/routes.rb b/config/routes.rb new file mode 100644 index 0000000..ec50107 --- /dev/null +++ b/config/routes.rb @@ -0,0 +1,4 @@ +GoogleSignIn::Engine.routes.draw do + resource :authorization, only: :create + resource :callback, only: :show +end diff --git a/google_sign_in.gemspec b/google_sign_in.gemspec index 0dd34ae..85551ab 100644 --- a/google_sign_in.gemspec +++ b/google_sign_in.gemspec @@ -11,6 +11,7 @@ Gem::Specification.new do |s| s.add_dependency 'activesupport', '>= 5.1' s.add_dependency 'google-id-token', '>= 1.4.0' + s.add_dependency 'oauth2', '>= 1.4.0' s.add_development_dependency 'bundler', '~> 1.15' diff --git a/lib/google_sign_in.rb b/lib/google_sign_in.rb index 61d9312..cd10e97 100644 --- a/lib/google_sign_in.rb +++ b/lib/google_sign_in.rb @@ -1,2 +1,10 @@ +require 'active_support' +require 'active_support/rails' + +module GoogleSignIn + mattr_accessor :client_id + mattr_accessor :client_secret +end + require 'google_sign_in/identity' require 'google_sign_in/engine' if defined?(Rails) diff --git a/lib/google_sign_in/engine.rb b/lib/google_sign_in/engine.rb index f575270..9ca4ec6 100644 --- a/lib/google_sign_in/engine.rb +++ b/lib/google_sign_in/engine.rb @@ -2,6 +2,17 @@ module GoogleSignIn class Engine < ::Rails::Engine + isolate_namespace GoogleSignIn + + config.google_sign_in = ActiveSupport::OrderedOptions.new + + initializer 'google_sign_in.config' do + config.after_initialize do + GoogleSignIn.client_id = config.google_sign_in.client_id + GoogleSignIn.client_secret = config.google_sign_in.client_secret + end + end + initializer 'google_sign_in.helper' do ActiveSupport.on_load :action_controller do require 'google_sign_in/helper' @@ -14,5 +25,11 @@ class Engine < ::Rails::Engine GoogleSignIn::Identity.logger = Rails.logger end end + + initializer 'google_sign_in.mount' do |app| + app.routes.append do + mount GoogleSignIn::Engine, at: app.config.google_sign_in.root || 'google_sign_in' + end + end end end diff --git a/lib/google_sign_in/helper.rb b/lib/google_sign_in/helper.rb index 26c2e9f..8f9e9e8 100644 --- a/lib/google_sign_in/helper.rb +++ b/lib/google_sign_in/helper.rb @@ -1,76 +1,7 @@ -require 'google_sign_in/identity' - module GoogleSignIn module Helper - def google_sign_in(**form_options, &block) - content_for :head, - google_sign_in_javacript_include_tag + - google_sign_in_client_id_meta_tag + - turbolinks_reload_meta_tag - - google_sign_in_javascript_tag + - google_sign_in_hidden_form_tag(**form_options) + - google_sign_in_click_handler(&block) + def google_sign_in_button(text = nil, proceed_to:, **options, &block) + button_to text, google_sign_in.authorization_path(proceed_to: proceed_to), **options, &block end - - private - def google_sign_in_javacript_include_tag - javascript_include_tag "https://apis.google.com/js/api.js", async: true, defer: true, - onload: "this.onload=function(){};setupGoogleSignIn()", - onreadystatechange: "if (this.readyState === 'complete') this.onload()" - end - - def google_sign_in_client_id_meta_tag - tag.meta name: "google-signin-client_id", content: GoogleSignIn::Identity.client_id - end - - def turbolinks_reload_meta_tag - tag.meta name: "turbolinks-visit-control", content: "reload" - end - - def google_sign_in_hidden_form_tag(**options) - options.reverse_merge!(html: { style: "display: none" }) - - form_with(**options) do |form| - form.hidden_field(:google_id_token, id: "google_sign_in_token") + form.submit(id: "google_sign_in_submit") - end - end - - def google_sign_in_click_handler(&block) - tag.div(id: "google_sign_in_container", style: "visibility: hidden") { capture(&block) } - end - - def google_sign_in_javascript_tag - javascript_tag <<-JS.strip_heredoc - (function() { - function installAuthClient(callback) { - gapi.load("client:auth2", function() { - gapi.auth2.init().then(callback) - }) - } - - function installClickHandler() { - var element = document.getElementById("google_sign_in_container") - var options = new gapi.auth2.SigninOptionsBuilder() - options.setPrompt("select_account") - gapi.auth2.getAuthInstance().attachClickHandler(element, options, handleSignIn) - element.style.visibility = "visible" - } - - function handleSignIn(googleUser) { - var token = googleUser.getAuthResponse().id_token - if (token) { - document.getElementById("google_sign_in_token").value = token - document.getElementById("google_sign_in_submit").click() - gapi.auth2.getAuthInstance().signOut() - } - } - - window.setupGoogleSignIn = function() { - installAuthClient(installClickHandler) - } - })() - JS - end end end diff --git a/lib/google_sign_in/identity.rb b/lib/google_sign_in/identity.rb index ec6bf29..346aea9 100644 --- a/lib/google_sign_in/identity.rb +++ b/lib/google_sign_in/identity.rb @@ -3,8 +3,6 @@ module GoogleSignIn class Identity - class_attribute :client_id - class_attribute :validator self.validator = GoogleIDToken::Validator.new @@ -60,5 +58,9 @@ def ensure_proper_audience raise "Failed to locate the client_id #{client_id} in the authorized audience (#{@payload["aud"]})" end end + + def client_id + GoogleSignIn.client_id + end end end diff --git a/lib/google_sign_in/redirect_protector.rb b/lib/google_sign_in/redirect_protector.rb new file mode 100644 index 0000000..aa07333 --- /dev/null +++ b/lib/google_sign_in/redirect_protector.rb @@ -0,0 +1,25 @@ +require 'uri' + +module GoogleSignIn + module RedirectProtector + extend self + + class Violation < StandardError; end + + QUALIFIED_URL_PATTERN = /\A#{URI::DEFAULT_PARSER.make_regexp}\z/ + + def ensure_same_origin(target, source) + if target =~ QUALIFIED_URL_PATTERN && origin_of(target) != origin_of(source) + raise Violation, "Redirect target #{target} does not have same origin as request (#{source})" + end + end + + private + def origin_of(url) + uri = URI(url) + "#{uri.scheme}://#{uri.host}:#{uri.port}" + rescue ArgumentError + nil + end + end +end diff --git a/test/identity_test.rb b/test/models/identity_test.rb similarity index 81% rename from test/identity_test.rb rename to test/models/identity_test.rb index e121e5c..0a8b1e6 100644 --- a/test/identity_test.rb +++ b/test/models/identity_test.rb @@ -1,9 +1,8 @@ require 'test_helper' -require 'google_sign_in/identity' class GoogleSignIn::IdentityTest < ActiveSupport::TestCase test "client_id must be set" do - GoogleSignIn::Identity.client_id = nil + GoogleSignIn.client_id = nil assert_raises(ArgumentError) { GoogleSignIn::Identity.new("some_fake_token") } end diff --git a/test/models/redirect_protector_test.rb b/test/models/redirect_protector_test.rb new file mode 100644 index 0000000..39eae4c --- /dev/null +++ b/test/models/redirect_protector_test.rb @@ -0,0 +1,34 @@ +require 'test_helper' +require 'google_sign_in/redirect_protector' + +class GoogleSignIn::RedirectProtectorTest < ActiveSupport::TestCase + test "disallows URL target with different host than source" do + assert_raises GoogleSignIn::RedirectProtector::Violation do + GoogleSignIn::RedirectProtector.ensure_same_origin 'https://malicious.example.com', 'https://basecamp.com' + end + end + + test "disallows URL target with different port than source" do + assert_raises GoogleSignIn::RedirectProtector::Violation do + GoogleSignIn::RedirectProtector.ensure_same_origin 'https://basecamp.com:10443', 'https://basecamp.com' + end + end + + test "disallows URL target with different protocol than source" do + assert_raises GoogleSignIn::RedirectProtector::Violation do + GoogleSignIn::RedirectProtector.ensure_same_origin 'http://basecamp.com', 'https://basecamp.com' + end + end + + test "allows URL target with same origin as source" do + assert_nothing_raised do + GoogleSignIn::RedirectProtector.ensure_same_origin 'https://basecamp.com', 'https://basecamp.com' + end + end + + test "allows path target" do + assert_nothing_raised do + GoogleSignIn::RedirectProtector.ensure_same_origin '/callback', 'https://basecamp.com' + end + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index c05ba0c..b32968b 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -2,3 +2,4 @@ require 'active_support' require 'active_support/testing/autorun' require 'byebug' +require 'google_sign_in'