Merge branch 'feature/enforce-2fa-per-group' into 'master'

Support 2FA requirement per-group

See merge request !8763

15 files changed, 216 insertions, 58 deletions

diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index cea3d088e94..f28bbdeff5a 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -72,7 +72,9 @@ class Admin::GroupsController < Admin::ApplicationController
       :name,
       :path,
       :request_access_enabled,
-      :visibility_level
+      :visibility_level,
+      :require_two_factor_authentication,
+      :two_factor_grace_period
     ]
   end
 end
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 6a6e335d314..e77094fe2a8 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -8,12 +8,12 @@ class ApplicationController < ActionController::Base
   include PageLayoutHelper
   include SentryHelper
   include WorkhorseHelper
+  include EnforcesTwoFactorAuthentication
 
   before_action :authenticate_user_from_private_token!
   before_action :authenticate_user!
   before_action :validate_user_service_ticket!
   before_action :check_password_expiration
-  before_action :check_2fa_requirement
   before_action :ldap_security_check
   before_action :sentry_context
   before_action :default_headers
@@ -151,12 +151,6 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def check_2fa_requirement
-    if two_factor_authentication_required? && current_user && !current_user.two_factor_enabled? && !skip_two_factor?
-      redirect_to profile_two_factor_auth_path
-    end
-  end
-
   def ldap_security_check
     if current_user && current_user.requires_ldap_check?
       return unless current_user.try_obtain_ldap_lease
@@ -265,23 +259,6 @@ class ApplicationController < ActionController::Base
     current_application_settings.import_sources.include?('gitlab_project')
   end
 
-  def two_factor_authentication_required?
-    current_application_settings.require_two_factor_authentication
-  end
-
-  def two_factor_grace_period
-    current_application_settings.two_factor_grace_period
-  end
-
-  def two_factor_grace_period_expired?
-    date = current_user.otp_grace_period_started_at
-    date && (date + two_factor_grace_period.hours) < Time.current
-  end
-
-  def skip_two_factor?
-    session[:skip_tfa] && session[:skip_tfa] > Time.current
-  end
-
   # U2F (universal 2nd factor) devices need a unique identifier for the application
   # to perform authentication.
   # https://developers.yubico.com/U2F/App_ID.html
diff --git a/app/controllers/concerns/enforces_two_factor_authentication.rb b/app/controllers/concerns/enforces_two_factor_authentication.rb
new file mode 100644
index 00000000000..688e8bd4a37
--- /dev/null
+++ b/app/controllers/concerns/enforces_two_factor_authentication.rb
@@ -0,0 +1,58 @@
+# == EnforcesTwoFactorAuthentication
+#
+# Controller concern to enforce two-factor authentication requirements
+#
+# Upon inclusion, adds `check_two_factor_requirement` as a before_action,
+# and makes `two_factor_grace_period_expired?` and `two_factor_skippable?`
+# available as view helpers.
+module EnforcesTwoFactorAuthentication
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :check_two_factor_requirement
+    helper_method :two_factor_grace_period_expired?, :two_factor_skippable?
+  end
+
+  def check_two_factor_requirement
+    if two_factor_authentication_required? && current_user && !current_user.two_factor_enabled? && !skip_two_factor?
+      redirect_to profile_two_factor_auth_path
+    end
+  end
+
+  def two_factor_authentication_required?
+    current_application_settings.require_two_factor_authentication? ||
+      current_user.try(:require_two_factor_authentication_from_group?)
+  end
+
+  def two_factor_authentication_reason(global: -> {}, group: -> {})
+    if two_factor_authentication_required?
+      if current_application_settings.require_two_factor_authentication?
+        global.call
+      else
+        groups = current_user.expanded_groups_requiring_two_factor_authentication.reorder(name: :asc)
+        group.call(groups)
+      end
+    end
+  end
+
+  def two_factor_grace_period
+    periods = [current_application_settings.two_factor_grace_period]
+    periods << current_user.two_factor_grace_period if current_user.try(:require_two_factor_authentication_from_group?)
+    periods.min
+  end
+
+  def two_factor_grace_period_expired?
+    date = current_user.otp_grace_period_started_at
+    date && (date + two_factor_grace_period.hours) < Time.current
+  end
+
+  def two_factor_skippable?
+    two_factor_authentication_required? &&
+      !current_user.two_factor_enabled? &&
+      !two_factor_grace_period_expired?
+  end
+
+  def skip_two_factor?
+    session[:skip_two_factor] && session[:skip_two_factor] > Time.current
+  end
+end
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 78c9f1f7004..593001e6396 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -151,7 +151,9 @@ class GroupsController < Groups::ApplicationController
       :visibility_level,
       :parent_id,
       :create_chat_team,
