diff --git a/.travis.yml b/.travis.yml index 28a0ca8..a01d453 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,10 +1,3 @@ language: ruby rvm: - - 1.8.7 - - 1.9.2 - - 1.9.3 - - jruby-18mode # JRuby in 1.8 mode - - jruby-19mode # JRuby in 1.9 mode - - rbx-18mode - - ree - - ruby-head + - 2.4.1 diff --git a/CHANGELOG.md b/CHANGELOG.md index 112c0e9..6979168 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,16 @@ +v1.2.0 2015/02/18 + +* Implement single sign out for cookie sessions of GitHub properties + +v1.0.3 2014/12/13 + +* Reintroduce membership caching to reduce API hits for validating team membership. + +v1.0.1 2014/03/24 +----------------- + +* Handle multiple X-Forwarded-Proto headers when comma delimited + v1.0.0 2013/09/03 ----------------- diff --git a/Gemfile b/Gemfile index b4d4a51..caa0989 100644 --- a/Gemfile +++ b/Gemfile @@ -1,7 +1,6 @@ source "https://rubygems.org" -gem 'debugger', :platforms => :ruby_19, :require => false -gem 'ruby-debug', :platforms => :ruby_18, :require => false +gem 'byebug', require: false # Specify your gem's dependencies in warden-github.gemspec gemspec diff --git a/README.md b/README.md index 4b61475..4f26c34 100644 --- a/README.md +++ b/README.md @@ -48,14 +48,19 @@ use Warden::Manager do |config| config.failure_app = BadAuthentication config.default_strategies :github - config.scope_defaults :default, :config => { :scope => 'user:email' } - config.scope_defaults :admin, :config => { :client_id => 'foobar', - :client_secret => 'barfoo', - :scope => 'user,repo', - :redirect_uri => '/admin/oauth/callback' } + config.scope_defaults :default, config: { scope: 'user:email' } + config.scope_defaults :admin, config: { client_id: 'foobar', + client_secret: 'barfoo', + scope: 'user,repo', + redirect_uri: '/admin/oauth/callback' } + + config.serialize_from_session { |key| Warden::GitHub::Verifier.load(key) } + config.serialize_into_session { |user| Warden::GitHub::Verifier.dump(user) } end ``` +The two serialization methods store the API token in the session securely via the `WARDEN_GITHUB_VERIFIER_SECRET` environmental variable. + ### Parameters The config parameters and their defaults are listed below. @@ -116,6 +121,7 @@ user.id # => The GitHub user id. user.login # => The GitHub username. user.name user.gravatar_id # => The md5 email hash to construct a gravatar image. +user.avatar_url user.email # => Requires user:email or user scope. user.company @@ -137,6 +143,32 @@ If you're looking for an easy way to integrate this into a Sinatra or Rails appl - [sinatra_auth_github](https://github.com/atmos/sinatra_auth_github) - [warden-github-rails](https://github.com/fphilipe/warden-github-rails) +## Single Sign Out + +OAuth applications owned by the GitHub organization are sent an extra browser parameter to ensure that the user remains logged in to github.com. Taking advantage of this is provided by a small module you include into your controller and a before filter. Your `ApplicationController` should resemble something like this. + + +```ruby +class ApplicationController < ActionController::Base + include Warden::GitHub::SSO + + protect_from_forgery with: :exception + + before_filter :verify_logged_in_user + + private + + def verify_logged_in_user + unless github_user && warden_github_sso_session_valid?(github_user, 120) + request.env['warden'].logout + request.env['warden'].authenticate! + end + end +end +``` + +You can also see single sign out in action in the example app. + ## Additional Information - [warden](https://github.com/hassox/warden) diff --git a/Rakefile b/Rakefile index 7422ba1..3b9f150 100644 --- a/Rakefile +++ b/Rakefile @@ -3,7 +3,7 @@ require 'rubygems/specification' require 'date' require 'bundler' -task :default => [:spec] +task default: [:spec] require 'rspec/core/rake_task' desc "Run specs" diff --git a/config.ru b/config.ru index 10e1170..2415247 100644 --- a/config.ru +++ b/config.ru @@ -1,19 +1,6 @@ ENV['RACK_ENV'] ||= 'development' -begin - require File.expand_path('../.bundle/environment', __FILE__) -rescue LoadError - require "rubygems" - require "bundler" - Bundler.setup -end - -begin - require 'debugger' -rescue LoadError - require 'ruby-debug' -end - +require "bundler/setup" require 'warden/github' if ENV['MULTI_SCOPE_APP'] diff --git a/example/multi_scope_app.rb b/example/multi_scope_app.rb index 82454ac..edbcccc 100644 --- a/example/multi_scope_app.rb +++ b/example/multi_scope_app.rb @@ -5,15 +5,15 @@ class MultiScopeApp < BaseApp enable :inline_templates GITHUB_CONFIG = { - :client_id => ENV['GITHUB_CLIENT_ID'] || 'test_client_id', - :client_secret => ENV['GITHUB_CLIENT_SECRET'] || 'test_client_secret' + client_id: ENV['GITHUB_CLIENT_ID'] || 'test_client_id', + client_secret: ENV['GITHUB_CLIENT_SECRET'] || 'test_client_secret' } use Warden::Manager do |config| config.failure_app = BadAuthentication config.default_strategies :github - config.scope_defaults :default, :config => GITHUB_CONFIG - config.scope_defaults :admin, :config => GITHUB_CONFIG.merge(:scope => 'user,notifications') + config.scope_defaults :default, config: GITHUB_CONFIG + config.scope_defaults :admin, config: GITHUB_CONFIG.merge(scope: 'user,notifications') end get '/' do @@ -26,7 +26,7 @@ class MultiScopeApp < BaseApp end get '/admin/login' do - env['warden'].authenticate!(:scope => :admin) + env['warden'].authenticate!(scope: :admin) redirect '/' end diff --git a/example/simple_app.rb b/example/simple_app.rb index 46407ef..9a083f3 100644 --- a/example/simple_app.rb +++ b/example/simple_app.rb @@ -2,18 +2,28 @@ module Example class SimpleApp < BaseApp + include Warden::GitHub::SSO + enable :inline_templates GITHUB_CONFIG = { - :client_id => ENV['GITHUB_CLIENT_ID'] || 'test_client_id', - :client_secret => ENV['GITHUB_CLIENT_SECRET'] || 'test_client_secret', - :scope => 'user' + client_id: ENV['GITHUB_CLIENT_ID'] || 'test_client_id', + client_secret: ENV['GITHUB_CLIENT_SECRET'] || 'test_client_secret', + scope: 'user' } use Warden::Manager do |config| config.failure_app = BadAuthentication config.default_strategies :github - config.scope_defaults :default, :config => GITHUB_CONFIG + config.scope_defaults :default, config: GITHUB_CONFIG + config.serialize_from_session { |key| Warden::GitHub::Verifier.load(key) } + config.serialize_into_session { |user| Warden::GitHub::Verifier.dump(user) } + end + + def verify_browser_session + if env['warden'].user && !warden_github_sso_session_valid?(env['warden'].user, 10) + env['warden'].logout + end end get '/' do @@ -21,11 +31,13 @@ class SimpleApp < BaseApp end get '/profile' do + verify_browser_session env['warden'].authenticate! erb :profile end get '/login' do + verify_browser_session env['warden'].authenticate! redirect '/' end @@ -66,7 +78,7 @@ def self.app @@ index <% if env['warden'].authenticated? %>

