Add enforcing 2fa on in Oauth controllers

Add one more spec

Add changelog entry

Fix changelog

Move concern to doorkeeper base controller

Add base metal controller

Add new controller

Fix

Remove 2FA from api-endpoints

Add if-clause block around helper method

Add controllers tests

And implement 2FA enforcement in tokens controllers

Remove obsolete let in spec

Fix

Fix

app/controllers/concerns/enforces_two_factor_authentication.rb

@@ -12,10 +12,17 @@ module EnforcesTwoFactorAuthentication
 
   included do
     before_action :check_two_factor_requirement
-    helper_method :two_factor_grace_period_expired?, :two_factor_skippable?
+
+    # to include this in controllers inheriting from `ActionController::Metal`
+    # we need to add this block
+    if respond_to?(:helper_method)
+      helper_method :two_factor_grace_period_expired?, :two_factor_skippable?
+    end
   end
 
   def check_two_factor_requirement
+    return unless respond_to?(:current_user)
+
     if two_factor_authentication_required? && current_user_requires_two_factor?
       redirect_to profile_two_factor_auth_path
     end

app/controllers/oauth/applications_controller.rb

@@ -2,7 +2,6 @@
 
 class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
   include Gitlab::GonHelper
-  include Gitlab::Allowable
   include PageLayoutHelper
   include OauthApplications
   include Gitlab::Experimentation::ControllerConcern
@@ -19,8 +18,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
 
   around_action :set_locale
 
-  helper_method :can?
-
   layout 'profile'
 
   def index

app/controllers/oauth/token_info_controller.rb

 # frozen_string_literal: true
 
 class Oauth::TokenInfoController < Doorkeeper::TokenInfoController
+  include EnforcesTwoFactorAuthentication
+
   def show
     if doorkeeper_token && doorkeeper_token.accessible?
       token_json = doorkeeper_token.as_json

app/controllers/oauth/tokens_controller.rb

+# frozen_string_literal: true
+
+class Oauth::TokensController < Doorkeeper::TokensController
+  include EnforcesTwoFactorAuthentication
+end

changelogs/unreleased/security-195-2fa-applications-view.yml

+---
+title: Enforce 2FA on Doorkeeper controllers
+merge_request:
+author:
+type: security

config/routes.rb

@@ -25,7 +25,8 @@ Rails.application.routes.draw do
     controllers applications: 'oauth/applications',
                 authorized_applications: 'oauth/authorized_applications',
                 authorizations: 'oauth/authorizations',
-                token_info: 'oauth/token_info'
+                token_info: 'oauth/token_info',
+                tokens: 'oauth/tokens'
   end
 
   # This prefixless path is required because Jira gets confused if we set it up with a path

lib/gitlab/base_doorkeeper_controller.rb

@@ -5,6 +5,8 @@
 module Gitlab
   class BaseDoorkeeperController < ActionController::Base
     include Gitlab::Allowable
+    include EnforcesTwoFactorAuthentication
+
     helper_method :can?
   end
 end

spec/controllers/oauth/applications_controller_spec.rb

@@ -19,12 +19,29 @@ RSpec.describe Oauth::ApplicationsController do
       it { is_expected.to redirect_to(new_user_session_path) }
     end
 
+    shared_examples 'redirects to 2fa setup page when the user requires it' do
+      context 'when 2fa is set up on application level' do
+        before do
+          stub_application_setting(require_two_factor_authentication: true)
+        end
+
+        it { is_expected.to redirect_to(profile_two_factor_auth_path) }
+      end
+
+      context 'when 2fa is set up on group level' do
+        let(:user) { create(:user, require_two_factor_authentication_from_group: true) }
+
+        it { is_expected.to redirect_to(profile_two_factor_auth_path) }
+      end
+    end
+
     describe 'GET #new' do
       subject { get :new }
 
       it { is_expected.to have_gitlab_http_status(:ok) }
 
       it_behaves_like 'redirects to login page when the user is not signed in'
+      it_behaves_like 'redirects to 2fa setup page when the user requires it'
     end
 
     describe 'DELETE #destroy' do
@@ -33,6 +50,7 @@ RSpec.describe Oauth::ApplicationsController do
       it { is_expected.to redirect_to(oauth_applications_url) }
 
       it_behaves_like 'redirects to login page when the user is not signed in'
