From 681a24361b2a1b642cf7b38f45517600b952e198 Mon Sep 17 00:00:00 2001 From: Ruben Davila Date: Thu, 8 Jun 2017 00:20:22 -0500 Subject: Escape the underscore char inside the LIKE operator An underscore can be used as a wildcard inside the LIKE operator, if we really want to match it then we need to escape it otherwise the query will return invalid results. --- app/models/concerns/routable.rb | 6 ++--- spec/models/concerns/routable_spec.rb | 23 +++++++++++++++++ .../refresh_authorized_projects_service_spec.rb | 30 ++++++++++++++++++++++ 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index c4463abdfe6..2f6520521c8 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -94,7 +94,7 @@ module Routable # Returns an ActiveRecord::Relation. def member_descendants(user_id) joins(:route). - joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(r2.path, '/%') + joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(REPLACE(r2.path, '_', '\\_'), '/%') INNER JOIN members ON members.source_id = r2.source_id AND members.source_type = r2.source_type"). where('members.user_id = ?', user_id) @@ -111,7 +111,7 @@ module Routable # Returns an ActiveRecord::Relation. def member_self_and_descendants(user_id) joins(:route). - joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(r2.path, '/%') + joins("INNER JOIN routes r2 ON routes.path LIKE CONCAT(REPLACE(r2.path, '_', '\\_'), '/%') OR routes.path = r2.path INNER JOIN members ON members.source_id = r2.source_id AND members.source_type = r2.source_type"). @@ -162,7 +162,7 @@ module Routable wheres = paths.map do |path| "#{connection.quote(path)} = routes.path OR - #{connection.quote(path)} LIKE CONCAT(routes.path, '/%')" + #{connection.quote(path)} LIKE CONCAT(REPLACE(routes.path, '_', '\\_'), '/%')" end joins(:route).where(wheres.join(' OR ')) diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index 49a4132f763..2f337c14d96 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -133,6 +133,29 @@ describe Group, 'Routable' do subject { described_class.member_self_and_descendants(user.id) } it { is_expected.to match_array [group, nested_group] } + + context 'when the group has special chars in its path' do + let(:user1) { create(:user) } + let(:group1) { create(:group, name: 'demo', path: 'demo') } + let(:nested_group1) { create(:group, name: 'nest', path: 'nest', parent: group1) } + let!(:project1) { create(:empty_project, group: nested_group1) } + + let(:user2) { create(:user) } + let(:group2) { create(:group, name: '____', path: '____') } + let(:nested_group2) { create(:group, name: 'test', path: 'test', parent: group2) } + let!(:project2) { create(:empty_project, group: nested_group2) } + + before do + group1.add_master(user1) + group2.add_master(user2) + end + + it 'only returns the right groups' do + groups = described_class.member_self_and_descendants(user2.id) + + expect(groups).to match_array([group2, nested_group2]) + end + end end describe '.member_hierarchy' do diff --git a/spec/services/users/refresh_authorized_projects_service_spec.rb b/spec/services/users/refresh_authorized_projects_service_spec.rb index b19374ef1a2..39fae22c623 100644 --- a/spec/services/users/refresh_authorized_projects_service_spec.rb +++ b/spec/services/users/refresh_authorized_projects_service_spec.rb @@ -115,6 +115,36 @@ describe Users::RefreshAuthorizedProjectsService do expect(user.authorized_projects_populated).to eq(true) end + + context 'when the group has special chars in its path' do + let(:user1) { create(:user) } + let(:group1) { create(:group, name: 'demo', path: 'demo') } + let(:nested_group1) { create(:group, name: 'nest', path: 'nest', parent: group1) } + let!(:project1) { create(:empty_project, group: nested_group1) } + + let(:user2) { create(:user) } + let(:group2) { create(:group, name: '____', path: '____') } + let(:nested_group2) { create(:group, name: 'test', path: 'test', parent: group2) } + let!(:project2) { create(:empty_project, group: nested_group2) } + + before do + group1.add_master(user1) + group2.add_master(user2) + + Users::RefreshAuthorizedProjectsService.new(user1).execute + Users::RefreshAuthorizedProjectsService.new(user2).execute + end + + it "it doesn't give authorization to foreign projects" do + expect(user1.authorized_projects).not_to include(project2) + expect(user2.authorized_projects).not_to include(project1) + end + + it 'only gives authorization to the right projects' do + expect(user1.authorized_projects).to match_array([project1]) + expect(user2.authorized_projects).to match_array([project2]) + end + end end describe '#fresh_access_levels_per_project' do -- cgit v1.2.1