From 20575859b1bf431421427d52c4ac5a33cf662df6 Mon Sep 17 00:00:00 2001
From: Alexis Reigel <mail@koffeinfrei.org>
Date: Thu, 9 Mar 2017 20:38:13 +0100
Subject: [PATCH] check all groups for 2fa requirement

---
 .../enforces_two_factor_authentication.rb     |  2 +-
 app/models/concerns/routable.rb               |  6 +--
 app/models/user.rb                            | 10 ++++-
 spec/models/user_spec.rb                      | 41 +++++++++++++++++++
 4 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/app/controllers/concerns/enforces_two_factor_authentication.rb b/app/controllers/concerns/enforces_two_factor_authentication.rb
index 3e0c62172de..28ffe0ba4b1 100644
--- a/app/controllers/concerns/enforces_two_factor_authentication.rb
+++ b/app/controllers/concerns/enforces_two_factor_authentication.rb
@@ -29,7 +29,7 @@ module EnforcesTwoFactorAuthentication
       if current_application_settings.require_two_factor_authentication?
         global.call
       else
-        groups = current_user.groups.where(require_two_factor_authentication: true).reorder(name: :asc)
+        groups = current_user.expanded_groups_requiring_two_factor_authentication.reorder(name: :asc)
         group.call(groups)
       end
     end
diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb
index 9b8970a1f38..b907e421743 100644
--- a/app/models/concerns/routable.rb
+++ b/app/models/concerns/routable.rb
@@ -143,10 +143,8 @@ module Routable
 
       return none if paths.empty?
 
-      leaf_paths = paths.group_by(&:length).flat_map(&:last)
-
-      wheres = leaf_paths.map do |leaf_path|
-        "#{connection.quote(leaf_path)} LIKE CONCAT(routes.path, '%')"
+      wheres = paths.map do |path|
+        "#{connection.quote(path)} LIKE CONCAT(routes.path, '%')"
       end
 
       joins(:route).where(wheres.join(' OR '))
diff --git a/app/models/user.rb b/app/models/user.rb
index 564e99df77b..fb30cebfda4 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)
@@ -964,7 +972,7 @@ class User < ActiveRecord::Base
   end
 
   def update_two_factor_requirement
-    periods = groups.where(require_two_factor_authentication: true).pluck(:two_factor_grace_period)
+    periods = expanded_groups_requiring_two_factor_authentication.pluck(:two_factor_grace_period)
 
     self.require_two_factor_authentication = periods.any?
     self.two_factor_grace_period = periods.min || User.column_defaults['two_factor_grace_period']
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index b2f686a1819..ddd438bfc25 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1407,6 +1407,17 @@ describe User, models: true do
     it { expect(user.nested_groups).to eq([nested_group]) }
   end
 
+  describe '#all_expanded_groups' do
+    let!(:user) { create(:user) }
+    let!(:group) { create(:group) }
+    let!(:nested_group_1) { create(:group, parent: group) }
+    let!(:nested_group_2) { create(:group, parent: group) }
+
+    before { nested_group_1.add_owner(user) }
+
+    it { expect(user.all_expanded_groups).to match_array [group, nested_group_1] }
+  end
+
   describe '#nested_groups_projects' do
     let!(:user) { create(:user) }
     let!(:group) { create(:group) }
@@ -1545,6 +1556,36 @@ describe User, models: true do
       end
     end
 
+    context 'with 2FA requirement on nested parent group' do
+      let!(:group1) { create :group, require_two_factor_authentication: true }
+      let!(:group1a) { create :group, require_two_factor_authentication: false, parent: group1 }
+
+      before do
+        group1a.add_user(user, GroupMember::OWNER)
+
+        user.update_two_factor_requirement
+      end
+
+      it 'requires 2FA' do
+        expect(user.require_two_factor_authentication).to be true
+      end
+    end
+
+    context 'with 2FA requirement on nested child group' do
+      let!(:group1) { create :group, require_two_factor_authentication: false }
+      let!(:group1a) { create :group, require_two_factor_authentication: true, parent: group1 }
+
+      before do
+        group1.add_user(user, GroupMember::OWNER)
+
+        user.update_two_factor_requirement
+      end
+
+      it 'requires 2FA' do
+        expect(user.require_two_factor_authentication).to be true
+      end
+    end
+
     context 'without 2FA requirement on groups' do
       let(:group) { create :group }