diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-09-07 15:08:27 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-09-07 15:08:27 +0300 |
commit | a8653790086d284cecffdc35892bb422cd6c9a7b (patch) | |
tree | 8d1f4dc69026a42a47b1026eef2566c7461a52fe /spec | |
parent | 444f662b8d8cbe47a8f3fa1db6ed926d64f3def3 (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'spec')
58 files changed, 2013 insertions, 255 deletions
diff --git a/spec/controllers/admin/sessions_controller_spec.rb b/spec/controllers/admin/sessions_controller_spec.rb index 82366cc6952..35982e57034 100644 --- a/spec/controllers/admin/sessions_controller_spec.rb +++ b/spec/controllers/admin/sessions_controller_spec.rb @@ -220,10 +220,8 @@ RSpec.describe Admin::SessionsController, :do_not_mock_admin_mode do end end - context 'when using two-factor authentication via U2F' do - let(:user) { create(:admin, :two_factor_via_u2f) } - - def authenticate_2fa_u2f(user_params) + shared_examples 'when using two-factor authentication via hardware device' do + def authenticate_2fa(user_params) post(:create, params: { user: user_params }, session: { otp_user_id: user.id }) end @@ -239,14 +237,18 @@ RSpec.describe Admin::SessionsController, :do_not_mock_admin_mode do end it 'can login with valid auth' do + # we can stub both without an differentiation between webauthn / u2f + # as these not interfere with each other und this saves us passing aroud + # parameters allow(U2fRegistration).to receive(:authenticate).and_return(true) + allow_any_instance_of(Webauthn::AuthenticateService).to receive(:execute).and_return(true) expect(controller.current_user_mode.admin_mode?).to be(false) controller.store_location_for(:redirect, admin_root_path) controller.current_user_mode.request_admin_mode! - authenticate_2fa_u2f(login: user.username, device_response: '{}') + authenticate_2fa(login: user.username, device_response: '{}') expect(response).to redirect_to admin_root_path expect(controller.current_user_mode.admin_mode?).to be(true) @@ -254,16 +256,33 @@ RSpec.describe Admin::SessionsController, :do_not_mock_admin_mode do it 'cannot login with invalid auth' do allow(U2fRegistration).to receive(:authenticate).and_return(false) + allow_any_instance_of(Webauthn::AuthenticateService).to receive(:execute).and_return(false) expect(controller.current_user_mode.admin_mode?).to be(false) controller.current_user_mode.request_admin_mode! - authenticate_2fa_u2f(login: user.username, device_response: '{}') + authenticate_2fa(login: user.username, device_response: '{}') expect(response).to render_template('admin/sessions/two_factor') expect(controller.current_user_mode.admin_mode?).to be(false) end end + + context 'when using two-factor authentication via U2F' do + it_behaves_like 'when using two-factor authentication via hardware device' do + let(:user) { create(:admin, :two_factor_via_u2f) } + + before do + stub_feature_flags(webauthn: false) + end + end + end + + context 'when using two-factor authentication via WebAuthn' do + it_behaves_like 'when using two-factor authentication via hardware device' do + let(:user) { create(:admin, :two_factor_via_webauthn) } + end + end end end diff --git a/spec/controllers/profiles/webauthn_registrations_controller_spec.rb b/spec/controllers/profiles/webauthn_registrations_controller_spec.rb new file mode 100644 index 00000000000..0c475039963 --- /dev/null +++ b/spec/controllers/profiles/webauthn_registrations_controller_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Profiles::WebauthnRegistrationsController do + let(:user) { create(:user, :two_factor_via_webauthn) } + + before do + sign_in(user) + end + + describe '#destroy' do + it 'deletes the given webauthn registration' do + registration_to_delete = user.webauthn_registrations.first + + expect { delete :destroy, params: { id: registration_to_delete.id } }.to change { user.webauthn_registrations.count }.by(-1) + expect(response).to be_redirect + end + end +end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index acec1cf22fd..2c3815b36af 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -416,6 +416,10 @@ RSpec.describe SessionsController do post(:create, params: { user: user_params }, session: { otp_user_id: user.id }) end + before do + stub_feature_flags(webauthn: false) + end + context 'remember_me field' do it 'sets a remember_user_token cookie when enabled' do allow(U2fRegistration).to receive(:authenticate).and_return(true) diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 7e121b10632..d4b2ac5f056 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -81,6 +81,14 @@ FactoryBot.define do end end + trait :two_factor_via_webauthn do + transient { registrations_count { 5 } } + + after(:create) do |user, evaluator| + create_list(:webauthn_registration, evaluator.registrations_count, user: user) + end + end + trait :readme do project_view { :readme } end diff --git a/spec/factories/webauthn_registrations.rb b/spec/factories/webauthn_registrations.rb new file mode 100644 index 00000000000..ac803885244 --- /dev/null +++ b/spec/factories/webauthn_registrations.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :webauthn_registration do + credential_xid { SecureRandom.base64(88) } + public_key { SecureRandom.base64(103) } + name { FFaker::BaconIpsum.characters(10) } + counter { 1 } + user + end +end diff --git a/spec/features/u2f_spec.rb b/spec/features/u2f_spec.rb index 8dbedc0a7ee..5762a54a717 100644 --- a/spec/features/u2f_spec.rb +++ b/spec/features/u2f_spec.rb @@ -3,22 +3,14 @@ require 'spec_helper' RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :js do - def manage_two_factor_authentication - click_on 'Manage two-factor authentication' - expect(page).to have_content("Set up new U2F device") - wait_for_requests - end + include Spec::Support::Helpers::Features::TwoFactorHelpers - def register_u2f_device(u2f_device = nil, name: 'My device') - u2f_device ||= FakeU2fDevice.new(page, name) - u2f_device.respond_to_u2f_registration - click_on 'Set up new U2F device' - expect(page).to have_content('Your device was successfully set up') - fill_in "Pick a name", with: name - click_on 'Register U2F device' - u2f_device + before do + stub_feature_flags(webauthn: false) end + it_behaves_like 'hardware device for 2fa', 'U2F' + describe "registration" do let(:user) { create(:user) } @@ -27,31 +19,7 @@ RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :j user.update_attribute(:otp_required_for_login, true) end - describe 'when 2FA via OTP is disabled' do - before do - user.update_attribute(:otp_required_for_login, false) - end - - it 'does not allow registering a new device' do - visit profile_account_path - click_on 'Enable two-factor authentication' - - expect(page).to have_button('Set up new U2F device', disabled: true) - end - end - describe 'when 2FA via OTP is enabled' do - it 'allows registering a new device with a name' do - visit profile_account_path - manage_two_factor_authentication - expect(page).to have_content("You've already enabled two-factor authentication using one time password authenticators") - - u2f_device = register_u2f_device - - expect(page).to have_content(u2f_device.name) - expect(page).to have_content('Your U2F device was registered') - end - it 'allows registering more than one device' do visit profile_account_path @@ -68,21 +36,6 @@ RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :j expect(page).to have_content(second_device.name) expect(U2fRegistration.count).to eq(2) end - - it 'allows deleting a device' do - visit profile_account_path - manage_two_factor_authentication - expect(page).to have_content("You've already enabled two-factor authentication using one time password authenticators") - - first_u2f_device = register_u2f_device - second_u2f_device = register_u2f_device(name: 'My other device') - - accept_confirm { click_on "Delete", match: :first } - - expect(page).to have_content('Successfully deleted') - expect(page.body).not_to match(first_u2f_device.name) - expect(page).to have_content(second_u2f_device.name) - end end it 'allows the same device to be registered for multiple users' do @@ -111,9 +64,9 @@ RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :j # Have the "u2f device" respond with bad data page.execute_script("u2f.register = function(_,_,_,callback) { callback('bad response'); };") - click_on 'Set up new U2F device' + click_on 'Set up new device' expect(page).to have_content('Your device was successfully set up') - click_on 'Register U2F device' + click_on 'Register device' expect(U2fRegistration.count).to eq(0) expect(page).to have_content("The form contains the following error") @@ -126,9 +79,9 @@ RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :j # Failed registration page.execute_script("u2f.register = function(_,_,_,callback) { callback('bad response'); };") - click_on 'Set up new U2F device' + click_on 'Set up new device' expect(page).to have_content('Your device was successfully set up') - click_on 'Register U2F device' + click_on 'Register device' expect(page).to have_content("The form contains the following error") # Successful registration @@ -228,12 +181,12 @@ RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :j user = gitlab_sign_in(:user) user.update_attribute(:otp_required_for_login, true) visit profile_two_factor_auth_path - expect(page).to have_content("Your U2F device needs to be set up.") + expect(page).to have_content("Your device needs to be set up.") first_device = register_u2f_device # Register second device visit profile_two_factor_auth_path - expect(page).to have_content("Your U2F device needs to be set up.") + expect(page).to have_content("Your device needs to be set up.") second_device = register_u2f_device(name: 'My other device') gitlab_sign_out @@ -249,50 +202,4 @@ RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :j end end end - - describe 'fallback code authentication' do - let(:user) { create(:user) } - - def assert_fallback_ui(page) - expect(page).to have_button('Verify code') - expect(page).to have_css('#user_otp_attempt') - expect(page).not_to have_link('Sign in via 2FA code') - expect(page).not_to have_css('#js-authenticate-token-2fa') - end - - before do - # Register and logout - gitlab_sign_in(user) - user.update_attribute(:otp_required_for_login, true) - visit profile_account_path - end - - describe 'when no u2f device is registered' do - before do - gitlab_sign_out - gitlab_sign_in(user) - end - - it 'shows the fallback otp code UI' do - assert_fallback_ui(page) - end - end - - describe 'when a u2f device is registered' do - before do - manage_two_factor_authentication - @u2f_device = register_u2f_device - gitlab_sign_out - gitlab_sign_in(user) - end - - it 'provides a button that shows the fallback otp code UI' do - expect(page).to have_link('Sign in via 2FA code') - - click_link('Sign in via 2FA code') - - assert_fallback_ui(page) - end - end - end end diff --git a/spec/features/webauthn_spec.rb b/spec/features/webauthn_spec.rb new file mode 100644 index 00000000000..2ffb6bb3477 --- /dev/null +++ b/spec/features/webauthn_spec.rb @@ -0,0 +1,234 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Using WebAuthn Devices for Authentication', :js do + include Spec::Support::Helpers::Features::TwoFactorHelpers + let(:app_id) { "http://#{Capybara.current_session.server.host}:#{Capybara.current_session.server.port}" } + + before do + WebAuthn.configuration.origin = app_id + end + + it_behaves_like 'hardware device for 2fa', 'WebAuthn' + + describe 'registration' do + let(:user) { create(:user) } + + before do + gitlab_sign_in(user) + user.update_attribute(:otp_required_for_login, true) + end + + describe 'when 2FA via OTP is enabled' do + it 'allows registering more than one device' do + visit profile_account_path + + # First device + manage_two_factor_authentication + first_device = register_webauthn_device + expect(page).to have_content('Your WebAuthn device was registered') + + # Second device + second_device = register_webauthn_device(name: 'My other device') + expect(page).to have_content('Your WebAuthn device was registered') + + expect(page).to have_content(first_device.name) + expect(page).to have_content(second_device.name) + expect(WebauthnRegistration.count).to eq(2) + end + end + + it 'allows the same device to be registered for multiple users' do + # First user + visit profile_account_path + manage_two_factor_authentication + webauthn_device = register_webauthn_device + expect(page).to have_content('Your WebAuthn device was registered') + gitlab_sign_out + + # Second user + user = gitlab_sign_in(:user) + user.update_attribute(:otp_required_for_login, true) + visit profile_account_path + manage_two_factor_authentication + register_webauthn_device(webauthn_device, name: 'My other device') + expect(page).to have_content('Your WebAuthn device was registered') + + expect(WebauthnRegistration.count).to eq(2) + end + + context 'when there are form errors' do + let(:mock_register_js) do + <<~JS + const mockResponse = { + type: 'public-key', + id: '', + rawId: '', + response: { + clientDataJSON: '', + attestationObject: '', + }, + getClientExtensionResults: () => {}, + }; + navigator.credentials.create = function(_) {return Promise.resolve(mockResponse);} + JS + end + + it "doesn't register the device if there are errors" do + visit profile_account_path + manage_two_factor_authentication + + # Have the "webauthn device" respond with bad data + page.execute_script(mock_register_js) + click_on 'Set up new device' + expect(page).to have_content('Your device was successfully set up') + click_on 'Register device' + + expect(WebauthnRegistration.count).to eq(0) + expect(page).to have_content('The form contains the following error') + expect(page).to have_content('did not send a valid JSON response') + end + + it 'allows retrying registration' do + visit profile_account_path + manage_two_factor_authentication + + # Failed registration + page.execute_script(mock_register_js) + click_on 'Set up new device' + expect(page).to have_content('Your device was successfully set up') + click_on 'Register device' + expect(page).to have_content('The form contains the following error') + + # Successful registration + register_webauthn_device + + expect(page).to have_content('Your WebAuthn device was registered') + expect(WebauthnRegistration.count).to eq(1) + end + end + end + + describe 'authentication' do + let(:otp_required_for_login) { true } + let(:user) { create(:user, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) } + + describe 'when there is only an U2F device' do + let!(:u2f_device) do + fake_device = U2F::FakeU2F.new(app_id) # "Client" + u2f = U2F::U2F.new(app_id) # "Server" + + challenges = u2f.registration_requests.map(&:challenge) + device_response = fake_device.register_response(challenges[0]) + device_registration_params = { device_response: device_response, + name: 'My device' } + + U2fRegistration.register(user, app_id, device_registration_params, challenges) + FakeU2fDevice.new(page, 'My device', fake_device) + end + + it 'falls back to U2F' do + gitlab_sign_in(user) + + u2f_device.respond_to_u2f_authentication + + expect(page).to have_css('.sign-out-link', visible: false) + end + end + + describe 'when there is a WebAuthn device' do + let!(:webauthn_device) do + add_webauthn_device(app_id, user) + end + + describe 'when 2FA via OTP is disabled' do + let(:otp_required_for_login) { false } + + it 'allows logging in with the WebAuthn device' do + gitlab_sign_in(user) + + webauthn_device.respond_to_webauthn_authentication + + expect(page).to have_css('.sign-out-link', visible: false) + end + end + + describe 'when 2FA via OTP is enabled' do + it 'allows logging in with the WebAuthn device' do + gitlab_sign_in(user) + + webauthn_device.respond_to_webauthn_authentication + + expect(page).to have_css('.sign-out-link', visible: false) + end + end + + describe 'when a given WebAuthn device has already been registered by another user' do + describe 'but not the current user' do + let(:other_user) { create(:user, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) } + + it 'does not allow logging in with that particular device' do + # Register other user with a different WebAuthn device + other_device = add_webauthn_device(app_id, other_user) + + # Try authenticating user with the old WebAuthn device + gitlab_sign_in(user) + other_device.respond_to_webauthn_authentication + expect(page).to have_content('Authentication via WebAuthn device failed') + end + end + + describe "and also the current user" do + # TODO Uncomment once WebAuthn::FakeClient supports passing credential options + # (especially allow_credentials, as this is needed to specify which credential the + # fake client should use. Currently, the first credential is always used). + # There is an issue open for this: https://github.com/cedarcode/webauthn-ruby/issues/259 + it "allows logging in with that particular device" do + pending("support for passing credential options in FakeClient") + # Register current user with the same WebAuthn device + current_user = gitlab_sign_in(:user) + visit profile_account_path + manage_two_factor_authentication + register_webauthn_device(webauthn_device) + gitlab_sign_out + + # Try authenticating user with the same WebAuthn device + gitlab_sign_in(current_user) + webauthn_device.respond_to_webauthn_authentication + + expect(page).to have_css('.sign-out-link', visible: false) + end + end + end + + describe 'when a given WebAuthn device has not been registered' do + it 'does not allow logging in with that particular device' do + unregistered_device = FakeWebauthnDevice.new(page, 'My device') + gitlab_sign_in(user) + unregistered_device.respond_to_webauthn_authentication + + expect(page).to have_content('Authentication via WebAuthn device failed') + end + end + + describe 'when more than one device has been registered by the same user' do + it 'allows logging in with either device' do + first_device = add_webauthn_device(app_id, user) + second_device = add_webauthn_device(app_id, user) + + # Authenticate as both devices + [first_device, second_device].each do |device| + gitlab_sign_in(user) + # register_webauthn_device(device) + device.respond_to_webauthn_authentication + + expect(page).to have_css('.sign-out-link', visible: false) + + gitlab_sign_out + end + end + end + end + end +end diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json index ac0a0314455..3c19528d71b 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_basic.json +++ b/spec/fixtures/api/schemas/entities/merge_request_basic.json @@ -15,6 +15,13 @@ "$ref": "../public_api/v4/user/basic.json" } }, + "reviewers": { + "type": ["array"], + "items": { + "type": "object", + "$ref": "../public_api/v4/user/basic.json" + } + }, "milestone": { "type": [ "object", "null" ] }, diff --git a/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json b/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json index 11076ec73de..c54ae551d6a 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json +++ b/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json @@ -17,6 +17,10 @@ "assignees": { "type": "array", "items": { "$ref": "../public_api/v4/user/basic.json" } + }, + "reviewers": { + "type": "array", + "items": { "$ref": "../public_api/v4/user/basic.json" } } }, "additionalProperties": false diff --git a/spec/frontend/authentication/u2f/authenticate_spec.js b/spec/frontend/authentication/u2f/authenticate_spec.js index 8abef2ae1b2..7a87b420195 100644 --- a/spec/frontend/authentication/u2f/authenticate_spec.js +++ b/spec/frontend/authentication/u2f/authenticate_spec.js @@ -76,7 +76,7 @@ describe('U2FAuthenticate', () => { describe('errors', () => { it('displays an error message', () => { - const setupButton = container.find('#js-login-u2f-device'); + const setupButton = container.find('#js-login-2fa-device'); setupButton.trigger('click'); u2fDevice.respondToAuthenticateRequest({ errorCode: 'error!', @@ -87,14 +87,14 @@ describe('U2FAuthenticate', () => { }); it('allows retrying authentication after an error', () => { - let setupButton = container.find('#js-login-u2f-device'); + let setupButton = container.find('#js-login-2fa-device'); setupButton.trigger('click'); u2fDevice.respondToAuthenticateRequest({ errorCode: 'error!', }); const retryButton = container.find('#js-token-2fa-try-again'); retryButton.trigger('click'); - setupButton = container.find('#js-login-u2f-device'); + setupButton = container.find('#js-login-2fa-device'); setupButton.trigger('click'); u2fDevice.respondToAuthenticateRequest({ deviceData: 'this is data from the device', diff --git a/spec/frontend/authentication/u2f/register_spec.js b/spec/frontend/authentication/u2f/register_spec.js index 3c2ecdbba66..e89ef773be6 100644 --- a/spec/frontend/authentication/u2f/register_spec.js +++ b/spec/frontend/authentication/u2f/register_spec.js @@ -13,8 +13,8 @@ describe('U2FRegister', () => { beforeEach(done => { loadFixtures('u2f/register.html'); u2fDevice = new MockU2FDevice(); - container = $('#js-register-u2f'); - component = new U2FRegister(container, $('#js-register-u2f-templates'), {}, 'token'); + container = $('#js-register-token-2fa'); + component = new U2FRegister(container, {}); component .start() .then(done) @@ -22,9 +22,9 @@ describe('U2FRegister', () => { }); it('allows registering a U2F device', () => { - const setupButton = container.find('#js-setup-u2f-device'); + const setupButton = container.find('#js-setup-token-2fa-device'); - expect(setupButton.text()).toBe('Set up new U2F device'); + expect(setupButton.text()).toBe('Set up new device'); setupButton.trigger('click'); const inProgressMessage = container.children('p'); @@ -41,7 +41,7 @@ describe('U2FRegister', () => { describe('errors', () => { it("doesn't allow the same device to be registered twice (for the same user", () => { - const setupButton = container.find('#js-setup-u2f-device'); + const setupButton = container.find('#js-setup-token-2fa-device'); setupButton.trigger('click'); u2fDevice.respondToRegisterRequest({ errorCode: 4, @@ -52,7 +52,7 @@ describe('U2FRegister', () => { }); it('displays an error message for other errors', () => { - const setupButton = container.find('#js-setup-u2f-device'); + const setupButton = container.find('#js-setup-token-2fa-device'); setupButton.trigger('click'); u2fDevice.respondToRegisterRequest({ errorCode: 'error!', @@ -63,14 +63,14 @@ describe('U2FRegister', () => { }); it('allows retrying registration after an error', () => { - let setupButton = container.find('#js-setup-u2f-device'); + let setupButton = container.find('#js-setup-token-2fa-device'); setupButton.trigger('click'); u2fDevice.respondToRegisterRequest({ errorCode: 'error!', }); - const retryButton = container.find('#U2FTryAgain'); + const retryButton = container.find('#js-token-2fa-try-again'); retryButton.trigger('click'); - setupButton = container.find('#js-setup-u2f-device'); + setupButton = container.find('#js-setup-token-2fa-device'); setupButton.trigger('click'); u2fDevice.respondToRegisterRequest({ deviceData: 'this is data from the device', diff --git a/spec/frontend/authentication/webauthn/authenticate_spec.js b/spec/frontend/authentication/webauthn/authenticate_spec.js new file mode 100644 index 00000000000..0a82adfd0ee --- /dev/null +++ b/spec/frontend/authentication/webauthn/authenticate_spec.js @@ -0,0 +1,132 @@ +import $ from 'jquery'; +import waitForPromises from 'helpers/wait_for_promises'; +import WebAuthnAuthenticate from '~/authentication/webauthn/authenticate'; +import MockWebAuthnDevice from './mock_webauthn_device'; +import { useMockNavigatorCredentials } from './util'; + +const mockResponse = { + type: 'public-key', + id: '', + rawId: '', + response: { clientDataJSON: '', authenticatorData: '', signature: '', userHandle: '' }, + getClientExtensionResults: () => {}, +}; + +describe('WebAuthnAuthenticate', () => { + preloadFixtures('webauthn/authenticate.html'); + useMockNavigatorCredentials(); + + let fallbackElement; + let webAuthnDevice; + let container; + let component; + let submitSpy; + + const findDeviceResponseInput = () => container[0].querySelector('#js-device-response'); + const findDeviceResponseInputValue = () => findDeviceResponseInput().value; + const findMessage = () => container[0].querySelector('p'); + const findRetryButton = () => container[0].querySelector('#js-token-2fa-try-again'); + const expectAuthenticated = () => { + expect(container.text()).toMatchInterpolatedText( + 'We heard back from your device. You have been authenticated.', + ); + expect(findDeviceResponseInputValue()).toBe(JSON.stringify(mockResponse)); + expect(submitSpy).toHaveBeenCalled(); + }; + + beforeEach(() => { + loadFixtures('webauthn/authenticate.html'); + fallbackElement = document.createElement('div'); + fallbackElement.classList.add('js-2fa-form'); + webAuthnDevice = new MockWebAuthnDevice(); + container = $('#js-authenticate-token-2fa'); + component = new WebAuthnAuthenticate( + container, + '#js-login-token-2fa-form', + { + options: + // we need some valid base64 for base64ToBuffer + // so we use "YQ==" = base64("a") + JSON.stringify({ + challenge: 'YQ==', + timeout: 120000, + allowCredentials: [ + { type: 'public-key', id: 'YQ==' }, + { type: 'public-key', id: 'YQ==' }, + ], + userVerification: 'discouraged', + }), + }, + document.querySelector('#js-login-2fa-device'), + fallbackElement, + ); + submitSpy = jest.spyOn(HTMLFormElement.prototype, 'submit'); + }); + + describe('with webauthn unavailable', () => { + let oldGetCredentials; + + beforeEach(() => { + oldGetCredentials = window.navigator.credentials.get; + window.navigator.credentials.get = null; + }); + + afterEach(() => { + window.navigator.credentials.get = oldGetCredentials; + }); + + it('falls back to normal 2fa', () => { + component.start(); + + expect(container.html()).toBe(''); + expect(container[0]).toHaveClass('hidden'); + expect(fallbackElement).not.toHaveClass('hidden'); + }); + }); + + describe('with webauthn available', () => { + beforeEach(() => { + component.start(); + }); + + it('shows in progress', () => { + const inProgressMessage = container.find('p'); + + expect(inProgressMessage.text()).toMatchInterpolatedText( + "Trying to communicate with your device. Plug it in (if you haven't already) and press the button on the device now.", + ); + }); + + it('allows authenticating via a WebAuthn device', () => { + webAuthnDevice.respondToAuthenticateRequest(mockResponse); + + return waitForPromises().then(() => { + expectAuthenticated(); + }); + }); + + describe('errors', () => { + beforeEach(() => { + webAuthnDevice.rejectAuthenticateRequest(new DOMException()); + + return waitForPromises(); + }); + + it('displays an error message', () => { + expect(submitSpy).not.toHaveBeenCalled(); + expect(findMessage().textContent).toMatchInterpolatedText( + 'There was a problem communicating with your device. (Error)', + ); + }); + + it('allows retrying authentication after an error', () => { + findRetryButton().click(); + webAuthnDevice.respondToAuthenticateRequest(mockResponse); + + return waitForPromises().then(() => { + expectAuthenticated(); + }); + }); + }); + }); +}); diff --git a/spec/frontend/authentication/webauthn/error_spec.js b/spec/frontend/authentication/webauthn/error_spec.js new file mode 100644 index 00000000000..26f1ca5e27d --- /dev/null +++ b/spec/frontend/authentication/webauthn/error_spec.js @@ -0,0 +1,50 @@ +import WebAuthnError from '~/authentication/webauthn/error'; + +describe('WebAuthnError', () => { + it.each([ + [ + 'NotSupportedError', + 'Your device is not compatible with GitLab. Please try another device', + 'authenticate', + ], + ['InvalidStateError', 'This device has not been registered with us.', 'authenticate'], + ['InvalidStateError', 'This device has already been registered with us.', 'register'], + ['UnknownError', 'There was a problem communicating with your device.', 'register'], + ])('exception %s will have message %s, flow type: %s', (exception, expectedMessage, flowType) => { + expect(new WebAuthnError(new DOMException('', exception), flowType).message()).toEqual( + expectedMessage, + ); + }); + + describe('SecurityError', () => { + const { location } = window; + + beforeEach(() => { + delete window.location; + window.location = {}; + }); + + afterEach(() => { + window.location = location; + }); + + it('returns a descriptive error if https is disabled', () => { + window.location.protocol = 'http:'; + + const expectedMessage = + 'WebAuthn only works with HTTPS-enabled websites. Contact your administrator for more details.'; + expect( + new WebAuthnError(new DOMException('', 'SecurityError'), 'authenticate').message(), + ).toEqual(expectedMessage); + }); + + it('returns a generic error if https is enabled', () => { + window.location.protocol = 'https:'; + + const expectedMessage = 'There was a problem communicating with your device.'; + expect( + new WebAuthnError(new DOMException('', 'SecurityError'), 'authenticate').message(), + ).toEqual(expectedMessage); + }); + }); +}); diff --git a/spec/frontend/authentication/webauthn/mock_webauthn_device.js b/spec/frontend/authentication/webauthn/mock_webauthn_device.js new file mode 100644 index 00000000000..39df94df46b --- /dev/null +++ b/spec/frontend/authentication/webauthn/mock_webauthn_device.js @@ -0,0 +1,35 @@ +/* eslint-disable no-unused-expressions */ + +export default class MockWebAuthnDevice { + constructor() { + this.respondToAuthenticateRequest = this.respondToAuthenticateRequest.bind(this); + this.respondToRegisterRequest = this.respondToRegisterRequest.bind(this); + window.navigator.credentials || (window.navigator.credentials = {}); + window.navigator.credentials.create = () => + new Promise((resolve, reject) => { + this.registerCallback = resolve; + this.registerRejectCallback = reject; + }); + window.navigator.credentials.get = () => + new Promise((resolve, reject) => { + this.authenticateCallback = resolve; + this.authenticateRejectCallback = reject; + }); + } + + respondToRegisterRequest(params) { + return this.registerCallback(params); + } + + respondToAuthenticateRequest(params) { + return this.authenticateCallback(params); + } + + rejectRegisterRequest(params) { + return this.registerRejectCallback(params); + } + + rejectAuthenticateRequest(params) { + return this.authenticateRejectCallback(params); + } +} diff --git a/spec/frontend/authentication/webauthn/register_spec.js b/spec/frontend/authentication/webauthn/register_spec.js new file mode 100644 index 00000000000..1de952d176d --- /dev/null +++ b/spec/frontend/authentication/webauthn/register_spec.js @@ -0,0 +1,131 @@ +import $ from 'jquery'; +import waitForPromises from 'helpers/wait_for_promises'; +import WebAuthnRegister from '~/authentication/webauthn/register'; +import MockWebAuthnDevice from './mock_webauthn_device'; +import { useMockNavigatorCredentials } from './util'; + +describe('WebAuthnRegister', () => { + preloadFixtures('webauthn/register.html'); + useMockNavigatorCredentials(); + + const mockResponse = { + type: 'public-key', + id: '', + rawId: '', + response: { + clientDataJSON: '', + attestationObject: '', + }, + getClientExtensionResults: () => {}, + }; + let webAuthnDevice; + let container; + let component; + + beforeEach(() => { + loadFixtures('webauthn/register.html'); + webAuthnDevice = new MockWebAuthnDevice(); + container = $('#js-register-token-2fa'); + component = new WebAuthnRegister(container, { + options: { + rp: '', + user: { + id: '', + name: '', + displayName: '', + }, + challenge: '', + pubKeyCredParams: '', + }, + }); + component.start(); + }); + + const findSetupButton = () => container.find('#js-setup-token-2fa-device'); + const findMessage = () => container.find('p'); + const findDeviceResponse = () => container.find('#js-device-response'); + const findRetryButton = () => container.find('#js-token-2fa-try-again'); + + it('shows setup button', () => { + expect(findSetupButton().text()).toBe('Set up new device'); + }); + + describe('when unsupported', () => { + const { location, PublicKeyCredential } = window; + + beforeEach(() => { + delete window.location; + delete window.credentials; + window.location = {}; + window.PublicKeyCredential = undefined; + }); + + afterEach(() => { + window.location = location; + window.PublicKeyCredential = PublicKeyCredential; + }); + + it.each` + httpsEnabled | expectedText + ${false} | ${'WebAuthn only works with HTTPS-enabled websites'} + ${true} | ${'Please use a supported browser, e.g. Chrome (67+) or Firefox'} + `('when https is $httpsEnabled', ({ httpsEnabled, expectedText }) => { + window.location.protocol = httpsEnabled ? 'https:' : 'http:'; + component.start(); + + expect(findMessage().text()).toContain(expectedText); + }); + }); + + describe('when setup', () => { + beforeEach(() => { + findSetupButton().trigger('click'); + }); + + it('shows in progress message', () => { + expect(findMessage().text()).toContain('Trying to communicate with your device'); + }); + + it('registers device', () => { + webAuthnDevice.respondToRegisterRequest(mockResponse); + + return waitForPromises().then(() => { + expect(findMessage().text()).toContain('Your device was successfully set up!'); + expect(findDeviceResponse().val()).toBe(JSON.stringify(mockResponse)); + }); + }); + + it.each` + errorName | expectedText + ${'NotSupportedError'} | ${'Your device is not compatible with GitLab'} + ${'NotAllowedError'} | ${'There was a problem communicating with your device'} + `('when fails with $errorName', ({ errorName, expectedText }) => { + webAuthnDevice.rejectRegisterRequest(new DOMException('', errorName)); + + return waitForPromises().then(() => { + expect(findMessage().text()).toContain(expectedText); + expect(findRetryButton().length).toBe(1); + }); + }); + + it('can retry', () => { + webAuthnDevice.respondToRegisterRequest({ + errorCode: 'error!', + }); + + return waitForPromises() + .then(() => { + findRetryButton().click(); + + expect(findMessage().text()).toContain('Trying to communicate with your device'); + + webAuthnDevice.respondToRegisterRequest(mockResponse); + return waitForPromises(); + }) + .then(() => { + expect(findMessage().text()).toContain('Your device was successfully set up!'); + expect(findDeviceResponse().val()).toBe(JSON.stringify(mockResponse)); + }); + }); + }); +}); diff --git a/spec/frontend/authentication/webauthn/util.js b/spec/frontend/authentication/webauthn/util.js new file mode 100644 index 00000000000..d8f5a67ee1f --- /dev/null +++ b/spec/frontend/authentication/webauthn/util.js @@ -0,0 +1,19 @@ +export function useMockNavigatorCredentials() { + let oldNavigatorCredentials; + let oldPublicKeyCredential; + + beforeEach(() => { + oldNavigatorCredentials = navigator.credentials; + oldPublicKeyCredential = window.PublicKeyCredential; + navigator.credentials = { + get: jest.fn(), + create: jest.fn(), + }; + window.PublicKeyCredential = function MockPublicKeyCredential() {}; + }); + + afterEach(() => { + navigator.credentials = oldNavigatorCredentials; + window.PublicKeyCredential = oldPublicKeyCredential; + }); +} diff --git a/spec/frontend/fixtures/u2f.rb b/spec/frontend/fixtures/u2f.rb index be3874d7c42..a6a8ba7318b 100644 --- a/spec/frontend/fixtures/u2f.rb +++ b/spec/frontend/fixtures/u2f.rb @@ -11,6 +11,10 @@ RSpec.context 'U2F' do clean_frontend_fixtures('u2f/') end + before do + stub_feature_flags(webauthn: false) + end + describe SessionsController, '(JavaScript fixtures)', type: :controller do include DeviseHelpers diff --git a/spec/frontend/fixtures/webauthn.rb b/spec/frontend/fixtures/webauthn.rb new file mode 100644 index 00000000000..b195fee76f0 --- /dev/null +++ b/spec/frontend/fixtures/webauthn.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.context 'WebAuthn' do + include JavaScriptFixturesHelpers + + let(:user) { create(:user, :two_factor_via_webauthn, otp_secret: 'otpsecret:coolkids') } + + before(:all) do + clean_frontend_fixtures('webauthn/') + end + + describe SessionsController, '(JavaScript fixtures)', type: :controller do + include DeviseHelpers + + render_views + + before do + set_devise_mapping(context: @request) + end + + it 'webauthn/authenticate.html' do + allow(controller).to receive(:find_user).and_return(user) + post :create, params: { user: { login: user.username, password: user.password } } + + expect(response).to be_successful + end + end + + describe Profiles::TwoFactorAuthsController, '(JavaScript fixtures)', type: :controller do + render_views + + before do + sign_in(user) + allow_next_instance_of(Profiles::TwoFactorAuthsController) do |instance| + allow(instance).to receive(:build_qr_code).and_return('qrcode:blackandwhitesquares') + end + end + + it 'webauthn/register.html' do + get :show + + expect(response).to be_successful + end + end +end diff --git a/spec/frontend/packages/details/components/__snapshots__/dependency_row_spec.js.snap b/spec/frontend/packages/details/components/__snapshots__/dependency_row_spec.js.snap index df2844eaa3c..39469bf4fd0 100644 --- a/spec/frontend/packages/details/components/__snapshots__/dependency_row_spec.js.snap +++ b/spec/frontend/packages/details/components/__snapshots__/dependency_row_spec.js.snap @@ -21,7 +21,7 @@ exports[`DependencyRow renders full dependency 1`] = ` </div> <div - class="table-section section-50 gl-display-flex gl-justify-content-md-end" + class="table-section section-50 gl-display-flex gl-md-justify-content-end" data-testid="version-pattern" > <span diff --git a/spec/frontend/vue_mr_widget/stores/get_state_key_spec.js b/spec/frontend/vue_mr_widget/stores/get_state_key_spec.js index 128e0f39c41..631d4647b17 100644 --- a/spec/frontend/vue_mr_widget/stores/get_state_key_spec.js +++ b/spec/frontend/vue_mr_widget/stores/get_state_key_spec.js @@ -30,6 +30,7 @@ describe('getStateKey', () => { expect(bound()).toEqual('notAllowedToMerge'); context.autoMergeEnabled = true; + context.hasMergeableDiscussionsState = true; expect(bound()).toEqual('autoMergeEnabled'); @@ -44,6 +45,7 @@ describe('getStateKey', () => { expect(bound()).toEqual('pipelineBlocked'); context.hasMergeableDiscussionsState = true; + context.autoMergeEnabled = false; expect(bound()).toEqual('unresolvedDiscussions'); diff --git a/spec/graphql/resolvers/project_pipeline_resolver_spec.rb b/spec/graphql/resolvers/project_pipeline_resolver_spec.rb index ef13f048e19..a6a86c49373 100644 --- a/spec/graphql/resolvers/project_pipeline_resolver_spec.rb +++ b/spec/graphql/resolvers/project_pipeline_resolver_spec.rb @@ -34,12 +34,10 @@ RSpec.describe Resolvers::ProjectPipelineResolver do expect { resolve_pipeline(project, {}) }.to raise_error(ArgumentError) end - context 'when the pipeline is not a ci_config_source' do + context 'when the pipeline is a dangling pipeline' do let(:pipeline) do - config_source_value = ::Enums::Ci::Pipeline.non_ci_config_source_values.first - config_source = ::Enums::Ci::Pipeline.config_sources.key(config_source_value) - - create(:ci_pipeline, config_source: config_source, project: project) + dangling_source = ::Enums::Ci::Pipeline.dangling_sources.each_value.first + create(:ci_pipeline, source: dangling_source, project: project) end it 'resolves pipeline for the passed iid' do diff --git a/spec/graphql/types/issuable_severity_enum_spec.rb b/spec/graphql/types/issuable_severity_enum_spec.rb new file mode 100644 index 00000000000..3f8bd76fa62 --- /dev/null +++ b/spec/graphql/types/issuable_severity_enum_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Types::IssuableSeverityEnum do + specify { expect(described_class.graphql_name).to eq('IssuableSeverity') } + + it 'exposes all the existing issuable severity values' do + expect(described_class.values.keys).to contain_exactly( + *%w[UNKNOWN LOW MEDIUM HIGH CRITICAL] + ) + end +end diff --git a/spec/graphql/types/issue_type_spec.rb b/spec/graphql/types/issue_type_spec.rb index 282d6b76023..db2a1751be0 100644 --- a/spec/graphql/types/issue_type_spec.rb +++ b/spec/graphql/types/issue_type_spec.rb @@ -15,7 +15,7 @@ RSpec.describe GitlabSchema.types['Issue'] do fields = %i[id iid title description state reference author assignees participants labels milestone due_date confidential discussion_locked upvotes downvotes user_notes_count web_path web_url relative_position subscribed time_estimate total_time_spent closed_at created_at updated_at task_completion_status - designs design_collection alert_management_alert] + designs design_collection alert_management_alert severity] fields.each do |field_name| expect(described_class).to have_graphql_field(field_name) diff --git a/spec/haml_lint/linter/documentation_links_spec.rb b/spec/haml_lint/linter/documentation_links_spec.rb index 68de8317b82..aa4c5dd7c39 100644 --- a/spec/haml_lint/linter/documentation_links_spec.rb +++ b/spec/haml_lint/linter/documentation_links_spec.rb @@ -20,6 +20,13 @@ RSpec.describe HamlLint::Linter::DocumentationLinks do it { is_expected.not_to report_lint } end + # TODO: Remove me after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/39715 is merged + context 'when link_to points to the existing file with partially matching anchor' do + let(:haml) { "= link_to 'Description', help_page_path('README.md', anchor: 'overview-premium'), target: '_blank'" } + + it { is_expected.not_to report_lint } + end + context 'when link_to points to the existing file path without .md extension' do let(:haml) { "= link_to 'Description', help_page_path('README')" } diff --git a/spec/helpers/submodule_helper_spec.rb b/spec/helpers/submodule_helper_spec.rb index 426bca2ced2..a419b6b9c84 100644 --- a/spec/helpers/submodule_helper_spec.rb +++ b/spec/helpers/submodule_helper_spec.rb @@ -24,7 +24,11 @@ RSpec.describe SubmoduleHelper do allow(Gitlab.config.gitlab_shell).to receive(:ssh_port).and_return(22) # set this just to be sure allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix)) stub_url([config.ssh_user, '@', config.host, ':gitlab-org/gitlab-foss.git'].join('')) - expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')]) + aggregate_failures do + expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss')) + expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')) + expect(subject.compare).to be_nil + end end it 'detects ssh on standard port without a username' do @@ -32,14 +36,22 @@ RSpec.describe SubmoduleHelper do allow(Gitlab.config.gitlab_shell).to receive(:ssh_user).and_return('') allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix)) stub_url([config.host, ':gitlab-org/gitlab-foss.git'].join('')) - expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')]) + aggregate_failures do + expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss')) + expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')) + expect(subject.compare).to be_nil + end end it 'detects ssh on non-standard port' do allow(Gitlab.config.gitlab_shell).to receive(:ssh_port).and_return(2222) allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix)) stub_url(['ssh://', config.ssh_user, '@', config.host, ':2222/gitlab-org/gitlab-foss.git'].join('')) - expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')]) + aggregate_failures do + expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss')) + expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')) + expect(subject.compare).to be_nil + end end it 'detects ssh on non-standard port without a username' do @@ -47,21 +59,33 @@ RSpec.describe SubmoduleHelper do allow(Gitlab.config.gitlab_shell).to receive(:ssh_user).and_return('') allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix)) stub_url(['ssh://', config.host, ':2222/gitlab-org/gitlab-foss.git'].join('')) - expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')]) + aggregate_failures do + expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss')) + expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')) + expect(subject.compare).to be_nil + end end it 'detects http on standard port' do allow(Gitlab.config.gitlab).to receive(:port).and_return(80) allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url)) stub_url(['http://', config.host, '/gitlab-org/gitlab-foss.git'].join('')) - expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')]) + aggregate_failures do + expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss')) + expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')) + expect(subject.compare).to be_nil + end end it 'detects http on non-standard port' do allow(Gitlab.config.gitlab).to receive(:port).and_return(3000) allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url)) stub_url(['http://', config.host, ':3000/gitlab-org/gitlab-foss.git'].join('')) - expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')]) + aggregate_failures do + expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss')) + expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')) + expect(subject.compare).to be_nil + end end it 'works with relative_url_root' do @@ -69,7 +93,11 @@ RSpec.describe SubmoduleHelper do allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return('/gitlab/root') allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url)) stub_url(['http://', config.host, '/gitlab/root/gitlab-org/gitlab-foss.git'].join('')) - expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')]) + aggregate_failures do + expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss')) + expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')) + expect(subject.compare).to be_nil + end end it 'works with subgroups' do @@ -77,61 +105,105 @@ RSpec.describe SubmoduleHelper do allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return('/gitlab/root') allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url)) stub_url(['http://', config.host, '/gitlab/root/gitlab-org/sub/gitlab-foss.git'].join('')) - expect(subject).to eq([namespace_project_path('gitlab-org/sub', 'gitlab-foss'), namespace_project_tree_path('gitlab-org/sub', 'gitlab-foss', 'hash')]) + aggregate_failures do + expect(subject.web).to eq(namespace_project_path('gitlab-org/sub', 'gitlab-foss')) + expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org/sub', 'gitlab-foss', 'hash')) + expect(subject.compare).to be_nil + end end end context 'submodule on gist.github.com' do it 'detects ssh' do stub_url('git@gist.github.com:gitlab-org/gitlab-foss.git') - is_expected.to eq(['https://gist.github.com/gitlab-org/gitlab-foss', 'https://gist.github.com/gitlab-org/gitlab-foss/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gist.github.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gist.github.com/gitlab-org/gitlab-foss/hash') + expect(subject.compare).to be_nil + end end it 'detects http' do stub_url('http://gist.github.com/gitlab-org/gitlab-foss.git') - is_expected.to eq(['https://gist.github.com/gitlab-org/gitlab-foss', 'https://gist.github.com/gitlab-org/gitlab-foss/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gist.github.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gist.github.com/gitlab-org/gitlab-foss/hash') + expect(subject.compare).to be_nil + end end it 'detects https' do stub_url('https://gist.github.com/gitlab-org/gitlab-foss.git') - is_expected.to eq(['https://gist.github.com/gitlab-org/gitlab-foss', 'https://gist.github.com/gitlab-org/gitlab-foss/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gist.github.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gist.github.com/gitlab-org/gitlab-foss/hash') + expect(subject.compare).to be_nil + end end it 'handles urls with no .git on the end' do stub_url('http://gist.github.com/gitlab-org/gitlab-foss') - is_expected.to eq(['https://gist.github.com/gitlab-org/gitlab-foss', 'https://gist.github.com/gitlab-org/gitlab-foss/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gist.github.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gist.github.com/gitlab-org/gitlab-foss/hash') + expect(subject.compare).to be_nil + end end it 'returns original with non-standard url' do stub_url('http://gist.github.com/another/gitlab-org/gitlab-foss.git') - is_expected.to eq([repo.submodule_url_for, nil]) + aggregate_failures do + expect(subject.web).to eq(repo.submodule_url_for) + expect(subject.tree).to be_nil + expect(subject.compare).to be_nil + end end end context 'submodule on github.com' do it 'detects ssh' do stub_url('git@github.com:gitlab-org/gitlab-foss.git') - expect(subject).to eq(['https://github.com/gitlab-org/gitlab-foss', 'https://github.com/gitlab-org/gitlab-foss/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://github.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://github.com/gitlab-org/gitlab-foss/tree/hash') + expect(subject.compare).to be_nil + end end it 'detects http' do stub_url('http://github.com/gitlab-org/gitlab-foss.git') - expect(subject).to eq(['https://github.com/gitlab-org/gitlab-foss', 'https://github.com/gitlab-org/gitlab-foss/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://github.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://github.com/gitlab-org/gitlab-foss/tree/hash') + expect(subject.compare).to be_nil + end end it 'detects https' do stub_url('https://github.com/gitlab-org/gitlab-foss.git') - expect(subject).to eq(['https://github.com/gitlab-org/gitlab-foss', 'https://github.com/gitlab-org/gitlab-foss/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://github.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://github.com/gitlab-org/gitlab-foss/tree/hash') + expect(subject.compare).to be_nil + end end it 'handles urls with no .git on the end' do stub_url('http://github.com/gitlab-org/gitlab-foss') - expect(subject).to eq(['https://github.com/gitlab-org/gitlab-foss', 'https://github.com/gitlab-org/gitlab-foss/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://github.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://github.com/gitlab-org/gitlab-foss/tree/hash') + expect(subject.compare).to be_nil + end end it 'returns original with non-standard url' do stub_url('http://github.com/another/gitlab-org/gitlab-foss.git') - expect(subject).to eq([repo.submodule_url_for, nil]) + aggregate_failures do + expect(subject.web).to eq(repo.submodule_url_for) + expect(subject.tree).to be_nil + expect(subject.compare).to be_nil + end end end @@ -143,39 +215,67 @@ RSpec.describe SubmoduleHelper do allow(repo).to receive(:project).and_return(project) stub_url('./') - expect(subject).to eq(["/master-project/#{project.path}", "/master-project/#{project.path}/-/tree/hash"]) + aggregate_failures do + expect(subject.web).to eq("/master-project/#{project.path}") + expect(subject.tree).to eq("/master-project/#{project.path}/-/tree/hash") + expect(subject.compare).to be_nil + end end end context 'submodule on gitlab.com' do it 'detects ssh' do stub_url('git@gitlab.com:gitlab-org/gitlab-foss.git') - expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to be_nil + end end it 'detects http' do stub_url('http://gitlab.com/gitlab-org/gitlab-foss.git') - expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to be_nil + end end it 'detects https' do stub_url('https://gitlab.com/gitlab-org/gitlab-foss.git') - expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to be_nil + end end it 'handles urls with no .git on the end' do stub_url('http://gitlab.com/gitlab-org/gitlab-foss') - expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to be_nil + end end it 'handles urls with trailing whitespace' do stub_url('http://gitlab.com/gitlab-org/gitlab-foss.git ') - expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to be_nil + end end it 'returns original with non-standard url' do stub_url('http://gitlab.com/another/gitlab-org/gitlab-foss.git') - expect(subject).to eq([repo.submodule_url_for, nil]) + aggregate_failures do + expect(subject.web).to eq(repo.submodule_url_for) + expect(subject.tree).to be_nil + expect(subject.compare).to be_nil + end end end @@ -183,25 +283,29 @@ RSpec.describe SubmoduleHelper do it 'sanitizes unsupported protocols' do stub_url('javascript:alert("XSS");') - expect(subject).to eq([nil, nil]) + expect(subject).to be_nil end it 'sanitizes unsupported protocols disguised as a repository URL' do stub_url('javascript:alert("XSS");foo/bar.git') - expect(subject).to eq([nil, nil]) + expect(subject).to be_nil end it 'sanitizes invalid URL with extended ASCII' do stub_url('é') - expect(subject).to eq([nil, nil]) + expect(subject).to be_nil end it 'returns original' do stub_url('http://mygitserver.com/gitlab-org/gitlab-foss') - expect(subject).to eq([repo.submodule_url_for, nil]) + aggregate_failures do + expect(subject.web).to eq(repo.submodule_url_for) + expect(subject.tree).to be_nil + expect(subject.compare).to be_nil + end end end @@ -214,7 +318,11 @@ RSpec.describe SubmoduleHelper do stub_url(relative_path) result = subject - expect(result).to eq([expected_path, "#{expected_path}/-/tree/#{submodule_item.id}"]) + aggregate_failures do + expect(result.web).to eq(expected_path) + expect(result.tree).to eq("#{expected_path}/-/tree/#{submodule_item.id}") + expect(result.compare).to be_nil + end end it 'handles project under same group' do @@ -235,7 +343,7 @@ RSpec.describe SubmoduleHelper do result = subject - expect(result).to eq([nil, nil]) + expect(result).to be_nil end end @@ -245,7 +353,7 @@ RSpec.describe SubmoduleHelper do result = subject - expect(result).to eq([nil, nil]) + expect(result).to be_nil end end @@ -289,7 +397,7 @@ RSpec.describe SubmoduleHelper do end it 'returns no links' do - expect(subject).to eq([nil, nil]) + expect(subject).to be_nil end end end diff --git a/spec/lib/api/helpers_spec.rb b/spec/lib/api/helpers_spec.rb index d0fe9163c6e..ccfde95b00c 100644 --- a/spec/lib/api/helpers_spec.rb +++ b/spec/lib/api/helpers_spec.rb @@ -189,6 +189,62 @@ RSpec.describe API::Helpers do end end + describe '#increment_unique_values' do + let(:value) { '9f302fea-f828-4ca9-aef4-e10bd723c0b3' } + let(:event_name) { 'my_event' } + let(:unknown_event) { 'unknown' } + let(:feature) { "usage_data_#{event_name}" } + + context 'with feature enabled' do + before do + stub_feature_flags(feature => true) + end + + it 'tracks redis hll event' do + stub_application_setting(usage_ping_enabled: true) + + expect(Gitlab::UsageDataCounters::HLLRedisCounter).to receive(:track_event).with(value, event_name) + + subject.increment_unique_values(event_name, value) + end + + it 'does not track event usage ping is not enabled' do + stub_application_setting(usage_ping_enabled: false) + expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event) + + subject.increment_unique_values(event_name, value) + end + + it 'logs an exception for unknown event' do + stub_application_setting(usage_ping_enabled: true) + + expect(Gitlab::AppLogger).to receive(:warn).with("Redis tracking event failed for event: #{unknown_event}, message: Unknown event #{unknown_event}") + + subject.increment_unique_values(unknown_event, value) + end + + it 'does not track event for nil values' do + stub_application_setting(usage_ping_enabled: true) + + expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event) + + subject.increment_unique_values(unknown_event, nil) + end + end + + context 'with feature disabled' do + before do + stub_feature_flags(feature => false) + end + + it 'does not track event' do + expect(Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event) + + subject.increment_unique_values(event_name, value) + end + end + end + describe '#order_options_with_tie_breaker' do subject { Class.new.include(described_class).new.order_options_with_tie_breaker } diff --git a/spec/lib/banzai/filter/table_of_contents_filter_spec.rb b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb index 2d17855707f..6e90f4457fa 100644 --- a/spec/lib/banzai/filter/table_of_contents_filter_spec.rb +++ b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb @@ -65,6 +65,11 @@ RSpec.describe Banzai::Filter::TableOfContentsFilter do expect(doc.css('h1 a').first.attr('href')).to eq '#title-with-spaces' end + it 'removes a product suffix' do + doc = filter(header(1, "Title with spaces (ULTIMATE)")) + expect(doc.css('h1 a').first.attr('href')).to eq '#title-with-spaces' + end + it 'appends a unique number to duplicates' do doc = filter(header(1, 'One') + header(2, 'One')) diff --git a/spec/lib/gitlab/cleanup/orphan_lfs_file_references_spec.rb b/spec/lib/gitlab/cleanup/orphan_lfs_file_references_spec.rb index 47b2cf5dc4a..507440064bc 100644 --- a/spec/lib/gitlab/cleanup/orphan_lfs_file_references_spec.rb +++ b/spec/lib/gitlab/cleanup/orphan_lfs_file_references_spec.rb @@ -9,6 +9,8 @@ RSpec.describe Gitlab::Cleanup::OrphanLfsFileReferences do let!(:invalid_reference) { create(:lfs_objects_project, project: project, lfs_object: lfs_object) } + subject(:service) { described_class.new(project, logger: null_logger, dry_run: dry_run) } + before do allow(null_logger).to receive(:info) @@ -21,25 +23,66 @@ RSpec.describe Gitlab::Cleanup::OrphanLfsFileReferences do end context 'dry run' do + let(:dry_run) { true } + it 'prints messages and does not delete references' do expect(null_logger).to receive(:info).with("[DRY RUN] Looking for orphan LFS files for project #{project.name_with_namespace}") expect(null_logger).to receive(:info).with("[DRY RUN] Found invalid references: 1") - expect { described_class.new(project, logger: null_logger).run! } - .not_to change { project.lfs_objects.count } + expect { service.run! }.not_to change { project.lfs_objects.count } end end context 'regular run' do + let(:dry_run) { false } + it 'prints messages and deletes invalid reference' do expect(null_logger).to receive(:info).with("Looking for orphan LFS files for project #{project.name_with_namespace}") expect(null_logger).to receive(:info).with("Removed invalid references: 1") expect(ProjectCacheWorker).to receive(:perform_async).with(project.id, [], [:lfs_objects_size]) - expect { described_class.new(project, logger: null_logger, dry_run: false).run! } - .to change { project.lfs_objects.count }.from(2).to(1) + expect { service.run! }.to change { project.lfs_objects.count }.from(2).to(1) expect(LfsObjectsProject.exists?(invalid_reference.id)).to be_falsey end + + context 'LFS object is in design repository' do + before do + expect(project.design_repository).to receive(:exists?).and_return(true) + + stub_lfs_pointers(project.design_repository, lfs_object.oid) + end + + it 'is not removed' do + expect { service.run! }.not_to change { project.lfs_objects.count } + end + end + + context 'LFS object is in wiki repository' do + before do + expect(project.wiki.repository).to receive(:exists?).and_return(true) + + stub_lfs_pointers(project.wiki.repository, lfs_object.oid) + end + + it 'is not removed' do + expect { service.run! }.not_to change { project.lfs_objects.count } + end + end + end + + context 'LFS for project snippets' do + let(:snippet) { create(:project_snippet) } + + it 'is disabled' do + # Support project snippets here before enabling LFS for them + expect(snippet.repository.lfs_enabled?).to be_falsy + end + end + + def stub_lfs_pointers(repo, *oids) + expect(repo.gitaly_blob_client) + .to receive(:get_all_lfs_pointers) + .and_return(oids.map { |oid| OpenStruct.new(lfs_oid: oid) }) end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 69a86225219..d0ffda61cc1 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -15,6 +15,7 @@ issues: - resource_iteration_events - sent_notifications - sentry_issue +- issuable_severity - label_links - labels - last_edited_by @@ -649,6 +650,8 @@ zoom_meetings: - issue sentry_issue: - issue +issuable_severity: +- issue design_versions: *version epic: - subscriptions diff --git a/spec/lib/gitlab/submodule_links_spec.rb b/spec/lib/gitlab/submodule_links_spec.rb index c69326e12be..e2bbda81780 100644 --- a/spec/lib/gitlab/submodule_links_spec.rb +++ b/spec/lib/gitlab/submodule_links_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Gitlab::SubmoduleLinks do end it 'returns no links' do - expect(subject).to eq([nil, nil]) + expect(subject).to be_nil end end @@ -28,17 +28,28 @@ RSpec.describe Gitlab::SubmoduleLinks do end it 'returns no links' do - expect(subject).to eq([nil, nil]) + expect(subject).to be_nil end end context 'when the submodule is known' do before do - stub_urls({ 'gitlab-foss' => 'git@gitlab.com:gitlab-org/gitlab-foss.git' }) + gitlab_foss = 'git@gitlab.com:gitlab-org/gitlab-foss.git' + + stub_urls({ + 'ref' => { 'gitlab-foss' => gitlab_foss }, + 'other_ref' => { 'gitlab-foss' => gitlab_foss }, + 'signed-commits' => { 'gitlab-foss' => gitlab_foss }, + 'special_ref' => { 'gitlab-foss' => 'git@OTHER.com:gitlab-org/gitlab-foss.git' } + }) end it 'returns links and caches the by ref' do - expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to be_nil + end cache_store = links.instance_variable_get("@cache_store") @@ -49,13 +60,46 @@ RSpec.describe Gitlab::SubmoduleLinks do let(:ref) { 'signed-commits' } it 'returns links' do - expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash']) + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to be_nil + end + end + end + + context 'and the diff information is available' do + let(:old_ref) { 'other_ref' } + let(:diff_file) { double(old_blob: double(id: 'old-hash', path: 'gitlab-foss'), old_content_sha: old_ref) } + + subject { links.for(submodule_item, ref, diff_file) } + + it 'the returned links include the compare link' do + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/compare/old-hash...hash') + end + end + + context 'but the submodule url has changed' do + let(:old_ref) { 'special_ref' } + + it 'the returned links do not include the compare link' do + aggregate_failures do + expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss') + expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash') + expect(subject.compare).to be_nil + end + end end end end end - def stub_urls(urls) - allow(repo).to receive(:submodule_urls_for).and_return(urls) + def stub_urls(urls_by_ref) + allow(repo).to receive(:submodule_urls_for) do |ref| + urls_by_ref[ref] if urls_by_ref + end end end diff --git a/spec/lib/gitlab/utils/markdown_spec.rb b/spec/lib/gitlab/utils/markdown_spec.rb index 001ff5bc487..0ac594ca830 100644 --- a/spec/lib/gitlab/utils/markdown_spec.rb +++ b/spec/lib/gitlab/utils/markdown_spec.rb @@ -52,6 +52,22 @@ RSpec.describe Gitlab::Utils::Markdown do end end + context 'when string has a product suffix' do + let(:string) { 'My Header (ULTIMATE)' } + + it 'ignores a product suffix' do + is_expected.to eq 'my-header' + end + + context 'with only modifier' do + let(:string) { 'My Header (STARTER ONLY)' } + + it 'ignores a product suffix' do + is_expected.to eq 'my-header' + end + end + end + context 'when string is empty' do let(:string) { '' } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 297afd4d3ad..35f912f4875 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -221,6 +221,26 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end end + describe '.ci_sources' do + subject { described_class.ci_sources } + + let!(:push_pipeline) { create(:ci_pipeline, source: :push) } + let!(:web_pipeline) { create(:ci_pipeline, source: :web) } + let!(:api_pipeline) { create(:ci_pipeline, source: :api) } + let!(:webide_pipeline) { create(:ci_pipeline, source: :webide) } + let!(:child_pipeline) { create(:ci_pipeline, source: :parent_pipeline) } + + it 'contains pipelines having CI only sources' do + expect(subject).to contain_exactly(push_pipeline, web_pipeline, api_pipeline) + end + + it 'filters on expected sources' do + expect(::Enums::Ci::Pipeline.ci_sources.keys).to contain_exactly( + *%i[unknown push web trigger schedule api external pipeline chat + merge_request_event external_pull_request_event]) + end + end + describe '.outside_pipeline_family' do subject(:outside_pipeline_family) { described_class.outside_pipeline_family(upstream_pipeline) } @@ -1239,14 +1259,42 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end describe 'pipeline caching' do - before do - pipeline.config_source = 'repository_source' + context 'if pipeline is cacheable' do + before do + pipeline.source = 'push' + end + + it 'performs ExpirePipelinesCacheWorker' do + expect(ExpirePipelineCacheWorker).to receive(:perform_async).with(pipeline.id) + + pipeline.cancel + end end - it 'performs ExpirePipelinesCacheWorker' do - expect(ExpirePipelineCacheWorker).to receive(:perform_async).with(pipeline.id) + context 'if pipeline is not cacheable' do + before do + pipeline.source = 'webide' + end - pipeline.cancel + it 'deos not perform ExpirePipelinesCacheWorker' do + expect(ExpirePipelineCacheWorker).not_to receive(:perform_async) + + pipeline.cancel + end + end + end + + describe '#dangling?' do + it 'returns true if pipeline comes from any dangling sources' do + pipeline.source = Enums::Ci::Pipeline.dangling_sources.each_key.first + + expect(pipeline).to be_dangling + end + + it 'returns true if pipeline comes from any CI sources' do + pipeline.source = Enums::Ci::Pipeline.ci_sources.each_key.first + + expect(pipeline).not_to be_dangling end end @@ -1959,12 +2007,12 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do describe '.last_finished_for_ref_id' do let(:branch) { project.default_branch } let(:ref) { project.ci_refs.take } - let(:config_source) { Enums::Ci::Pipeline.config_sources[:parameter_source] } + let(:dangling_source) { Enums::Ci::Pipeline.sources[:ondemand_dast_scan] } let!(:pipeline1) { create(:ci_pipeline, :success, project: project, ref: branch) } let!(:pipeline2) { create(:ci_pipeline, :success, project: project, ref: branch) } let!(:pipeline3) { create(:ci_pipeline, :failed, project: project, ref: branch) } let!(:pipeline4) { create(:ci_pipeline, :success, project: project, ref: branch) } - let!(:pipeline5) { create(:ci_pipeline, :success, project: project, ref: branch, config_source: config_source) } + let!(:pipeline5) { create(:ci_pipeline, :success, project: project, ref: branch, source: dangling_source) } it 'returns the expected pipeline' do result = described_class.last_finished_for_ref_id(ref.id) diff --git a/spec/models/ci/ref_spec.rb b/spec/models/ci/ref_spec.rb index c9069d82e07..cb62646532c 100644 --- a/spec/models/ci/ref_spec.rb +++ b/spec/models/ci/ref_spec.rb @@ -107,8 +107,8 @@ RSpec.describe Ci::Ref do describe '#last_finished_pipeline_id' do let(:pipeline_status) { :running } - let(:config_source) { Enums::Ci::Pipeline.config_sources[:repository_source] } - let(:pipeline) { create(:ci_pipeline, pipeline_status, config_source: config_source) } + let(:pipeline_source) { Enums::Ci::Pipeline.sources[:push] } + let(:pipeline) { create(:ci_pipeline, pipeline_status, source: pipeline_source) } let(:ci_ref) { pipeline.ci_ref } context 'when there are no finished pipelines' do @@ -124,8 +124,8 @@ RSpec.describe Ci::Ref do expect(ci_ref.last_finished_pipeline_id).to eq(pipeline.id) end - context 'when the pipeline is not a ci_source' do - let(:config_source) { Enums::Ci::Pipeline.config_sources[:parameter_source] } + context 'when the pipeline a dangling pipeline' do + let(:pipeline_source) { Enums::Ci::Pipeline.sources[:ondemand_dast_scan] } it 'returns nil' do expect(ci_ref.last_finished_pipeline_id).to be_nil diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 8d2eb3b5e2a..b00395cd926 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -854,4 +854,41 @@ RSpec.describe Issuable do it { is_expected.to eq(incident) } end end + + describe '#severity' do + subject { issuable.severity } + + context 'when issuable is not an incident' do + using RSpec::Parameterized::TableSyntax + + where(:issuable_type, :severity) do + :issue | 'unknown' + :merge_request | 'unknown' + end + + with_them do + let(:issuable) { build_stubbed(issuable_type) } + + it { is_expected.to eq(severity) } + end + end + + context 'when issuable type is an incident' do + let!(:issuable) { build_stubbed(:incident) } + + context 'when incident does not have issuable_severity' do + it 'returns default serverity' do + is_expected.to eq(IssuableSeverity::DEFAULT) + end + end + + context 'when incident has issuable_severity' do + let!(:issuable_severity) { build_stubbed(:issuable_severity, issue: issuable, severity: 'critical') } + + it 'returns issuable serverity' do + is_expected.to eq('critical') + end + end + end + end end diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index 6ea94b4cd1a..1c7b11257ce 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -44,10 +44,14 @@ RSpec.describe Deployment do describe 'modules' do it_behaves_like 'AtomicInternalId' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:deployable) { create(:ci_build, project: project) } + let_it_be(:environment) { create(:environment, project: project) } + let(:internal_id_attribute) { :iid } - let(:instance) { build(:deployment) } + let(:instance) { build(:deployment, deployable: deployable, environment: environment) } let(:scope) { :project } - let(:scope_attrs) { { project: instance.project } } + let(:scope_attrs) { { project: project } } let(:usage) { :deployments } end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index f869b586fbe..f1902d19993 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -1212,4 +1212,14 @@ RSpec.describe Issue do end end end + + describe '#allows_reviewers?' do + it 'returns false as issues do not support reviewers feature' do + stub_feature_flags(merge_request_reviewers: true) + + issue = build_stubbed(:issue) + + expect(issue.allows_reviewers?).to be(false) + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c9389ad7f43..3563bbb2867 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -881,8 +881,10 @@ RSpec.describe MergeRequest, factory_default: :keep do end describe '#diff_size' do + let_it_be(:project) { create(:project, :repository) } + let(:merge_request) do - build(:merge_request, source_branch: 'expand-collapse-files', target_branch: 'master') + build(:merge_request, source_project: project, source_branch: 'expand-collapse-files', target_branch: 'master') end context 'when there are MR diffs' do @@ -1512,7 +1514,9 @@ RSpec.describe MergeRequest, factory_default: :keep do end context 'new merge request' do - subject { build(:merge_request) } + let_it_be(:project) { create(:project, :repository) } + + subject { build(:merge_request, source_project: project) } context 'compare commits' do before do @@ -2116,11 +2120,13 @@ RSpec.describe MergeRequest, factory_default: :keep do end context 'when merge request is not persisted' do + let_it_be(:project) { create(:project, :repository) } + context 'when compare commits are set in the service' do let(:commit) { spy('commit') } subject do - build(:merge_request, compare_commits: [commit, commit]) + build(:merge_request, source_project: project, compare_commits: [commit, commit]) end it 'returns commits from compare commits temporary data' do @@ -2129,7 +2135,7 @@ RSpec.describe MergeRequest, factory_default: :keep do end context 'when compare commits are not set in the service' do - subject { build(:merge_request) } + subject { build(:merge_request, source_project: project) } it 'returns array with diff head sha element only' do expect(subject.all_commit_shas).to eq [subject.diff_head_sha] @@ -2467,6 +2473,57 @@ RSpec.describe MergeRequest, factory_default: :keep do expect(subject.mergeable?).to be_truthy end + + context 'with skip_ci_check option' do + using RSpec::Parameterized::TableSyntax + + before do + allow(subject).to receive_messages(check_mergeability: nil, + can_be_merged?: true, + broken?: false) + end + + where(:mergeable_ci_state, :skip_ci_check, :expected_mergeable) do + false | false | false + false | true | true + true | false | true + true | true | true + end + + with_them do + it 'overrides mergeable_ci_state?' do + allow(subject).to receive(:mergeable_ci_state?) { mergeable_ci_state } + + expect(subject.mergeable?(skip_ci_check: skip_ci_check)).to eq(expected_mergeable) + end + end + end + + context 'with skip_discussions_check option' do + using RSpec::Parameterized::TableSyntax + + before do + allow(subject).to receive_messages(mergeable_ci_state?: true, + check_mergeability: nil, + can_be_merged?: true, + broken?: false) + end + + where(:mergeable_discussions_state, :skip_discussions_check, :expected_mergeable) do + false | false | false + false | true | true + true | false | true + true | true | true + end + + with_them do + it 'overrides mergeable_discussions_state?' do + allow(subject).to receive(:mergeable_discussions_state?) { mergeable_discussions_state } + + expect(subject.mergeable?(skip_discussions_check: skip_discussions_check)).to eq(expected_mergeable) + end + end + end end describe '#check_mergeability' do @@ -2570,6 +2627,10 @@ RSpec.describe MergeRequest, factory_default: :keep do it 'returns false' do expect(subject.mergeable_state?).to be_falsey end + + it 'returns true when skipping ci check' do + expect(subject.mergeable_state?(skip_ci_check: true)).to be(true) + end end context 'when #mergeable_discussions_state? is false' do @@ -2637,9 +2698,9 @@ RSpec.describe MergeRequest, factory_default: :keep do let(:pipeline) { create(:ci_empty_pipeline) } context 'when it is only allowed to merge when build is green' do - let_it_be(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) } + let_it_be(:project) { create(:project, :repository, only_allow_merge_if_pipeline_succeeds: true) } - subject { build(:merge_request, target_project: project) } + subject { build(:merge_request, source_project: project) } context 'and a failed pipeline is associated' do before do @@ -2678,9 +2739,9 @@ RSpec.describe MergeRequest, factory_default: :keep do end context 'when it is only allowed to merge when build is green or skipped' do - let_it_be(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true, allow_merge_on_skipped_pipeline: true) } + let_it_be(:project) { create(:project, :repository, only_allow_merge_if_pipeline_succeeds: true, allow_merge_on_skipped_pipeline: true) } - subject { build(:merge_request, target_project: project) } + subject { build(:merge_request, source_project: project) } context 'and a failed pipeline is associated' do before do @@ -2719,9 +2780,9 @@ RSpec.describe MergeRequest, factory_default: :keep do end context 'when merges are not restricted to green builds' do - let_it_be(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: false) } + let_it_be(:project) { create(:project, :repository, only_allow_merge_if_pipeline_succeeds: false) } - subject { build(:merge_request, target_project: project) } + subject { build(:merge_request, source_project: project) } context 'and a failed pipeline is associated' do before do @@ -4145,4 +4206,22 @@ RSpec.describe MergeRequest, factory_default: :keep do .with_arguments(allow_nil: true) end end + + describe '#allows_reviewers?' do + it 'returns false without merge_request_reviewers feature' do + stub_feature_flags(merge_request_reviewers: false) + + merge_request = build_stubbed(:merge_request) + + expect(merge_request.allows_reviewers?).to be(false) + end + + it 'returns true with merge_request_reviewers feature' do + stub_feature_flags(merge_request_reviewers: true) + + merge_request = build_stubbed(:merge_request) + + expect(merge_request.allows_reviewers?).to be(true) + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8ed0672af25..efa2353259c 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -522,9 +522,10 @@ RSpec.describe Project do before do create(:ci_pipeline, project: project, ref: 'master', source: :web) create(:ci_pipeline, project: project, ref: 'master', source: :external) + create(:ci_pipeline, project: project, ref: 'master', source: :webide) end - it 'has ci pipelines' do + it 'excludes dangling pipelines such as :webide' do expect(project.ci_pipelines.size).to eq(2) end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3733e47334a..a0ec61c4117 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -712,30 +712,40 @@ RSpec.describe User do expect(users_with_two_factor).not_to include(user_without_2fa.id) end - it "returns users with 2fa enabled via U2F" do - user_with_2fa = create(:user, :two_factor_via_u2f) - user_without_2fa = create(:user) - users_with_two_factor = described_class.with_two_factor.pluck(:id) + shared_examples "returns the right users" do |trait| + it "returns users with 2fa enabled via hardware token" do + user_with_2fa = create(:user, trait) + user_without_2fa = create(:user) + users_with_two_factor = described_class.with_two_factor.pluck(:id) - expect(users_with_two_factor).to include(user_with_2fa.id) - expect(users_with_two_factor).not_to include(user_without_2fa.id) - end + expect(users_with_two_factor).to include(user_with_2fa.id) + expect(users_with_two_factor).not_to include(user_without_2fa.id) + end - it "returns users with 2fa enabled via OTP and U2F" do - user_with_2fa = create(:user, :two_factor_via_otp, :two_factor_via_u2f) - user_without_2fa = create(:user) - users_with_two_factor = described_class.with_two_factor.pluck(:id) + it "returns users with 2fa enabled via OTP and hardware token" do + user_with_2fa = create(:user, :two_factor_via_otp, trait) + user_without_2fa = create(:user) + users_with_two_factor = described_class.with_two_factor.pluck(:id) - expect(users_with_two_factor).to eq([user_with_2fa.id]) - expect(users_with_two_factor).not_to include(user_without_2fa.id) + expect(users_with_two_factor).to eq([user_with_2fa.id]) + expect(users_with_two_factor).not_to include(user_without_2fa.id) + end + + it 'works with ORDER BY' do + user_with_2fa = create(:user, :two_factor_via_otp, trait) + + expect(described_class + .with_two_factor + .reorder_by_name).to eq([user_with_2fa]) + end end - it 'works with ORDER BY' do - user_with_2fa = create(:user, :two_factor_via_otp, :two_factor_via_u2f) + describe "and U2F" do + it_behaves_like "returns the right users", :two_factor_via_u2f + end - expect(described_class - .with_two_factor - .reorder_by_name).to eq([user_with_2fa]) + describe "and WebAuthn" do + it_behaves_like "returns the right users", :two_factor_via_webauthn end end @@ -749,22 +759,44 @@ RSpec.describe User do expect(users_without_two_factor).not_to include(user_with_2fa.id) end - it "excludes users with 2fa enabled via U2F" do - user_with_2fa = create(:user, :two_factor_via_u2f) - user_without_2fa = create(:user) - users_without_two_factor = described_class.without_two_factor.pluck(:id) + describe "and u2f" do + it "excludes users with 2fa enabled via U2F" do + user_with_2fa = create(:user, :two_factor_via_u2f) + user_without_2fa = create(:user) + users_without_two_factor = described_class.without_two_factor.pluck(:id) - expect(users_without_two_factor).to include(user_without_2fa.id) - expect(users_without_two_factor).not_to include(user_with_2fa.id) + expect(users_without_two_factor).to include(user_without_2fa.id) + expect(users_without_two_factor).not_to include(user_with_2fa.id) + end + + it "excludes users with 2fa enabled via OTP and U2F" do + user_with_2fa = create(:user, :two_factor_via_otp, :two_factor_via_u2f) + user_without_2fa = create(:user) + users_without_two_factor = described_class.without_two_factor.pluck(:id) + + expect(users_without_two_factor).to include(user_without_2fa.id) + expect(users_without_two_factor).not_to include(user_with_2fa.id) + end end - it "excludes users with 2fa enabled via OTP and U2F" do - user_with_2fa = create(:user, :two_factor_via_otp, :two_factor_via_u2f) - user_without_2fa = create(:user) - users_without_two_factor = described_class.without_two_factor.pluck(:id) + describe "and webauthn" do + it "excludes users with 2fa enabled via WebAuthn" do + user_with_2fa = create(:user, :two_factor_via_webauthn) + user_without_2fa = create(:user) + users_without_two_factor = described_class.without_two_factor.pluck(:id) - expect(users_without_two_factor).to include(user_without_2fa.id) - expect(users_without_two_factor).not_to include(user_with_2fa.id) + expect(users_without_two_factor).to include(user_without_2fa.id) + expect(users_without_two_factor).not_to include(user_with_2fa.id) + end + + it "excludes users with 2fa enabled via OTP and WebAuthn" do + user_with_2fa = create(:user, :two_factor_via_otp, :two_factor_via_webauthn) + user_without_2fa = create(:user) + users_without_two_factor = described_class.without_two_factor.pluck(:id) + + expect(users_without_two_factor).to include(user_without_2fa.id) + expect(users_without_two_factor).not_to include(user_with_2fa.id) + end end end diff --git a/spec/requests/api/ci/pipelines_spec.rb b/spec/requests/api/ci/pipelines_spec.rb index 20602f24078..3b611b2bb9f 100644 --- a/spec/requests/api/ci/pipelines_spec.rb +++ b/spec/requests/api/ci/pipelines_spec.rb @@ -476,17 +476,18 @@ RSpec.describe API::Ci::Pipelines do end end - context 'when config source is not ci' do - let(:non_ci_config_source) { Enums::Ci::Pipeline.non_ci_config_source_values.first } - let(:pipeline_not_ci) do - create(:ci_pipeline, config_source: non_ci_config_source, project: project) + context 'when pipeline is a dangling pipeline' do + let(:dangling_source) { Enums::Ci::Pipeline.dangling_sources.each_value.first } + + let(:dangling_pipeline) do + create(:ci_pipeline, source: dangling_source, project: project) end it 'returns the specified pipeline' do - get api("/projects/#{project.id}/pipelines/#{pipeline_not_ci.id}", user) + get api("/projects/#{project.id}/pipelines/#{dangling_pipeline.id}", user) expect(response).to have_gitlab_http_status(:ok) - expect(json_response['sha']).to eq(pipeline_not_ci.sha) + expect(json_response['sha']).to eq(dangling_pipeline.sha) end end end diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index f90e04bbd2e..b63a155a729 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -239,13 +239,13 @@ RSpec.describe API::Jobs do end end - context 'when config source not ci' do - let(:non_ci_config_source) { Enums::Ci::Pipeline.non_ci_config_source_values.first } + context 'when jobs belong to a dangling pipeline' do + let(:dangling_source) { Enums::Ci::Pipeline.dangling_sources.each_value.first } let(:pipeline) do - create(:ci_pipeline, config_source: non_ci_config_source, project: project) + create(:ci_pipeline, source: dangling_source, project: project) end - it 'returns the specified pipeline' do + it 'returns pipeline jobs' do expect(response).to have_gitlab_http_status(:ok) expect(json_response[0]['pipeline']['sha']).to eq(pipeline.sha.to_s) end diff --git a/spec/serializers/diff_file_base_entity_spec.rb b/spec/serializers/diff_file_base_entity_spec.rb index bf69a50a072..94c39e11790 100644 --- a/spec/serializers/diff_file_base_entity_spec.rb +++ b/spec/serializers/diff_file_base_entity_spec.rb @@ -7,21 +7,50 @@ RSpec.describe DiffFileBaseEntity do let(:repository) { project.repository } let(:entity) { described_class.new(diff_file, options).as_json } - context 'diff for a changed submodule' do - let(:commit_sha_with_changed_submodule) do - "cfe32cf61b73a0d5e9f13e774abde7ff789b1660" - end - - let(:commit) { project.commit(commit_sha_with_changed_submodule) } + context 'submodule information for a' do + let(:commit_sha) { "" } + let(:commit) { project.commit(commit_sha) } let(:options) { { request: {}, submodule_links: Gitlab::SubmoduleLinks.new(repository) } } let(:diff_file) { commit.diffs.diff_files.to_a.last } - it do - expect(entity[:submodule]).to eq(true) - expect(entity[:submodule_link]).to eq("https://github.com/randx/six") - expect(entity[:submodule_tree_url]).to eq( - "https://github.com/randx/six/tree/409f37c4f05865e4fb208c771485f211a22c4c2d" - ) + context 'newly added submodule' do + let(:commit_sha) { "cfe32cf61b73a0d5e9f13e774abde7ff789b1660" } + + it 'says it is a submodule and contains links' do + expect(entity[:submodule]).to eq(true) + expect(entity[:submodule_link]).to eq("https://github.com/randx/six") + expect(entity[:submodule_tree_url]).to eq( + "https://github.com/randx/six/tree/409f37c4f05865e4fb208c771485f211a22c4c2d" + ) + end + + it 'has no compare url because the submodule was newly added' do + expect(entity[:submodule_compare]).to eq(nil) + end + end + + context 'changed submodule' do + # Test repo does not contain a commit that changes a submodule, so we have create such a commit here + let(:commit_sha) { repository.update_submodule(project.members[0].user, "six", "c6bc3aa2ee76cadaf0cbd325067c55450997632e", message: "Go one commit back", branch: "master") } + + it 'contains a link to compare the changes' do + expect(entity[:submodule_compare]).to eq( + { + url: "https://github.com/randx/six/compare/409f37c4f05865e4fb208c771485f211a22c4c2d...c6bc3aa2ee76cadaf0cbd325067c55450997632e", + old_sha: "409f37c4f05865e4fb208c771485f211a22c4c2d", + new_sha: "c6bc3aa2ee76cadaf0cbd325067c55450997632e" + } + ) + end + end + + context 'normal file (no submodule)' do + let(:commit_sha) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' } + + it 'sets submodule to false' do + expect(entity[:submodule]).to eq(false) + expect(entity[:submodule_compare]).to eq(nil) + end end end diff --git a/spec/serializers/merge_request_basic_entity_spec.rb b/spec/serializers/merge_request_basic_entity_spec.rb index 1cddd87e917..4a8bcd72d9c 100644 --- a/spec/serializers/merge_request_basic_entity_spec.rb +++ b/spec/serializers/merge_request_basic_entity_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe MergeRequestBasicEntity do - let(:resource) { build(:merge_request) } + let(:resource) { build(:merge_request, params) } + let(:params) { {} } subject do described_class.new(resource).as_json @@ -14,4 +15,28 @@ RSpec.describe MergeRequestBasicEntity do expect(subject[:merge_status]).to eq 'checking' end + + describe '#reviewers' do + let(:params) { { reviewers: [reviewer] } } + let(:reviewer) { build(:user) } + + context 'when merge_request_reviewers feature is disabled' do + it 'does not contain assignees attributes' do + stub_feature_flags(merge_request_reviewers: false) + + expect(subject[:reviewers]).to be_nil + end + end + + context 'when merge_request_reviewers feature is enabled' do + it 'contains reviewers attributes' do + stub_feature_flags(merge_request_reviewers: true) + + expect(subject[:reviewers].count).to be 1 + expect(subject[:reviewers].first.keys).to include( + :id, :name, :username, :state, :avatar_url, :web_url + ) + end + end + end end diff --git a/spec/serializers/merge_request_sidebar_extras_entity_spec.rb b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb new file mode 100644 index 00000000000..74e9956c8a0 --- /dev/null +++ b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequestSidebarExtrasEntity do + let_it_be(:assignee) { build(:user) } + let_it_be(:reviewer) { build(:user) } + let_it_be(:user) { build(:user) } + let_it_be(:project) { create :project, :repository } + + let(:params) do + { + source_project: project, + target_project: project, + assignees: [assignee], + reviewers: [reviewer] + } + end + + let(:resource) do + build(:merge_request, params) + end + + let(:request) { double('request', current_user: user, project: project) } + + let(:entity) { described_class.new(resource, request: request).as_json } + + describe '#assignees' do + it 'contains assignees attributes' do + expect(entity[:assignees].count).to be 1 + expect(entity[:assignees].first.keys).to include( + :id, :name, :username, :state, :avatar_url, :web_url, :can_merge + ) + end + end + + describe '#reviewers' do + context 'when merge_request_reviewers feature is disabled' do + it 'does not contain reviewers attributes' do + stub_feature_flags(merge_request_reviewers: false) + + expect(entity[:reviewers]).to be_nil + end + end + + context 'when merge_request_reviewers feature is enabled' do + it 'contains reviewers attributes' do + stub_feature_flags(merge_request_reviewers: true) + + expect(entity[:reviewers].count).to be 1 + expect(entity[:reviewers].first.keys).to include( + :id, :name, :username, :state, :avatar_url, :web_url, :can_merge + ) + end + end + end +end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index 5ccefe3d129..941e1f20999 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -339,6 +339,10 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do end end end + + it_behaves_like 'reviewer_ids filter' do + let(:execute) { service.execute } + end end it_behaves_like 'issuable record that supports quick actions' do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 57733c7772d..2b0adbd7b5d 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -435,6 +435,43 @@ RSpec.describe MergeRequests::MergeService do end end end + + context 'when not mergeable' do + let!(:error_message) { 'Merge request is not mergeable' } + + context 'with failing CI' do + before do + allow(merge_request).to receive(:mergeable_ci_state?) { false } + end + + it 'logs and saves error' do + service.execute(merge_request) + + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + end + + context 'with unresolved discussions' do + before do + allow(merge_request).to receive(:mergeable_discussions_state?) { false } + end + + it 'logs and saves error' do + service.execute(merge_request) + + expect(Gitlab::AppLogger).to have_received(:error).with(a_string_matching(error_message)) + end + + context 'when passing `skip_discussions_check: true` as `options` parameter' do + it 'merges the merge request' do + service.execute(merge_request, skip_discussions_check: true) + + expect(merge_request).to be_valid + expect(merge_request).to be_merged + end + end + end + end end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 5f5a94da400..097115af4ef 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -161,6 +161,29 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request.merge_params["force_remove_source_branch"]).to eq("1") end end + + it_behaves_like 'reviewer_ids filter' do + let(:opts) { {} } + let(:execute) { update_merge_request(opts) } + end + + context 'with an existing reviewer' do + let(:merge_request) do + create(:merge_request, :simple, source_project: project, reviewer_ids: [user2.id]) + end + + context 'when merge_request_reviewer feature is enabled' do + before do + stub_feature_flags(merge_request_reviewer: true) + end + + let(:opts) { { reviewer_ids: [IssuableFinder::Params::NONE] } } + + it 'removes reviewers' do + expect(update_merge_request(opts).reviewers).to eq [] + end + end + end end context 'after_save callback to store_mentions' do diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 87d6f99e8aa..498566c5d60 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -8,9 +8,9 @@ RSpec.describe NotificationService, :mailer do include NotificationHelpers let_it_be(:project, reload: true) { create(:project, :public) } + let_it_be_with_refind(:assignee) { create(:user) } let(:notification) { described_class.new } - let(:assignee) { create(:user) } around(:example, :deliver_mails_inline) do |example| # This is a temporary `around` hook until all the examples check the @@ -1585,20 +1585,21 @@ RSpec.describe NotificationService, :mailer do describe 'Merge Requests', :deliver_mails_inline do let(:another_project) { create(:project, :public, namespace: group) } let(:assignees) { Array.wrap(assignee) } - let(:author) { create(:user) } let(:merge_request) { create :merge_request, author: author, source_project: project, assignees: assignees, description: 'cc @participant' } + let_it_be_with_reload(:author) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, :public, :repository, namespace: group) } before_all do build_team(project) add_users(project) + + project.add_maintainer(author) + project.add_maintainer(assignee) end before do - project.add_maintainer(author) - assignees.each { |assignee| project.add_maintainer(assignee) } add_user_subscriptions(merge_request) update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) update_custom_notification(:new_merge_request, @u_custom_global) diff --git a/spec/services/webauthn/authenticate_service_spec.rb b/spec/services/webauthn/authenticate_service_spec.rb new file mode 100644 index 00000000000..61f64f24f5e --- /dev/null +++ b/spec/services/webauthn/authenticate_service_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'webauthn/fake_client' + +RSpec.describe Webauthn::AuthenticateService do + let(:client) { WebAuthn::FakeClient.new(origin) } + let(:user) { create(:user) } + let(:challenge) { Base64.strict_encode64(SecureRandom.random_bytes(32)) } + + let(:origin) { 'http://localhost' } + + before do + create_result = client.create(challenge: challenge) # rubocop:disable Rails/SaveBang + + webauthn_credential = WebAuthn::Credential.from_create(create_result) + + registration = WebauthnRegistration.new(credential_xid: Base64.strict_encode64(webauthn_credential.raw_id), + public_key: webauthn_credential.public_key, + counter: 0, + name: 'name', + user_id: user.id) + registration.save! + end + + describe '#execute' do + it 'returns true if the response is valid and a matching stored credential is present' do + get_result = client.get(challenge: challenge) + + get_result['clientExtensionResults'] = {} + service = Webauthn::AuthenticateService.new(user, get_result.to_json, challenge) + + expect(service.execute).to be_truthy + end + + it 'returns false if the response is valid but no matching stored credential is present' do + other_client = WebAuthn::FakeClient.new(origin) + other_client.create(challenge: challenge) # rubocop:disable Rails/SaveBang + + get_result = other_client.get(challenge: challenge) + + get_result['clientExtensionResults'] = {} + service = Webauthn::AuthenticateService.new(user, get_result.to_json, challenge) + + expect(service.execute).to be_falsey + end + end +end diff --git a/spec/services/webauthn/register_service_spec.rb b/spec/services/webauthn/register_service_spec.rb new file mode 100644 index 00000000000..bb9fa2080d2 --- /dev/null +++ b/spec/services/webauthn/register_service_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'webauthn/fake_client' + +RSpec.describe Webauthn::RegisterService do + let(:client) { WebAuthn::FakeClient.new(origin) } + let(:user) { create(:user) } + let(:challenge) { Base64.strict_encode64(SecureRandom.random_bytes(32)) } + + let(:origin) { 'http://localhost' } + + describe '#execute' do + it 'returns a registration if challenge matches' do + create_result = client.create(challenge: challenge) # rubocop:disable Rails/SaveBang + webauthn_credential = WebAuthn::Credential.from_create(create_result) + + params = { device_response: create_result.to_json, name: 'abc' } + service = Webauthn::RegisterService.new(user, params, challenge) + + registration = service.execute + expect(registration.credential_xid).to eq(Base64.strict_encode64(webauthn_credential.raw_id)) + expect(registration.errors.size).to eq(0) + end + + it 'returns an error if challenge does not match' do + create_result = client.create(challenge: Base64.strict_encode64(SecureRandom.random_bytes(16))) # rubocop:disable Rails/SaveBang + + params = { device_response: create_result.to_json, name: 'abc' } + service = Webauthn::RegisterService.new(user, params, challenge) + + registration = service.execute + expect(registration.errors.size).to eq(1) + end + end +end diff --git a/spec/support/helpers/fake_u2f_device.rb b/spec/support/helpers/fake_u2f_device.rb index f765b277175..2ed1222ebd3 100644 --- a/spec/support/helpers/fake_u2f_device.rb +++ b/spec/support/helpers/fake_u2f_device.rb @@ -3,9 +3,10 @@ class FakeU2fDevice attr_reader :name - def initialize(page, name) + def initialize(page, name, device = nil) @page = page @name = name + @u2f_device = device end def respond_to_u2f_registration diff --git a/spec/support/helpers/fake_webauthn_device.rb b/spec/support/helpers/fake_webauthn_device.rb new file mode 100644 index 00000000000..d2c2f7d6bf3 --- /dev/null +++ b/spec/support/helpers/fake_webauthn_device.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true +require 'webauthn/fake_client' + +class FakeWebauthnDevice + attr_reader :name + + def initialize(page, name, device = nil) + @page = page + @name = name + @webauthn_device = device + end + + def respond_to_webauthn_registration + app_id = @page.evaluate_script('gon.webauthn.app_id') + challenge = @page.evaluate_script('gon.webauthn.options.challenge') + + json_response = webauthn_device(app_id).create(challenge: challenge).to_json # rubocop:disable Rails/SaveBang + @page.execute_script <<~JS + var result = #{json_response}; + result.getClientExtensionResults = () => ({}); + navigator.credentials.create = function(_) { + return Promise.resolve(result); + }; + JS + end + + def respond_to_webauthn_authentication + app_id = @page.evaluate_script('JSON.parse(gon.webauthn.options).extensions.appid') + challenge = @page.evaluate_script('JSON.parse(gon.webauthn.options).challenge') + + begin + json_response = webauthn_device(app_id).get(challenge: challenge).to_json + + rescue RuntimeError + # A runtime error is raised from fake webauthn if no credentials have been registered yet. + # To be able to test non registered devices, credentials are created ad-hoc + webauthn_device(app_id).create # rubocop:disable Rails/SaveBang + json_response = webauthn_device(app_id).get(challenge: challenge).to_json + end + + @page.execute_script <<~JS + var result = #{json_response}; + result.getClientExtensionResults = () => ({}); + navigator.credentials.get = function(_) { + return Promise.resolve(result); + }; + JS + @page.click_link('Try again?', href: false) + end + + def fake_webauthn_authentication + @page.execute_script <<~JS + const mockResponse = { + type: 'public-key', + id: '', + rawId: '', + response: { clientDataJSON: '', authenticatorData: '', signature: '', userHandle: '' }, + getClientExtensionResults: () => {}, + }; + window.gl.resolveWebauthn(mockResponse); + JS + end + + def add_credential(app_id, credential_id, credential_key) + credentials = { URI.parse(app_id).host => { credential_id => { credential_key: credential_key, sign_count: 0 } } } + webauthn_device(app_id).send(:authenticator).instance_variable_set(:@credentials, credentials) + end + + private + + def webauthn_device(app_id) + @webauthn_device ||= WebAuthn::FakeClient.new(app_id) + end +end diff --git a/spec/support/helpers/features/two_factor_helpers.rb b/spec/support/helpers/features/two_factor_helpers.rb new file mode 100644 index 00000000000..08a7665201f --- /dev/null +++ b/spec/support/helpers/features/two_factor_helpers.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true +# These helpers allow you to manage and register +# U2F and WebAuthn devices +# +# Usage: +# describe "..." do +# include Spec::Support::Helpers::Features::TwoFactorHelpers +# ... +# +# manage_two_factor_authentication +# +module Spec + module Support + module Helpers + module Features + module TwoFactorHelpers + def manage_two_factor_authentication + click_on 'Manage two-factor authentication' + expect(page).to have_content("Set up new device") + wait_for_requests + end + + def register_u2f_device(u2f_device = nil, name: 'My device') + u2f_device ||= FakeU2fDevice.new(page, name) + u2f_device.respond_to_u2f_registration + click_on 'Set up new device' + expect(page).to have_content('Your device was successfully set up') + fill_in "Pick a name", with: name + click_on 'Register device' + u2f_device + end + + # Registers webauthn device via UI + def register_webauthn_device(webauthn_device = nil, name: 'My device') + webauthn_device ||= FakeWebauthnDevice.new(page, name) + webauthn_device.respond_to_webauthn_registration + click_on 'Set up new device' + expect(page).to have_content('Your device was successfully set up') + fill_in 'Pick a name', with: name + click_on 'Register device' + webauthn_device + end + + # Adds webauthn device directly via database + def add_webauthn_device(app_id, user, fake_device = nil, name: 'My device') + fake_device ||= WebAuthn::FakeClient.new(app_id) + + options_for_create = WebAuthn::Credential.options_for_create( + user: { id: user.webauthn_xid, name: user.username }, + authenticator_selection: { user_verification: 'discouraged' }, + rp: { name: 'GitLab' } + ) + challenge = options_for_create.challenge + + device_response = fake_device.create(challenge: challenge).to_json # rubocop:disable Rails/SaveBang + device_registration_params = { device_response: device_response, + name: name } + + Webauthn::RegisterService.new( + user, device_registration_params, challenge).execute + FakeWebauthnDevice.new(page, name, fake_device) + end + + def assert_fallback_ui(page) + expect(page).to have_button('Verify code') + expect(page).to have_css('#user_otp_attempt') + expect(page).not_to have_link('Sign in via 2FA code') + expect(page).not_to have_css("#js-authenticate-token-2fa") + end + end + end + end + end +end diff --git a/spec/support/helpers/login_helpers.rb b/spec/support/helpers/login_helpers.rb index 1118cfcf7ac..e21d4497cda 100644 --- a/spec/support/helpers/login_helpers.rb +++ b/spec/support/helpers/login_helpers.rb @@ -111,6 +111,11 @@ module LoginHelpers FakeU2fDevice.new(page, nil).fake_u2f_authentication end + def fake_successful_webauthn_authentication + allow_any_instance_of(Webauthn::AuthenticateService).to receive(:execute).and_return(true) + FakeWebauthnDevice.new(page, nil).fake_webauthn_authentication + end + def mock_auth_hash_with_saml_xml(provider, uid, email, saml_response) response_object = { document: saml_xml(saml_response) } mock_auth_hash(provider, uid, email, response_object: response_object) diff --git a/spec/support/shared_examples/features/2fa_shared_examples.rb b/spec/support/shared_examples/features/2fa_shared_examples.rb new file mode 100644 index 00000000000..ddc03e178ba --- /dev/null +++ b/spec/support/shared_examples/features/2fa_shared_examples.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'hardware device for 2fa' do |device_type| + include Spec::Support::Helpers::Features::TwoFactorHelpers + + def register_device(device_type, **kwargs) + case device_type.downcase + when "u2f" + register_u2f_device(**kwargs) + when "webauthn" + register_webauthn_device(**kwargs) + else + raise "Unknown device type #{device_type}" + end + end + + describe "registration" do + let(:user) { create(:user) } + + before do + gitlab_sign_in(user) + user.update_attribute(:otp_required_for_login, true) + end + + describe 'when 2FA via OTP is disabled' do + before do + user.update_attribute(:otp_required_for_login, false) + end + + it 'does not allow registering a new device' do + visit profile_account_path + click_on 'Enable two-factor authentication' + + expect(page).to have_button("Set up new device", disabled: true) + end + end + + describe 'when 2FA via OTP is enabled' do + it 'allows registering a new device with a name' do + visit profile_account_path + manage_two_factor_authentication + expect(page).to have_content("You've already enabled two-factor authentication using one time password authenticators") + + device = register_device(device_type) + + expect(page).to have_content(device.name) + expect(page).to have_content("Your #{device_type} device was registered") + end + + it 'allows deleting a device' do + visit profile_account_path + manage_two_factor_authentication + expect(page).to have_content("You've already enabled two-factor authentication using one time password authenticators") + + first_device = register_device(device_type) + second_device = register_device(device_type, name: 'My other device') + + expect(page).to have_content(first_device.name) + expect(page).to have_content(second_device.name) + + accept_confirm { click_on 'Delete', match: :first } + + expect(page).to have_content('Successfully deleted') + expect(page.body).not_to have_content(first_device.name) + expect(page.body).to have_content(second_device.name) + end + end + end + + describe 'fallback code authentication' do + let(:user) { create(:user) } + + before do + # Register and logout + gitlab_sign_in(user) + user.update_attribute(:otp_required_for_login, true) + visit profile_account_path + end + + describe 'when no device is registered' do + before do + gitlab_sign_out + gitlab_sign_in(user) + end + + it 'shows the fallback otp code UI' do + assert_fallback_ui(page) + end + end + + describe 'when a device is registered' do + before do + manage_two_factor_authentication + register_device(device_type) + gitlab_sign_out + gitlab_sign_in(user) + end + + it 'provides a button that shows the fallback otp code UI' do + expect(page).to have_link('Sign in via 2FA code') + + click_link('Sign in via 2FA code') + + assert_fallback_ui(page) + end + end + end +end diff --git a/spec/support/shared_examples/services/merge_request_shared_examples.rb b/spec/support/shared_examples/services/merge_request_shared_examples.rb new file mode 100644 index 00000000000..a7032640217 --- /dev/null +++ b/spec/support/shared_examples/services/merge_request_shared_examples.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'reviewer_ids filter' do + context 'filter_reviewer' do + let(:opts) { super().merge(reviewer_ids_param) } + + context 'without reviewer_ids' do + let(:reviewer_ids_param) { {} } + + it 'contains no reviewer_ids' do + expect(execute.reviewers).to eq [] + end + end + + context 'with reviewer_ids' do + let(:reviewer_ids_param) { { reviewer_ids: [reviewer1.id, reviewer2.id, reviewer3.id] } } + + let(:reviewer1) { create(:user) } + let(:reviewer2) { create(:user) } + let(:reviewer3) { create(:user) } + + context 'when the current user can admin the merge_request' do + context 'when merge_request_reviewer feature is enabled' do + before do + stub_feature_flags(merge_request_reviewer: true) + end + + context 'with reviewers who can read the merge_request' do + before do + project.add_developer(reviewer1) + project.add_developer(reviewer2) + end + + it 'contains reviewers who can read the merge_request' do + expect(execute.reviewers).to contain_exactly(reviewer1, reviewer2) + end + end + end + + context 'when merge_request_reviewer feature is disabled' do + before do + stub_feature_flags(merge_request_reviewer: false) + end + + it 'contains no reviewers' do + expect(execute.reviewers).to eq [] + end + end + end + + context 'when the current_user cannot admin the merge_request' do + before do + project.add_developer(user) + end + + it 'contains no reviewers' do + expect(execute.reviewers).to eq [] + end + end + end + end +end diff --git a/spec/views/admin/sessions/two_factor.html.haml_spec.rb b/spec/views/admin/sessions/two_factor.html.haml_spec.rb index 9c5ff9925c1..c7e0edbcd58 100644 --- a/spec/views/admin/sessions/two_factor.html.haml_spec.rb +++ b/spec/views/admin/sessions/two_factor.html.haml_spec.rb @@ -32,6 +32,10 @@ RSpec.describe 'admin/sessions/two_factor.html.haml' do context 'user has u2f active' do let(:user) { create(:admin, :two_factor_via_u2f) } + before do + stub_feature_flags(webauthn: false) + end + it 'shows enter u2f form' do render diff --git a/spec/workers/ci/build_trace_chunk_flush_worker_spec.rb b/spec/workers/ci/build_trace_chunk_flush_worker_spec.rb new file mode 100644 index 00000000000..352ad6d4cf6 --- /dev/null +++ b/spec/workers/ci/build_trace_chunk_flush_worker_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::BuildTraceChunkFlushWorker do + let(:data) { 'x' * Ci::BuildTraceChunk::CHUNK_SIZE } + + let(:chunk) do + create(:ci_build_trace_chunk, :redis_with_data, initial_data: data) + end + + it 'migrates chunk to a permanent store' do + expect(chunk).to be_live + + described_class.new.perform(chunk.id) + + expect(chunk.reload).to be_persisted + end + + describe '#perform' do + it_behaves_like 'an idempotent worker' do + let(:job_args) { [chunk.id] } + + it 'migrates build trace chunk to a safe store' do + subject + + expect(chunk.reload).to be_persisted + end + end + end +end |