Merge branch 'handle-new-action-mask-urls' into 'master'

Handle new action and other missed actions

See merge request gitlab-org/gitlab!71183

app/helpers/routing/pseudonymization_helper.rb

@@ -2,58 +2,86 @@
 
 module Routing
   module PseudonymizationHelper
-    def masked_page_url
-      return unless Feature.enabled?(:mask_page_urls, type: :ops)
+    class MaskHelper
+      QUERY_PARAMS_TO_MASK = %w[
+        assignee_username
+        author_username
+      ].freeze
 
-      mask_params(Rails.application.routes.recognize_path(request.original_fullpath))
-    rescue ActionController::RoutingError, URI::InvalidURIError => e
-      Gitlab::ErrorTracking.track_exception(e, url: request.original_fullpath)
-      nil
-    end
+      def initialize(request_object, group, project)
+        @request = request_object
+        @group = group
+        @project = project
+      end
 
-    private
+      def mask_params
+        return default_root_url + @request.original_fullpath unless has_maskable_params?
 
-    def mask_params(request_params)
-      return if request_params[:action] == 'new'
+        masked_params = @request.path_parameters.to_h do |key, value|
+          case key
+          when :project_id
+            [key, "project#{@project&.id}"]
+          when :namespace_id
+            namespace = @group || @project&.namespace
+            [key, "namespace#{namespace&.id}"]
+          when :id
+            [key, mask_id(value)]
+          else
+            [key, value]
+          end
+        end
 
-      namespace_type = request_params[:controller].split('/')[1]
+        Gitlab::Routing.url_helpers.url_for(masked_params.merge(masked_query_params))
+      end
 
-      namespace_type.present? ? url_with_namespace_type(request_params, namespace_type) : url_without_namespace_type(request_params)
-    end
+      private
 
-    def url_without_namespace_type(request_params)
-      masked_url = "#{request.protocol}#{request.host_with_port}"
+      def mask_id(value)
+        if @request.path_parameters[:controller] == 'projects/blob'
+          ':repository_path'
+        elsif @request.path_parameters[:controller] == 'projects'
+          "project#{@project&.id}"
+        elsif @request.path_parameters[:controller] == 'groups'
+          "namespace#{@group&.id}"
+        else
+          value
+        end
+      end
 
-      masked_url += case request_params[:controller]
-                    when 'groups'
-                      "/namespace:#{group.id}"
-                    when 'projects'
-                      "/namespace:#{project.namespace_id}/project:#{project.id}"
-                    when 'root'
-                      ''
-                    else
-                      "#{request.path}"
-                    end
+      def has_maskable_params?
+        request_params = @request.path_parameters.to_h
+        request_params.has_key?(:namespace_id) || request_params.has_key?(:project_id) || request_params.has_key?(:id) || @request.query_string.present?
+      end
 
-      masked_url += request.query_string.present? ? "?#{request.query_string}" : ''
+      def masked_query_params
+        return {} unless @request.query_string.present?
 
-      masked_url
-    end
+        query_string_hash = Rack::Utils.parse_nested_query(@request.query_string)
+
+        QUERY_PARAMS_TO_MASK.each do |maskable_attribute|
+          next unless query_string_hash.has_key?(maskable_attribute)
 
-    def url_with_namespace_type(request_params, namespace_type)
-      masked_url = "#{request.protocol}#{request.host_with_port}"
+          query_string_hash[maskable_attribute] = "masked_#{maskable_attribute}"
+        end
 
-      if request_params.has_key?(:project_id)
-        masked_url += "/namespace:#{project.namespace_id}/project:#{project.id}/-/#{namespace_type}"
+        query_string_hash
       end
 
-      if request_params.has_key?(:id)
-        masked_url += namespace_type == 'blob' ? '/:repository_path' : "/#{request_params[:id]}"
+      def default_root_url
+        Gitlab::Routing.url_helpers.root_url(only_path: false)
       end
+    end
 
-      masked_url += request.query_string.present? ? "?#{request.query_string}" : ''
+    def masked_page_url
+      return unless Feature.enabled?(:mask_page_urls, type: :ops)
 