+      it_behaves_like 'redirects to 2fa setup page when the user requires it'
     end
 
     describe 'GET #edit' do
@@ -41,6 +59,7 @@ RSpec.describe Oauth::ApplicationsController do
       it { is_expected.to have_gitlab_http_status(:ok) }
 
       it_behaves_like 'redirects to login page when the user is not signed in'
+      it_behaves_like 'redirects to 2fa setup page when the user requires it'
     end
 
     describe 'PUT #update' do
@@ -49,6 +68,7 @@ RSpec.describe Oauth::ApplicationsController do
       it { is_expected.to redirect_to(oauth_application_url(application)) }
 
       it_behaves_like 'redirects to login page when the user is not signed in'
+      it_behaves_like 'redirects to 2fa setup page when the user requires it'
     end
 
     describe 'GET #show' do
@@ -57,6 +77,7 @@ RSpec.describe Oauth::ApplicationsController do
       it { is_expected.to have_gitlab_http_status(:ok) }
 
       it_behaves_like 'redirects to login page when the user is not signed in'
+      it_behaves_like 'redirects to 2fa setup page when the user requires it'
     end
 
     describe 'GET #index' do
@@ -73,6 +94,7 @@ RSpec.describe Oauth::ApplicationsController do
       end
 
       it_behaves_like 'redirects to login page when the user is not signed in'
+      it_behaves_like 'redirects to 2fa setup page when the user requires it'
     end
 
     describe 'POST #create' do
@@ -112,6 +134,7 @@ RSpec.describe Oauth::ApplicationsController do
       end
 
       it_behaves_like 'redirects to login page when the user is not signed in'
+      it_behaves_like 'redirects to 2fa setup page when the user requires it'
     end
   end
 
@@ -119,6 +142,10 @@ RSpec.describe Oauth::ApplicationsController do
     it 'current_user_mode available' do
       expect(subject.current_user_mode).not_to be_nil
     end
+
+    it 'includes Two-factor enforcement concern' do
+      expect(described_class.included_modules.include?(EnforcesTwoFactorAuthentication)).to eq(true)
+    end
   end
 
   describe 'locale' do

spec/controllers/oauth/authorizations_controller_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Oauth::AuthorizationsController do
   let(:user) { create(:user, confirmed_at: confirmed_at) }
+  let(:confirmed_at) { 1.hour.ago }
   let!(:application) { create(:oauth_application, scopes: 'api read_user', redirect_uri: 'http://example.com') }
   let(:params) do
     {
@@ -20,8 +21,6 @@ RSpec.describe Oauth::AuthorizationsController do
 
   shared_examples 'OAuth Authorizations require confirmed user' do
     context 'when the user is confirmed' do
-      let(:confirmed_at) { 1.hour.ago }
-
       context 'when there is already an access token for the application with a matching scope' do
         before do
           scopes = Doorkeeper::OAuth::Scopes.from_string('api')
@@ -103,4 +102,8 @@ RSpec.describe Oauth::AuthorizationsController do
 
     include_examples 'OAuth Authorizations require confirmed user'
   end
+
+  it 'includes Two-factor enforcement concern' do
+    expect(described_class.included_modules.include?(EnforcesTwoFactorAuthentication)).to eq(true)
+  end
 end

spec/controllers/oauth/authorized_applications_controller_spec.rb

@@ -34,4 +34,8 @@ RSpec.describe Oauth::AuthorizedApplicationsController do
       expect(access_token.reload).to be_revoked
     end
   end
+
+  it 'includes Two-factor enforcement concern' do
+    expect(described_class.included_modules.include?(EnforcesTwoFactorAuthentication)).to eq(true)
+  end
 end

spec/controllers/oauth/token_info_controller_spec.rb

@@ -68,4 +68,8 @@ RSpec.describe Oauth::TokenInfoController do
       end
     end
   end
+
+  it 'includes Two-factor enforcement concern' do
+    expect(described_class.included_modules.include?(EnforcesTwoFactorAuthentication)).to eq(true)
+  end
 end

spec/controllers/oauth/tokens_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Oauth::TokensController do
+  it 'includes Two-factor enforcement concern' do
+    expect(described_class.included_modules.include?(EnforcesTwoFactorAuthentication)).to eq(true)
+  end
+end