From 51fcb290e70ae5e75d040500bd544b446f6cc2a9 Mon Sep 17 00:00:00 2001 From: Antti Hukkanen Date: Tue, 13 Feb 2024 13:21:46 +0200 Subject: [PATCH] Update rubocop rules and fix lint violations --- .rubocop.yml | 13 ++--- .simplecov | 8 +-- lib/omniauth-suomifi/ruby_saml_extensions.rb | 6 ++- .../test/certificate_generator.rb | 4 +- lib/omniauth-suomifi/test/xml_encryptor.rb | 4 +- lib/omniauth/strategies/suomifi.rb | 15 +++--- omniauth-suomifi.gemspec | 3 +- spec/omniauth/strategies/suomifi_spec.rb | 50 +++++++++---------- spec/spec_helper.rb | 2 +- 9 files changed, 54 insertions(+), 51 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index d1ac164..f5fa143 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,19 +1,24 @@ AllCops: TargetRubyVersion: 2.6 + NewCops: enable Layout/AccessModifierIndentation: EnforcedStyle: outdent -Layout/AlignHash: +Layout/HashAlignment: Enabled: false Layout/DotPosition: EnforcedStyle: trailing +Layout/LineLength: + AllowURI: true + Enabled: false + Layout/SpaceInsideHashLiteralBraces: EnforcedStyle: no_space -Lint/HandleExceptions: +Lint/SuppressedException: Enabled: false Metrics/BlockLength: @@ -25,10 +30,6 @@ Metrics/BlockNesting: Metrics/ClassLength: Enabled: false -Metrics/LineLength: - AllowURI: true - Enabled: false - Metrics/MethodLength: CountComments: false Max: 15 diff --git a/.simplecov b/.simplecov index 10bb120..d9da758 100644 --- a/.simplecov +++ b/.simplecov @@ -1,13 +1,13 @@ # frozen_string_literal: true SimpleCov.start do - root ENV["ENGINE_ROOT"] + root ENV.fetch('ENGINE_ROOT', nil) - add_filter "lib/omniauth-suomifi/version.rb" - add_filter "/spec" + add_filter 'lib/omniauth-suomifi/version.rb' + add_filter '/spec' end -SimpleCov.command_name ENV["COMMAND_NAME"] || File.basename(Dir.pwd) +SimpleCov.command_name ENV['COMMAND_NAME'] || File.basename(Dir.pwd) SimpleCov.merge_timeout 1800 diff --git a/lib/omniauth-suomifi/ruby_saml_extensions.rb b/lib/omniauth-suomifi/ruby_saml_extensions.rb index 78b488b..f9b49ff 100644 --- a/lib/omniauth-suomifi/ruby_saml_extensions.rb +++ b/lib/omniauth-suomifi/ruby_saml_extensions.rb @@ -28,6 +28,7 @@ # @param symmetric_key [String] The symetric key used to encrypt the text # @param algorithm [String] The encrypted algorithm # @return [String] The deciphered text + # rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength def self.retrieve_plaintext(cipher_text, symmetric_key, algorithm) case algorithm when 'http://www.w3.org/2001/04/xmlenc#tripledes-cbc' then cipher = OpenSSL::Cipher.new('DES-EDE3-CBC').decrypt @@ -43,7 +44,7 @@ def self.retrieve_plaintext(cipher_text, symmetric_key, algorithm) if cipher iv_len = cipher.iv_len - data = cipher_text[iv_len..-1] + data = cipher_text[iv_len..] cipher.padding = 0 cipher.key = symmetric_key cipher.iv = cipher_text[0..iv_len - 1] @@ -58,7 +59,7 @@ def self.retrieve_plaintext(cipher_text, symmetric_key, algorithm) auth_cipher.key = symmetric_key auth_cipher.iv = cipher_text[0..iv_len - 1] auth_cipher.auth_data = '' - auth_cipher.auth_tag = cipher_text[text_len - tag_len..-1] + auth_cipher.auth_tag = cipher_text[text_len - tag_len..] assertion_plaintext = auth_cipher.update(data) assertion_plaintext << auth_cipher.final elsif rsa @@ -69,4 +70,5 @@ def self.retrieve_plaintext(cipher_text, symmetric_key, algorithm) cipher_text end end + # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity, Metrics/MethodLength end diff --git a/lib/omniauth-suomifi/test/certificate_generator.rb b/lib/omniauth-suomifi/test/certificate_generator.rb index c7b4e5c..5e4c161 100644 --- a/lib/omniauth-suomifi/test/certificate_generator.rb +++ b/lib/omniauth-suomifi/test/certificate_generator.rb @@ -17,14 +17,14 @@ def certificate cert = OpenSSL::X509::Certificate.new cert.subject = cert.issuer = OpenSSL::X509::Name.parse(subject) cert.not_before = Time.now - cert.not_after = Time.now + 365 * 24 * 60 * 60 + cert.not_after = Time.now + (365 * 24 * 60 * 60) cert.public_key = public_key cert.serial = 0x0 cert.version = 2 inject_certificate_extensions(cert) - cert.sign(private_key, OpenSSL::Digest::SHA1.new) + cert.sign(private_key, OpenSSL::Digest.new('SHA1')) cert end diff --git a/lib/omniauth-suomifi/test/xml_encryptor.rb b/lib/omniauth-suomifi/test/xml_encryptor.rb index 159bb2e..3f422ab 100644 --- a/lib/omniauth-suomifi/test/xml_encryptor.rb +++ b/lib/omniauth-suomifi/test/xml_encryptor.rb @@ -33,7 +33,7 @@ def encrypt(raw_xml) end def self.encrypted_xml(raw_xml_file, cert, sign_cert, sign_key) - raw_xml = IO.read(raw_xml_file) + raw_xml = File.read(raw_xml_file) encrypted_xml_from_string(raw_xml, cert, sign_cert, sign_key) end @@ -53,7 +53,7 @@ def encryption_template template_path = Utility.template_filepath( 'encrypted_data_template.xml' ) - template_io = IO.read(template_path) + template_io = File.read(template_path) Nokogiri::XML::Document.parse(template_io).root end diff --git a/lib/omniauth/strategies/suomifi.rb b/lib/omniauth/strategies/suomifi.rb index 1a9ff46..b06e3e2 100644 --- a/lib/omniauth/strategies/suomifi.rb +++ b/lib/omniauth/strategies/suomifi.rb @@ -458,7 +458,7 @@ class Suomifi < SAML eidas_id = find_attribute_by( ['http://eidas.europa.eu/attributes/naturalperson/PersonIdentifier'] ) - hash_salt = begin + hash_salt = if options.uid_salt options.uid_salt elsif defined?(::Rails) && ::Rails.application @@ -466,14 +466,13 @@ class Suomifi < SAML else '' end - end if !electronic_id.nil? - 'FINUID:' + electronic_id + "FINUID:#{electronic_id}" elsif !national_id.nil? - 'FIHETU:' + Digest::MD5.hexdigest("FI:#{national_id}:#{hash_salt}") + "FIHETU:#{Digest::MD5.hexdigest("FI:#{national_id}:#{hash_salt}")}" elsif !eidas_id.nil? - 'EIDASPID:' + Digest::MD5.hexdigest("EIDAS:#{eidas_id}:#{hash_salt}") + "EIDASPID:#{Digest::MD5.hexdigest("EIDAS:#{eidas_id}:#{hash_salt}")}" else @name_id end @@ -491,6 +490,7 @@ class Suomifi < SAML attr_accessor :options attr_reader :suomifi_thread + # rubocop:disable Metrics/MethodLength def initialize(app, *args, &block) super @@ -520,6 +520,7 @@ def initialize(app, *args, &block) ) end end + # rubocop:enable Metrics/MethodLength # Override the request phase to be able to pass the locale parameter to # the redirect URL. Note that this needs to be the last parameter to @@ -590,7 +591,7 @@ def handle_logout_request(raw_request, settings) # currently open at the website. logout_request = OneLogin::RubySaml::SloLogoutrequest.new( raw_request, - { settings: settings, get_params: @request.params } + {settings: settings, get_params: @request.params} ) raise OmniAuth::Strategies::SAML::ValidationError.new('SAML failed to process LogoutRequest') unless logout_request.is_valid? @@ -665,6 +666,7 @@ def idp_metadata_url end end + # rubocop:disable Metrics/MethodLength def suomifi_options idp_metadata_parser = OneLogin::RubySaml::IdpMetadataParser.new @@ -705,6 +707,7 @@ def suomifi_options settings end + # rubocop:enable Metrics/MethodLength # This will return true if the VTJ search (population information system, # väestötietojärjestelmä) was successful and information about the person diff --git a/omniauth-suomifi.gemspec b/omniauth-suomifi.gemspec index 48e06b3..2036843 100644 --- a/omniauth-suomifi.gemspec +++ b/omniauth-suomifi.gemspec @@ -7,9 +7,10 @@ require 'omniauth-suomifi/version' Gem::Specification.new do |spec| spec.name = 'omniauth-suomifi' spec.version = OmniAuth::Suomifi::VERSION - spec.required_ruby_version = ">= 2.5" + spec.required_ruby_version = '>= 2.6' spec.authors = ['Antti Hukkanen'] spec.email = ['antti.hukkanen@mainiotech.fi'] + spec.metadata['rubygems_mfa_required'] = 'true' spec.summary = 'Provides a Suomi.fi strategy for OmniAuth.' spec.description = 'Suomi.fi e-Identification service integration for OmniAuth.' diff --git a/spec/omniauth/strategies/suomifi_spec.rb b/spec/omniauth/strategies/suomifi_spec.rb index 7123de4..00cefbd 100644 --- a/spec/omniauth/strategies/suomifi_spec.rb +++ b/spec/omniauth/strategies/suomifi_spec.rb @@ -29,10 +29,7 @@ let(:sp_entity_id) { 'https://www.service.fi/auth/suomifi/metadata' } let(:scope_of_data) { :medium_extensive } let(:strategy) { [OmniAuth::Strategies::Suomifi, saml_options] } - let(:thread) { double( - join: nil, - alive?: false - )} + let(:thread) { double(join: nil, alive?: false) } before do # Stub the metadata to return the locally stored metadata for easier @@ -143,8 +140,8 @@ attr_reader :temp_dir before do - File.open(certificate_file, 'w') { |f| f.write(certificate.to_pem) } - File.open(private_key_file, 'w') { |f| f.write(private_key.to_pem) } + File.write(certificate_file, certificate.to_pem) + File.write(private_key_file, private_key.to_pem) end it 'should read the certificate and private key from the files' do @@ -343,9 +340,16 @@ end raw_xml_file = support_filepath("#{xml}.xml") - xml_signed = begin - if !custom_saml_attributes.empty? - xml_io = IO.read(raw_xml_file) + xml_signed = + if custom_saml_attributes.empty? + OmniAuth::Suomifi::Test::Utility.encrypted_signed_xml( + raw_xml_file, + certificate: certificate, + sign_certificate: sign_certificate, + sign_private_key: sign_private_key + ) + else + xml_io = File.read(raw_xml_file) doc = Nokogiri::XML::Document.parse(xml_io) statements_node = doc.root.at_xpath( '//saml2:Assertion//saml2:AttributeStatement', @@ -387,15 +391,7 @@ sign_certificate: sign_certificate, sign_private_key: sign_private_key ) - else - OmniAuth::Suomifi::Test::Utility.encrypted_signed_xml( - raw_xml_file, - certificate: certificate, - sign_certificate: sign_certificate, - sign_private_key: sign_private_key - ) end - end saml_response = Base64.encode64(xml_signed) @@ -406,7 +402,7 @@ end after :each do - Object.send(:remove_const, :Rails) if defined?(::Rails) + Object.send(:remove_const, :Rails) if defined?(Rails) end it 'should set the info hash correctly' do @@ -513,7 +509,7 @@ # The HETU is already set in the sample XML it 'should set the uid to hashed HETU ID' do expect(auth_hash['uid']).to eq( - 'FIHETU:' + Digest::MD5.hexdigest("FI:210281-9988:#{uid_salt}") + "FIHETU:#{Digest::MD5.hexdigest("FI:210281-9988:#{uid_salt}")}" ) end @@ -523,7 +519,7 @@ it 'should set the uid to hashed eIDAS PID' do expect(auth_hash['uid']).to eq( - 'FIHETU:' + Digest::MD5.hexdigest("FI:210281-9988:#{rails_salt}") + "FIHETU:#{Digest::MD5.hexdigest("FI:210281-9988:#{rails_salt}")}" ) end end @@ -533,7 +529,7 @@ it 'should set the uid to hashed eIDAS PID' do expect(auth_hash['uid']).to eq( - 'FIHETU:' + Digest::MD5.hexdigest('FI:210281-9988:') + "FIHETU:#{Digest::MD5.hexdigest('FI:210281-9988:')}" ) end end @@ -555,7 +551,7 @@ it 'should set the uid to hashed eIDAS PID' do expect(auth_hash['uid']).to eq( - 'EIDASPID:' + Digest::MD5.hexdigest("EIDAS:28493196Z:#{uid_salt}") + "EIDASPID:#{Digest::MD5.hexdigest("EIDAS:28493196Z:#{uid_salt}")}" ) end @@ -565,7 +561,7 @@ it 'should set the uid to hashed eIDAS PID' do expect(auth_hash['uid']).to eq( - 'EIDASPID:' + Digest::MD5.hexdigest("EIDAS:28493196Z:#{rails_salt}") + "EIDASPID:#{Digest::MD5.hexdigest("EIDAS:28493196Z:#{rails_salt}")}" ) end end @@ -575,7 +571,7 @@ it 'should set the uid to hashed eIDAS PID' do expect(auth_hash['uid']).to eq( - 'EIDASPID:' + Digest::MD5.hexdigest('EIDAS:28493196Z:') + "EIDASPID:#{Digest::MD5.hexdigest('EIDAS:28493196Z:')}" ) end end @@ -734,9 +730,9 @@ it 'should call the application' do expect(last_response.body).to eq(app_response) expect(logout_request).to be_a(OneLogin::RubySaml::SloLogoutrequest) - expect(logout_response.scheme).to eq("https") - expect(logout_response.host).to eq("testi.apro.tunnistus.fi") - expect(logout_response.path).to eq("/idp/profile/SAML2/Redirect/SLO") + expect(logout_response.scheme).to eq('https') + expect(logout_response.host).to eq('testi.apro.tunnistus.fi') + expect(logout_response.path).to eq('/idp/profile/SAML2/Redirect/SLO') expect(actual_relay_state).to eq('/') end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 793eade..6e2d1b8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -26,7 +26,7 @@ def support_filepath(filename) end def support_file_io(filename) - IO.read(support_filepath(filename)) + File.read(support_filepath(filename)) end def base64_file(filename)