-      masked_url
+      current_group = group if defined?(group)
+      current_project = project if defined?(project)
+      mask_helper = MaskHelper.new(request, current_group, current_project)
+      mask_helper.mask_params
+    rescue ActionController::RoutingError, URI::InvalidURIError => e
+      Gitlab::ErrorTracking.track_exception(e, url: request.original_fullpath)
+      nil
     end
   end
 end

spec/helpers/routing/pseudonymization_helper_spec.rb

@@ -25,95 +25,155 @@ RSpec.describe ::Routing::PseudonymizationHelper do
 
   describe 'when url has params to mask' do
     context 'with controller for MR' do
-      let(:masked_url) { "http://test.host/namespace:#{group.id}/project:#{project.id}/-/merge_requests/#{merge_request.id}" }
+      let(:masked_url) { "http://localhost/namespace#{group.id}/project#{project.id}/-/merge_requests/#{merge_request.id}" }
+      let(:request) do
+        double(:Request,
+               path_parameters: {
+                controller: "projects/merge_requests",
+                action: "show",
+                namespace_id: group.name,
+                project_id: project.name,
+                id: merge_request.id.to_s
+               },
+               protocol: 'http',
+               host: 'localhost',
+               query_string: '')
+      end
 
       before do
-        allow(Rails.application.routes).to receive(:recognize_path).and_return({
-         controller: "projects/merge_requests",
-         action: "show",
-         namespace_id: group.name,
-         project_id: project.name,
-         id: merge_request.id.to_s
-        })
+        allow(helper).to receive(:request).and_return(request)
       end
 
       it_behaves_like 'masked url'
     end
 
     context 'with controller for issue' do
-      let(:masked_url) { "http://test.host/namespace:#{group.id}/project:#{project.id}/-/issues/#{issue.id}" }
+      let(:masked_url) { "http://localhost/namespace#{group.id}/project#{project.id}/-/issues/#{issue.id}" }
+      let(:request) do
+        double(:Request,
+               path_parameters: {
+                controller: "projects/issues",
+                action: "show",
+                namespace_id: group.name,
+                project_id: project.name,
+                id: issue.id.to_s
+               },
+               protocol: 'http',
+               host: 'localhost',
+               query_string: '')
+      end
 
       before do
-        allow(Rails.application.routes).to receive(:recognize_path).and_return({
-         controller: "projects/issues",
-         action: "show",
-         namespace_id: group.name,
-         project_id: project.name,
-         id: issue.id.to_s
-        })
+        allow(helper).to receive(:request).and_return(request)
       end
 
       it_behaves_like 'masked url'
     end
 
     context 'with controller for groups with subgroups and project' do
-      let(:masked_url) { "http://test.host/namespace:#{subgroup.id}/project:#{subproject.id}"}
+      let(:masked_url) { "http://localhost/namespace#{subgroup.id}/project#{subproject.id}"}
+      let(:request) do
+        double(:Request,
+               path_parameters: {
+                controller: 'projects',
+                action: 'show',
+                namespace_id: subgroup.name,
+                id: subproject.name
+               },
+               protocol: 'http',
+               host: 'localhost',
+               query_string: '')
+      end
 
       before do
         allow(helper).to receive(:group).and_return(subgroup)
         allow(helper).to receive(:project).and_return(subproject)
-        allow(Rails.application.routes).to receive(:recognize_path).and_return({
-          controller: 'projects',
-          action: 'show',
-          namespace_id: subgroup.name,
-          id: subproject.name
-        })
+        allow(helper).to receive(:request).and_return(request)
       end
 
       it_behaves_like 'masked url'
     end
 
     context 'with controller for groups and subgroups' do
-      let(:masked_url) { "http://test.host/namespace:#{subgroup.id}"}
+      let(:masked_url) { "http://localhost/groups/namespace#{subgroup.id}/-/shared"}
+      let(:request) do
+        double(:Request,
+               path_parameters: {
+                controller: 'groups',
+                action: 'show',
+                id: subgroup.name
+               },
+               protocol: 'http',
+               host: 'localhost',
+               query_string: '')
+      end
 
       before do
         allow(helper).to receive(:group).and_return(subgroup)
-        allow(Rails.application.routes).to receive(:recognize_path).and_return({
-          controller: 'groups',
-          action: 'show',
-          id: subgroup.name
-        })
+        allow(helper).to receive(:request).and_return(request)
       end
 
       it_behaves_like 'masked url'
     end
 
     context 'with controller for blob with file path' do
