diff --git a/app/views/doorkeeper/authorizations/new.html.erb b/app/views/doorkeeper/authorizations/new.html.erb index c6f738b33..aecdbf811 100644 --- a/app/views/doorkeeper/authorizations/new.html.erb +++ b/app/views/doorkeeper/authorizations/new.html.erb @@ -26,6 +26,8 @@ <%= hidden_field_tag :state, @pre_auth.state %> <%= hidden_field_tag :response_type, @pre_auth.response_type %> <%= hidden_field_tag :scope, @pre_auth.scope %> + <%= hidden_field_tag :code_challenge, @pre_auth.code_challenge %> + <%= hidden_field_tag :code_challenge_method, @pre_auth.code_challenge_method %> <%= submit_tag t('doorkeeper.authorizations.buttons.authorize'), class: "btn btn-success btn-lg btn-block" %> <% end %> <%= form_tag oauth_authorization_path, method: :delete do %> @@ -34,6 +36,8 @@ <%= hidden_field_tag :state, @pre_auth.state %> <%= hidden_field_tag :response_type, @pre_auth.response_type %> <%= hidden_field_tag :scope, @pre_auth.scope %> + <%= hidden_field_tag :code_challenge, @pre_auth.code_challenge %> + <%= hidden_field_tag :code_challenge_method, @pre_auth.code_challenge_method %> <%= submit_tag t('doorkeeper.authorizations.buttons.deny'), class: "btn btn-danger btn-lg btn-block" %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 7d2d215da..dc15ae874 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -79,6 +79,7 @@ en: unauthorized_client: 'The client is not authorized to perform this request using this method.' access_denied: 'The resource owner or authorization server denied the request.' invalid_scope: 'The requested scope is invalid, unknown, or malformed.' + invalid_code_challenge_method: 'The code challenge method must be plain or S256.' server_error: 'The authorization server encountered an unexpected condition which prevented it from fulfilling the request.' temporarily_unavailable: 'The authorization server is currently unable to handle the request due to a temporary overloading or maintenance of the server.' diff --git a/doorkeeper.gemspec b/doorkeeper.gemspec index 02efee088..b9a7da6af 100644 --- a/doorkeeper.gemspec +++ b/doorkeeper.gemspec @@ -1,27 +1,28 @@ -$:.push File.expand_path("../lib", __FILE__) +$:.push File.expand_path('lib', __dir__) -require "doorkeeper/version" +require 'doorkeeper/version' Gem::Specification.new do |s| - s.name = "doorkeeper" + s.name = 'doorkeeper' s.version = Doorkeeper::VERSION - s.authors = ["Felipe Elias Philipp", "Tute Costa"] - s.email = %w(tutecosta@gmail.com) - s.homepage = "https://github.com/doorkeeper-gem/doorkeeper" - s.summary = "OAuth 2 provider for Rails and Grape" - s.description = "Doorkeeper is an OAuth 2 provider for Rails and Grape." + s.authors = ['Felipe Elias Philipp', 'Tute Costa'] + s.email = %w[tutecosta@gmail.com] + s.homepage = 'https://github.com/doorkeeper-gem/doorkeeper' + s.summary = 'OAuth 2 provider for Rails and Grape' + s.description = 'Doorkeeper is an OAuth 2 provider for Rails and Grape.' s.license = 'MIT' s.files = `git ls-files`.split("\n") s.test_files = `git ls-files -- spec/*`.split("\n") - s.require_paths = ["lib"] + s.require_paths = ['lib'] - s.add_dependency "railties", ">= 3.2" + s.add_dependency 'railties', '>= 3.2' - s.add_development_dependency "rspec-rails", "~> 3.4.0" - s.add_development_dependency "capybara", "~> 2.3.0" - s.add_development_dependency "generator_spec", "~> 0.9.0" - s.add_development_dependency "factory_girl", "~> 4.5.0" - s.add_development_dependency "timecop", "~> 0.7.0" - s.add_development_dependency "database_cleaner", "~> 1.3.0" + s.add_development_dependency 'rspec-rails', '~> 3.4.0' + s.add_development_dependency 'capybara', '~> 2.3.0' + s.add_development_dependency 'generator_spec', '~> 0.9.0' + s.add_development_dependency 'factory_girl', '~> 4.5.0' + s.add_development_dependency 'timecop', '~> 0.7.0' + s.add_development_dependency 'database_cleaner', '~> 1.3.0' + s.add_development_dependency 'bigdecimal', '1.3.5' end diff --git a/lib/doorkeeper/models/access_grant_mixin.rb b/lib/doorkeeper/models/access_grant_mixin.rb index bb9c3c7c6..706b3a078 100644 --- a/lib/doorkeeper/models/access_grant_mixin.rb +++ b/lib/doorkeeper/models/access_grant_mixin.rb @@ -13,7 +13,8 @@ module AccessGrantMixin belongs_to :application, class_name: 'Doorkeeper::Application', inverse_of: :access_grants if respond_to?(:attr_accessible) - attr_accessible :resource_owner_id, :application_id, :expires_in, :redirect_uri, :scopes + attr_accessible :resource_owner_id, :application_id, :expires_in, :redirect_uri, :scopes, + :code_challenge, :code_challenge_method end validates :resource_owner_id, :application_id, :token, :expires_in, :redirect_uri, presence: true @@ -22,10 +23,66 @@ module AccessGrantMixin before_validation :generate_token, on: :create end + # never uses pkce, if pkce migrations were not generated + def uses_pkce? + pkce_supported? && code_challenge.present? + end + + def pkce_supported? + respond_to? :code_challenge + end + module ClassMethods def by_token(token) where(token: token.to_s).limit(1).to_a.first end + + # Implements PKCE code_challenge encoding without base64 padding as described in the spec. + # https://tools.ietf.org/html/rfc7636#appendix-A + # Appendix A. Notes on Implementing Base64url Encoding without Padding + # + # This appendix describes how to implement a base64url-encoding + # function without padding, based upon the standard base64-encoding + # function that uses padding. + # + # To be concrete, example C# code implementing these functions is shown + # below. Similar code could be used in other languages. + # + # static string base64urlencode(byte [] arg) + # { + # string s = Convert.ToBase64String(arg); // Regular base64 encoder + # s = s.Split('=')[0]; // Remove any trailing '='s + # s = s.Replace('+', '-'); // 62nd char of encoding + # s = s.Replace('/', '_'); // 63rd char of encoding + # return s; + # } + # + # An example correspondence between unencoded and encoded values + # follows. The octet sequence below encodes into the string below, + # which when decoded, reproduces the octet sequence. + # + # 3 236 255 224 193 + # + # A-z_4ME + # + # https://ruby-doc.org/stdlib-2.1.3/libdoc/base64/rdoc/Base64.html#method-i-urlsafe_encode64 + # + # urlsafe_encode64(bin) + # Returns the Base64-encoded version of bin. This method complies with + # “Base 64 Encoding with URL and Filename Safe Alphabet” in RFC 4648. + # The alphabet uses ‘-’ instead of ‘+’ and ‘_’ instead of ‘/’. + + # @param code_verifier [#to_s] a one time use value (any object that responds to `#to_s`) + # + # @return [#to_s] An encoded code challenge based on the provided verifier suitable for PKCE validation + def generate_code_challenge(code_verifier) + padded_result = Base64.urlsafe_encode64(Digest::SHA256.digest(code_verifier)) + padded_result.split('=')[0] # Remove any trailing '=' + end + + def pkce_supported? + new.pkce_supported? + end end private diff --git a/lib/doorkeeper/oauth/authorization/code.rb b/lib/doorkeeper/oauth/authorization/code.rb index 1479f6936..4aabf27fd 100644 --- a/lib/doorkeeper/oauth/authorization/code.rb +++ b/lib/doorkeeper/oauth/authorization/code.rb @@ -5,18 +5,12 @@ class Code attr_accessor :pre_auth, :resource_owner, :token def initialize(pre_auth, resource_owner) - @pre_auth = pre_auth + @pre_auth = pre_auth @resource_owner = resource_owner end def issue_token - @token ||= AccessGrant.create!( - application_id: pre_auth.client.id, - resource_owner_id: resource_owner.id, - expires_in: configuration.authorization_code_expires_in, - redirect_uri: pre_auth.redirect_uri, - scopes: pre_auth.scopes.to_s - ) + @token ||= AccessGrant.create! access_grant_attributes end def native_redirect @@ -26,6 +20,37 @@ def native_redirect def configuration Doorkeeper.configuration end + + private + + def authorization_code_expires_in + configuration.authorization_code_expires_in + end + + def access_grant_attributes + pkce_attributes.merge application_id: pre_auth.client.id, + resource_owner_id: resource_owner.id, + expires_in: authorization_code_expires_in, + redirect_uri: pre_auth.redirect_uri, + scopes: pre_auth.scopes.to_s + end + + def pkce_attributes + if pkce_supported? + { + code_challenge: pre_auth.code_challenge, + code_challenge_method: pre_auth.code_challenge_method + } + else + {} + end + end + + # ensures firstly, if migration with additional pcke columns was + # generated and migrated + def pkce_supported? + Doorkeeper::AccessGrant.pkce_supported? + end end end end diff --git a/lib/doorkeeper/oauth/authorization_code_request.rb b/lib/doorkeeper/oauth/authorization_code_request.rb index 73197b2fe..67a557979 100644 --- a/lib/doorkeeper/oauth/authorization_code_request.rb +++ b/lib/doorkeeper/oauth/authorization_code_request.rb @@ -8,14 +8,17 @@ class AuthorizationCodeRequest validate :client, error: :invalid_client validate :grant, error: :invalid_grant validate :redirect_uri, error: :invalid_grant + validate :code_verifier, error: :invalid_grant - attr_accessor :server, :grant, :client, :redirect_uri, :access_token + attr_accessor :server, :grant, :client, :redirect_uri, :access_token, + :code_verifier def initialize(server, grant, client, parameters = {}) @server = server @client = client @grant = grant @redirect_uri = parameters[:redirect_uri] + @code_verifier = parameters[:code_verifier] end private @@ -34,6 +37,9 @@ def before_successful_response end def validate_attributes + return false if grant && grant.uses_pkce? && code_verifier.blank? + return false if grant && !grant.pkce_supported? && !code_verifier.blank? + redirect_uri.present? end @@ -43,12 +49,28 @@ def validate_client def validate_grant return false unless grant && grant.application_id == client.id + grant.accessible? end def validate_redirect_uri grant.redirect_uri == redirect_uri end + + # if either side (server or client) request pkce, check the verifier + # against the DB - if pkce is supported + def validate_code_verifier + return true unless grant.uses_pkce? || code_verifier + return false unless grant.pkce_supported? + + if grant.code_challenge_method == 'S256' + grant.code_challenge == AccessGrant.generate_code_challenge(code_verifier) + elsif grant.code_challenge_method == 'plain' + grant.code_challenge == code_verifier + else + false + end + end end end end diff --git a/lib/doorkeeper/oauth/pre_authorization.rb b/lib/doorkeeper/oauth/pre_authorization.rb index dae2ce51f..92d1f9153 100644 --- a/lib/doorkeeper/oauth/pre_authorization.rb +++ b/lib/doorkeeper/oauth/pre_authorization.rb @@ -7,8 +7,10 @@ class PreAuthorization validate :client, error: :invalid_client validate :scopes, error: :invalid_scope validate :redirect_uri, error: :invalid_redirect_uri + validate :code_challenge_method, error: :invalid_code_challenge_method - attr_accessor :server, :client, :response_type, :redirect_uri, :state + attr_accessor :server, :client, :response_type, :redirect_uri, :state, + :code_challenge, :code_challenge_method attr_writer :scope def initialize(server, client, attrs = {}) @@ -18,6 +20,8 @@ def initialize(server, client, attrs = {}) @redirect_uri = attrs[:redirect_uri] @scope = attrs[:scope] @state = attrs[:state] + @code_challenge = attrs[:code_challenge] + @code_challenge_method = attrs[:code_challenge_method] end def authorizable? @@ -48,6 +52,7 @@ def validate_client def validate_scopes return true unless scope.present? + Helpers::ScopeChecker.valid?( scope, server.scopes, @@ -58,9 +63,14 @@ def validate_scopes # TODO: test uri should be matched against the client's one def validate_redirect_uri return false unless redirect_uri.present? + Helpers::URIChecker.native_uri?(redirect_uri) || Helpers::URIChecker.valid_for_authorization?(redirect_uri, client.redirect_uri) end + + def validate_code_challenge_method + !code_challenge.present? || (code_challenge_method.present? && code_challenge_method =~ /^plain$|^S256$/) + end end end end diff --git a/lib/doorkeeper/request/authorization_code.rb b/lib/doorkeeper/request/authorization_code.rb index d5e2c8cb4..0c20a84e6 100644 --- a/lib/doorkeeper/request/authorization_code.rb +++ b/lib/doorkeeper/request/authorization_code.rb @@ -4,15 +4,24 @@ module Doorkeeper module Request class AuthorizationCode < Strategy delegate :grant, :client, :parameters, to: :server + delegate :client_via_uid, :parameters, to: :server def request @request ||= OAuth::AuthorizationCodeRequest.new( Doorkeeper.configuration, grant, - client, + client_for_request, parameters ) end + + def client_for_request + if parameters.include?(:code_verifier) && parameters[:code_verifier].present? + client_via_uid + else + client + end + end end end end diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb index 27ae363e6..0515314e6 100644 --- a/spec/dummy/db/schema.rb +++ b/spec/dummy/db/schema.rb @@ -22,6 +22,8 @@ t.datetime "created_at", null: false t.datetime "revoked_at" t.string "scopes" + t.string "code_challenge" + t.string "code_challenge_method" end add_index "oauth_access_grants", ["token"], name: "index_oauth_access_grants_on_token", unique: true diff --git a/spec/lib/oauth/code_request_spec.rb b/spec/lib/oauth/code_request_spec.rb index 28996d475..a0b0e7b97 100644 --- a/spec/lib/oauth/code_request_spec.rb +++ b/spec/lib/oauth/code_request_spec.rb @@ -10,7 +10,9 @@ module Doorkeeper::OAuth scopes: nil, state: nil, error: nil, - authorizable?: true + authorizable?: true, + code_challenge: nil, + code_challenge_method: nil, ) end diff --git a/spec/requests/flows/authorization_code_spec.rb b/spec/requests/flows/authorization_code_spec.rb index 435cc77b1..2c5668c7b 100644 --- a/spec/requests/flows/authorization_code_spec.rb +++ b/spec/requests/flows/authorization_code_spec.rb @@ -58,6 +58,178 @@ should_have_json_within 'expires_in', Doorkeeper::AccessToken.first.expires_in, 1 end + scenario 'resource owner requests an access token with authorization code but without secret' do + visit authorization_endpoint_url(client: @client) + click_on 'Authorize' + + authorization_code = Doorkeeper::AccessGrant.first.token + page.driver.post token_endpoint_url( + code: authorization_code, + client_id: @client.uid, + redirect_uri: @client.redirect_uri + ) + + expect(Doorkeeper::AccessToken).not_to exist + + driver_should_have_json 'error', 'invalid_client' + end + + context 'with PKCE' do + context 'plain' do + let(:code_challenge) { 'a45a9fea-0676-477e-95b1-a40f72ac3cfb' } + let(:code_verifier) { 'a45a9fea-0676-477e-95b1-a40f72ac3cfb' } + + scenario 'resource owner authorizes the client with code_challenge parameter set' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge, + code_challenge_method: 'plain') + click_on 'Authorize' + + url_should_have_param('code', Doorkeeper::AccessGrant.first.token) + url_should_not_have_param('code_challenge_method') + url_should_not_have_param('code_challenge') + end + + scenario 'mobile app requests an access token with authorization code but not pkce token' do + visit authorization_endpoint_url(client: @client) + click_on 'Authorize' + + authorization_code = current_params['code'] + create_access_token authorization_code, @client, code_verifier + + driver_should_have_json 'error', 'invalid_grant' + end + + scenario 'mobile app requests an access token with authorization code and plain code challenge method' do + visit authorization_endpoint_url( + client: @client, + code_challenge: code_challenge, + code_challenge_method: 'plain' + ) + click_on 'Authorize' + + authorization_code = current_params['code'] + create_access_token authorization_code, @client, code_verifier + + access_token_should_exist_for(@client, @resource_owner) + + driver_should_not_have_json 'error' + driver_should_have_json 'access_token', Doorkeeper::AccessToken.first.token + driver_should_have_json 'token_type', 'bearer' + driver_should_have_json_within 'expires_in', Doorkeeper::AccessToken.first.expires_in, 1 + end + + scenario 'mobile app requests an access token with authorization code and code_challenge' do + visit authorization_endpoint_url( + client: @client, + code_challenge: code_verifier, + code_challenge_method: 'plain', + ) + click_on 'Authorize' + + authorization_code = current_params['code'] + create_access_token authorization_code, @client, code_verifier: nil + + driver_should_not_have_json 'access_token' + driver_should_have_json 'error', 'invalid_grant' + end + end + + context 's256' do + let(:code_challenge) { 'Oz733NtQ0rJP8b04fgZMJMwprn6Iw8sMCT_9bR1q4tA' } + let(:code_verifier) { 'a45a9fea-0676-477e-95b1-a40f72ac3cfb' } + + scenario 'resource owner authorizes the client with code_challenge parameter set' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge, code_challenge_method: 'S256') + click_on 'Authorize' + + url_should_have_param('code', Doorkeeper::AccessGrant.first.token) + url_should_not_have_param('code_challenge_method') + url_should_not_have_param('code_challenge') + end + + scenario 'mobile app requests an access token with authorization code and S256 code challenge method' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge, code_challenge_method: 'S256') + click_on 'Authorize' + + authorization_code = current_params['code'] + create_access_token authorization_code, @client, code_verifier + + access_token_should_exist_for(@client, @resource_owner) + + driver_should_not_have_json 'error' + driver_should_have_json 'access_token', Doorkeeper::AccessToken.first.token + driver_should_have_json 'token_type', 'bearer' + driver_should_have_json_within 'expires_in', Doorkeeper::AccessToken.first.expires_in, 1 + end + + scenario 'mobile app requests an access token with authorization code and without code_verifier' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge, code_challenge_method: 'S256') + click_on 'Authorize' + authorization_code = current_params['code'] + create_access_token authorization_code, @client + + driver_should_have_json 'error', 'invalid_request' + driver_should_not_have_json 'access_token' + end + + scenario 'mobile app requests an access token with authorization code and without secret' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge, code_challenge_method: 'S256') + click_on 'Authorize' + + authorization_code = current_params['code'] + page.driver.post token_endpoint_url( + code: authorization_code, + client_id: @client.uid, + redirect_uri: @client.redirect_uri, + code_verifier: code_verifier + ) + + driver_should_not_have_json 'error' + driver_should_have_json 'access_token', Doorkeeper::AccessToken.first.token + end + + scenario 'mobile app requests an access token with authorization code but no code verifier' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge, code_challenge_method: 'S256') + click_on 'Authorize' + + authorization_code = current_params['code'] + create_access_token authorization_code, @client + + driver_should_not_have_json 'access_token' + driver_should_have_json 'error', 'invalid_request' + end + + scenario 'mobile app requests an access token with authorization code with wrong verifier' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge, code_challenge_method: 'S256') + click_on 'Authorize' + + authorization_code = current_params['code'] + create_access_token authorization_code, @client, 'incorrect-code-verifier' + + driver_should_not_have_json 'access_token' + driver_should_have_json 'error', 'invalid_grant' + end + + scenario 'code_challenge_method in token request is totally ignored' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge, code_challenge_method: 'S256') + click_on 'Authorize' + + authorization_code = current_params['code'] + page.driver.post token_endpoint_url(code: authorization_code, client: @client, code_verifier: code_challenge, + code_challenge_method: 'plain') + + driver_should_not_have_json 'access_token' + driver_should_have_json 'error', 'invalid_grant' + end + + scenario 'expects to set code_challenge_method explicitely without fallback' do + visit authorization_endpoint_url(client: @client, code_challenge: code_challenge) + + expect(page).to have_content('The code challenge method must be plain or S256.') + end + end + end + context 'with scopes' do background do default_scopes_exist :public diff --git a/spec/support/helpers/request_spec_helper.rb b/spec/support/helpers/request_spec_helper.rb index 22c1572e8..78b263d07 100644 --- a/spec/support/helpers/request_spec_helper.rb +++ b/spec/support/helpers/request_spec_helper.rb @@ -55,6 +55,18 @@ def should_not_have_json(key) expect(JSON.parse(response.body)).not_to have_key(key) end + def driver_should_have_json(key, value) + expect(JSON.parse(page.driver.response.body).fetch(key)).to eq(value) + end + + def driver_should_have_json_within(key, value, range) + expect(JSON.parse(page.driver.response.body).fetch(key)).to be_within(range).of(value) + end + + def driver_should_not_have_json(key) + expect(JSON.parse(page.driver.response.body)).not_to have_key(key) + end + def sign_in visit '/' click_on 'Sign in' @@ -65,12 +77,16 @@ def i_should_see_translated_error_message(key) end def translated_error_message(key) - I18n.translate key, scope: [:doorkeeper, :errors, :messages] + I18n.translate key, scope: %i[doorkeeper errors messages] end def response_status_should_be(status) expect(page.driver.response.status.to_i).to eq(status) end + + def create_access_token(authorization_code, client, code_verifier = nil) + page.driver.post token_endpoint_url(code: authorization_code, client: client, code_verifier: code_verifier) + end end RSpec.configuration.send :include, RequestSpecHelper diff --git a/spec/support/helpers/url_helper.rb b/spec/support/helpers/url_helper.rb index 28cde715e..cdd2650f7 100644 --- a/spec/support/helpers/url_helper.rb +++ b/spec/support/helpers/url_helper.rb @@ -5,7 +5,9 @@ def token_endpoint_url(options = {}) client_id: options[:client_id] || (options[:client] ? options[:client].uid : nil), client_secret: options[:client_secret] || (options[:client] ? options[:client].secret : nil), redirect_uri: options[:redirect_uri] || (options[:client] ? options[:client].redirect_uri : nil), - grant_type: options[:grant_type] || 'authorization_code' + grant_type: options[:grant_type] || 'authorization_code', + code_verifier: options[:code_verifier], + code_challenge_method: options[:code_challenge_method] } "/oauth/token?#{build_query(parameters)}" end @@ -28,7 +30,9 @@ def authorization_endpoint_url(options = {}) redirect_uri: options[:redirect_uri] || options[:client].redirect_uri, response_type: options[:response_type] || 'code', scope: options[:scope], - state: options[:state] + state: options[:state], + code_challenge: options[:code_challenge], + code_challenge_method: options[:code_challenge_method] }.reject { |k, v| v.blank? } "/oauth/authorize?#{build_query(parameters)}" end