improve logic re: overwriting requested identifier format from saml r…

…efresh also default to unspecified for new configs test plan: * set up a new SAML config against an ADFS server, specifying a metadata url * the identifer format should stay as unspecified * logins should work Change-Id: I9cdf106aa3a708984a1eb3985b2520210ee6a606 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/256225 Tested-by: Service Cloud Jenkins <[email protected]> Reviewed-by: Simon Williams <[email protected]> QA-Review: Cody Cutrer <[email protected]> Product-Review: Cody Cutrer <[email protected]>
ralacas · Jan 7, 2021 · dd5614f · dd5614f
1 parent 305f587
commit dd5614f
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 4 deletions.
diff --git a/app/models/authentication_provider/saml.rb b/app/models/authentication_provider/saml.rb
@@ -105,7 +105,10 @@ def self.debugging_keys
   before_validation :download_metadata
   after_initialize do |ap|
     # default to the most secure signature we support, but only for new objects
-    ap.sig_alg ||= 'RSA-SHA256' if ap.new_record?
+    if ap.new_record?
+      ap.sig_alg ||= 'RSA-SHA256' 
+      ap.identifier_format ||= SAML2::NameID::Format::UNSPECIFIED
+    end
   end
 
   def destroy
@@ -241,7 +244,15 @@ def populate_from_metadata(entity)
     self.log_in_url = idp.single_sign_on_services.find { |ep| ep.binding == SAML2::Bindings::HTTPRedirect::URN }.try(:location)
     self.log_out_url = idp.single_logout_services.find { |ep| ep.binding == SAML2::Bindings::HTTPRedirect::URN }.try(:location)
     self.certificate_fingerprint = idp.signing_keys.map(&:fingerprint).join(' ').presence || idp.keys.first&.fingerprint
-    self.identifier_format = (idp.name_id_formats & self.class.name_id_formats).first
+
+    recognized_formats = (idp.name_id_formats & self.class.name_id_formats)
+    if recognized_formats.length == 1
+      self.identifier_format = recognized_formats.first
+    elsif identifier_format != SAML2::NameID::Format::UNSPECIFIED &&
+      !recognized_formats.include?(identifier_format)
+      self.identifier_format = SAML2::NameID::Format::UNSPECIFIED
+    end
+
     self.settings[:signing_certificates] = idp.signing_keys.map(&:x509)
     case idp.want_authn_requests_signed?
     when true

diff --git a/spec/models/authentication_provider/saml_spec.rb b/spec/models/authentication_provider/saml_spec.rb
@@ -112,6 +112,45 @@
       saml.populate_from_metadata(entity)
       expect(saml.sig_alg).to eq SAML2::Bindings::HTTPRedirect::SigAlgs::RSA_SHA1
     end
+
+    context "identifier format" do
+      let(:saml) { AuthenticationProvider::SAML.new }
+      let(:entity) { SAML2::Entity.new }
+      let(:idp) { SAML2::IdentityProvider.new }
+
+      before { entity.roles << idp }
+
+      it "overwrites if metadata only has one" do
+        idp.name_id_formats << SAML2::NameID::Format::EMAIL_ADDRESS
+        expect(saml.identifier_format).to eq(SAML2::NameID::Format::UNSPECIFIED)
+        saml.populate_from_metadata(entity)
+        expect(saml.identifier_format).to eq(SAML2::NameID::Format::EMAIL_ADDRESS)
+      end
+
+      it "does not overwrite if there are multiple and we're set to unspecified" do
+        idp.name_id_formats << SAML2::NameID::Format::EMAIL_ADDRESS
+        idp.name_id_formats << SAML2::NameID::Format::TRANSIENT
+        saml.identifier_format = SAML2::NameID::Format::UNSPECIFIED
+        saml.populate_from_metadata(entity)
+        expect(saml.identifier_format).to eq(SAML2::NameID::Format::UNSPECIFIED)
+      end
+
+      it "sets to unspecified if there are multiple and we're set to something else" do
+        idp.name_id_formats << SAML2::NameID::Format::EMAIL_ADDRESS
+        idp.name_id_formats << SAML2::NameID::Format::TRANSIENT
+        saml.identifier_format = SAML2::NameID::Format::PERSISTENT
+        saml.populate_from_metadata(entity)
+        expect(saml.identifier_format).to eq(SAML2::NameID::Format::UNSPECIFIED)
+      end
+
+      it "does not overwrite if there are multiple and we're set to one of the valid ones" do
+        idp.name_id_formats << SAML2::NameID::Format::EMAIL_ADDRESS
+        idp.name_id_formats << SAML2::NameID::Format::TRANSIENT
+        saml.identifier_format = SAML2::NameID::Format::TRANSIENT
+        saml.populate_from_metadata(entity)
+        expect(saml.identifier_format).to eq(SAML2::NameID::Format::TRANSIENT)
+      end
+    end
   end
 
   describe '.resolve_saml_key_path' do

diff --git a/spec/selenium/admin/account_admin_auth_providers_spec.rb b/spec/selenium/admin/account_admin_auth_providers_spec.rb
@@ -142,7 +142,7 @@
         expect(config.log_out_url).to eq 'logout.example'
         expect(config.certificate_fingerprint).to eq 'abc123'
         expect(config.login_attribute).to eq 'NameID'
-        expect(config.identifier_format).to eq 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress'
+        expect(config.identifier_format).to eq 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified'
         expect(config.requested_authn_context).to eq nil
         expect(config.parent_registration).to be_falsey
       end
@@ -166,7 +166,7 @@
         expect(config.log_out_url).to eq ''
         expect(config.certificate_fingerprint).to eq ''
         expect(config.login_attribute).to eq 'NameID'
-        expect(config.identifier_format).to eq 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress'
+        expect(config.identifier_format).to eq 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified'
         expect(config.requested_authn_context).to eq nil
         expect(config.parent_registration).to be_falsey
       end