From dd5379271410219de0806bb33e7cdfa427ea6988 Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Fri, 26 Jul 2024 16:53:49 +0200 Subject: [PATCH] Move UI tests from controller to system spec (#31158) --- .../oauth/authorizations_controller_spec.rb | 128 +----------------- spec/system/oauth_spec.rb | 86 +++++++++++- 2 files changed, 82 insertions(+), 132 deletions(-) diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 3908ef8db0..9bb520211c 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -3,8 +3,6 @@ require 'rails_helper' RSpec.describe Oauth::AuthorizationsController do - render_views - let(:app) { Doorkeeper::Application.create!(name: 'test', redirect_uri: 'http://localhost/', scopes: 'read') } describe 'GET #new' do @@ -12,26 +10,6 @@ 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 @@ -56,8 +34,6 @@ RSpec.describe Oauth::AuthorizationsController do expect(response.headers['Cache-Control']).to include('private, no-store') end - include_examples 'shows authorize and deny buttons' - include_examples 'stores location for user' context 'when app is already authorized' do @@ -79,109 +55,7 @@ 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' } - 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 + expect(response).to have_http_status(:success) end end end diff --git a/spec/system/oauth_spec.rb b/spec/system/oauth_spec.rb index 1c9aca3114..42ff4a48ac 100644 --- a/spec/system/oauth_spec.rb +++ b/spec/system/oauth_spec.rb @@ -2,8 +2,13 @@ require 'rails_helper' -describe 'Using OAuth from an external app', :js, :streaming do +describe 'Using OAuth from an external app' do + subject { visit "/oauth/authorize?#{params.to_query}" } + let(:client_app) { Doorkeeper::Application.create!(name: 'test', redirect_uri: about_url(host: Rails.application.config.x.local_domain), scopes: 'read') } + let(:params) do + { client_id: client_app.uid, response_type: 'code', redirect_uri: client_app.redirect_uri, scope: 'read' } + end context 'when the user is already logged in' do let!(:user) { Fabricate(:user) } @@ -14,8 +19,7 @@ describe 'Using OAuth from an external app', :js, :streaming do end it 'when accepting the authorization request' do - params = { client_id: client_app.uid, response_type: 'code', redirect_uri: client_app.redirect_uri, scope: 'read' } - visit "/oauth/authorize?#{params.to_query}" + subject # It presents the user with an authorization page expect(page).to have_content(I18n.t('doorkeeper.authorizations.buttons.authorize')) @@ -29,8 +33,7 @@ describe 'Using OAuth from an external app', :js, :streaming do end it 'when rejecting the authorization request' do - params = { client_id: client_app.uid, response_type: 'code', redirect_uri: client_app.redirect_uri, scope: 'read' } - visit "/oauth/authorize?#{params.to_query}" + subject # It presents the user with an authorization page expect(page).to have_content(I18n.t('doorkeeper.authorizations.buttons.deny')) @@ -42,6 +45,79 @@ describe 'Using OAuth from an external app', :js, :streaming do # It does not grant the app access to the account expect(Doorkeeper::AccessGrant.exists?(application: client_app, resource_owner_id: user.id)).to be false 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 + it 'does not include the PKCE values in the hidden inputs' do + subject + + code_challenge_inputs = all('.oauth-prompt input[name=code_challenge]', visible: false) + code_challenge_method_inputs = all('.oauth-prompt input[name=code_challenge_method]', visible: false) + + expect(code_challenge_inputs).to_not be_empty + expect(code_challenge_method_inputs).to_not be_empty + + (code_challenge_inputs.to_a + code_challenge_method_inputs.to_a).each do |input| + expect(input.value).to be_nil + end + end + end + + context 'when using PKCE' do + let(:params) do + { client_id: client_app.uid, response_type: 'code', redirect_uri: client_app.redirect_uri, 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 'includes the PKCE values in the hidden inputs' do + subject + + code_challenge_inputs = all('.oauth-prompt input[name=code_challenge]', visible: false) + code_challenge_method_inputs = all('.oauth-prompt input[name=code_challenge_method]', visible: false) + + expect(code_challenge_inputs).to_not be_empty + expect(code_challenge_method_inputs).to_not be_empty + + code_challenge_inputs.each do |input| + expect(input.value).to eq pkce_code_challenge + end + code_challenge_method_inputs.each do |input| + expect(input.value).to eq pkce_code_challenge_method + end + end + end + + context 'when using plain code challenge method' do + let(:pkce_code_challenge_method) { 'plain' } + + it 'does not include the PKCE values in the response' do + subject + + expect(page).to have_no_css('.oauth-prompt input[name=code_challenge]') + expect(page).to have_no_css('.oauth-prompt input[name=code_challenge_method]') + end + + it 'does not include the authorize button' do + subject + + expect(page).to have_no_css('.oauth-prompt button[type="submit"]') + end + + it 'includes an error message' do + subject + + within '.form-container .flash-message' do + expect(page).to have_content(I18n.t('doorkeeper.errors.messages.invalid_code_challenge_method')) + end + end + end + end end context 'when the user is not already logged in' do