-      :chat_team_name
+      :chat_team_name,
+      :require_two_factor_authentication,
+      :two_factor_grace_period
     ]
   end
 
diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb
index 26e7e93533e..d3fa81cd623 100644
--- a/app/controllers/profiles/two_factor_auths_controller.rb
+++ b/app/controllers/profiles/two_factor_auths_controller.rb
@@ -1,5 +1,5 @@
 class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
-  skip_before_action :check_2fa_requirement
+  skip_before_action :check_two_factor_requirement
 
   def show
     unless current_user.otp_secret
@@ -13,11 +13,24 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
     current_user.save! if current_user.changed?
 
     if two_factor_authentication_required? && !current_user.two_factor_enabled?
-      if two_factor_grace_period_expired?
-        flash.now[:alert] = 'You must enable Two-Factor Authentication for your account.'
-      else
+      two_factor_authentication_reason(
+        global: lambda do
+          flash.now[:alert] =
+            'The global settings require you to enable Two-Factor Authentication for your account.'
+        end,
+        group: lambda do |groups|
+          group_links = groups.map { |group| view_context.link_to group.full_name, group_path(group) }.to_sentence
+
+          flash.now[:alert] = %{
+            The group settings for #{group_links} require you to enable
+            Two-Factor Authentication for your account.
+          }.html_safe
+        end
+      )
+
+      unless two_factor_grace_period_expired?
         grace_period_deadline = current_user.otp_grace_period_started_at + two_factor_grace_period.hours
-        flash.now[:alert] = "You must enable Two-Factor Authentication for your account before #{l(grace_period_deadline)}."
+        flash.now[:alert] << " You need to do this before #{l(grace_period_deadline)}."
       end
     end
 
@@ -71,7 +84,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
     if two_factor_grace_period_expired?
       redirect_to new_profile_two_factor_auth_path, alert: 'Cannot skip two factor authentication setup'
     else
-      session[:skip_tfa] = current_user.otp_grace_period_started_at + two_factor_grace_period.hours
+      session[:skip_two_factor] = current_user.otp_grace_period_started_at + two_factor_grace_period.hours
       redirect_to root_path
     end
   end
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index d8561871098..d3091a4f8e9 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -3,7 +3,7 @@ class SessionsController < Devise::SessionsController
   include Devise::Controllers::Rememberable
   include Recaptcha::ClientHelper
 
-  skip_before_action :check_2fa_requirement, only: [:destroy]
+  skip_before_action :check_two_factor_requirement, only: [:destroy]
 
   prepend_before_action :check_initial_setup, only: [:new]
   prepend_before_action :authenticate_with_two_factor,
diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb
index 101fe579da2..9c71d6c7f4c 100644
--- a/app/helpers/auth_helper.rb
+++ b/app/helpers/auth_helper.rb
@@ -64,18 +64,6 @@ module AuthHelper
     current_user.identities.exists?(provider: provider.to_s)
   end
 
-  def two_factor_skippable?
-    current_application_settings.require_two_factor_authentication &&
-      !current_user.two_factor_enabled? &&
-      current_application_settings.two_factor_grace_period &&
-      !two_factor_grace_period_expired?
-  end
-
-  def two_factor_grace_period_expired?
-    current_user.otp_grace_period_started_at &&
-      (current_user.otp_grace_period_started_at + current_application_settings.two_factor_grace_period.hours) < Time.current
-  end
-
   def unlink_allowed?(provider)
     %w(saml cas3).exclude?(provider.to_s)
   end
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index 529fb5ce988..aca99feee53 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -83,6 +83,74 @@ module Routable
                AND members.source_type = r2.source_type").
         where('members.user_id = ?', user_id)
     end
