diff options
Diffstat (limited to 'spec')
-rw-r--r-- | spec/lib/gitlab/middleware/multipart_spec.rb | 11 | ||||
-rw-r--r-- | spec/lib/gitlab/wiki_pages/front_matter_parser_spec.rb | 274 | ||||
-rw-r--r-- | spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb | 28 | ||||
-rw-r--r-- | spec/models/active_session_spec.rb | 48 | ||||
-rw-r--r-- | spec/models/wiki_page_spec.rb | 202 | ||||
-rw-r--r-- | spec/requests/api/runner_spec.rb | 61 | ||||
-rw-r--r-- | spec/services/groups/destroy_service_spec.rb | 51 | ||||
-rw-r--r-- | spec/support/helpers/workhorse_helpers.rb | 36 | ||||
-rw-r--r-- | spec/support/rails/test_case_patch.rb | 53 |
9 files changed, 710 insertions, 54 deletions
diff --git a/spec/lib/gitlab/middleware/multipart_spec.rb b/spec/lib/gitlab/middleware/multipart_spec.rb index 1d3ad3afb56..ec153e25d44 100644 --- a/spec/lib/gitlab/middleware/multipart_spec.rb +++ b/spec/lib/gitlab/middleware/multipart_spec.rb @@ -89,6 +89,17 @@ describe Gitlab::Middleware::Multipart do end end + it 'allows files in the job artifact upload path' do + with_tmp_dir('artifacts') do |dir, env| + expect(JobArtifactUploader).to receive(:workhorse_upload_path).and_return(File.join(dir, 'artifacts')) + expect(app).to receive(:call) do |env| + expect(get_params(env)['file']).to be_a(::UploadedFile) + end + + middleware.call(env) + end + end + it 'allows symlinks for uploads dir' do Tempfile.open('two-levels') do |tempfile| symlinked_dir = '/some/dir/uploads' diff --git a/spec/lib/gitlab/wiki_pages/front_matter_parser_spec.rb b/spec/lib/gitlab/wiki_pages/front_matter_parser_spec.rb new file mode 100644 index 00000000000..c606ba11b9c --- /dev/null +++ b/spec/lib/gitlab/wiki_pages/front_matter_parser_spec.rb @@ -0,0 +1,274 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::WikiPages::FrontMatterParser do + subject(:parser) { described_class.new(raw_content, gate) } + + let(:content) { 'This is the content' } + let(:end_divider) { '---' } + let(:gate) { double('Gate') } + + let(:with_front_matter) do + <<~MD + --- + a: 1 + b: 2 + c: + - foo + - bar + date: I am safe. Not actually a date + #{end_divider} + #{content} + MD + end + + def have_correct_front_matter + include(a: 1, b: 2, c: %w(foo bar)) + end + + describe '#parse' do + subject { parser.parse } + + context 'there is front matter' do + let(:raw_content) { with_front_matter } + + it do + is_expected.to have_attributes( + front_matter: have_correct_front_matter, + content: content + "\n", + error: be_nil + ) + end + end + + context 'there is no content' do + let(:raw_content) { '' } + + it do + is_expected.to have_attributes( + front_matter: {}, + content: raw_content, + error: be_nil + ) + end + end + + context 'there is no front_matter' do + let(:raw_content) { content } + + it { is_expected.to have_attributes(front_matter: be_empty, content: raw_content) } + + it { is_expected.to have_attributes(reason: :no_match) } + end + + context 'the feature flag is disabled' do + let(:raw_content) { with_front_matter } + + before do + stub_feature_flags(Gitlab::WikiPages::FrontMatterParser::FEATURE_FLAG => false) + end + + it { is_expected.to have_attributes(front_matter: be_empty, content: raw_content) } + end + + context 'the feature flag is enabled for the gated object' do + let(:raw_content) { with_front_matter } + + before do + stub_feature_flags(Gitlab::WikiPages::FrontMatterParser::FEATURE_FLAG => false) + stub_feature_flags(Gitlab::WikiPages::FrontMatterParser::FEATURE_FLAG => { + enabled: true, + thing: gate + }) + end + + it do + is_expected.to have_attributes( + front_matter: have_correct_front_matter, + content: content + "\n", + reason: be_nil + ) + end + end + + context 'the end divider is ...' do + let(:end_divider) { '...' } + let(:raw_content) { with_front_matter } + + it { is_expected.to have_attributes(front_matter: have_correct_front_matter) } + end + + context 'the front-matter is not a mapping' do + let(:raw_content) do + <<~MD + --- + - thing one + - thing two + --- + #{content} + MD + end + + it { is_expected.to have_attributes(reason: :not_mapping) } + end + + context 'there is nothing in the front-matter block' do + let(:raw_content) do + <<~MD + --- + --- + My content here + MD + end + + it { is_expected.to have_attributes(reason: :not_mapping) } + end + + context 'there is a string in the YAML block' do + let(:raw_content) do + <<~MD + --- + This is a string + --- + #{content} + MD + end + + it { is_expected.to have_attributes(reason: :not_mapping) } + end + + context 'there is dangerous YAML in the block' do + let(:raw_content) do + <<~MD + --- + date: 2010-02-11 11:02:57 + --- + #{content} + MD + end + + it { is_expected.to have_attributes(reason: :parse_error, error: be_present) } + end + + context 'there is acceptably long YAML in the front-matter block' do + let(:raw_content) do + key = 'title: ' + length = described_class::MAX_FRONT_MATTER_LENGTH - key.size + + <<~MD + --- + title: #{FFaker::Lorem.characters(length)} + --- + #{content} + MD + end + + it { is_expected.to have_attributes(front_matter: include(title: be_present)) } + end + + context 'there is suspiciously long YAML in the front-matter block' do + let(:raw_content) do + <<~MD + --- + title: #{FFaker::Lorem.characters(described_class::MAX_FRONT_MATTER_LENGTH)} + --- + #{content} + MD + end + + it { is_expected.to have_attributes(reason: :too_long) } + end + + context 'TOML front matter' do + let(:raw_content) do + <<~MD + +++ + title = "My title" + +++ + #{content} + MD + end + + it { is_expected.to have_attributes(reason: :not_yaml) } + end + + context 'TOML style fences, advertised as YAML' do + let(:raw_content) do + <<~MD + +++ yaml + title: "My title" + +++ + #{content} + MD + end + + it { is_expected.to have_attributes(front_matter: include(title: 'My title')) } + end + + context 'YAML, advertised as something else' do + let(:raw_content) do + <<~MD + --- toml + title: My title + --- + #{content} + MD + end + + it { is_expected.to have_attributes(reason: :not_yaml) } + end + + context 'there is text content in the YAML block, in comments' do + let(:raw_content) do + <<~MD + --- + # This is YAML + # + # It has comments though. Explaining things + foo: 1 + + ## It has headings + + headings: + + - heading one + - heading two + + # And lists + + lists: + + - and lists + - with things in them + --- + #{content} + MD + end + + it { is_expected.to have_attributes(front_matter: include(foo: 1)) } + end + + context 'there is text content in the YAML block' do + let(:raw_content) do + <<~MD + --- + # This is not YAML + + In fact is looks like markdown + + ## It has headings + + Paragraphs + + - and lists + - with things in them + --- + #{content} + MD + end + + it { is_expected.to have_attributes(reason: :not_mapping) } + end + end +end diff --git a/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb b/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb new file mode 100644 index 00000000000..19ba8984224 --- /dev/null +++ b/spec/migrations/schedule_recalculate_project_authorizations_third_run_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20200204113225_schedule_recalculate_project_authorizations_third_run.rb') + +describe ScheduleRecalculateProjectAuthorizationsThirdRun do + let(:users_table) { table(:users) } + + before do + stub_const("#{described_class}::BATCH_SIZE", 2) + + 1.upto(4) do |i| + users_table.create!(id: i, name: "user#{i}", email: "user#{i}@example.com", projects_limit: 1) + end + end + + it 'schedules background migration' do + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + expect(described_class::MIGRATION).to be_scheduled_migration(1, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(3, 4) + end + end + end +end diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb index 27a80f93566..6a97d91b3ca 100644 --- a/spec/models/active_session_spec.rb +++ b/spec/models/active_session_spec.rb @@ -9,10 +9,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do end end - let(:session) do - double(:session, { id: '6919a6f1bb119dd7396fadc38fd18d0d', - '[]': {} }) - end + let(:rack_session) { Rack::Session::SessionId.new('6919a6f1bb119dd7396fadc38fd18d0d') } + let(:session) { instance_double(ActionDispatch::Request::Session, id: rack_session, '[]': {}) } let(:request) do double(:request, { @@ -25,13 +23,13 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do describe '#current?' do it 'returns true if the active session matches the current session' do - active_session = ActiveSession.new(session_id: '6919a6f1bb119dd7396fadc38fd18d0d') + active_session = ActiveSession.new(session_id: rack_session) expect(active_session.current?(session)).to be true end it 'returns false if the active session does not match the current session' do - active_session = ActiveSession.new(session_id: '59822c7d9fcdfa03725eff41782ad97d') + active_session = ActiveSession.new(session_id: Rack::Session::SessionId.new('59822c7d9fcdfa03725eff41782ad97d')) expect(active_session.current?(session)).to be false end @@ -46,14 +44,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do describe '#public_id' do it 'returns an encrypted, url-encoded session id' do - original_session_id = "!*'();:@&\n=+$,/?%abcd#123[4567]8" + original_session_id = Rack::Session::SessionId.new("!*'();:@&\n=+$,/?%abcd#123[4567]8") active_session = ActiveSession.new(session_id: original_session_id) - encrypted_encoded_id = active_session.public_id - - encrypted_id = CGI.unescape(encrypted_encoded_id) + encrypted_id = active_session.public_id derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id) - expect(original_session_id).to eq derived_session_id + expect(original_session_id.public_id).to eq derived_session_id end end @@ -104,7 +100,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do describe '.list_sessions' do it 'uses the ActiveSession lookup to return original sessions' do Gitlab::Redis::SharedState.with do |redis| - redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", Marshal.dump({ _csrf_token: 'abcd' })) + # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88 + redis.set("session:gitlab:#{rack_session.private_id}", Marshal.dump({ _csrf_token: 'abcd' })) redis.sadd( "session:lookup:user:gitlab:#{user.id}", @@ -127,17 +124,18 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do redis.sadd("session:lookup:user:gitlab:#{user.id}", session_ids) end - expect(ActiveSession.session_ids_for_user(user.id)).to eq(session_ids) + expect(ActiveSession.session_ids_for_user(user.id).map(&:to_s)).to eq(session_ids) end end describe '.sessions_from_ids' do it 'uses the ActiveSession lookup to return original sessions' do Gitlab::Redis::SharedState.with do |redis| - redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", Marshal.dump({ _csrf_token: 'abcd' })) + # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88 + redis.set("session:gitlab:#{rack_session.private_id}", Marshal.dump({ _csrf_token: 'abcd' })) end - expect(ActiveSession.sessions_from_ids(['6919a6f1bb119dd7396fadc38fd18d0d'])).to eq [{ _csrf_token: 'abcd' }] + expect(ActiveSession.sessions_from_ids([rack_session])).to eq [{ _csrf_token: 'abcd' }] end it 'avoids a redis lookup for an empty array' do @@ -152,11 +150,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do redis = double(:redis) expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) - sessions = %w[session-a session-b] + sessions = %w[session-a session-b session-c session-d] mget_responses = sessions.map { |session| [Marshal.dump(session)]} - expect(redis).to receive(:mget).twice.and_return(*mget_responses) + expect(redis).to receive(:mget).exactly(4).times.and_return(*mget_responses) - expect(ActiveSession.sessions_from_ids([1, 2])).to eql(sessions) + session_ids = [1, 2].map { |id| Rack::Session::SessionId.new(id.to_s) } + expect(ActiveSession.sessions_from_ids(session_ids).map(&:to_s)).to eql(sessions) end end @@ -212,6 +211,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do end describe '.destroy' do + it 'gracefully handles a nil session ID' do + expect(described_class).not_to receive(:destroy_sessions) + + ActiveSession.destroy(user, nil) + end + it 'removes the entry associated with the currently killed user session' do Gitlab::Redis::SharedState.with do |redis| redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '') @@ -244,8 +249,9 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do it 'removes the devise session' do Gitlab::Redis::SharedState.with do |redis| - redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '') - redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", '') + redis.set("session:user:gitlab:#{user.id}:#{rack_session.public_id}", '') + # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88 + redis.set("session:gitlab:#{rack_session.private_id}", '') end ActiveSession.destroy(user, request.session.id) @@ -322,7 +328,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do (1..max_number_of_sessions_plus_two).each do |number| redis.set( "session:user:gitlab:#{user.id}:#{number}", - Marshal.dump(ActiveSession.new(session_id: "#{number}", updated_at: number.days.ago)) + Marshal.dump(ActiveSession.new(session_id: number.to_s, updated_at: number.days.ago)) ) redis.sadd( "session:lookup:user:gitlab:#{user.id}", diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 0a0611bdeb8..718b386b3fd 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -20,6 +20,17 @@ describe WikiPage do subject { new_page } + def disable_front_matter + stub_feature_flags(Gitlab::WikiPages::FrontMatterParser::FEATURE_FLAG => false) + end + + def enable_front_matter_for_project + stub_feature_flags(Gitlab::WikiPages::FrontMatterParser::FEATURE_FLAG => { + thing: project, + enabled: true + }) + end + describe '.group_by_directory' do context 'when there are no pages' do it 'returns an empty array' do @@ -101,6 +112,119 @@ describe WikiPage do end end + describe '#front_matter' do + let_it_be(:project) { create(:project) } + let(:wiki_page) { create(:wiki_page, project: project, content: content) } + + shared_examples 'a page without front-matter' do + it { expect(wiki_page).to have_attributes(front_matter: {}, content: content) } + end + + shared_examples 'a page with front-matter' do + let(:front_matter) { { title: 'Foo', slugs: %w[slug_a slug_b] } } + + it { expect(wiki_page.front_matter).to eq(front_matter) } + end + + context 'the wiki page has front matter' do + let(:content) do + <<~MD + --- + title: Foo + slugs: + - slug_a + - slug_b + --- + + My actual content + MD + end + + it_behaves_like 'a page with front-matter' + + it 'strips the front matter from the content' do + expect(wiki_page.content.strip).to eq('My actual content') + end + + context 'the feature flag is off' do + before do + disable_front_matter + end + + it_behaves_like 'a page without front-matter' + + context 'but enabled for the project' do + before do + enable_front_matter_for_project + end + + it_behaves_like 'a page with front-matter' + end + end + end + + context 'the wiki page does not have front matter' do + let(:content) { 'My actual content' } + + it_behaves_like 'a page without front-matter' + end + + context 'the wiki page has fenced blocks, but nothing in them' do + let(:content) do + <<~MD + --- + --- + + My actual content + MD + end + + it_behaves_like 'a page without front-matter' + end + + context 'the wiki page has invalid YAML type in fenced blocks' do + let(:content) do + <<~MD + --- + this isn't YAML + --- + + My actual content + MD + end + + it_behaves_like 'a page without front-matter' + end + + context 'the wiki page has a disallowed class in fenced block' do + let(:content) do + <<~MD + --- + date: 2010-02-11 11:02:57 + --- + + My actual content + MD + end + + it_behaves_like 'a page without front-matter' + end + + context 'the wiki page has invalid YAML in fenced block' do + let(:content) do + <<~MD + --- + invalid-use-of-reserved-indicator: @text + --- + + My actual content + MD + end + + it_behaves_like 'a page without front-matter' + end + end + describe '.unhyphenize' do it 'removes hyphens from a name' do name = 'a-name--with-hyphens' @@ -155,8 +279,8 @@ describe WikiPage do end describe '#validate_path_limits' do - let(:max_title) { described_class::MAX_TITLE_BYTES } - let(:max_directory) { described_class::MAX_DIRECTORY_BYTES } + let(:max_title) { Gitlab::WikiPages::MAX_TITLE_BYTES } + let(:max_directory) { Gitlab::WikiPages::MAX_DIRECTORY_BYTES } where(:character) do ['a', 'รค', '๐'] @@ -296,7 +420,7 @@ describe WikiPage do subject.update(content: "new content") page = wiki.find_page(title) - expect(page.content).to eq('new content') + expect([subject.content, page.content]).to all(eq('new content')) end it "returns true" do @@ -333,7 +457,7 @@ describe WikiPage do subject.update(content: new_content) page = wiki.find_page('test page') - expect(page.content).to eq("new content") + expect([subject.content, page.content]).to all(eq("new content")) end it "updates the title of the page" do @@ -342,7 +466,75 @@ describe WikiPage do subject.update(title: new_title) page = wiki.find_page(new_title) - expect(page.title).to eq(new_title) + expect([subject.title, page.title]).to all(eq(new_title)) + end + + describe 'updating front_matter' do + shared_examples 'able to update front-matter' do + it 'updates the wiki-page front-matter' do + title = subject.title + content = subject.content + subject.update(front_matter: { slugs: ['x'] }) + page = wiki.find_page(title) + + expect([subject, page]).to all( + have_attributes( + front_matter: include(slugs: include('x')), + content: content + )) + end + end + + it_behaves_like 'able to update front-matter' + + context 'the front matter is too long' do + let(:new_front_matter) do + { + title: generate(:wiki_page_title), + slugs: Array.new(51).map { FFaker::Lorem.characters(512) } + } + end + + it 'raises an error' do + expect { subject.update(front_matter: new_front_matter) }.to raise_error(described_class::FrontMatterTooLong) + end + end + + context 'the front-matter feature flag is not enabled' do + before do + disable_front_matter + end + + it 'does not update the front-matter' do + content = subject.content + subject.update(front_matter: { slugs: ['x'] }) + + page = wiki.find_page(subject.title) + + expect([subject, page]).to all(have_attributes(front_matter: be_empty, content: content)) + end + + context 'but it is enabled for the project' do + before do + enable_front_matter_for_project + end + + it_behaves_like 'able to update front-matter' + end + end + + it 'updates the wiki-page front-matter and content together' do + title = subject.title + content = 'totally new content' + subject.update(content: content, front_matter: { slugs: ['x'] }) + page = wiki.find_page(title) + + expect([subject, page]).to all( + have_attributes( + front_matter: include(slugs: include('x')), + content: content + )) + end end it "returns true" do diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index a67c1a48ad4..bc2da8a2b9a 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -1422,8 +1422,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do describe 'artifacts' do let(:job) { create(:ci_build, :pending, user: user, project: project, pipeline: pipeline, runner_id: runner.id) } - let(:jwt_token) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') } - let(:headers) { { 'GitLab-Workhorse' => '1.0', Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => jwt_token } } + let(:jwt) { JWT.encode({ 'iss' => 'gitlab-workhorse' }, Gitlab::Workhorse.secret, 'HS256') } + let(:headers) { { 'GitLab-Workhorse' => '1.0', Gitlab::Workhorse::INTERNAL_API_REQUEST_HEADER => jwt } } let(:headers_with_token) { headers.merge(API::Helpers::Runner::JOB_TOKEN_HEADER => job.token) } let(:file_upload) { fixture_file_upload('spec/fixtures/banana_sample.gif', 'image/gif') } let(:file_upload2) { fixture_file_upload('spec/fixtures/dk.png', 'image/gif') } @@ -1703,12 +1703,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do it 'fails to post artifacts without GitLab-Workhorse' do post api("/jobs/#{job.id}/artifacts"), params: { token: job.token }, headers: {} - expect(response).to have_gitlab_http_status(:forbidden) + expect(response).to have_gitlab_http_status(:bad_request) end end context 'Is missing GitLab Workhorse token headers' do - let(:jwt_token) { JWT.encode({ 'iss' => 'invalid-header' }, Gitlab::Workhorse.secret, 'HS256') } + let(:jwt) { JWT.encode({ 'iss' => 'invalid-header' }, Gitlab::Workhorse.secret, 'HS256') } it 'fails to post artifacts without GitLab-Workhorse' do expect(Gitlab::ErrorTracking).to receive(:track_exception).once @@ -1722,15 +1722,14 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when setting an expire date' do let(:default_artifacts_expire_in) {} let(:post_data) do - { 'file.path' => file_upload.path, - 'file.name' => file_upload.original_filename, - 'expire_in' => expire_in } + { file: file_upload, + expire_in: expire_in } end before do stub_application_setting(default_artifacts_expire_in: default_artifacts_expire_in) - post(api("/jobs/#{job.id}/artifacts"), params: post_data, headers: headers_with_token) + upload_artifacts(file_upload, headers_with_token, post_data) end context 'when an expire_in is given' do @@ -1783,20 +1782,22 @@ describe API::Runner, :clean_gitlab_redis_shared_state do let(:stored_artifacts_size) { job.reload.artifacts_size } let(:stored_artifacts_sha256) { job.reload.job_artifacts_archive.file_sha256 } let(:stored_metadata_sha256) { job.reload.job_artifacts_metadata.file_sha256 } + let(:file_keys) { post_data.keys } + let(:send_rewritten_field) { true } before do - post(api("/jobs/#{job.id}/artifacts"), params: post_data, headers: headers_with_token) + workhorse_finalize_with_multiple_files( + api("/jobs/#{job.id}/artifacts"), + method: :post, + file_keys: file_keys, + params: post_data, + headers: headers_with_token, + send_rewritten_field: send_rewritten_field + ) end context 'when posts data accelerated by workhorse is correct' do - let(:post_data) do - { 'file.path' => artifacts.path, - 'file.name' => artifacts.original_filename, - 'file.sha256' => artifacts_sha256, - 'metadata.path' => metadata.path, - 'metadata.name' => metadata.original_filename, - 'metadata.sha256' => metadata_sha256 } - end + let(:post_data) { { file: artifacts, metadata: metadata } } it 'stores artifacts and artifacts metadata' do expect(response).to have_gitlab_http_status(:created) @@ -1808,9 +1809,30 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end end + context 'with a malicious file.path param' do + let(:post_data) { {} } + let(:tmp_file) { Tempfile.new('crafted.file.path') } + let(:url) { "/jobs/#{job.id}/artifacts?file.path=#{tmp_file.path}" } + + it 'rejects the request' do + expect(response).to have_gitlab_http_status(:bad_request) + expect(stored_artifacts_size).to be_nil + end + end + + context 'when workhorse header is missing' do + let(:post_data) { { file: artifacts, metadata: metadata } } + let(:send_rewritten_field) { false } + + it 'rejects the request' do + expect(response).to have_gitlab_http_status(:bad_request) + expect(stored_artifacts_size).to be_nil + end + end + context 'when there is no artifacts file in post data' do let(:post_data) do - { 'metadata' => metadata } + { metadata: metadata } end it 'is expected to respond with bad request' do @@ -2053,7 +2075,8 @@ describe API::Runner, :clean_gitlab_redis_shared_state do method: :post, file_key: :file, params: params.merge(file: file), - headers: headers + headers: headers, + send_rewritten_field: true ) end end diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index a45c7cdffa6..bf639153b99 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -133,4 +133,55 @@ describe Groups::DestroyService do end end end + + describe 'authorization updates', :sidekiq_inline do + context 'shared groups' do + let!(:shared_group) { create(:group, :private) } + let!(:shared_group_child) { create(:group, :private, parent: shared_group) } + + let!(:project) { create(:project, group: shared_group) } + let!(:project_child) { create(:project, group: shared_group_child) } + + before do + create(:group_group_link, shared_group: shared_group, shared_with_group: group) + group.refresh_members_authorized_projects + end + + it 'updates project authorization' do + expect(user.can?(:read_project, project)).to eq(true) + expect(user.can?(:read_project, project_child)).to eq(true) + + destroy_group(group, user, false) + + expect(user.can?(:read_project, project)).to eq(false) + expect(user.can?(:read_project, project_child)).to eq(false) + end + end + + context 'shared groups in the same group hierarchy' do + let!(:subgroup) { create(:group, :private, parent: group) } + let!(:subgroup_user) { create(:user) } + + before do + subgroup.add_user(subgroup_user, Gitlab::Access::MAINTAINER) + + create(:group_group_link, shared_group: group, shared_with_group: subgroup) + subgroup.refresh_members_authorized_projects + end + + context 'group is deleted' do + it 'updates project authorization' do + expect { destroy_group(group, user, false) }.to( + change { subgroup_user.can?(:read_project, project) }.from(true).to(false)) + end + end + + context 'subgroup is deleted' do + it 'updates project authorization' do + expect { destroy_group(subgroup, user, false) }.to( + change { subgroup_user.can?(:read_project, project) }.from(true).to(false)) + end + end + end + end end diff --git a/spec/support/helpers/workhorse_helpers.rb b/spec/support/helpers/workhorse_helpers.rb index de232da3c8c..53b36b3dd45 100644 --- a/spec/support/helpers/workhorse_helpers.rb +++ b/spec/support/helpers/workhorse_helpers.rb @@ -33,22 +33,36 @@ module WorkhorseHelpers # workhorse_finalize will transform file_key inside params as if it was the finalize call of an inline object storage upload. # note that based on the content of the params it can simulate a disc acceleration or an object storage upload def workhorse_finalize(url, method: :post, file_key:, params:, headers: {}, send_rewritten_field: false) - workhorse_request_with_file(method, url, - file_key: file_key, - params: params, - extra_headers: headers, - send_rewritten_field: send_rewritten_field + workhorse_finalize_with_multiple_files(url, method: method, file_keys: file_key, params: params, headers: headers, send_rewritten_field: send_rewritten_field) + end + + def workhorse_finalize_with_multiple_files(url, method: :post, file_keys:, params:, headers: {}, send_rewritten_field: false) + workhorse_request_with_multiple_files(method, url, + file_keys: file_keys, + params: params, + extra_headers: headers, + send_rewritten_field: send_rewritten_field ) end def workhorse_request_with_file(method, url, file_key:, params:, env: {}, extra_headers: {}, send_rewritten_field:) + workhorse_request_with_multiple_files(method, url, file_keys: file_key, params: params, env: env, extra_headers: extra_headers, send_rewritten_field: send_rewritten_field) + end + + def workhorse_request_with_multiple_files(method, url, file_keys:, params:, env: {}, extra_headers: {}, send_rewritten_field:) workhorse_params = params.dup - file = workhorse_params.delete(file_key) - workhorse_params = workhorse_disk_accelerated_file_params(file_key, file).merge(workhorse_params) + file_keys = Array(file_keys) + rewritten_fields = {} + + file_keys.each do |key| + file = workhorse_params.delete(key) + rewritten_fields[key] = file.path if file + workhorse_params = workhorse_disk_accelerated_file_params(key, file).merge(workhorse_params) + end headers = if send_rewritten_field - workhorse_rewritten_fields_header(file_key => file.path) + workhorse_rewritten_fields_header(rewritten_fields) else {} end @@ -75,7 +89,11 @@ module WorkhorseHelpers "#{key}.name" => file.original_filename, "#{key}.size" => file.size }.tap do |params| - params["#{key}.path"] = file.path if file.path + if file.path + params["#{key}.path"] = file.path + params["#{key}.sha256"] = Digest::SHA256.file(file.path).hexdigest + end + params["#{key}.remote_id"] = file.remote_id if file.respond_to?(:remote_id) && file.remote_id.present? end end diff --git a/spec/support/rails/test_case_patch.rb b/spec/support/rails/test_case_patch.rb new file mode 100644 index 00000000000..161e1ef2a4c --- /dev/null +++ b/spec/support/rails/test_case_patch.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true +# +# This file pulls in the changes in https://github.com/rails/rails/pull/38063 +# to fix controller specs updated with the latest Rack versions. +# +# This file should be removed after that change ships. It is not +# present in Rails 6.0.2.2. +module ActionController + class TestRequest < ActionDispatch::TestRequest #:nodoc: + def self.new_session + TestSessionPatched.new + end + end + + # Methods #destroy and #load! are overridden to avoid calling methods on the + # @store object, which does not exist for the TestSession class. + class TestSessionPatched < Rack::Session::Abstract::PersistedSecure::SecureSessionHash #:nodoc: + DEFAULT_OPTIONS = Rack::Session::Abstract::Persisted::DEFAULT_OPTIONS + + def initialize(session = {}) + super(nil, nil) + @id = Rack::Session::SessionId.new(SecureRandom.hex(16)) + @data = stringify_keys(session) + @loaded = true + end + + def exists? + true + end + + def keys + @data.keys + end + + def values + @data.values + end + + def destroy + clear + end + + def fetch(key, *args, &block) + @data.fetch(key.to_s, *args, &block) + end + + private + + def load! + @id + end + end +end |