Merge branch '65742-make-be_url-stricter' into 'master'

Add `be_http_url` matcher

Closes #65742

See merge request gitlab-org/gitlab-ce!32054

3 files changed, 42 insertions, 11 deletions

diff --git a/app/services/chat_names/authorize_user_service.rb b/app/services/chat_names/authorize_user_service.rb
index 78b53cb3637..f7780488923 100644
--- a/app/services/chat_names/authorize_user_service.rb
+++ b/app/services/chat_names/authorize_user_service.rb
@@ -24,16 +24,16 @@ module ChatNames
     end
 
     def chat_name_token
-      Gitlab::ChatNameToken.new
+      @chat_name_token ||= Gitlab::ChatNameToken.new
     end
 
     def chat_name_params
       {
-        service_id: @service.id,
-        team_id: @params[:team_id],
+        service_id:  @service.id,
+        team_id:     @params[:team_id],
         team_domain: @params[:team_domain],
-        chat_id: @params[:user_id],
-        chat_name: @params[:user_name]
+        chat_id:     @params[:user_id],
+        chat_name:   @params[:user_name]
       }
     end
   end
diff --git a/spec/services/chat_names/authorize_user_service_spec.rb b/spec/services/chat_names/authorize_user_service_spec.rb
index 41cbac4e8e9..7f32948daad 100644
--- a/spec/services/chat_names/authorize_user_service_spec.rb
+++ b/spec/services/chat_names/authorize_user_service_spec.rb
@@ -4,23 +4,36 @@ require 'spec_helper'
 
 describe ChatNames::AuthorizeUserService do
   describe '#execute' do
-    let(:service) { create(:service) }
+    subject { described_class.new(service, params) }
 
-    subject { described_class.new(service, params).execute }
+    let(:result) { subject.execute }
+    let(:service) { create(:service) }
 
     context 'when all parameters are valid' do
       let(:params) { { team_id: 'T0001', team_domain: 'myteam', user_id: 'U0001', user_name: 'user' } }
 
+      it 'produces a valid HTTP URL' do
+        expect(result).to be_http_url
+      end
+
       it 'requests a new token' do
-        is_expected.to be_url
+        expect(subject).to receive(:request_token).once.and_call_original
+
+        subject.execute
       end
     end
 
     context 'when there are missing parameters' do
       let(:params) { {} }
 
+      it 'does not produce a URL' do
+        expect(result).to be_nil
+      end
+
       it 'does not request a new token' do
-        is_expected.to be_nil
+        expect(subject).not_to receive(:request_token)
+
+        subject.execute
       end
     end
   end
diff --git a/spec/support/matchers/be_url.rb b/spec/support/matchers/be_url.rb
index 69171f53891..388c1b384c7 100644
--- a/spec/support/matchers/be_url.rb
+++ b/spec/support/matchers/be_url.rb
@@ -1,11 +1,29 @@
 # frozen_string_literal: true
 
-RSpec::Matchers.define :be_url do |_|
+# Assert that this value is a valid URL of at least one type.
+#
+# By default, this checks that the URL is either a HTTP or HTTPS URI,
+# but you can check other URI schemes by passing the type, eg:
+#
+# ```
+# expect(value).to be_url(URI::FTP)
+# ```
+#
+# Pass an empty array of types if you want to match any URI scheme (be
+# aware that this might not do what you think it does! `foo` is a valid
+# URI, for instance).
+RSpec::Matchers.define :be_url do |types = [URI::HTTP, URI::HTTPS]|
   match do |actual|
-    URI.parse(actual) rescue false
+    next false unless actual.present?
+
+    uri = URI.parse(actual)
+    Array.wrap(types).any? { |t| uri.is_a?(t) }
+  rescue URI::InvalidURIError
+    false
   end
 end
 
 # looks better when used like:
 #   expect(thing).to receive(:method).with(a_valid_url)
 RSpec::Matchers.alias_matcher :a_valid_url, :be_url
+RSpec::Matchers.alias_matcher :be_http_url, :be_url