allow requiring MFA per-auth-provider

closes FOO-1520

test plan:
 * go to auth providers page; notice that there are no MFA settings
 * set MFA to optional on the account
 * there should now be a checkbox on the auth providers page for each
   provider to make it required
 * make it required for canvas auth
 * ensure your login is explicitly associated with canvas auth
 * log in to canvas again via canvas auth
 * it should make you set up MFA

Change-Id: Ied6068ad282bde831964259bdd39721bf81c87fe
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/264543
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Simon Williams <simon@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>

app/controllers/authentication_providers_controller.rb

@@ -112,6 +112,10 @@
 #         },
 #         "federated_attributes": {
 #           "$ref": "FederatedAttributesConfig"
+#         },
+#         "mfa_required": {
+#           "description": "If multi-factor authentication is required when logging in with this authentication provider. The account must not have MFA disabled.",
+#           "type": "boolean"
 #         }
 #       }
 #     }
@@ -252,9 +256,9 @@ class AuthenticationProvidersController < ApplicationController
   # auth_type; unrecognized parameters are discarded. Provider specifications
   # not specifying a valid auth_type are ignored.
   #
-  # You can set the 'position' for any configuration. The config in the 1st position
-  # is considered the default. You can set 'jit_provisioning' for any configuration
-  # besides Canvas.
+  # You can set the 'position' for any provider. The config in the 1st position
+  # is considered the default. You can set 'jit_provisioning' for any provider
+  # besides Canvas. You can set 'mfa_required' for any provider.
   #
   # For Apple, the additional recognized parameters are:
   #
@@ -669,7 +673,9 @@ class AuthenticationProvidersController < ApplicationController
     end
 
     position = aac_data.delete(:position)
-    data = filter_data(aac_data)
+    # this may seem odd, but we need to ensure that account is set before
+    # mfa_required might be set, and ruby maintains insertion order in hashes
+    data = { account: @account }.merge(filter_data(aac_data))
     deselect_parent_registration(data)
     account_config = @account.authentication_providers.build(data)
     if account_config.class.singleton? && @account.authentication_providers.active.where(auth_type: account_config.auth_type).exists?

app/models/authentication_provider.rb

@@ -131,7 +131,7 @@ class AuthenticationProvider < ActiveRecord::Base
   end
 
   def self.recognized_params
-    [].freeze
+    [:mfa_required].freeze
   end
 
   def self.site_admin_params
@@ -204,6 +204,19 @@ class AuthenticationProvider < ActiveRecord::Base
     settings['federated_attributes'] ||= {}
   end
 
+  def mfa_required?
+    return false if account.mfa_settings == :disabled
+    return true if account.mfa_settings == :required
+
+    !!settings['mfa_required']
+  end
+  alias mfa_required mfa_required?
+
+  def mfa_required=(value)
+    value = false if account.mfa_settings == :disabled
+    settings['mfa_required'] = ::Canvas::Plugin.value_to_boolean(value)
+  end
+
   def federated_attributes_for_api
     if jit_provisioning?
       federated_attributes
@@ -211,6 +224,7 @@ class AuthenticationProvider < ActiveRecord::Base
       result = {}
       federated_attributes.each do |(canvas_attribute_name, provider_attribute_config)|
         next if provider_attribute_config['provisioning_only']
+
         result[canvas_attribute_name] = provider_attribute_config['attribute']
       end
       result

app/models/authentication_provider/canvas.rb

@@ -28,7 +28,7 @@ class AuthenticationProvider::Canvas < AuthenticationProvider
   end
 
   def self.recognized_params
-    [ :self_registration, :enable_captcha ].freeze
+    super + [ :self_registration, :enable_captcha ].freeze
   end
 
   def self.login_button?

app/models/authentication_provider/cas.rb

@@ -27,7 +27,7 @@ class AuthenticationProvider::CAS < AuthenticationProvider::Delegated
   end
 
   def self.recognized_params
-    [ :auth_base, :log_in_url, :jit_provisioning ].freeze
+    super + [ :auth_base, :log_in_url, :jit_provisioning ].freeze
   end
 
   def self.deprecated_params

app/models/authentication_provider/clever.rb

@@ -28,7 +28,7 @@ class AuthenticationProvider::Clever < AuthenticationProvider::Oauth2
   end
 
   def self.recognized_params
-    [ :login_attribute, :district_id, :jit_provisioning ].freeze
+    super + [ :login_attribute, :district_id, :jit_provisioning ].freeze
   end
 
   def self.login_attributes

app/models/authentication_provider/facebook.rb

@@ -38,7 +38,7 @@ class AuthenticationProvider::Facebook < AuthenticationProvider::Oauth2
   end
 
   def self.recognized_params
-    [ :login_attribute, :jit_provisioning ].freeze
+    super + [ :login_attribute, :jit_provisioning ].freeze
   end
 
   def self.login_attributes

app/models/authentication_provider/git_hub.rb

@@ -28,7 +28,7 @@ class AuthenticationProvider::GitHub < AuthenticationProvider::Oauth2
   end
 
   def self.recognized_params