- .png?r=PG&s=50' /> + ' width='50' height='50' /> Welcome <%= env['warden'].user.name %>

<% else %> @@ -84,4 +96,12 @@ def self.app
<%= env['warden'].user.team_member?(632) %>
GitHub Site Admin:
<%= env['warden'].user.site_admin? %>
+ <% if env['warden'].user.using_single_sign_out? %> +
GitHub Browser Session ID
+
<%= env['warden'].user.browser_session_id %>
+
GitHub Browser Session Valid
+
<%= warden_github_sso_session_valid?(env['warden'].user, 10) %>
+
GitHub Browser Session Verified At
+
<%= Time.at(warden_github_sso_session_verified_at) %>
+ <% end %> diff --git a/lib/warden/github.rb b/lib/warden/github.rb index e4c8012..7a5b87f 100644 --- a/lib/warden/github.rb +++ b/lib/warden/github.rb @@ -1,5 +1,8 @@ +require 'json' require 'warden' +require 'octokit' +require 'warden/github/sso' require 'warden/github/user' require 'warden/github/oauth' require 'warden/github/version' @@ -7,5 +10,7 @@ require 'warden/github/hook' require 'warden/github/config' require 'warden/github/membership_cache' +require 'warden/github/verifier' +require 'active_support/message_verifier' require 'securerandom' diff --git a/lib/warden/github/config.rb b/lib/warden/github/config.rb index 0248fe9..3ab4b4e 100644 --- a/lib/warden/github/config.rb +++ b/lib/warden/github/config.rb @@ -43,10 +43,10 @@ module GitHub # # # This configures an additional scope that uses the github strategy # # with custom configuration. - # config.scope_defaults :admin, :config => { :client_id => 'foobar', - # :client_secret => 'barfoo', - # :scope => 'user,repo', - # :redirect_uri => '/admin/oauth/callback' } + # config.scope_defaults :admin, config: { client_id: 'foobar', + # client_secret: 'barfoo', + # scope: 'user,repo', + # redirect_uri: '/admin/oauth/callback' } # end class Config BadConfig = Class.new(StandardError) @@ -88,10 +88,10 @@ def scope end def to_hash - { :client_id => client_id, - :client_secret => client_secret, - :redirect_uri => redirect_uri, - :scope => scope } + { client_id: client_id, + client_secret: client_secret, + redirect_uri: redirect_uri, + scope: scope } end private @@ -131,16 +131,21 @@ def extract_path(uri) end end + def https_forwarded_proto? + env['HTTP_X_FORWARDED_PROTO'] && + env['HTTP_X_FORWARDED_PROTO'].split(',')[0] == "https" + end + def correct_scheme(uri) - if uri.scheme != 'https' && env['HTTP_X_FORWARDED_PROTO'] == 'https' - uri.port = nil if uri.port == 80 + if uri.scheme != 'https' && https_forwarded_proto? uri.scheme = 'https' # Reparsing will use a different URI subclass, namely URI::HTTPS which # knows the default port for https and strips it if present. uri = URI(uri.to_s) end + uri.port = nil if uri.port == 80 - uri + URI(uri.to_s) end end end diff --git a/lib/warden/github/hook.rb b/lib/warden/github/hook.rb index f72bba7..c006c36 100644 --- a/lib/warden/github/hook.rb +++ b/lib/warden/github/hook.rb @@ -4,3 +4,10 @@ strategy.finalize_flow! if strategy.class == Warden::GitHub::Strategy end + +Warden::Manager.after_set_user do |user, auth, opts| + if user.is_a?(Warden::GitHub::User) + session = auth.session(opts.fetch(:scope)) + user.memberships = session[:_memberships] ||= {} + end +end diff --git a/lib/warden/github/membership_cache.rb b/lib/warden/github/membership_cache.rb index 91add5f..b8f5310 100644 --- a/lib/warden/github/membership_cache.rb +++ b/lib/warden/github/membership_cache.rb @@ -3,16 +3,20 @@ module GitHub # A hash subclass that acts as a cache for organization and team # membership states. Only membership states that are true are cached. These # are invalidated after a certain time. - class MembershipCache < ::Hash + class MembershipCache CACHE_TIMEOUT = 60 * 5 + def initialize(data) + @data = data + end + # Fetches a membership status by type and id (e.g. 'org', 'my_company') # from cache. If no cached value is present or if the cached value # expired, the block will be invoked and the return value, if true, # cached for e certain time. def fetch_membership(type, id) type = type.to_s - id = id.to_s if id.is_a?(Symbol) + id = id.to_s if cached_membership_valid?(type, id) true @@ -27,10 +31,10 @@ def fetch_membership(type, id) private def cached_membership_valid?(type, id) - timestamp = fetch(type).fetch(id) + timestamp = @data.fetch(type).fetch(id) if Time.now.to_i > timestamp + CACHE_TIMEOUT - fetch(type).delete(id) + @data.fetch(type).delete(id) false else true @@ -40,7 +44,7 @@ def cached_membership_valid?(type, id) end def cache_membership(type, id) - hash = self[type] ||= {} + hash = @data[type] ||= {} hash[id] = Time.now.to_i end end diff --git a/lib/warden/github/oauth.rb b/lib/warden/github/oauth.rb index c7e3afa..3a0dfdc 100644 --- a/lib/warden/github/oauth.rb +++ b/lib/warden/github/oauth.rb @@ -25,10 +25,10 @@ def initialize(attrs={}) def authorize_uri @authorize_uri ||= build_uri( '/login/oauth/authorize', - :client_id => client_id, - :redirect_uri => redirect_uri, - :scope => scope, - :state => state) + client_id: client_id, + redirect_uri: redirect_uri, + scope: scope, + state: state) end def access_token @@ -53,9 +53,9 @@ def load_access_token def access_token_uri @access_token_uri ||= build_uri( '/login/oauth/access_token', - :client_id => client_id, - :client_secret => client_secret, - :code => code) + client_id: client_id, + client_secret: client_secret, + code: code) end def build_uri(path, params) diff --git a/lib/warden/github/sso.rb b/lib/warden/github/sso.rb new file mode 100644 index 0000000..23d0635 --- /dev/null +++ b/lib/warden/github/sso.rb @@ -0,0 +1,30 @@ +module Warden + module GitHub + module SSO + def warden_github_sso_session_valid?(user, expiry_in_seconds = 30) + return true if defined?(::Rails) && ::Rails.env.test? + if warden_github_sso_session_needs_reverification?(user, expiry_in_seconds) + if user.browser_session_valid?(expiry_in_seconds) + warden_github_sso_session_reverify! + return true + end + return false + end + true + end + + def warden_github_sso_session_verified_at + session[:warden_github_sso_session_verified_at] || Time.now.utc.to_i - 86400 + end + + def warden_github_sso_session_reverify! + session[:warden_github_sso_session_verified_at] = Time.now.utc.to_i + end + + def warden_github_sso_session_needs_reverification?(user, expiry_in_seconds) + user.using_single_sign_out? && + (warden_github_sso_session_verified_at <= (Time.now.utc.to_i - expiry_in_seconds)) + end + end + end +end diff --git a/lib/warden/github/strategy.rb b/lib/warden/github/strategy.rb index 4b457fb..6ddd6fd 100644 --- a/lib/warden/github/strategy.rb +++ b/lib/warden/github/strategy.rb @@ -70,6 +70,10 @@ def validate_flow! elsif (error = params['error']) && !error.empty? abort_flow!(error.gsub(/_/, ' ')) end + + if params['browser_session_id'] + custom_session['browser_session_id'] = params['browser_session_id'] + end end def custom_session @@ -77,7 +81,7 @@ def custom_session end def load_user - User.load(oauth.access_token) + User.load(oauth.access_token, custom_session['browser_session_id']) rescue OAuth::BadVerificationCode => e abort_flow!(e.message) end @@ -88,7 +92,7 @@ def state def oauth @oauth ||= OAuth.new( - config.to_hash.merge(:code => params['code'], :state => state)) + config.to_hash.merge(code: params['code'], state: state)) end def config diff --git a/lib/warden/github/user.rb b/lib/warden/github/user.rb index 4a6b222..936a0a1 100644 --- a/lib/warden/github/user.rb +++ b/lib/warden/github/user.rb @@ -1,19 +1,19 @@ -require 'octokit' - module Warden module GitHub - class User < Struct.new(:attribs, :token) - ATTRIBUTES = %w[id login name gravatar_id email company site_admin].freeze + class User < Struct.new(:attribs, :token, :browser_session_id) + ATTRIBUTES = %w[id login name gravatar_id avatar_url email company site_admin].freeze + + attr_accessor :memberships - def self.load(access_token) - api = Octokit::Client.new(:access_token => access_token) + def self.load(access_token, browser_session_id = nil) + api = Octokit::Client.new(access_token: access_token) data = { } api.user.to_hash.each do |k,v| data[k.to_s] = v if ATTRIBUTES.include?(k.to_s) end - new(data, access_token) + new(data, access_token, browser_session_id) end def marshal_dump @@ -34,7 +34,7 @@ def marshal_load(hash) # # Returns: true if the user is publicized as an org member def organization_public_member?(org_name) - memberships.fetch_membership(:org_pub, org_name) do + membership_cache.fetch_membership(:org_pub, org_name) do api.organization_public_member?(org_name, login) end end @@ -48,7 +48,7 @@ def organization_public_member?(org_name) # # Returns: true if the user has access, false otherwise def organization_member?(org_name) - memberships.fetch_membership(:org, org_name) do + membership_cache.fetch_membership(:org, org_name) do api.organization_member?(org_name, login) end end @@ -59,7 +59,9 @@ def organization_member?(org_name) # # Returns: true if the user has access, false otherwise def team_member?(team_id) - api.team_member?(team_id, login) + membership_cache.fetch_membership(:team, team_id) do + api.team_member?(team_id, login) + end end # Identify GitHub employees/staff members. @@ -69,6 +71,27 @@ def site_admin? !!site_admin end + # Identify if the browser session is still valid + # + # Returns: true if the browser session is still active or the GitHub API is unavailable + def browser_session_valid?(since = 120) + return true unless using_single_sign_out? + client = api + client.get("/user/sessions/active", browser_session_id: browser_session_id) + client.last_response.status == 204 + rescue Octokit::ServerError # GitHub API unavailable + true + rescue Octokit::ClientError => e # GitHub API failed + false + end + + # Identify if the user is on a GitHub SSO property + # + # Returns: true if a browser_session_id is present, false otherwise. + def using_single_sign_out? + !(browser_session_id.nil? || browser_session_id == "") + end + # Access the GitHub API from Octokit # # Octokit is a robust client library for the GitHub API @@ -79,13 +102,14 @@ def api # Don't cache instance for now because of a ruby marshaling bug present # in MRI 1.9.3 (Bug #7627) that causes instance variables to be # marshaled even when explicitly specifying #marshal_dump. - Octokit::Client.new(:login => login, :access_token => token) + Octokit::Client.new(login: login, access_token: token) end private - def memberships - attribs['member'] ||= MembershipCache.new + def membership_cache + self.memberships ||= {} + @membership_cache ||= MembershipCache.new(memberships) end end end diff --git a/lib/warden/github/verifier.rb b/lib/warden/github/verifier.rb new file mode 100644 index 0000000..d0ec95a --- /dev/null +++ b/lib/warden/github/verifier.rb @@ -0,0 +1,38 @@ +module Warden + module GitHub + class Verifier + def self.dump(user) + new.serialize(user) + end + + def self.load(key) + new.deserialize(key) + end + + def serialize(user) + cookie_verifier.generate(user.marshal_dump) + end + + def deserialize(key) + User.new.tap do |u| + u.marshal_load(cookie_verifier.verify(key)) + end + rescue ::ActiveSupport::MessageVerifier::InvalidSignature + nil + end + + def verifier_key + self.class.verifier_key + end + + private + def self.verifier_key + @verifier_key ||= ENV['WARDEN_GITHUB_VERIFIER_SECRET'] || SecureRandom.hex + end + + def cookie_verifier + @cookie_verifier ||= ::ActiveSupport::MessageVerifier.new(verifier_key, serializer: JSON) + end + end + end +end diff --git a/lib/warden/github/version.rb b/lib/warden/github/version.rb index 507f87c..2126db8 100644 --- a/lib/warden/github/version.rb +++ b/lib/warden/github/version.rb @@ -1,5 +1,5 @@ module Warden module GitHub - VERSION = "1.0.0" + VERSION = "1.3.2" end end diff --git a/spec/integration/oauth_spec.rb b/spec/integration/oauth_spec.rb index 8a38d9a..c687d50 100644 --- a/spec/integration/oauth_spec.rb +++ b/spec/integration/oauth_spec.rb @@ -5,17 +5,17 @@ def stub_code_for_token_exchange(answer='access_token=the_token') stub_request(:post, 'https://github.com/login/oauth/access_token'). - with(:body => hash_including(:code => code)). - to_return(:status => 200, :body => answer) + with(body: hash_including(code: code)). + to_return(status: 200, body: answer) end def stub_user_retrieval stub_request(:get, 'https://api.github.com/user'). - with(:headers => { 'Authorization' => 'token the_token' }). + with(headers: { 'Authorization' => 'token the_token' }). to_return( - :status => 200, - :body => File.read('spec/fixtures/user.json'), - :headers => { 'Content-Type' => 'application/json; charset=utf-8' }) + status: 200, + body: File.read('spec/fixtures/user.json'), + headers: { 'Content-Type' => 'application/json; charset=utf-8' }) end def redirect_uri(response) @@ -27,12 +27,12 @@ def redirect_uri(response) unauthenticated_response = get '/profile' github_uri = redirect_uri(unauthenticated_response) - github_uri.scheme.should eq 'https' - github_uri.host.should eq 'github.com' - github_uri.path.should eq '/login/oauth/authorize' - github_uri.query_values['client_id'].should =~ /\w+/ - github_uri.query_values['state'].should =~ /\w+/ - github_uri.query_values['redirect_uri'].should =~ /^http.*\/profile$/ + expect(github_uri.scheme).to eq 'https' + expect(github_uri.host).to eq 'github.com' + expect(github_uri.path).to eq '/login/oauth/authorize' + expect(github_uri.query_values['client_id']).to match(/\w+/) + expect(github_uri.query_values['state']).to match(/\w+/) + expect(github_uri.query_values['redirect_uri']).to match(/^http.*\/profile$/) end end @@ -53,8 +53,8 @@ def redirect_uri(response) get '/login' response = get "/login?code=#{code}&state=foobar" - response.should_not be_successful - response.body.should include 'State mismatch' + expect(response).not_to be_successful + expect(response.body).to include 'State mismatch' end end @@ -67,8 +67,8 @@ def redirect_uri(response) state = github_uri.query_values['state'] response = get "/login?code=#{code}&state=#{state}" - response.should_not be_successful - response.body.should include 'Bad verification code' + expect(response).not_to be_successful + expect(response.body).to include 'Bad verification code' end end @@ -79,8 +79,8 @@ def redirect_uri(response) state = github_uri.query_values['state'] response = get "/login?error=access_denied&state=#{state}" - response.should_not be_successful - response.body.should include 'access denied' + expect(response).not_to be_successful + expect(response.body).to include 'access denied' end end @@ -96,8 +96,25 @@ def redirect_uri(response) callback_response = get "/profile?code=#{code}&state=#{state}" authenticated_uri = redirect_uri(callback_response) - authenticated_uri.path.should eq '/profile' - authenticated_uri.query.should eq 'foo=bar' + expect(authenticated_uri.path).to eq '/profile' + expect(authenticated_uri.query).to eq 'foo=bar' + end + end + + context 'with GitHub SSO and code was exchanged for an access token' do + it 'redirects back to the original path' do + stub_code_for_token_exchange + stub_user_retrieval + + unauthenticated_response = get '/profile?foo=bar' + github_uri = redirect_uri(unauthenticated_response) + state = github_uri.query_values['state'] + + callback_response = get "/profile?code=#{code}&state=#{state}&browser_session_id=abcdefghijklmnop" + authenticated_uri = redirect_uri(callback_response) + + expect(authenticated_uri.path).to eq '/profile' + expect(authenticated_uri.query).to eq 'foo=bar' end end end @@ -106,8 +123,8 @@ def redirect_uri(response) it 'does not recognize a seeming callback url as an actual callback' do response = get '/profile?state=foo&code=bar' - a_request(:post, 'https://github.com/login/oauth/access_token'). - should have_not_been_made + expect(a_request(:post, 'https://github.com/login/oauth/access_token')). + to have_not_been_made end end @@ -125,7 +142,7 @@ def redirect_uri(response) get authenticated_uri.path logged_in_response = get '/login' - redirect_uri(logged_in_response).path.should eq '/' + expect(redirect_uri(logged_in_response).path).to eq '/' end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 2cbdf21..6379fcd 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -16,4 +16,9 @@ def app Example.app end + + def stub_user_session_request + stub_request(:get, "https://api.github.com/user/sessions/active?browser_session_id=abcdefghijklmnop"). + with(headers: {'Accept'=>'application/vnd.github.v3+json', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'token the_token', 'Content-Type'=>'application/json', 'User-Agent'=>"Octokit Ruby Gem #{Octokit::VERSION}"}) + end end diff --git a/spec/unit/config_spec.rb b/spec/unit/config_spec.rb index 9328e97..88215aa 100644 --- a/spec/unit/config_spec.rb +++ b/spec/unit/config_spec.rb @@ -4,11 +4,11 @@ let(:warden_scope) { :test_scope } let(:env) do - { 'warden' => double(:config => warden_config) } + { 'warden' => double(config: warden_config) } end let(:warden_config) do - { :scope_defaults => { warden_scope => { :config => scope_config } } } + { scope_defaults: { warden_scope => { config: scope_config } } } end let(:scope_config) do @@ -16,7 +16,7 @@ end let(:request) do - double(:url => 'http://example.com/the/path', :path => '/the/path') + double(url: 'http://example.com/the/path', path: '/the/path') end subject(:config) do @@ -24,7 +24,7 @@ end before do - config.stub(:request => request) + allow(config).to receive_messages(request: request) end def silence_warnings @@ -38,7 +38,7 @@ def silence_warnings context 'when specified in scope config' do it 'returns the client id' do scope_config[:client_id] = 'foobar' - config.client_id.should eq 'foobar' + expect(config.client_id).to eq 'foobar' end end @@ -46,15 +46,15 @@ def silence_warnings it 'returns the client id' do warden_config[:github_client_id] = 'foobar' silence_warnings do - config.client_id.should eq 'foobar' + expect(config.client_id).to eq 'foobar' end end end context 'when specified in ENV' do it 'returns the client id' do - ENV.stub(:[]).with('GITHUB_CLIENT_ID').and_return('foobar') - config.client_id.should eq 'foobar' + allow(ENV).to receive(:[]).with('GITHUB_CLIENT_ID').and_return('foobar') + expect(config.client_id).to eq 'foobar' end end @@ -69,7 +69,7 @@ def silence_warnings context 'when specified in scope config' do it 'returns the client secret' do scope_config[:client_secret] = 'foobar' - config.client_secret.should eq 'foobar' + expect(config.client_secret).to eq 'foobar' end end @@ -77,16 +77,16 @@ def silence_warnings it 'returns the client secret' do warden_config[:github_secret] = 'foobar' silence_warnings do - config.client_secret.should eq 'foobar' + expect(config.client_secret).to eq 'foobar' end end end context 'when specified in ENV' do it 'returns the client secret' do - ENV.stub(:[]).with('GITHUB_CLIENT_SECRET').and_return('foobar') + allow(ENV).to receive(:[]).with('GITHUB_CLIENT_SECRET').and_return('foobar') silence_warnings do - config.client_secret.should eq 'foobar' + expect(config.client_secret).to eq 'foobar' end end end @@ -102,14 +102,14 @@ def silence_warnings context 'when specified in scope config' do it 'returns the expanded redirect uri' do scope_config[:redirect_uri] = '/callback' - config.redirect_uri.should eq 'http://example.com/callback' + expect(config.redirect_uri).to eq 'http://example.com/callback' end end context 'when specified path lacks leading slash' do it 'corrects the path and returns the expanded uri' do scope_config[:redirect_uri] = 'callback' - config.redirect_uri.should eq 'http://example.com/callback' + expect(config.redirect_uri).to eq 'http://example.com/callback' end end @@ -117,34 +117,40 @@ def silence_warnings it 'returns the expanded redirect uri' do warden_config[:github_callback_url] = '/callback' silence_warnings do - config.redirect_uri.should eq 'http://example.com/callback' + expect(config.redirect_uri).to eq 'http://example.com/callback' end end end context 'when not specified' do it 'returns the expanded redirect uri with the current path' do - config.redirect_uri.should eq 'http://example.com/the/path' + expect(config.redirect_uri).to eq 'http://example.com/the/path' end end context 'when HTTP_X_FORWARDED_PROTO is set to https' do it 'returns the expanded redirect uri(with port) with adjusted scheme' do env['HTTP_X_FORWARDED_PROTO'] = 'https' - request.stub(:url => 'http://example.com:443/the/path') - config.redirect_uri.should eq 'https://example.com/the/path' + allow(request).to receive_messages(url: 'http://example.com:443/the/path') + expect(config.redirect_uri).to eq 'https://example.com/the/path' end it 'returns the expanded redirect uri with adjusted scheme including port 80' do env['HTTP_X_FORWARDED_PROTO'] = 'https' - request.stub(:url => 'http://example.com:80/the/path') - config.redirect_uri.should eq 'https://example.com/the/path' + allow(request).to receive_messages(url: 'http://example.com:80/the/path') + expect(config.redirect_uri).to eq 'https://example.com/the/path' + end + + it 'returns the expanded redirect uri with adjusted scheme including port 80 with multiple forwarded protocols' do + env['HTTP_X_FORWARDED_PROTO'] = 'https,https' + allow(request).to receive_messages(url: 'https://example.com:80/the/path') + expect(config.redirect_uri).to eq 'https://example.com/the/path' end it 'returns the expanded redirect uri(without port) with adjusted scheme' do env['HTTP_X_FORWARDED_PROTO'] = 'https' - request.stub(:url => 'http://example.com/the/path') - config.redirect_uri.should eq 'https://example.com/the/path' + allow(request).to receive_messages(url: 'http://example.com/the/path') + expect(config.redirect_uri).to eq 'https://example.com/the/path' end end end @@ -153,7 +159,7 @@ def silence_warnings context 'when specified in scope config' do it 'returns the client secret' do scope_config[:scope] = 'user' - config.scope.should eq 'user' + expect(config.scope).to eq 'user' end end @@ -161,14 +167,14 @@ def silence_warnings it 'returns the client secret' do warden_config[:github_scopes] = 'user' silence_warnings do - config.scope.should eq 'user' + expect(config.scope).to eq 'user' end end end context 'when not specified' do it 'returns nil' do - config.scope.should be_nil + expect(config.scope).to be_nil end end end @@ -176,13 +182,13 @@ def silence_warnings describe '#to_hash' do it 'includes all configs' do scope_config.merge!( - :scope => 'user', - :client_id => 'abc', - :client_secret => '123', - :redirect_uri => '/foo') + scope: 'user', + client_id: 'abc', + client_secret: '123', + redirect_uri: '/foo') - config.to_hash.keys. - should =~ [:scope, :client_id, :client_secret, :redirect_uri] + expect(config.to_hash.keys). + to match_array([:scope, :client_id, :client_secret, :redirect_uri]) end end end diff --git a/spec/unit/membership_cache_spec.rb b/spec/unit/membership_cache_spec.rb index d080c50..3a37954 100644 --- a/spec/unit/membership_cache_spec.rb +++ b/spec/unit/membership_cache_spec.rb @@ -1,48 +1,46 @@ require 'spec_helper' describe Warden::GitHub::MembershipCache do - subject(:cache) do - described_class.new - end - describe '#fetch_membership' do it 'returns false by default' do - cache.fetch_membership('foo', 'bar').should be_false + cache = described_class.new({}) + expect(cache.fetch_membership('foo', 'bar')).to be_falsey end context 'when cache valid' do - before do - cache['foo'] = {} - cache['foo']['bar'] = Time.now.to_i - described_class::CACHE_TIMEOUT + 5 + let(:cache) do + described_class.new('foo' => { + 'bar' => Time.now.to_i - described_class::CACHE_TIMEOUT + 5 + }) end it 'returns true' do - cache.fetch_membership('foo', 'bar').should be_true + expect(cache.fetch_membership('foo', 'bar')).to be_truthy end it 'does not invoke the block' do expect { |b| cache.fetch_membership('foo', 'bar', &b) }. to_not yield_control end + + it 'converts type and id to strings' do + expect(cache.fetch_membership(:foo, :bar)).to be_truthy + end end context 'when cache expired' do - before do - cache['foo'] = {} - cache['foo']['bar'] = Time.now.to_i - described_class::CACHE_TIMEOUT - 5 + let(:cache) do + described_class.new('foo' => { + 'bar' => Time.now.to_i - described_class::CACHE_TIMEOUT - 5 + }) end context 'when no block given' do it 'returns false' do - cache.fetch_membership('foo', 'bar').should be_false + expect(cache.fetch_membership('foo', 'bar')).to be_falsey end end - it 'deletes the cached value' do - cache.fetch_membership('foo', 'bar') - cache['foo'].should_not have_key('bar') - end - it 'invokes the block' do expect { |b| cache.fetch_membership('foo', 'bar', &b) }. to yield_control @@ -50,13 +48,40 @@ end it 'caches the value when block returns true' do + cache = described_class.new({}) cache.fetch_membership('foo', 'bar') { true } - cache.fetch_membership('foo', 'bar').should be_true + expect(cache.fetch_membership('foo', 'bar')).to be_truthy end it 'does not cache the value when block returns false' do + cache = described_class.new({}) cache.fetch_membership('foo', 'bar') { false } - cache.fetch_membership('foo', 'bar').should be_false + expect(cache.fetch_membership('foo', 'bar')).to be_falsey end end + + it 'uses the hash that is passed to the initializer as storage' do + time = Time.now.to_i + hash = { + 'foo' => { + 'valid' => time, + 'timedout' => time - described_class::CACHE_TIMEOUT - 5 + } + } + cache = described_class.new(hash) + + # Verify that existing data in the hash is used: + expect(cache.fetch_membership('foo', 'valid')).to be(true) + expect(cache.fetch_membership('foo', 'timedout')).to be(false) + + # Add new data to the hash: + cache.fetch_membership('foo', 'new') { true } + cache.fetch_membership('foo', 'false') { false } + + # Verify the final hash state: + expect(hash).to eq('foo' => { + 'valid' => time, + 'new' => time + }) + end end diff --git a/spec/unit/oauth_spec.rb b/spec/unit/oauth_spec.rb index bbf60eb..ab901f2 100644 --- a/spec/unit/oauth_spec.rb +++ b/spec/unit/oauth_spec.rb @@ -2,10 +2,10 @@ describe Warden::GitHub::OAuth do let(:default_attrs) do - { :state => 'abc', - :client_id => 'foo', - :client_secret => 'bar', - :redirect_uri => 'http://example.com/callback' } + { state: 'abc', + client_id: 'foo', + client_secret: 'bar', + redirect_uri: 'http://example.com/callback' } end def oauth(attrs=default_attrs) @@ -14,7 +14,7 @@ def oauth(attrs=default_attrs) describe '#authorize_uri' do it 'contains the base uri' do - oauth.authorize_uri.to_s.should \ + expect(oauth.authorize_uri.to_s).to \ include Octokit.web_endpoint end @@ -22,15 +22,15 @@ def oauth(attrs=default_attrs) it "contains the correct #{name} param" do uri = Addressable::URI.parse(oauth.authorize_uri) - uri.query_values[name].should eq default_attrs[name.to_sym] + expect(uri.query_values[name]).to eq default_attrs[name.to_sym] end end - { :nil => nil, :empty => '' }.each do |desc, value| + { nil: nil, empty: '' }.each do |desc, value| it "does not contain the scope param if #{desc}" do - uri = oauth(default_attrs.merge(:scope => value)).authorize_uri + uri = oauth(default_attrs.merge(scope: value)).authorize_uri - uri.to_s.should_not include 'scope' + expect(uri.to_s).not_to include 'scope' end end end @@ -38,19 +38,19 @@ def oauth(attrs=default_attrs) describe '#access_token' do def expect_request(attrs={}) stub_request(:post, %r{\/login\/oauth\/access_token$}). - with(:body => hash_including(attrs.fetch(:params, {}))). - to_return(:status => 200, - :body => attrs.fetch(:answer, 'access_token=foobar')) + with(body: hash_including(attrs.fetch(:params, {}))). + to_return(status: 200, + body: attrs.fetch(:answer, 'access_token=foobar')) end it 'exchanges the code for an access token' do - expect_request(:answer => 'access_token=the_token&token_type=bearer') + expect_request(answer: 'access_token=the_token&token_type=bearer') - oauth.access_token.should eq 'the_token' + expect(oauth.access_token).to eq 'the_token' end it 'raises BadVerificationCode if no access token is returned' do - expect_request(:answer => 'error=bad_verification_code') + expect_request(answer: 'error=bad_verification_code') expect { oauth.access_token }. to raise_error(described_class::BadVerificationCode) @@ -58,8 +58,8 @@ def expect_request(attrs={}) %w[ client_id client_secret code ].each do |name| it "performs a request containing the correct #{name} param" do - oauth(default_attrs.merge(:code => 'the_code')).tap do |o| - expect_request(:params => { name => o.send(name) }) + oauth(default_attrs.merge(code: 'the_code')).tap do |o| + expect_request(params: { name => o.send(name) }) o.access_token end end diff --git a/spec/unit/sso_spec.rb b/spec/unit/sso_spec.rb new file mode 100644 index 0000000..0894937 --- /dev/null +++ b/spec/unit/sso_spec.rb @@ -0,0 +1,42 @@ +require 'spec_helper' + +class FakeController + def session + @session ||= {} + end + include Warden::GitHub::SSO +end + +describe Warden::GitHub::SSO do + let(:default_attrs) do + { 'login' => 'john', + 'name' => 'John Doe', + 'gravatar_id' => '38581cb351a52002548f40f8066cfecg', + 'avatar_url' => 'http://example.com/avatar.jpg', + 'email' => 'john@doe.com', + 'company' => 'Doe, Inc.' } + end + + let(:user) do + Warden::GitHub::User.new(default_attrs, "the_token", "abcdefghijklmnop") + end + + subject do + FakeController.new + end + + describe "warden_github_sso_session_valid?" do + it "identifies when browsers need to be reverified" do + subject.session[:warden_github_sso_session_verified_at] = Time.now.utc.to_i - 10 + expect(subject).to be_warden_github_sso_session_valid(user) + + subject.session[:warden_github_sso_session_verified_at] = Time.now.utc.to_i - 300 + stub_user_session_request.to_return(status: 204, body: "", headers: {}) + expect(subject).to be_warden_github_sso_session_valid(user) + + subject.session[:warden_github_sso_session_verified_at] = Time.now.utc.to_i - 300 + stub_user_session_request.to_return(status: 404, body: "", headers: {}) + expect(subject).not_to be_warden_github_sso_session_valid(user) + end + end +end diff --git a/spec/unit/user_spec.rb b/spec/unit/user_spec.rb index 594041c..026a712 100644 --- a/spec/unit/user_spec.rb +++ b/spec/unit/user_spec.rb @@ -5,6 +5,7 @@ { 'login' => 'john', 'name' => 'John Doe', 'gravatar_id' => '38581cb351a52002548f40f8066cfecg', + 'avatar_url' => 'http://example.com/avatar.jpg', 'email' => 'john@doe.com', 'company' => 'Doe, Inc.' } end @@ -14,16 +15,20 @@ described_class.new(default_attrs, token) end + let(:sso_user) do + described_class.new(default_attrs, token, "abcdefghijklmnop") + end + describe '#token' do it 'returns the token' do - user.token.should eq token + expect(user.token).to eq token end end - %w[login name gravatar_id email company].each do |name| + %w[login name gravatar_id avatar_url email company].each do |name| describe "##{name}" do it "returns the #{name}" do - user.send(name).should eq default_attrs[name] + expect(user.send(name)).to eq default_attrs[name] end end end @@ -32,16 +37,16 @@ it 'returns a preconfigured Octokit client for the user' do api = user.api - api.should be_an Octokit::Client - api.login.should eq user.login - api.access_token.should eq user.token + expect(api).to be_an Octokit::Client + expect(api.login).to eq user.login + expect(api.access_token).to eq user.token end end def stub_api(user, method, args, ret) api = double - user.stub(:api => api) - api.should_receive(method).with(*args).and_return(ret) + allow(user).to receive_messages(api: api) + expect(api).to receive(method).with(*args).and_return(ret) end [:organization_public_member?, :organization_member?].each do |method| @@ -49,14 +54,14 @@ def stub_api(user, method, args, ret) context 'when user is not member' do it 'returns false' do stub_api(user, method, ['rails', user.login], false) - user.send(method, 'rails').should be_false + expect(user.send(method, 'rails')).to be_falsey end end context 'when user is member' do it 'returns true' do stub_api(user, method, ['rails', user.login], true) - user.send(method, 'rails').should be_true + expect(user.send(method, 'rails')).to be_truthy end end end @@ -66,21 +71,21 @@ def stub_api(user, method, args, ret) context 'when user is not member' do it 'returns false' do api = double() - user.stub(:api => api) + allow(user).to receive_messages(api: api) - api.stub(:team_member?).with(123, user.login).and_return(false) + allow(api).to receive(:team_member?).with(123, user.login).and_return(false) - user.should_not be_team_member(123) + expect(user).not_to be_team_member(123) end end context 'when user is member' do it 'returns true' do api = double() - user.stub(:api => api) - api.stub(:team_member?).with(123, user.login).and_return(true) + allow(user).to receive_messages(api: api) + allow(api).to receive(:team_member?).with(123, user.login).and_return(true) - user.should be_team_member(123) + expect(user).to be_team_member(123) end end end @@ -90,21 +95,50 @@ def stub_api(user, method, args, ret) client = double attrs = {} - Octokit::Client. - should_receive(:new). - with(:access_token => token). + expect(Octokit::Client). + to receive(:new). + with(access_token: token). and_return(client) - client.should_receive(:user).and_return(attrs) + expect(client).to receive(:user).and_return(attrs) user = described_class.load(token) - user.attribs.should eq attrs - user.token.should eq token + expect(user.attribs).to eq attrs + expect(user.token).to eq token end end # NOTE: This always passes on MRI 1.9.3 because of ruby bug #7627. it 'marshals correctly' do - Marshal.load(Marshal.dump(user)).should eq user + expect(Marshal.load(Marshal.dump(user))).to eq user + end + + describe 'single sign out' do + it "knows if the user is using single sign out" do + expect(user).not_to be_using_single_sign_out + expect(sso_user).to be_using_single_sign_out + end + + context "browser reverification" do + it "handles success" do + stub_user_session_request.to_return(status: 204, body: "", headers: {}) + expect(sso_user).to be_browser_session_valid + end + + it "handles failure" do + stub_user_session_request.to_return(status: 404, body: "", headers: {}) + expect(sso_user).not_to be_browser_session_valid + end + + it "handles GitHub being unavailable" do + stub_user_session_request.to_raise(Octokit::ServerError.new) + expect(sso_user).to be_browser_session_valid + end + + it "handles authentication failures" do + stub_user_session_request.to_return(status: 403, body: "", headers: {}) + expect(sso_user).not_to be_browser_session_valid + end + end end end diff --git a/warden-github.gemspec b/warden-github.gemspec index d09671c..245e23c 100644 --- a/warden-github.gemspec +++ b/warden-github.gemspec @@ -9,21 +9,23 @@ Gem::Specification.new do |s| s.email = ["atmos@atmos.org"] s.homepage = "http://github.com/atmos/warden-github" s.summary = "A warden strategy for easy oauth integration with github" + s.license = 'MIT' s.description = s.summary s.rubyforge_project = "warden-github" - s.add_dependency "warden", ">1.0" - s.add_dependency "octokit", ">2.1.0" + s.add_dependency "warden", ">1.0" + s.add_dependency "octokit", ">2.1.0" + s.add_dependency "activesupport", ">3.0" s.add_development_dependency "rack", "~>1.4.1" s.add_development_dependency "rake" - s.add_development_dependency "rspec", "~>2.8" + s.add_development_dependency "rspec", "~>3.6" s.add_development_dependency "simplecov" s.add_development_dependency "webmock", "~>1.9" s.add_development_dependency "sinatra" s.add_development_dependency "shotgun" - s.add_development_dependency "addressable", "~>2.2.0" + s.add_development_dependency "addressable", ">2.2.0" s.add_development_dependency "rack-test", "~>0.5.3" s.add_development_dependency "yajl-ruby"