Add latest changes from gitlab-org/security/gitlab@12-10-stable-ee

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