-    [ :login_attribute, :jit_provisioning ].freeze
+    super + [ :login_attribute, :jit_provisioning ].freeze
   end
 
   def self.login_attributes

app/models/authentication_provider/google.rb

@@ -28,7 +28,7 @@ class AuthenticationProvider::Google < AuthenticationProvider::OpenIDConnect
   end
 
   def self.recognized_params
-    [ :login_attribute, :jit_provisioning, :hosted_domain ].freeze
+    super + [ :login_attribute, :jit_provisioning, :hosted_domain ].freeze
   end
 
   # Rename db field

app/models/authentication_provider/ldap.rb

@@ -27,9 +27,10 @@ class AuthenticationProvider::LDAP < AuthenticationProvider
   before_save :clear_last_timeout_failure
 
   def self.recognized_params
-    [ :auth_host, :auth_port, :auth_over_tls, :auth_base,
-      :auth_filter, :auth_username, :auth_password,
-      :identifier_format, :jit_provisioning ].freeze
+    super + 
+      [ :auth_host, :auth_port, :auth_over_tls, :auth_base,
+        :auth_filter, :auth_username, :auth_password,
+        :identifier_format, :jit_provisioning ].freeze
   end
 
   SENSITIVE_PARAMS = [ :auth_password ].freeze

app/models/authentication_provider/linked_in.rb

@@ -28,7 +28,7 @@ class AuthenticationProvider::LinkedIn < AuthenticationProvider::Oauth2
   end
 
   def self.recognized_params
-    [ :login_attribute, :jit_provisioning ].freeze
+    super + [ :login_attribute, :jit_provisioning ].freeze
   end
 
   def self.login_attributes

app/models/authentication_provider/microsoft.rb

@@ -50,7 +50,7 @@ class AuthenticationProvider::Microsoft < AuthenticationProvider::OpenIDConnect
   end
 
   def self.recognized_params
-    [:tenant, :login_attribute, :jit_provisioning].freeze
+    super + [:tenant, :login_attribute, :jit_provisioning].freeze
   end
 
   def self.login_attributes

app/models/authentication_provider/open_id_connect.rb

@@ -30,15 +30,16 @@ class AuthenticationProvider::OpenIDConnect < AuthenticationProvider::Oauth2
   end
 
   def self.recognized_params
-    [ :client_id,
-      :client_secret,
-      :authorize_url,
-      :token_url,
-      :scope,
-      :login_attribute,
-      :end_session_endpoint,
-      :userinfo_endpoint,
-      :jit_provisioning ].freeze
+    super + 
+      [ :client_id,
+        :client_secret,
+        :authorize_url,
+        :token_url,
+        :scope,
+        :login_attribute,
+        :end_session_endpoint,
+        :userinfo_endpoint,
+        :jit_provisioning ].freeze
   end
 
   def self.recognized_federated_attributes

app/models/authentication_provider/saml.rb

@@ -30,7 +30,7 @@ class AuthenticationProvider::SAML < AuthenticationProvider::Delegated
   end
 
   def self.recognized_params
