From 693d9b03ed37f5d74cae66dd8bfece4447cb973f Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Fri, 26 Jul 2024 10:53:10 +0200 Subject: [PATCH] Enable OAuth PKCE Extension (#31129) --- app/lib/oauth_pre_authorization_extension.rb | 13 ++ app/presenters/oauth_metadata_presenter.rb | 5 + app/serializers/oauth_metadata_serializer.rb | 1 + app/views/oauth/authorizations/new.html.haml | 4 + config/application.rb | 1 + config/locales/doorkeeper.en.yml | 1 + db/migrate/20240724181224_enable_pkce.rb | 8 ++ db/schema.rb | 4 +- .../oauth/authorizations_controller_spec.rb | 130 +++++++++++++++++- .../well_known/oauth_metadata_spec.rb | 3 + 10 files changed, 164 insertions(+), 6 deletions(-) create mode 100644 app/lib/oauth_pre_authorization_extension.rb create mode 100644 db/migrate/20240724181224_enable_pkce.rb diff --git a/app/lib/oauth_pre_authorization_extension.rb b/app/lib/oauth_pre_authorization_extension.rb new file mode 100644 index 0000000000..1885e0823d --- /dev/null +++ b/app/lib/oauth_pre_authorization_extension.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module OauthPreAuthorizationExtension + extend ActiveSupport::Concern + + included do + validate :code_challenge_method_s256, error: Doorkeeper::Errors::InvalidCodeChallengeMethod + end + + def validate_code_challenge_method_s256 + code_challenge.blank? || code_challenge_method == 'S256' + end +end diff --git a/app/presenters/oauth_metadata_presenter.rb b/app/presenters/oauth_metadata_presenter.rb index 546503bfcc..1e4d25165c 100644 --- a/app/presenters/oauth_metadata_presenter.rb +++ b/app/presenters/oauth_metadata_presenter.rb @@ -7,6 +7,7 @@ class OauthMetadataPresenter < ActiveModelSerializers::Model :revocation_endpoint, :scopes_supported, :response_types_supported, :response_modes_supported, :grant_types_supported, :token_endpoint_auth_methods_supported, + :code_challenge_methods_supported, :service_documentation, :app_registration_endpoint def issuer @@ -59,6 +60,10 @@ class OauthMetadataPresenter < ActiveModelSerializers::Model %w(client_secret_basic client_secret_post) end + def code_challenge_methods_supported + %w(S256) + end + private def doorkeeper diff --git a/app/serializers/oauth_metadata_serializer.rb b/app/serializers/oauth_metadata_serializer.rb index 5f3dc7b87e..2afb4208fb 100644 --- a/app/serializers/oauth_metadata_serializer.rb +++ b/app/serializers/oauth_metadata_serializer.rb @@ -5,5 +5,6 @@ class OauthMetadataSerializer < ActiveModel::Serializer :revocation_endpoint, :scopes_supported, :response_types_supported, :response_modes_supported, :grant_types_supported, :token_endpoint_auth_methods_supported, + :code_challenge_methods_supported, :service_documentation, :app_registration_endpoint end diff --git a/app/views/oauth/authorizations/new.html.haml b/app/views/oauth/authorizations/new.html.haml index c50c224cce..d4563b2f01 100644 --- a/app/views/oauth/authorizations/new.html.haml +++ b/app/views/oauth/authorizations/new.html.haml @@ -26,6 +26,8 @@ value: @pre_auth.client.uid = form.hidden_field :redirect_uri, value: @pre_auth.redirect_uri + = form.hidden_field :code_challenge, value: @pre_auth.code_challenge + = form.hidden_field :code_challenge_method, value: @pre_auth.code_challenge_method = form.hidden_field :state, value: @pre_auth.state = form.hidden_field :response_type, @@ -40,6 +42,8 @@ value: @pre_auth.client.uid = form.hidden_field :redirect_uri, value: @pre_auth.redirect_uri + = form.hidden_field :code_challenge, value: @pre_auth.code_challenge + = form.hidden_field :code_challenge_method, value: @pre_auth.code_challenge_method = form.hidden_field :state, value: @pre_auth.state = form.hidden_field :response_type, diff --git a/config/application.rb b/config/application.rb index 5aca74fd1c..3c62a49228 100644 --- a/config/application.rb +++ b/config/application.rb @@ -118,6 +118,7 @@ module Mastodon Doorkeeper::Application.include ApplicationExtension Doorkeeper::AccessGrant.include AccessGrantExtension Doorkeeper::AccessToken.include AccessTokenExtension + Doorkeeper::OAuth::PreAuthorization.include OauthPreAuthorizationExtension Devise::FailureApp.include AbstractController::Callbacks Devise::FailureApp.include Localized end diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml index b623cc7135..e28f6a7966 100644 --- a/config/locales/doorkeeper.en.yml +++ b/config/locales/doorkeeper.en.yml @@ -83,6 +83,7 @@ en: access_denied: The resource owner or authorization server denied the request. credential_flow_not_configured: Resource Owner Password Credentials flow failed due to Doorkeeper.configure.resource_owner_from_credentials being unconfigured. invalid_client: Client authentication failed due to unknown client, no client authentication included, or unsupported authentication method. + invalid_code_challenge_method: The code challenge method must be S256, plain is unsupported. invalid_grant: The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client. invalid_redirect_uri: The redirect uri included is not valid. invalid_request: diff --git a/db/migrate/20240724181224_enable_pkce.rb b/db/migrate/20240724181224_enable_pkce.rb new file mode 100644 index 0000000000..4816f58f07 --- /dev/null +++ b/db/migrate/20240724181224_enable_pkce.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class EnablePkce < ActiveRecord::Migration[7.1] + def change + add_column :oauth_access_grants, :code_challenge, :string, null: true + add_column :oauth_access_grants, :code_challenge_method, :string, null: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 453f1f7b89..d4796079ca 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_07_13_171909) do +ActiveRecord::Schema[7.1].define(version: 2024_07_24_181224) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -739,6 +739,8 @@ ActiveRecord::Schema[7.1].define(version: 2024_07_13_171909) do t.string "scopes" t.bigint "application_id", null: false t.bigint "resource_owner_id", null: false + t.string "code_challenge" + t.string "code_challenge_method" t.index ["resource_owner_id"], name: "index_oauth_access_grants_on_resource_owner_id" t.index ["token"], name: "index_oauth_access_grants_on_token", unique: true end diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 9b3ae251e6..3908ef8db0 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -12,6 +12,26 @@ RSpec.describe Oauth::AuthorizationsController do get :new, params: { client_id: app.uid, response_type: 'code', redirect_uri: 'http://localhost/', scope: 'read' } end + def body + Nokogiri::Slop(response.body) + end + + def error_message + body.at_css('.form-container .flash-message').text + end + + shared_examples 'shows authorize and deny buttons' do + it 'gives options to authorize and deny' do + subject + + authorize_button = body.at_css('.oauth-prompt button[type="submit"]:not(.negative)') + deny_button = body.at_css('.oauth-prompt button[type="submit"].negative') + + expect(authorize_button).to be_present + expect(deny_button).to be_present + end + end + shared_examples 'stores location for user' do it 'stores location for user' do subject @@ -36,10 +56,7 @@ RSpec.describe Oauth::AuthorizationsController do expect(response.headers['Cache-Control']).to include('private, no-store') end - it 'gives options to authorize and deny' do - subject - expect(response.body).to match(/Authorize/) - end + include_examples 'shows authorize and deny buttons' include_examples 'stores location for user' @@ -61,7 +78,110 @@ RSpec.describe Oauth::AuthorizationsController do it 'does not redirect to callback with force_login=true' do get :new, params: { client_id: app.uid, response_type: 'code', redirect_uri: 'http://localhost/', scope: 'read', force_login: 'true' } - expect(response.body).to match(/Authorize/) + + authorize_button = body.at_css('.oauth-prompt button[type="submit"]:not(.negative)') + deny_button = body.at_css('.oauth-prompt button[type="submit"].negative') + + expect(authorize_button).to be_present + expect(deny_button).to be_present + end + end + + # The tests in this context ensures that requests without PKCE parameters + # still work; In the future we likely want to force usage of PKCE for + # security reasons, as per: + # + # https://www.ietf.org/archive/id/draft-ietf-oauth-security-topics-27.html#section-2.1.1-9 + context 'when not using PKCE' do + subject do + get :new, params: { client_id: app.uid, response_type: 'code', redirect_uri: 'http://localhost/', scope: 'read' } + end + + it 'returns http success' do + subject + expect(response).to have_http_status(200) + end + + it 'does not include the PKCE values in the response' do + subject + + code_challenge_input = body.at_css('.oauth-prompt input[name=code_challenge]') + code_challenge_method_input = body.at_css('.oauth-prompt input[name=code_challenge_method]') + + expect(code_challenge_input).to be_present + expect(code_challenge_method_input).to be_present + + expect(code_challenge_input.attr('value')).to be_nil + expect(code_challenge_method_input.attr('value')).to be_nil + end + + include_examples 'shows authorize and deny buttons' + end + + context 'when using PKCE' do + subject do + get :new, params: { client_id: app.uid, response_type: 'code', redirect_uri: 'http://localhost/', scope: 'read', code_challenge_method: pkce_code_challenge_method, code_challenge: pkce_code_challenge } + end + + let(:pkce_code_challenge) { SecureRandom.hex(32) } + let(:pkce_code_challenge_method) { 'S256' } + + context 'when using S256 code challenge method' do + it 'returns http success' do + subject + expect(response).to have_http_status(200) + end + + it 'includes the PKCE values in the response' do + subject + + code_challenge_input = body.at_css('.oauth-prompt input[name=code_challenge]') + code_challenge_method_input = body.at_css('.oauth-prompt input[name=code_challenge_method]') + + expect(code_challenge_input).to be_present + expect(code_challenge_method_input).to be_present + + expect(code_challenge_input.attr('value')).to eq pkce_code_challenge + expect(code_challenge_method_input.attr('value')).to eq pkce_code_challenge_method + end + + include_examples 'shows authorize and deny buttons' + end + + context 'when using plain code challenge method' do + subject do + get :new, params: { client_id: app.uid, response_type: 'code', redirect_uri: 'http://localhost/', scope: 'read', code_challenge_method: pkce_code_challenge_method, code_challenge: pkce_code_challenge } + end + + let(:pkce_code_challenge_method) { 'plain' } + + it 'returns http success' do + subject + expect(response).to have_http_status(400) + end + + it 'does not include the PKCE values in the response' do + subject + + code_challenge_input = body.at_css('.oauth-prompt input[name=code_challenge]') + code_challenge_method_input = body.at_css('.oauth-prompt input[name=code_challenge_method]') + + expect(code_challenge_input).to_not be_present + expect(code_challenge_method_input).to_not be_present + end + + it 'does not include the authorize button' do + subject + + authorize_button = body.at_css('.oauth-prompt button[type="submit"]') + + expect(authorize_button).to_not be_present + end + + it 'includes an error message' do + subject + expect(error_message).to match I18n.t('doorkeeper.errors.messages.invalid_code_challenge_method') + end end end end diff --git a/spec/requests/well_known/oauth_metadata_spec.rb b/spec/requests/well_known/oauth_metadata_spec.rb index 3350d59315..9d2d202286 100644 --- a/spec/requests/well_known/oauth_metadata_spec.rb +++ b/spec/requests/well_known/oauth_metadata_spec.rb @@ -29,7 +29,10 @@ describe 'The /.well-known/oauth-authorization-server request' do revocation_endpoint: oauth_revoke_url(protocol: protocol), scopes_supported: Doorkeeper.configuration.scopes.map(&:to_s), response_types_supported: Doorkeeper.configuration.authorization_response_types, + response_modes_supported: Doorkeeper.configuration.authorization_response_flows.flat_map(&:response_mode_matches).uniq, + token_endpoint_auth_methods_supported: %w(client_secret_basic client_secret_post), grant_types_supported: grant_types_supported, + code_challenge_methods_supported: ['S256'], # non-standard extension: app_registration_endpoint: api_v1_apps_url(protocol: protocol) )