From cb1cd55169384d46bc79942c733c7beb3a78bae9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Oliver=20G=C3=BCnther?= <mail@oliverguenther.de>
Date: Thu, 4 Jan 2018 08:23:30 +0100
Subject: [PATCH] Add test for merging config and settings

---
 .../two_factor_authentication/engine.rb       |  2 +-
 .../token_strategy_manager.rb                 | 17 +++++--
 spec/lib/token_strategy_manager_spec.rb       | 50 ++++++++++++++++++-
 3 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/lib/open_project/two_factor_authentication/engine.rb b/lib/open_project/two_factor_authentication/engine.rb
index cdb61e42ae..b72c0a0e80 100644
--- a/lib/open_project/two_factor_authentication/engine.rb
+++ b/lib/open_project/two_factor_authentication/engine.rb
@@ -11,7 +11,7 @@ module OpenProject::TwoFactorAuthentication
              settings: {
                  default: {
                      enforced: false,
-                     active_strategies: [:totp]
+                     active_strategies: []
                  },
                  partial: 'settings/openproject_two_factor_authentication'
              },
diff --git a/lib/open_project/two_factor_authentication/token_strategy_manager.rb b/lib/open_project/two_factor_authentication/token_strategy_manager.rb
index b221f18cba..4ede4b42b1 100644
--- a/lib/open_project/two_factor_authentication/token_strategy_manager.rb
+++ b/lib/open_project/two_factor_authentication/token_strategy_manager.rb
@@ -55,7 +55,6 @@ module OpenProject::TwoFactorAuthentication
         enforced? && !enabled?
       end
 
-
       ##
       # Fetch all active strategies
       def active_strategies
@@ -87,13 +86,23 @@ module OpenProject::TwoFactorAuthentication
         config = OpenProject::Configuration['2fa'] || {}
         settings = Setting.plugin_openproject_two_factor_authentication || {}
 
-        # Allow enforcing from settings if not true in configuration
-        config[:enforced] ||= settings[:enforced]
-        config[:active_strategies] ||= settings[:active_strategies]
+        merge_with_settings! config, settings
 
         config
       end
 
+      def merge_with_settings!(config, settings)
+        # Allow enforcing from settings if not true in configuration
+        unless config[:enforced]
+          config[:enforced] = settings[:enforced]
+        end
+
+        predefined_strategies = config.fetch(:active_strategies, [])
+        additional_strategies = settings.fetch(:active_strategies, [])
+
+        config[:active_strategies] = predefined_strategies | additional_strategies
+      end
+
       def lookup_active_strategy(klazz)
         ::OpenProject::TwoFactorAuthentication::TokenStrategy.const_get klazz.to_s.camelize
       rescue NameError => e
diff --git a/spec/lib/token_strategy_manager_spec.rb b/spec/lib/token_strategy_manager_spec.rb
index ab6167cb02..a13c33b0ff 100644
--- a/spec/lib/token_strategy_manager_spec.rb
+++ b/spec/lib/token_strategy_manager_spec.rb
@@ -1,7 +1,8 @@
-require 'spec_helper'
+require_relative '../spec_helper'
 
 describe ::OpenProject::TwoFactorAuthentication::TokenStrategyManager do
   let(:dev_strategy) { ::OpenProject::TwoFactorAuthentication::TokenStrategy::Developer }
+  let(:totp_strategy) { ::OpenProject::TwoFactorAuthentication::TokenStrategy::Totp }
   let(:configuration) do
     {
       active_strategies: active_strategies,
@@ -70,6 +71,53 @@ describe ::OpenProject::TwoFactorAuthentication::TokenStrategyManager do
       end
     end
 
+    describe 'with additional settings given' do
+      let(:active_strategies) { [:developer] }
+      let(:enforced) { false }
+
+      before do
+        allow(Setting).to receive(:plugin_openproject_two_factor_authentication).and_return(settings)
+      end
+
+      context 'when nothing given' do
+        let(:settings) { nil }
+
+        it 'uses the configuration' do
+          expect(described_class.active_strategies).to eq([dev_strategy])
+          expect(described_class).not_to be_enforced
+        end
+      end
+
+      context 'when additional strategy given' do
+        let(:settings) { { active_strategies: [:totp] } }
+
+        it 'merges configuration and settings' do
+          expect(described_class.active_strategies).to eq([dev_strategy, totp_strategy])
+          expect(described_class).not_to be_enforced
+        end
+      end
+
+      context 'when enforced set' do
+        context 'when true and config is false' do
+          let(:enforced) { false }
+          let(:settings) { { enforced: true } }
+
+          it 'does override the configuration' do
+            expect(described_class).to be_enforced
+          end
+        end
+
+        context 'when false and config is true' do
+          let(:enforced) { true }
+          let(:settings) { { enforced: false } }
+
+          it 'does not override the configuration' do
+            expect(described_class).to be_enforced
+          end
+        end
+      end
+    end
+
     describe '#validate_active_strategies!' do
       subject { described_class.validate_active_strategies! }
       context 'when no strategy is set' do