-      let(:masked_url) { "http://test.host/namespace:#{group.id}/project:#{project.id}/-/blob/:repository_path" }
+      let(:masked_url) { "http://localhost/namespace#{group.id}/project#{project.id}/-/blob/:repository_path" }
+      let(:request) do
+        double(:Request,
+               path_parameters: {
+                controller: 'projects/blob',
+                action: 'show',
+                namespace_id: group.name,
+                project_id: project.name,
+                id: 'master/README.md'
+               },
+               protocol: 'http',
+               host: 'localhost',
+               query_string: '')
+      end
 
       before do
-        allow(Rails.application.routes).to receive(:recognize_path).and_return({
-          controller: 'projects/blob',
-          action: 'show',
-          namespace_id: group.name,
-          project_id: project.name,
-          id: 'master/README.md'
-        })
+        allow(helper).to receive(:request).and_return(request)
       end
 
       it_behaves_like 'masked url'
     end
 
-    context 'with non identifiable controller' do
-      let(:masked_url) { "http://test.host/dashboard/issues?assignee_username=root" }
+    context 'when assignee_username is present' do
+      let(:masked_url) { "http://localhost/dashboard/issues?assignee_username=masked_assignee_username" }
+      let(:request) do
+        double(:Request,
+               path_parameters: {
+                controller: 'dashboard',
+                action: 'issues'
+               },
+               protocol: 'http',
+               host: 'localhost',
+               query_string: 'assignee_username=root')
+      end
 
       before do
-        controller.request.path = '/dashboard/issues'
-        controller.request.query_string = 'assignee_username=root'
-        allow(Rails.application.routes).to receive(:recognize_path).and_return({
-          controller: 'dashboard',
-          action: 'issues'
-        })
+        allow(helper).to receive(:request).and_return(request)
+      end
+
+      it_behaves_like 'masked url'
+    end
+
+    context 'when author_username is present' do
+      let(:masked_url) { "http://localhost/dashboard/issues?author_username=masked_author_username&scope=all&state=opened" }
+      let(:request) do
+        double(:Request,
+               path_parameters: {
+                controller: 'dashboard',
+                action: 'issues'
+               },
+               protocol: 'http',
+               host: 'localhost',
+               query_string: 'author_username=root&scope=all&state=opened')
+      end
+
+      before do
+        allow(helper).to receive(:request).and_return(request)
       end
 
       it_behaves_like 'masked url'
@@ -121,9 +181,13 @@ RSpec.describe ::Routing::PseudonymizationHelper do
   end
 
   describe 'when url has no params to mask' do
-    let(:root_url) { 'http://test.host' }
+    let(:root_url) { 'http://localhost/some/path' }
 
     context 'returns root url' do
+      before do
+        controller.request.path = 'some/path'
+      end
+
       it 'masked_page_url' do
         expect(helper.masked_page_url).to eq(root_url)
       end
@@ -132,17 +196,26 @@ RSpec.describe ::Routing::PseudonymizationHelper do
 
   describe 'when it raises exception' do
     context 'calls error tracking' do
+      let(:request) do
+        double(:Request,
+               path_parameters: {
+                controller: 'dashboard',
+                action: 'issues'
+               },
+               protocol: 'http',
+               host: 'localhost',
+               query_string: 'assignee_username=root',
+               original_fullpath: '/dashboard/issues?assignee_username=root')
+      end
+
       before do
-        controller.request.path = '/dashboard/issues'
-        controller.request.query_string = 'assignee_username=root'
-        allow(Rails.application.routes).to receive(:recognize_path).and_return({
-          controller: 'dashboard',
-          action: 'issues'
-        })
+        allow(helper).to receive(:request).and_return(request)
       end
 
       it 'sends error to sentry and returns nil' do
-        allow(helper).to receive(:mask_params).with(anything).and_raise(ActionController::RoutingError, 'Some routing error')
+        allow_next_instance_of(Routing::PseudonymizationHelper::MaskHelper) do |mask_helper|
+          allow(mask_helper).to receive(:mask_params).and_raise(ActionController::RoutingError, 'Some routing error')
+        end
 
         expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
           ActionController::RoutingError,