-    [
+    super + [
       :log_in_url,
       :log_out_url,
       :requested_authn_context,

app/models/authentication_provider/twitter.rb

@@ -24,7 +24,7 @@ class AuthenticationProvider::Twitter < AuthenticationProvider::Oauth
   plugin_settings :consumer_key, consumer_secret: :consumer_secret_dec
 
   def self.recognized_params
-    [ :login_attribute, :jit_provisioning ].freeze
+    super + [ :login_attribute, :jit_provisioning ].freeze
   end
 
   def self.login_attributes

app/models/user.rb

@@ -2820,29 +2820,34 @@ class User < ActiveRecord::Base
       return :required if mfa_settings == :required ||
           mfa_settings == :required_for_admins && !pseudonym_hint.account.cached_all_account_users_for(self).empty?
     end
+    return :required if pseudonym_hint&.authentication_provider&.mfa_required?
 
-    result = self.pseudonyms.shard(self).preload(:account).map(&:account).uniq.map do |account|
+    pseudonyms = self.pseudonyms.shard(self).preload(:account, authentication_provider: :account)
+    return :required if pseudonyms.any? { |p| p.authentication_provider&.mfa_required? }
+
+    result = pseudonyms.map(&:account).uniq.map do |account|
       case account.mfa_settings
-        when :disabled
-          0
-        when :optional
+      when :disabled
+        0
+      when :optional
+        1
+      when :required_for_admins
+        # if pseudonym_hint is given, and we got to here, we don't need
+        # to redo the expensive all_account_users_for check
+        if (pseudonym_hint && pseudonym_hint.account == account) ||
+            account.cached_all_account_users_for(self).empty?
           1
-        when :required_for_admins
-          # if pseudonym_hint is given, and we got to here, we don't need
-          # to redo the expensive all_account_users_for check
-          if (pseudonym_hint && pseudonym_hint.account == account) ||
-              account.cached_all_account_users_for(self).empty?
-            1
-          else
-            # short circuit the entire method
-            return :required
-          end
-        when :required
+        else
           # short circuit the entire method
           return :required
+        end
+      when :required
+        # short circuit the entire method
+        return :required
       end
     end.max
     return :disabled if result.nil?
+
     [ :disabled, :optional ][result]
   end
 

app/views/authentication_providers/_aac_settings.html.erb

@@ -50,6 +50,13 @@
           <%= render partial: "debugging", locals: { provider: aac } %>
         <% end %>
 
+        <% if aac.account.mfa_settings != :disabled && aac.account.mfa_settings != :required %>
+          <div class="ic-Form-control">
+            <%= f.label :mfa_required, t('MFA Required'), class: 'ic-Label', for: "mfa_required_#{presenter.id_suffix(aac)}" %>
+            <%= f.check_box :mfa_required, class: 'mfa_required_checkbox', id: "mfa_required_#{presenter.id_suffix(aac)}" %>
+          </div>
+        <% end %>
+
         <% if presenter.configs.length > 1 %>
           <div class="ic-Form-control">
             <%= f.label(:position,

spec/apis/v1/authentication_providers_api_spec.rb

@@ -93,6 +93,22 @@ describe "AuthenticationProviders API", type: :request do
       expect(aac.position).to eq 1
     end
 
+    it "can create an initially mfa-required provider" do
+      @account.settings[:mfa_settings] = :optional
+      @account.save!
+      @saml_hash[:mfa_required] = true
+      call_create(@saml_hash)
+      ap = @account.authentication_providers.first
+      expect(ap).to be_mfa_required
+    end
+
+    it "ignores mfa_required if the account doesn't have it enabled" do
+      @saml_hash[:mfa_required] = true
+      call_create(@saml_hash)
+      ap = @account.authentication_providers.first
+      expect(ap).not_to be_mfa_required
+    end
+
     it "should work with rails form style params" do
       call_create({:authentication_provider => @saml_hash})
       aac = @account.authentication_providers.first
@@ -259,6 +275,7 @@ describe "AuthenticationProviders API", type: :request do
       @saml_hash['metadata_uri'] = nil
       @saml_hash['sig_alg'] = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"
       @saml_hash['strip_domain_from_login_attribute'] = false
+      @saml_hash['mfa_required'] = false
       expect(json).to eq @saml_hash
     end
 
@@ -273,6 +290,7 @@ describe "AuthenticationProviders API", type: :request do
       @ldap_hash['auth_over_tls'] = 'start_tls'
       @ldap_hash['identifier_format'] = nil
       @ldap_hash['position'] = 1
+      @ldap_hash['mfa_required'] = false
       expect(json).to eq @ldap_hash
     end
 
@@ -285,6 +303,7 @@ describe "AuthenticationProviders API", type: :request do
       @cas_hash['position'] = 1
       @cas_hash['unknown_user_url'] = nil
       @cas_hash['federated_attributes'] = {}
+      @cas_hash['mfa_required'] = false
       expect(json).to eq @cas_hash
     end
 
@@ -392,6 +411,19 @@ describe "AuthenticationProviders API", type: :request do
       course_with_student(:course => @course)
       call_update(0, {}, 401)
     end
+
+    it "can disable MFA" do
+      @account.settings[:mfa_settings] = :optional
+      @account.save!
+      aac = @account.authentication_providers.new(@cas_hash)
+      aac.mfa_required = true
+      aac.save!
+      @cas_hash['mfa_required'] = '0'
+      call_update(aac.id, @cas_hash)
+
+      aac.reload
+      expect(aac).not_to be_mfa_required
+    end
   end
 
   context "/destroy" do

spec/models/authentication_provider/plugin_settings_spec.rb

@@ -54,14 +54,14 @@ describe AuthenticationProvider::PluginSettings do
     context 'with plugin config' do
       it 'returns nothing' do
         allow(plugin).to receive(:enabled?).and_return(true)
-        expect(klass.recognized_params).to eq []
+        expect(klass.recognized_params).to eq [:mfa_required]
       end
     end
 
     context 'without plugin config' do
       it 'returns plugin params' do
         allow(plugin).to receive(:enabled?).and_return(false)
-        expect(klass.recognized_params).to eq [:auth_host, :noninherited_method]
+        expect(klass.recognized_params).to eq [:auth_host, :noninherited_method, :mfa_required]
       end
     end
   end

spec/models/user_spec.rb

@@ -2299,7 +2299,19 @@ describe User do
       p = user.pseudonyms.create!(:account => account, :unique_id => 'user')
       account.account_users.create!(user: user)
 
-      expect(user).to receive(:pseudonyms).never
+      expect(user).not_to receive(:pseudonyms)
+      expect(user.mfa_settings(pseudonym_hint: p)).to eq :required
+    end
+
+    it "is required for an auth provider that has it required" do
+      account = Account.create(settings: { mfa_settings: :optional })
+      ap = account.canvas_authentication_provider
+      ap.update!(mfa_required: true)
+      p = user.pseudonyms.create!(account: account, unique_id: 'user', authentication_provider: ap)
+
+      expect(user.mfa_settings).to eq :required
+
+      expect(user).not_to receive(:pseudonyms)
       expect(user.mfa_settings(pseudonym_hint: p)).to eq :required
     end
   end