+
+    # Builds a relation to find multiple objects that are nested under user
+    # membership. Includes the parent, as opposed to `#member_descendants`
+    # which only includes the descendants.
+    #
+    # Usage:
+    #
+    #     Klass.member_self_and_descendants(1)
+    #
+    # Returns an ActiveRecord::Relation.
+    def member_self_and_descendants(user_id)
+      joins(:route).
+        joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(r2.path, '/%')
+               OR routes.path = r2.path
+               INNER JOIN members ON members.source_id = r2.source_id
+               AND members.source_type = r2.source_type").
+        where('members.user_id = ?', user_id)
+    end
+
+    # Returns all objects in a hierarchy, where any node in the hierarchy is
+    # under the user membership.
+    #
+    # Usage:
+    #
+    #     Klass.member_hierarchy(1)
+    #
+    # Examples:
+    #
+    #     Given the following group tree...
+    #
+    #            _______group_1_______
+    #           |                     |
+    #           |                     |
+    #     nested_group_1        nested_group_2
+    #           |                     |
+    #           |                     |
+    #     nested_group_1_1      nested_group_2_1
+    #
+    #
+    #     ... the following results are returned:
+    #
+    #     * the user is a member of group 1
+    #       => 'group_1',
+    #          'nested_group_1', nested_group_1_1',
+    #          'nested_group_2', 'nested_group_2_1'
+    #
+    #     * the user is a member of nested_group_2
+    #       => 'group1',
+    #          'nested_group_2', 'nested_group_2_1'
+    #
+    #     * the user is a member of nested_group_2_1
+    #       => 'group1',
+    #          'nested_group_2', 'nested_group_2_1'
+    #
+    # Returns an ActiveRecord::Relation.
+    def member_hierarchy(user_id)
+      paths = member_self_and_descendants(user_id).pluck('routes.path')
+
+      return none if paths.empty?
+
+      wheres = paths.map do |path|
+        "#{connection.quote(path)} = routes.path
+         OR
+         #{connection.quote(path)} LIKE CONCAT(routes.path, '/%')"
+      end
+
+      joins(:route).where(wheres.join(' OR '))
+    end
   end
 
   def full_name
diff --git a/app/models/group.rb b/app/models/group.rb
index 60274386103..106084175ff 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -27,11 +27,14 @@ class Group < Namespace
 
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
+  validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
+
   mount_uploader :avatar, AvatarUploader
   has_many :uploads, as: :model, dependent: :destroy
 
   after_create :post_create_hook
   after_destroy :post_destroy_hook
+  after_save :update_two_factor_requirement
 
   class << self
     # Searches for groups matching the given query.
@@ -223,4 +226,12 @@ class Group < Namespace
       type: public? ? 'O' : 'I' # Open vs Invite-only
     }
   end
+
+  protected
+
+  def update_two_factor_requirement
+    return unless require_two_factor_authentication_changed? || two_factor_grace_period_changed?
+
+    users.find_each(&:update_two_factor_requirement)
+  end
 end
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 446f9f8f8a7..483425cd30f 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -3,11 +3,16 @@ class GroupMember < Member
 
   belongs_to :group, foreign_key: 'source_id'
 
+  delegate :update_two_factor_requirement, to: :user
+
   # Make sure group member points only to group as it source
   default_value_for :source_type, SOURCE_TYPE
   validates :source_type, format: { with: /\ANamespace\z/ }
   default_scope { where(source_type: SOURCE_TYPE) }
 
+  after_create :update_two_factor_requirement, unless: :invite?
+  after_destroy :update_two_factor_requirement, unless: :invite?
+
   def self.access_level_roles
     Gitlab::Access.options_with_owner
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 95a766f2ede..c358b1b8d2b 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -484,6 +484,14 @@ class User < ActiveRecord::Base
     Group.member_descendants(id)
   end
 
