diff options
Diffstat (limited to 'spec')
26 files changed, 524 insertions, 167 deletions
diff --git a/spec/controllers/graphql_controller_spec.rb b/spec/controllers/graphql_controller_spec.rb index f2d86b1b166..aed97a01a72 100644 --- a/spec/controllers/graphql_controller_spec.rb +++ b/spec/controllers/graphql_controller_spec.rb @@ -44,7 +44,7 @@ RSpec.describe GraphqlController do expect(response).to have_gitlab_http_status(:ok) end - it 'returns access denied template when user cannot access API' do + it 'returns forbidden when user cannot access API' do # User cannot access API in a couple of cases # * When user is internal(like ghost users) # * When user is blocked @@ -54,7 +54,9 @@ RSpec.describe GraphqlController do post :execute expect(response).to have_gitlab_http_status(:forbidden) - expect(response).to render_template('errors/access_denied') + expect(json_response).to include( + 'errors' => include(a_hash_including('message' => /API not accessible/)) + ) end it 'updates the users last_activity_on field' do diff --git a/spec/deprecation_toolkit_env.rb b/spec/deprecation_toolkit_env.rb index 57d32b5423c..00d66ff3cdc 100644 --- a/spec/deprecation_toolkit_env.rb +++ b/spec/deprecation_toolkit_env.rb @@ -55,9 +55,9 @@ module DeprecationToolkitEnv # one by one def self.allowed_kwarg_warning_paths %w[ - activerecord-6.0.3.6/lib/active_record/migration.rb - activesupport-6.0.3.6/lib/active_support/cache.rb - activerecord-6.0.3.6/lib/active_record/relation.rb + activerecord-6.0.3.7/lib/active_record/migration.rb + activesupport-6.0.3.7/lib/active_support/cache.rb + activerecord-6.0.3.7/lib/active_record/relation.rb asciidoctor-2.0.12/lib/asciidoctor/extensions.rb attr_encrypted-3.1.0/lib/attr_encrypted/adapters/active_record.rb ] diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb index cbd1ae628d1..add4af2bcdb 100644 --- a/spec/features/expand_collapse_diffs_spec.rb +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -253,7 +253,7 @@ RSpec.describe 'Expand and collapse diffs', :js do click_link('Expand all') # Wait for elements to appear to ensure full page reload - expect(page).to have_content('This diff was suppressed by a .gitattributes entry') + expect(page).to have_content("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") expect(page).to have_content('This source diff could not be displayed because it is too large.') expect(page).to have_content('too_large_image.jpg') find('.note-textarea') diff --git a/spec/features/projects/diffs/diff_show_spec.rb b/spec/features/projects/diffs/diff_show_spec.rb index e47f36c4b7a..56506ada3ce 100644 --- a/spec/features/projects/diffs/diff_show_spec.rb +++ b/spec/features/projects/diffs/diff_show_spec.rb @@ -174,4 +174,14 @@ RSpec.describe 'Diff file viewer', :js, :with_clean_rails_cache do end end end + + context 'when the the encoding of the file is unsupported' do + before do + visit_commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') + end + + it 'shows it is not diffable' do + expect(page).to have_content("File suppressed by a .gitattributes entry or the file's encoding is unsupported.") + end + end end diff --git a/spec/features/snippets/notes_on_personal_snippets_spec.rb b/spec/features/snippets/notes_on_personal_snippets_spec.rb index 47dad9bd88e..e03f71c5352 100644 --- a/spec/features/snippets/notes_on_personal_snippets_spec.rb +++ b/spec/features/snippets/notes_on_personal_snippets_spec.rb @@ -65,18 +65,6 @@ RSpec.describe 'Comments on personal snippets', :js do expect(page).to have_content(user_name) end end - - context 'when the author name contains HTML' do - let(:user_name) { '<h1><a href="https://bad.link/malicious.exe" class="evil">Fake Content<img class="fake-icon" src="image.png"></a></h1>' } - - it 'renders the name as plain text' do - visit snippet_path(snippet) - - content = find("#note_#{snippet_notes[0].id} .note-header-author-name").text - - expect(content).to eq user_name - end - end end context 'when submitting a note' do diff --git a/spec/frontend/behaviors/copy_as_gfm_spec.js b/spec/frontend/behaviors/copy_as_gfm_spec.js index acff990e84a..557b609f5f9 100644 --- a/spec/frontend/behaviors/copy_as_gfm_spec.js +++ b/spec/frontend/behaviors/copy_as_gfm_spec.js @@ -1,50 +1,54 @@ import initCopyAsGFM, { CopyAsGFM } from '~/behaviors/markdown/copy_as_gfm'; -import * as commonUtils from '~/lib/utils/common_utils'; describe('CopyAsGFM', () => { describe('CopyAsGFM.pasteGFM', () => { - function callPasteGFM() { + let target; + + beforeEach(() => { + target = document.createElement('input'); + target.value = 'This is code: '; + }); + + // When GFM code is copied, we put the regular plain text + // on the clipboard as `text/plain`, and the GFM as `text/x-gfm`. + // This emulates the behavior of `getData` with that data. + function callPasteGFM(data = { 'text/plain': 'code', 'text/x-gfm': '`code`' }) { const e = { originalEvent: { clipboardData: { getData(mimeType) { - // When GFM code is copied, we put the regular plain text - // on the clipboard as `text/plain`, and the GFM as `text/x-gfm`. - // This emulates the behavior of `getData` with that data. - if (mimeType === 'text/plain') { - return 'code'; - } - if (mimeType === 'text/x-gfm') { - return '`code`'; - } - return null; + return data[mimeType] || null; }, }, }, preventDefault() {}, + target, }; CopyAsGFM.pasteGFM(e); } it('wraps pasted code when not already in code tags', () => { - jest.spyOn(commonUtils, 'insertText').mockImplementation((el, textFunc) => { - const insertedText = textFunc('This is code: ', ''); + callPasteGFM(); - expect(insertedText).toEqual('`code`'); - }); + expect(target.value).toBe('This is code: `code`'); + }); + + it('does not wrap pasted code when already in code tags', () => { + target.value = 'This is code: `'; callPasteGFM(); + + expect(target.value).toBe('This is code: `code'); }); - it('does not wrap pasted code when already in code tags', () => { - jest.spyOn(commonUtils, 'insertText').mockImplementation((el, textFunc) => { - const insertedText = textFunc('This is code: `', '`'); + it('does not allow xss in x-gfm-html', () => { + const testEl = document.createElement('div'); + jest.spyOn(document, 'createElement').mockReturnValueOnce(testEl); - expect(insertedText).toEqual('code'); - }); + callPasteGFM({ 'text/plain': 'code', 'text/x-gfm-html': 'code<img/src/onerror=alert(1)>' }); - callPasteGFM(); + expect(testEl.innerHTML).toBe('code<img src="">'); }); }); diff --git a/spec/frontend/lib/utils/url_utility_spec.js b/spec/frontend/lib/utils/url_utility_spec.js index e12cd8b0e37..3b2852d08e6 100644 --- a/spec/frontend/lib/utils/url_utility_spec.js +++ b/spec/frontend/lib/utils/url_utility_spec.js @@ -1,3 +1,4 @@ +import { TEST_HOST } from 'helpers/test_constants'; import * as urlUtils from '~/lib/utils/url_utility'; const shas = { @@ -921,4 +922,37 @@ describe('URL utility', () => { expect(urlUtils.encodeSaferUrl(input)).toBe(input); }); }); + + describe('isSameOriginUrl', () => { + // eslint-disable-next-line no-script-url + const javascriptUrl = 'javascript:alert(1)'; + + beforeEach(() => { + setWindowLocation({ origin: TEST_HOST }); + }); + + it.each` + url | expected + ${TEST_HOST} | ${true} + ${`${TEST_HOST}/a/path`} | ${true} + ${'//test.host/no-protocol'} | ${true} + ${'/a/root/relative/path'} | ${true} + ${'a/relative/path'} | ${true} + ${'#hash'} | ${true} + ${'?param=foo'} | ${true} + ${''} | ${true} + ${'../../../'} | ${true} + ${`${TEST_HOST}:8080/wrong-port`} | ${false} + ${'ws://test.host/wrong-protocol'} | ${false} + ${'http://phishing.test'} | ${false} + ${'//phishing.test'} | ${false} + ${'//invalid:url'} | ${false} + ${javascriptUrl} | ${false} + ${'data:,Hello%2C%20World%21'} | ${false} + ${null} | ${false} + ${undefined} | ${false} + `('returns $expected given $url', ({ url, expected }) => { + expect(urlUtils.isSameOriginUrl(url)).toBe(expected); + }); + }); }); diff --git a/spec/frontend/releases/components/app_edit_new_spec.js b/spec/frontend/releases/components/app_edit_new_spec.js index 65ed6d6166f..748b48dacaa 100644 --- a/spec/frontend/releases/components/app_edit_new_spec.js +++ b/spec/frontend/releases/components/app_edit_new_spec.js @@ -4,6 +4,7 @@ import MockAdapter from 'axios-mock-adapter'; import { merge } from 'lodash'; import Vuex from 'vuex'; import { getJSONFixture } from 'helpers/fixtures'; +import { TEST_HOST } from 'helpers/test_constants'; import * as commonUtils from '~/lib/utils/common_utils'; import ReleaseEditNewApp from '~/releases/components/app_edit_new.vue'; import AssetLinksForm from '~/releases/components/asset_links_form.vue'; @@ -11,6 +12,7 @@ import { BACK_URL_PARAM } from '~/releases/constants'; const originalRelease = getJSONFixture('api/releases/release.json'); const originalMilestones = originalRelease.milestones; +const releasesPagePath = 'path/to/releases/page'; describe('Release edit/new component', () => { let wrapper; @@ -24,7 +26,7 @@ describe('Release edit/new component', () => { state = { release, markdownDocsPath: 'path/to/markdown/docs', - releasesPagePath: 'path/to/releases/page', + releasesPagePath, projectId: '8', groupId: '42', groupMilestonesAvailable: true, @@ -75,6 +77,8 @@ describe('Release edit/new component', () => { }; beforeEach(() => { + global.jsdom.reconfigure({ url: TEST_HOST }); + mock = new MockAdapter(axios); gon.api_version = 'v4'; @@ -146,22 +150,33 @@ describe('Release edit/new component', () => { }); }); - describe(`when the URL contains a "${BACK_URL_PARAM}" parameter`, () => { - const backUrl = 'https://example.gitlab.com/back/url'; - - beforeEach(async () => { - commonUtils.getParameterByName = jest - .fn() - .mockImplementation((paramToGet) => ({ [BACK_URL_PARAM]: backUrl }[paramToGet])); + // eslint-disable-next-line no-script-url + const xssBackUrl = 'javascript:alert(1)'; + describe.each` + backUrl | expectedHref + ${`${TEST_HOST}/back/url`} | ${`${TEST_HOST}/back/url`} + ${`/back/url?page=2`} | ${`/back/url?page=2`} + ${`back/url?page=3`} | ${`back/url?page=3`} + ${'http://phishing.test/back/url'} | ${releasesPagePath} + ${'//phishing.test/back/url'} | ${releasesPagePath} + ${xssBackUrl} | ${releasesPagePath} + `( + `when the URL contains a "${BACK_URL_PARAM}=$backUrl" parameter`, + ({ backUrl, expectedHref }) => { + beforeEach(async () => { + global.jsdom.reconfigure({ + url: `${TEST_HOST}?${BACK_URL_PARAM}=${encodeURIComponent(backUrl)}`, + }); - await factory(); - }); + await factory(); + }); - it('renders a "Cancel" button with an href pointing to the main Releases page', () => { - const cancelButton = wrapper.find('.js-cancel-button'); - expect(cancelButton.attributes().href).toBe(backUrl); - }); - }); + it(`renders a "Cancel" button with an href pointing to ${expectedHref}`, () => { + const cancelButton = wrapper.find('.js-cancel-button'); + expect(cancelButton.attributes().href).toBe(expectedHref); + }); + }, + ); describe('when creating a new release', () => { beforeEach(async () => { diff --git a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb index 03a9b9bd21e..d401c42fed7 100644 --- a/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb +++ b/spec/lib/gitlab/diff/file_collection/merge_request_diff_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiff do it 'does not highlight files marked as undiffable in .gitattributes' do allow_next_instance_of(Gitlab::Diff::File) do |instance| - allow(instance).to receive(:diffable?).and_return(false) + allow(instance).to receive(:diffable_by_attribute?).and_return(false) end expect_next_instance_of(Gitlab::Diff::File) do |instance| diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 78be89c449b..1800d2d6b60 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -186,26 +186,46 @@ RSpec.describe Gitlab::Diff::File do end describe '#diffable?' do - let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') } - let(:diffs) { commit.diffs } + context 'when attributes exist' do + let(:commit) { project.commit('1a0b36b3cdad1d2ee32457c102a8c0b7056fa863') } + let(:diffs) { commit.diffs } - before do - info_dir_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do - File.join(project.repository.path_to_repo, 'info') + before do + info_dir_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do + File.join(project.repository.path_to_repo, 'info') + end + + FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path) + File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n") end - FileUtils.mkdir(info_dir_path) unless File.exist?(info_dir_path) - File.write(File.join(info_dir_path, 'attributes'), "*.md -diff\n") + it "returns true for files that do not have attributes" do + diff_file = diffs.diff_file_with_new_path('LICENSE') + expect(diff_file.diffable?).to be_truthy + end + + it "returns false for files that have been marked as not being diffable in attributes" do + diff_file = diffs.diff_file_with_new_path('README.md') + expect(diff_file.diffable?).to be_falsey + end end - it "returns true for files that do not have attributes" do - diff_file = diffs.diff_file_with_new_path('LICENSE') - expect(diff_file.diffable?).to be_truthy + context 'when the text has binary notice' do + let(:commit) { project.commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('VERSION') } + + it "returns false" do + expect(diff_file.diffable?).to be_falsey + end end - it "returns false for files that have been marked as not being diffable in attributes" do - diff_file = diffs.diff_file_with_new_path('README.md') - expect(diff_file.diffable?).to be_falsey + context 'when the content is binary' do + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('files/images/6049019_460s.jpg') } + + it "returns true" do + expect(diff_file.diffable?).to be_truthy + end end end @@ -729,6 +749,18 @@ RSpec.describe Gitlab::Diff::File do end end + context 'when the the encoding of the file is unsupported' do + let(:commit) { project.commit('f05a98786e4274708e1fa118c7ad3a29d1d1b9a3') } + let(:diff_file) { commit.diffs.diff_file_with_new_path('VERSION') } + + it 'returns a Not Diffable viewer' do + expect(diff_file.simple_viewer).to be_a(DiffViewer::NotDiffable) + end + + it { expect(diff_file.highlighted_diff_lines).to eq([]) } + it { expect(diff_file.parallel_diff_lines).to eq([]) } + end + describe '#diff_hunk' do context 'when first line is a match' do let(:raw_diff) do diff --git a/spec/lib/gitlab/diff/parser_spec.rb b/spec/lib/gitlab/diff/parser_spec.rb index 7448ae0b2ea..c8069f82f04 100644 --- a/spec/lib/gitlab/diff/parser_spec.rb +++ b/spec/lib/gitlab/diff/parser_spec.rb @@ -146,6 +146,16 @@ eos it { expect(parser.parse(nil)).to eq([]) } end + context 'when it is a binary notice' do + let(:diff) do + <<~END + Binary files a/test and b/test differ + END + end + + it { expect(parser.parse(diff.each_line)).to eq([]) } + end + describe 'tolerates special diff markers in a content' do it "counts lines correctly" do diff = <<~END diff --git a/spec/lib/gitlab/http_spec.rb b/spec/lib/gitlab/http_spec.rb index 308f7f46251..71e80de9f89 100644 --- a/spec/lib/gitlab/http_spec.rb +++ b/spec/lib/gitlab/http_spec.rb @@ -27,6 +27,47 @@ RSpec.describe Gitlab::HTTP do end end + context 'when reading the response is too slow' do + before do + stub_const("#{described_class}::DEFAULT_READ_TOTAL_TIMEOUT", 0.001.seconds) + + WebMock.stub_request(:post, /.*/).to_return do |request| + sleep 0.002.seconds + { body: 'I\m slow', status: 200 } + end + end + + let(:options) { {} } + + subject(:request_slow_responder) { described_class.post('http://example.org', **options) } + + specify do + expect { request_slow_responder }.not_to raise_error + end + + context 'with use_read_total_timeout option' do + let(:options) { { use_read_total_timeout: true } } + + it 'raises a timeout error' do + expect { request_slow_responder }.to raise_error(Gitlab::HTTP::ReadTotalTimeout, /Request timed out after ?([0-9]*[.])?[0-9]+ seconds/) + end + + context 'and timeout option' do + let(:options) { { use_read_total_timeout: true, timeout: 10.seconds } } + + it 'overrides the default timeout when timeout option is present' do + expect { request_slow_responder }.not_to raise_error + end + end + end + end + + it 'calls a block' do + WebMock.stub_request(:post, /.*/) + + expect { |b| described_class.post('http://example.org', &b) }.to yield_with_args + end + describe 'allow_local_requests_from_web_hooks_and_services is' do before do WebMock.stub_request(:get, /.*/).to_return(status: 200, body: 'Success') diff --git a/spec/models/audit_event_spec.rb b/spec/models/audit_event_spec.rb index 5c87c2e68db..bc603bc5ab6 100644 --- a/spec/models/audit_event_spec.rb +++ b/spec/models/audit_event_spec.rb @@ -3,9 +3,6 @@ require 'spec_helper' RSpec.describe AuditEvent do - let_it_be(:audit_event) { create(:project_audit_event) } - subject { audit_event } - describe 'validations' do include_examples 'validates IP address' do let(:attribute) { :ip_address } @@ -13,6 +10,15 @@ RSpec.describe AuditEvent do end end + it 'sanitizes custom_message in the details hash' do + audit_event = create(:project_audit_event, details: { target_id: 678, custom_message: '<strong>Arnold</strong>' }) + + expect(audit_event.details).to include( + target_id: 678, + custom_message: 'Arnold' + ) + end + describe '#as_json' do context 'ip_address' do subject { build(:group_audit_event, ip_address: '192.168.1.1').as_json } diff --git a/spec/models/protected_branch/push_access_level_spec.rb b/spec/models/protected_branch/push_access_level_spec.rb index 17a589f0485..fa84cd660cb 100644 --- a/spec/models/protected_branch/push_access_level_spec.rb +++ b/spec/models/protected_branch/push_access_level_spec.rb @@ -44,7 +44,7 @@ RSpec.describe ProtectedBranch::PushAccessLevel do let(:can_push) { true } before_all do - project.add_guest(user) + project.add_maintainer(user) end context 'when this push_access_level is tied to a deploy key' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 05610057363..bce3083ab94 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -376,6 +376,19 @@ RSpec.describe User do expect(user.errors.full_messages).to eq(['Username has already been taken']) end end + + it 'validates format' do + Mime::EXTENSION_LOOKUP.keys.each do |type| + user = build(:user, username: "test.#{type}") + + expect(user).not_to be_valid + expect(user.errors.full_messages).to include('Username ending with MIME type format is not allowed.') + end + end + + it 'validates format on updated record' do + expect(create(:user).update(username: 'profile.html')).to be_falsey + end end it 'has a DB-level NOT NULL constraint on projects_limit' do @@ -2877,7 +2890,7 @@ RSpec.describe User do end describe '#sanitize_attrs' do - let(:user) { build(:user, name: 'test & user', skype: 'test&user') } + let(:user) { build(:user, name: 'test <& user', skype: 'test&user') } it 'encodes HTML entities in the Skype attribute' do expect { user.sanitize_attrs }.to change { user.skype }.to('test&user') @@ -2886,6 +2899,25 @@ RSpec.describe User do it 'does not encode HTML entities in the name attribute' do expect { user.sanitize_attrs }.not_to change { user.name } end + + it 'sanitizes attr from html tags' do + user = create(:user, name: '<a href="//example.com">Test<a>', twitter: '<a href="//evil.com">https://twitter.com<a>') + + expect(user.name).to eq('Test') + expect(user.twitter).to eq('https://twitter.com') + end + + it 'sanitizes attr from js scripts' do + user = create(:user, name: '<script>alert("Test")</script>') + + expect(user.name).to eq("alert(\"Test\")") + end + + it 'sanitizes attr from iframe scripts' do + user = create(:user, name: 'User"><iframe src=javascript:alert()><iframe>') + + expect(user.name).to eq('User">') + end end describe '#starred?' do diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index 441cb9e653b..e83242cba40 100644 --- a/spec/policies/global_policy_spec.rb +++ b/spec/policies/global_policy_spec.rb @@ -239,6 +239,14 @@ RSpec.describe GlobalPolicy do it { is_expected.not_to be_allowed(:access_api) } end + context 'with a deactivated user' do + before do + current_user.deactivate! + end + + it { is_expected.not_to be_allowed(:access_api) } + end + context 'user with expired password' do before do current_user.update!(password_expires_at: 2.minutes.ago) diff --git a/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb index cf113c30ba6..be8b767bfd4 100644 --- a/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb +++ b/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe 'Setting assignees of a merge request' do +RSpec.describe 'Setting assignees of a merge request', :clean_gitlab_redis_shared_state do include GraphqlHelpers let_it_be(:project) { create(:project, :repository) } @@ -46,6 +46,8 @@ RSpec.describe 'Setting assignees of a merge request' do end def run_mutation! + post_graphql_mutation(mutation, current_user: current_user) # warm-up + recorder = ActiveRecord::QueryRecorder.new do post_graphql_mutation(mutation, current_user: current_user) end diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index a336d74b135..463fca43cb5 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -6,6 +6,9 @@ RSpec.describe 'GraphQL' do include AfterNextHelpers let(:query) { graphql_query_for('echo', text: 'Hello world') } + let(:mutation) { 'mutation { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + + let_it_be(:user) { create(:user) } describe 'logging' do shared_examples 'logging a graphql query' do @@ -70,6 +73,139 @@ RSpec.describe 'GraphQL' do end end + context 'when executing mutations' do + let(:mutation_with_variables) do + <<~GQL + mutation($a: String!, $b: String!) { + echoCreate(input: { messages: [$a, $b] }) { echoes } + } + GQL + end + + context 'with POST' do + it 'succeeds' do + post_graphql(mutation, current_user: user) + + expect(graphql_data_at(:echo_create, :echoes)).to eq %w[hello world] + end + + context 'with variables' do + it 'succeeds' do + post_graphql(mutation_with_variables, current_user: user, variables: { a: 'Yo', b: 'there' }) + + expect(graphql_data_at(:echo_create, :echoes)).to eq %w[Yo there] + end + end + end + + context 'with GET' do + it 'fails' do + get_graphql(mutation, current_user: user) + + expect(graphql_errors).to include(a_hash_including('message' => /Mutations are forbidden/)) + end + + context 'with variables' do + it 'fails' do + get_graphql(mutation_with_variables, current_user: user, variables: { a: 'Yo', b: 'there' }) + + expect(graphql_errors).to include(a_hash_including('message' => /Mutations are forbidden/)) + end + end + end + end + + context 'when executing queries' do + context 'with POST' do + it 'succeeds' do + post_graphql(query, current_user: user) + + expect(graphql_data_at(:echo)).to include 'Hello world' + end + end + + context 'with GET' do + it 'succeeds' do + get_graphql(query, current_user: user) + + expect(graphql_data_at(:echo)).to include 'Hello world' + end + end + end + + context 'when selecting a query by operation name' do + let(:query) { "query A #{graphql_query_for('echo', text: 'Hello world')}" } + let(:mutation) { 'mutation B { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + + let(:combined) { [query, mutation].join("\n\n") } + + context 'with POST' do + it 'succeeds when selecting the query' do + post_graphql(combined, current_user: user, params: { operationName: 'A' }) + + resp = json_response + + expect(resp.dig('data', 'echo')).to include 'Hello world' + end + + it 'succeeds when selecting the mutation' do + post_graphql(combined, current_user: user, params: { operationName: 'B' }) + + resp = json_response + + expect(resp.dig('data', 'echoCreate', 'echoes')).to eq %w[hello world] + end + end + + context 'with GET' do + it 'succeeds when selecting the query' do + get_graphql(combined, current_user: user, params: { operationName: 'A' }) + + resp = json_response + + expect(resp.dig('data', 'echo')).to include 'Hello world' + end + + it 'fails when selecting the mutation' do + get_graphql(combined, current_user: user, params: { operationName: 'B' }) + + resp = json_response + + expect(resp.dig('errors', 0, 'message')).to include "Mutations are forbidden" + end + end + end + + context 'when batching mutations and queries' do + let(:batched) do + [ + { query: "query A #{graphql_query_for('echo', text: 'Hello world')}" }, + { query: 'mutation B { echoCreate(input: { messages: ["hello", "world"] }) { echoes } }' } + ] + end + + context 'with POST' do + it 'succeeds' do + post_multiplex(batched, current_user: user) + + resp = json_response + + expect(resp.dig(0, 'data', 'echo')).to include 'Hello world' + expect(resp.dig(1, 'data', 'echoCreate', 'echoes')).to eq %w[hello world] + end + end + + context 'with GET' do + it 'fails with a helpful error message' do + get_multiplex(batched, current_user: user) + + resp = json_response + + expect(resp.dig('errors', 0, 'message')).to include "Mutations are forbidden" + end + end + end + context 'with invalid variables' do it 'returns an error' do post_graphql(query, variables: "This is not JSON") @@ -80,8 +216,6 @@ RSpec.describe 'GraphQL' do end describe 'authentication', :allow_forgery_protection do - let(:user) { create(:user) } - it 'allows access to public data without authentication' do post_graphql(query) @@ -109,11 +243,9 @@ RSpec.describe 'GraphQL' do context 'with token authentication' do let(:token) { create(:personal_access_token) } - before do + it 'authenticates users with a PAT' do stub_authentication_activity_metrics(debug: false) - end - it 'authenticates users with a PAT' do expect(authentication_metrics) .to increment(:user_authenticated_counter) .and increment(:user_session_override_counter) @@ -124,6 +256,14 @@ RSpec.describe 'GraphQL' do expect(graphql_data['echo']).to eq("\"#{token.user.username}\" says: Hello world") end + it 'prevents access by deactived users' do + token.user.deactivate! + + post_graphql(query, headers: { 'PRIVATE-TOKEN' => token.token }) + + expect(graphql_errors).to include({ 'message' => /API not accessible/ }) + end + context 'when the personal access token has no api scope' do it 'does not log the user in' do token.update!(scopes: [:read_user]) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 7f804186bc7..dc7ee4098fe 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -56,7 +56,7 @@ RSpec.describe API::Projects do let_it_be(:project, reload: true) { create(:project, :repository, namespace: user.namespace) } let_it_be(:project2, reload: true) { create(:project, namespace: user.namespace) } let_it_be(:project_member) { create(:project_member, :developer, user: user3, project: project) } - let_it_be(:user4) { create(:user, username: 'user.with.dot') } + let_it_be(:user4) { create(:user, username: 'user.withdot') } let_it_be(:project3, reload: true) do create(:project, :private, diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 71fdd986f20..629576134a0 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe API::Users do let_it_be(:admin) { create(:admin) } - let_it_be(:user, reload: true) { create(:user, username: 'user.with.dot') } + let_it_be(:user, reload: true) { create(:user, username: 'user.withdot') } let_it_be(:key) { create(:key, user: user) } let_it_be(:gpg_key) { create(:gpg_key, user: user) } let_it_be(:email) { create(:email, user: user) } diff --git a/spec/requests/ide_controller_spec.rb b/spec/requests/ide_controller_spec.rb index 9b0d8dcd828..4bf1e43ba40 100644 --- a/spec/requests/ide_controller_spec.rb +++ b/spec/requests/ide_controller_spec.rb @@ -3,7 +3,14 @@ require 'spec_helper' RSpec.describe IdeController do - let_it_be(:project) { create(:project, :public) } + let_it_be(:reporter) { create(:user) } + + let_it_be(:project) do + create(:project, :private).tap do |p| + p.add_reporter(reporter) + end + end + let_it_be(:creator) { project.creator } let_it_be(:other_user) { create(:user) } @@ -14,48 +21,62 @@ RSpec.describe IdeController do sign_in(user) end - it 'increases the views counter' do - expect(Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_views_count) - - get ide_url - end - describe '#index', :aggregate_failures do subject { get route } - shared_examples 'user cannot push code' do - include ProjectForksHelper - - let(:user) { other_user } + shared_examples 'user access rights check' do + context 'user can read project' do + it 'increases the views counter' do + expect(Gitlab::UsageDataCounters::WebIdeCounter).to receive(:increment_views_count) - context 'when user does not have fork' do - it 'instantiates fork_info instance var with fork_path and return 200' do subject - - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:project)).to eq project - expect(assigns(:fork_info)).to eq({ fork_path: controller.helpers.ide_fork_and_edit_path(project, branch, '', with_notice: false) }) end - it 'has nil fork_info if user cannot fork' do - project.project_feature.update!(forking_access_level: ProjectFeature::DISABLED) + context 'user can read project but cannot push code' do + include ProjectForksHelper - subject + let(:user) { reporter } - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:fork_info)).to be_nil + context 'when user does not have fork' do + it 'instantiates fork_info instance var with fork_path and returns 200' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:project)).to eq project + expect(assigns(:fork_info)).to eq({ fork_path: controller.helpers.ide_fork_and_edit_path(project, branch, '', with_notice: false) }) + end + + it 'has nil fork_info if user cannot fork' do + project.project_feature.update!(forking_access_level: ProjectFeature::DISABLED) + + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:fork_info)).to be_nil + end + end + + context 'when user has fork' do + let!(:fork) { fork_project(project, user, repository: true, namespace: user.namespace) } + + it 'instantiates fork_info instance var with ide_path and returns 200' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(assigns(:project)).to eq project + expect(assigns(:fork_info)).to eq({ ide_path: controller.helpers.ide_edit_path(fork, branch, '') }) + end + end end end - context 'when user has fork' do - let!(:fork) { fork_project(project, user, repository: true, namespace: user.namespace) } + context 'user cannot read project' do + let(:user) { other_user } - it 'instantiates fork_info instance var with ide_path and return 200' do + it 'returns 404' do subject - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:project)).to eq project - expect(assigns(:fork_info)).to eq({ ide_path: controller.helpers.ide_edit_path(fork, branch, '') }) + expect(response).to have_gitlab_http_status(:not_found) end end end @@ -63,37 +84,27 @@ RSpec.describe IdeController do context '/-/ide' do let(:route) { '/-/ide' } - it 'does not instantiate any instance var and return 200' do + it 'returns 404' do subject - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:project)).to be_nil - expect(assigns(:branch)).to be_nil - expect(assigns(:path)).to be_nil - expect(assigns(:merge_request)).to be_nil - expect(assigns(:fork_info)).to be_nil + expect(response).to have_gitlab_http_status(:not_found) end end context '/-/ide/project' do let(:route) { '/-/ide/project' } - it 'does not instantiate any instance var and return 200' do + it 'returns 404' do subject - expect(response).to have_gitlab_http_status(:ok) - expect(assigns(:project)).to be_nil - expect(assigns(:branch)).to be_nil - expect(assigns(:path)).to be_nil - expect(assigns(:merge_request)).to be_nil - expect(assigns(:fork_info)).to be_nil + expect(response).to have_gitlab_http_status(:not_found) end end context '/-/ide/project/:project' do let(:route) { "/-/ide/project/#{project.full_path}" } - it 'instantiates project instance var and return 200' do + it 'instantiates project instance var and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -104,13 +115,13 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' %w(edit blob tree).each do |action| context "/-/ide/project/:project/#{action}" do let(:route) { "/-/ide/project/#{project.full_path}/#{action}" } - it 'instantiates project instance var and return 200' do + it 'instantiates project instance var and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -121,13 +132,13 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' context "/-/ide/project/:project/#{action}/:branch" do let(:branch) { 'master' } let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}" } - it 'instantiates project and branch instance vars and return 200' do + it 'instantiates project and branch instance vars and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -138,13 +149,13 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' context "/-/ide/project/:project/#{action}/:branch/-" do let(:branch) { 'branch/slash' } let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}/-" } - it 'instantiates project and branch instance vars and return 200' do + it 'instantiates project and branch instance vars and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -155,13 +166,13 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' context "/-/ide/project/:project/#{action}/:branch/-/:path" do let(:branch) { 'master' } let(:route) { "/-/ide/project/#{project.full_path}/#{action}/#{branch}/-/foo/.bar" } - it 'instantiates project, branch, and path instance vars and return 200' do + it 'instantiates project, branch, and path instance vars and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -172,7 +183,7 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' end end end @@ -184,7 +195,7 @@ RSpec.describe IdeController do let(:route) { "/-/ide/project/#{project.full_path}/merge_requests/#{merge_request.id}" } - it 'instantiates project and merge_request instance vars and return 200' do + it 'instantiates project and merge_request instance vars and returns 200' do subject expect(response).to have_gitlab_http_status(:ok) @@ -195,7 +206,7 @@ RSpec.describe IdeController do expect(assigns(:fork_info)).to be_nil end - it_behaves_like 'user cannot push code' + it_behaves_like 'user access rights check' end end end diff --git a/spec/services/feature_flags/create_service_spec.rb b/spec/services/feature_flags/create_service_spec.rb index 2e0c162ebc1..4eb2b25fb64 100644 --- a/spec/services/feature_flags/create_service_spec.rb +++ b/spec/services/feature_flags/create_service_spec.rb @@ -68,12 +68,12 @@ RSpec.describe FeatureFlags::CreateService do end it 'creates audit event' do - expected_message = 'Created feature flag <strong>feature_flag</strong> '\ - 'with description <strong>"description"</strong>. '\ - 'Created rule <strong>*</strong> and set it as <strong>active</strong> '\ - 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>. '\ - 'Created rule <strong>production</strong> and set it as <strong>inactive</strong> '\ - 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.' + expected_message = 'Created feature flag feature_flag '\ + 'with description "description". '\ + 'Created rule * and set it as active '\ + 'with strategies [{"name"=>"default", "parameters"=>{}}]. '\ + 'Created rule production and set it as inactive '\ + 'with strategies [{"name"=>"default", "parameters"=>{}}].' expect { subject }.to change { AuditEvent.count }.by(1) expect(AuditEvent.last.details[:custom_message]).to eq(expected_message) diff --git a/spec/services/feature_flags/destroy_service_spec.rb b/spec/services/feature_flags/destroy_service_spec.rb index ee30474873c..d3796ef6b4d 100644 --- a/spec/services/feature_flags/destroy_service_spec.rb +++ b/spec/services/feature_flags/destroy_service_spec.rb @@ -33,7 +33,7 @@ RSpec.describe FeatureFlags::DestroyService do it 'creates audit log' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(audit_event_message).to eq("Deleted feature flag <strong>#{feature_flag.name}</strong>.") + expect(audit_event_message).to eq("Deleted feature flag #{feature_flag.name}.") end context 'when user is reporter' do diff --git a/spec/services/feature_flags/update_service_spec.rb b/spec/services/feature_flags/update_service_spec.rb index 1a127a0d472..a9e9ea78d42 100644 --- a/spec/services/feature_flags/update_service_spec.rb +++ b/spec/services/feature_flags/update_service_spec.rb @@ -38,9 +38,9 @@ RSpec.describe FeatureFlags::UpdateService do expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to( - eq("Updated feature flag <strong>new_name</strong>. "\ - "Updated name from <strong>\"#{name_was}\"</strong> "\ - "to <strong>\"new_name\"</strong>.") + eq("Updated feature flag new_name. "\ + "Updated name from \"#{name_was}\" "\ + "to \"new_name\".") ) end @@ -94,8 +94,8 @@ RSpec.describe FeatureFlags::UpdateService do it 'creates audit event with changed description' do expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to( - include("Updated description from <strong>\"\"</strong>"\ - " to <strong>\"new description\"</strong>.") + include("Updated description from \"\""\ + " to \"new description\".") ) end end @@ -110,7 +110,7 @@ RSpec.describe FeatureFlags::UpdateService do it 'creates audit event about changing active state' do expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to( - include('Updated active from <strong>"true"</strong> to <strong>"false"</strong>.') + include('Updated active from "true" to "false".') ) end @@ -132,8 +132,8 @@ RSpec.describe FeatureFlags::UpdateService do it 'creates audit event about changing active state' do expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to( - include("Updated rule <strong>*</strong> active state "\ - "from <strong>true</strong> to <strong>false</strong>.") + include("Updated rule * active state "\ + "from true to false.") ) end end @@ -149,8 +149,8 @@ RSpec.describe FeatureFlags::UpdateService do it 'creates audit event with changed name' do expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to( - include("Updated rule <strong>staging</strong> environment scope "\ - "from <strong>review</strong> to <strong>staging</strong>.") + include("Updated rule staging environment scope "\ + "from review to staging.") ) end @@ -185,7 +185,7 @@ RSpec.describe FeatureFlags::UpdateService do it 'creates audit event with deleted scope' do expect { subject }.to change { AuditEvent.count }.by(1) - expect(audit_event_message).to include("Deleted rule <strong>review</strong>.") + expect(audit_event_message).to include("Deleted rule review.") end context 'when scope can not be deleted' do @@ -210,8 +210,8 @@ RSpec.describe FeatureFlags::UpdateService do end it 'creates audit event with new scope' do - expected = 'Created rule <strong>review</strong> and set it as <strong>active</strong> '\ - 'with strategies <strong>[{"name"=>"default", "parameters"=>{}}]</strong>.' + expected = 'Created rule review and set it as active '\ + 'with strategies [{"name"=>"default", "parameters"=>{}}].' subject @@ -260,7 +260,7 @@ RSpec.describe FeatureFlags::UpdateService do end it 'creates an audit event' do - expected = %r{Updated rule <strong>sandbox</strong> strategies from <strong>.*</strong> to <strong>.*</strong>.} + expected = %r{Updated rule sandbox strategies from .* to .*.} expect { subject }.to change { AuditEvent.count }.by(1) expect(audit_event_message).to match(expected) diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 276656656ec..d710e4a777f 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -184,14 +184,6 @@ RSpec.describe Projects::ForkService do end end - context 'GitLab CI is enabled' do - it "forks and enables CI for fork" do - @from_project.enable_ci - @to_project = fork_project(@from_project, @to_user, using_service: true) - expect(@to_project.builds_enabled?).to be_truthy - end - end - context "CI/CD settings" do let(:to_project) { fork_project(@from_project, @to_user, using_service: true) } @@ -366,6 +358,19 @@ RSpec.describe Projects::ForkService do expect(forked_project.visibility_level).to eq(Gitlab::VisibilityLevel::PRIVATE) end + + it 'copies project features visibility settings to the fork', :aggregate_failures do + attrs = ProjectFeature::FEATURES.to_h do |f| + ["#{f}_access_level", ProjectFeature::PRIVATE] + end + + public_project.project_feature.update!(attrs) + + user = create(:user, developer_projects: [public_project]) + forked_project = described_class.new(public_project, user).execute + + expect(forked_project.project_feature.slice(attrs.keys)).to eq(attrs) + end end end diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index 5dc6945ec5e..16a7f403e36 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -396,8 +396,13 @@ module GraphqlHelpers post api('/', current_user, version: 'graphql'), params: { _json: queries }, headers: headers end - def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}) - params = { query: query, variables: serialize_variables(variables) } + def get_multiplex(queries, current_user: nil, headers: {}) + path = "/?#{queries.to_query('_json')}" + get api(path, current_user, version: 'graphql'), headers: headers + end + + def post_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}, params: {}) + params = params.merge(query: query, variables: serialize_variables(variables)) post api('/', current_user, version: 'graphql', **token), params: params, headers: headers return unless graphql_errors @@ -406,6 +411,18 @@ module GraphqlHelpers expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error')) end + def get_graphql(query, current_user: nil, variables: nil, headers: {}, token: {}, params: {}) + vars = "variables=#{CGI.escape(serialize_variables(variables))}" if variables + params = params.to_a.map { |k, v| v.to_query(k) } + path = ["/?query=#{CGI.escape(query)}", vars, *params].join('&') + get api(path, current_user, version: 'graphql', **token), headers: headers + + return unless graphql_errors + + # Errors are acceptable, but not this one: + expect(graphql_errors).not_to include(a_hash_including('message' => 'Internal server error')) + end + def post_graphql_mutation(mutation, current_user: nil, token: {}) post_graphql(mutation.query, current_user: current_user, |