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

OAuth2 - PKCE #131

Merged
merged 4 commits into from Aug 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 29 additions & 4 deletions .rubocop.yml
@@ -1,15 +1,34 @@
AllCops:
NewCops: enable

Gemspec/RequiredRubyVersion:
Enabled: false

Layout/AccessModifierIndentation:
EnforcedStyle: outdent

Layout/LineLength:
AllowURI: true
Enabled: false

Layout/SpaceInsideHashLiteralBraces:
EnforcedStyle: no_space

Lint/MissingSuper:
Enabled: false

Metrics/AbcSize:
Max: 18

Metrics/BlockLength:
Exclude:
- spec/omniauth/strategies/oauth2_spec.rb

Metrics/BlockNesting:
Max: 2

Metrics/LineLength:
AllowURI: true
Enabled: false
Metrics/ClassLength:
Max: 110

Metrics/MethodLength:
CountComments: false
Expand All @@ -19,6 +38,10 @@ Metrics/ParameterLists:
Max: 4
CountKeywordArgs: true

Naming/FileName:
Exclude:
- lib/omniauth-oauth2.rb

Style/CollectionMethods:
PreferredMethods:
map: 'collect'
Expand All @@ -35,6 +58,9 @@ Style/DoubleNegation:
Style/ExpandPathArguments:
Enabled: false

Style/FrozenStringLiteralComment:
Enabled: false

Style/HashSyntax:
EnforcedStyle: hash_rockets

Expand All @@ -52,4 +78,3 @@ Style/TrailingCommaInHashLiteral:

Style/TrailingCommaInArrayLiteral:
EnforcedStyleForMultiline: comma

1 change: 1 addition & 0 deletions Rakefile
@@ -1,4 +1,5 @@
#!/usr/bin/env rake

require "bundler/gem_tasks"
require "rspec/core/rake_task"

Expand Down
2 changes: 1 addition & 1 deletion lib/omniauth-oauth2.rb
@@ -1,2 +1,2 @@
require "omniauth-oauth2/version" # rubocop:disable FileName
require "omniauth-oauth2/version"
require "omniauth/strategies/oauth2"
56 changes: 45 additions & 11 deletions lib/omniauth/strategies/oauth2.rb
Expand Up @@ -24,11 +24,22 @@ def self.inherited(subclass)
option :client_secret, nil
option :client_options, {}
option :authorize_params, {}
option :authorize_options, [:scope, :state]
option :authorize_options, %i[scope state]
option :token_params, {}
option :token_options, []
option :auth_token_params, {}
option :provider_ignores_state, false
option :pkce, false
option :pkce_verifier, nil
option :pkce_options, {
:code_challenge => proc { |verifier|
Base64.urlsafe_encode64(
Digest::SHA2.digest(verifier),
padding: false
)
},
:code_challenge_method => "S256"
}

attr_accessor :access_token

Expand All @@ -50,20 +61,27 @@ def request_phase

def authorize_params
options.authorize_params[:state] = SecureRandom.hex(24)
params = options.authorize_params.merge(options_for("authorize"))

if OmniAuth.config.test_mode
@env ||= {}
@env["rack.session"] ||= {}
end

params = options.authorize_params
.merge(options_for("authorize"))
.merge(pkce_authorize_params)

session["omniauth.pkce.verifier"] = options.pkce_verifier if options.pkce
session["omniauth.state"] = params[:state]

params
end

def token_params
options.token_params.merge(options_for("token"))
options.token_params.merge(options_for("token")).merge(pkce_token_params)
end

def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength, PerceivedComplexity
def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity
error = request.params["error_reason"] || request.params["error"]
if error
fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"]))
Expand All @@ -84,27 +102,43 @@ def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength

protected

def pkce_authorize_params
return {} unless options.pkce
options.pkce_verifier = SecureRandom.hex(64)

# NOTE: see https://tools.ietf.org/html/rfc7636#appendix-A
{
:code_challenge => options.pkce_options[:code_challenge]
.call(options.pkce_verifier),
:code_challenge_method => options.pkce_options[:code_challenge_method]
}
end

def pkce_token_params
return {} unless options.pkce

{:code_verifier => session.delete("omniauth.pkce.verifier")}
end

def build_access_token
verifier = request.params["code"]
client.auth_code.get_token(verifier, {:redirect_uri => callback_url}.merge(token_params.to_hash(:symbolize_keys => true)), deep_symbolize(options.auth_token_params))
end

def deep_symbolize(options)
hash = {}
options.each do |key, value|
options.each_with_object({}) do |(key, value), hash|
hash[key.to_sym] = value.is_a?(Hash) ? deep_symbolize(value) : value
end
hash
end

def options_for(option)
hash = {}
options.send(:"#{option}_options").select { |key| options[key] }.each do |key|
hash[key.to_sym] = if options[key].respond_to?(:call)
options[key].call(env)
else
options[key]
end
options[key].call(env)
else
options[key]
end
end
hash
end
Expand Down
18 changes: 16 additions & 2 deletions spec/omniauth/strategies/oauth2_spec.rb
@@ -1,6 +1,6 @@
require "helper"

describe OmniAuth::Strategies::OAuth2 do # rubocop:disable Metrics/BlockLength
describe OmniAuth::Strategies::OAuth2 do
def app
lambda do |_env|
[200, {}, ["Hello."]]
Expand Down Expand Up @@ -62,10 +62,17 @@ def app
end

it "includes custom state in the authorize params" do
instance = subject.new("abc", "def", state: Proc.new { "qux" } )
instance = subject.new("abc", "def", :state => proc { "qux" })
expect(instance.authorize_params.keys).to eq(["state"])
expect(instance.session["omniauth.state"]).to eq("qux")
end

it "includes PKCE parameters if enabled" do
instance = subject.new("abc", "def", :pkce => true)
expect(instance.authorize_params[:code_challenge]).to be_a(String)
expect(instance.authorize_params[:code_challenge_method]).to eq("S256")
expect(instance.session["omniauth.pkce.verifier"]).to be_a(String)
end
end

describe "#token_params" do
Expand All @@ -80,6 +87,13 @@ def app
instance = subject.new("abc", "def", :token_options => %i[scope foo], :scope => "bar", :foo => "baz")
expect(instance.token_params).to eq("scope" => "bar", "foo" => "baz")
end

it "includes the PKCE code_verifier if enabled" do
instance = subject.new("abc", "def", :pkce => true)
# setup session
instance.authorize_params
expect(instance.token_params[:code_verifier]).to be_a(String)
end
end

describe "#callback_phase" do
Expand Down