+  def all_expanded_groups
+    Group.member_hierarchy(id)
+  end
+
+  def expanded_groups_requiring_two_factor_authentication
+    all_expanded_groups.where(require_two_factor_authentication: true)
+  end
+
   def nested_groups_projects
     Project.joins(:namespace).where('namespaces.parent_id IS NOT NULL').
       member_descendants(id)
@@ -955,6 +963,15 @@ class User < ActiveRecord::Base
     self.admin = (new_level == 'admin')
   end
 
+  def update_two_factor_requirement
+    periods = expanded_groups_requiring_two_factor_authentication.pluck(:two_factor_grace_period)
+
+    self.require_two_factor_authentication_from_group = periods.any?
+    self.two_factor_grace_period = periods.min || User.column_defaults['two_factor_grace_period']
+
+    save
+  end
+
   protected
 
   # override, from Devise::Validatable
diff --git a/app/views/admin/groups/_form.html.haml b/app/views/admin/groups/_form.html.haml
index 589f4557b52..d9f05003904 100644
--- a/app/views/admin/groups/_form.html.haml
+++ b/app/views/admin/groups/_form.html.haml
@@ -13,7 +13,7 @@
     .col-sm-offset-2.col-sm-10
       = render 'shared/allow_request_access', form: f
 
-  = render 'groups/group_lfs_settings', f: f
+  = render 'groups/group_admin_settings', f: f
 
   - if @group.new_record?
     .form-group
diff --git a/app/views/groups/_group_admin_settings.html.haml b/app/views/groups/_group_admin_settings.html.haml
new file mode 100644
index 00000000000..2ace1e2dd1e
--- /dev/null
+++ b/app/views/groups/_group_admin_settings.html.haml
@@ -0,0 +1,28 @@
+- if current_user.admin?
+  .form-group
+    = f.label :lfs_enabled, 'Large File Storage', class: 'control-label'
+    .col-sm-10
+      .checkbox
+        = f.label :lfs_enabled do
+          = f.check_box :lfs_enabled, checked: @group.lfs_enabled?
+          %strong
+            Allow projects within this group to use Git LFS
+            = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs')
+          %br/
+          %span.descr This setting can be overridden in each project.
+
+- if can? current_user, :admin_group, @group
+  .form-group
+    = f.label :require_two_factor_authentication, 'Two-factor authentication', class: 'control-label col-sm-2'
+    .col-sm-10
+      .checkbox
+        = f.label :require_two_factor_authentication do
+          = f.check_box :require_two_factor_authentication
+          %strong
+            Require all users in this group to setup Two-factor authentication
+            = link_to icon('question-circle'), help_page_path('security/two_factor_authentication', anchor: 'enforcing-2fa-for-all-users-in-a-group')
+  .form-group
+    .col-sm-offset-2.col-sm-10
+      .checkbox
+        = f.text_field :two_factor_grace_period, class: 'form-control'
+        .help-block Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication
diff --git a/app/views/groups/_group_lfs_settings.html.haml b/app/views/groups/_group_lfs_settings.html.haml
deleted file mode 100644
index 3c622ca5c3c..00000000000
--- a/app/views/groups/_group_lfs_settings.html.haml
+++ /dev/null
@@ -1,11 +0,0 @@
-- if current_user.admin?
-  .form-group
-    .col-sm-offset-2.col-sm-10
-      .checkbox
-        = f.label :lfs_enabled do
-          = f.check_box :lfs_enabled, checked: @group.lfs_enabled?
-          %strong
-            Allow projects within this group to use Git LFS
-            = link_to icon('question-circle'), help_page_path('workflow/lfs/manage_large_binaries_with_git_lfs')
-          %br/
-          %span.descr This setting can be overridden in each project.
diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml
index 80a77dab97f..00ff40224ba 100644
--- a/app/views/groups/edit.html.haml
+++ b/app/views/groups/edit.html.haml
@@ -27,7 +27,7 @@
         .col-sm-offset-2.col-sm-10
           = render 'shared/allow_request_access', form: f
 
-      = render 'group_lfs_settings', f: f
+      = render 'group_admin_settings', f: f
 
       .form-group
         %hr