diff options
6 files changed, 103 insertions, 32 deletions
diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 2a4e659c5b9..f6ad2bf5312 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -4,6 +4,8 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController include Gitlab::Experimentation::ControllerConcern include InitializesCurrentUserMode + before_action :verify_confirmed_email!, only: [:new] + layout 'profile' # Overridden from Doorkeeper::AuthorizationsController to @@ -21,4 +23,13 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController render "doorkeeper/authorizations/error" end end + + private + + def verify_confirmed_email! + return if current_user&.confirmed? + + pre_auth.error = :unconfirmed_email + render "doorkeeper/authorizations/error" + end end diff --git a/changelogs/unreleased/security-dblessing-oauth-email-verification.yml b/changelogs/unreleased/security-dblessing-oauth-email-verification.yml new file mode 100644 index 00000000000..1f9a06d10d5 --- /dev/null +++ b/changelogs/unreleased/security-dblessing-oauth-email-verification.yml @@ -0,0 +1,5 @@ +--- +title: Require confirmed email address for GitLab OAuth authentication +merge_request: +author: +type: security diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml index c9dbde23d35..e0e167a914d 100644 --- a/config/locales/doorkeeper.en.yml +++ b/config/locales/doorkeeper.en.yml @@ -36,6 +36,7 @@ en: access_denied: 'The resource owner or authorization server denied the request.' invalid_scope: 'The requested scope is invalid, unknown, or malformed.' server_error: 'The authorization server encountered an unexpected condition which prevented it from fulfilling the request.' + unconfirmed_email: 'Verify the email address in your account profile before you sign in.' temporarily_unavailable: 'The authorization server is currently unable to handle the request due to a temporary overloading or maintenance of the server.' #configuration error messages diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 1b4bebd9707..f975502ca4e 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -3,7 +3,6 @@ require 'spec_helper' describe Oauth::AuthorizationsController do - let(:user) { create(:user) } let!(:application) { create(:oauth_application, scopes: 'api read_user', redirect_uri: 'http://example.com') } let(:params) do { @@ -19,53 +18,68 @@ describe Oauth::AuthorizationsController do end describe 'GET #new' do - context 'without valid params' do - it 'returns 200 code and renders error view' do - get :new + context 'when the user is confirmed' do + let(:user) { create(:user) } - expect(response).to have_gitlab_http_status(:ok) - expect(response).to render_template('doorkeeper/authorizations/error') + context 'without valid params' do + it 'returns 200 code and renders error view' do + get :new + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('doorkeeper/authorizations/error') + end end - end - context 'with valid params' do - render_views + context 'with valid params' do + render_views - it 'returns 200 code and renders view' do - get :new, params: params + it 'returns 200 code and renders view' do + get :new, params: params - expect(response).to have_gitlab_http_status(:ok) - expect(response).to render_template('doorkeeper/authorizations/new') - end + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('doorkeeper/authorizations/new') + end - it 'deletes session.user_return_to and redirects when skip authorization' do - application.update(trusted: true) - request.session['user_return_to'] = 'http://example.com' + it 'deletes session.user_return_to and redirects when skip authorization' do + application.update(trusted: true) + request.session['user_return_to'] = 'http://example.com' - get :new, params: params + get :new, params: params - expect(request.session['user_return_to']).to be_nil - expect(response).to have_gitlab_http_status(:found) - end + expect(request.session['user_return_to']).to be_nil + expect(response).to have_gitlab_http_status(:found) + end - context 'when there is already an access token for the application' do - context 'when the request scope matches any of the created token scopes' do - before do - scopes = Doorkeeper::OAuth::Scopes.from_string('api') + context 'when there is already an access token for the application' do + context 'when the request scope matches any of the created token scopes' do + before do + scopes = Doorkeeper::OAuth::Scopes.from_string('api') - allow(Doorkeeper.configuration).to receive(:scopes).and_return(scopes) + allow(Doorkeeper.configuration).to receive(:scopes).and_return(scopes) - create :oauth_access_token, application: application, resource_owner_id: user.id, scopes: scopes - end + create :oauth_access_token, application: application, resource_owner_id: user.id, scopes: scopes + end - it 'authorizes the request and redirects' do - get :new, params: params + it 'authorizes the request and redirects' do + get :new, params: params - expect(request.session['user_return_to']).to be_nil - expect(response).to have_gitlab_http_status(:found) + expect(request.session['user_return_to']).to be_nil + expect(response).to have_gitlab_http_status(:found) + end end end end end + + context 'when the user is unconfirmed' do + let(:user) { create(:user, confirmed_at: nil) } + + it 'returns 200 and renders error view' do + get :new, params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('doorkeeper/authorizations/error') + end + end end end diff --git a/spec/features/oauth_provider_authorize_spec.rb b/spec/features/oauth_provider_authorize_spec.rb new file mode 100644 index 00000000000..284fe3b0af9 --- /dev/null +++ b/spec/features/oauth_provider_authorize_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'OAuth Provider' do + describe 'Standard OAuth Authorization' do + let(:application) { create(:oauth_application, scopes: 'read_user') } + + before do + sign_in(user) + + visit oauth_authorization_path(client_id: application.uid, + redirect_uri: application.redirect_uri.split.first, + response_type: 'code', + state: 'my_state', + scope: 'read_user') + end + + it_behaves_like 'Secure OAuth Authorizations' + end +end diff --git a/spec/support/shared_examples/features/secure_oauth_authorizations_shared_examples.rb b/spec/support/shared_examples/features/secure_oauth_authorizations_shared_examples.rb new file mode 100644 index 00000000000..028e075c87a --- /dev/null +++ b/spec/support/shared_examples/features/secure_oauth_authorizations_shared_examples.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'Secure OAuth Authorizations' do + context 'when user is confirmed' do + let(:user) { create(:user) } + + it 'asks the user to authorize the application' do + expect(page).to have_text "Authorize #{application.name} to use your account?" + end + end + + context 'when user is unconfirmed' do + let(:user) { create(:user, confirmed_at: nil) } + + it 'displays an error' do + expect(page).to have_text I18n.t('doorkeeper.errors.messages.unconfirmed_email') + end + end +end |