diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-09-30 22:02:13 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-09-30 22:02:13 +0000 |
commit | 516fba52cf280b9d5bad08dce9f0150f859b6cea (patch) | |
tree | 4dad71be856651af62c9a281b01087ae15480810 /spec | |
parent | c90be62bdefdb6bb67c73a9c4a6d164c9f78a28d (diff) | |
download | gitlab-ce-516fba52cf280b9d5bad08dce9f0150f859b6cea.tar.gz |
Add latest changes from gitlab-org/security/gitlab@13-4-stable-ee
Diffstat (limited to 'spec')
18 files changed, 799 insertions, 102 deletions
diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index 85f1b247ee9..4b9dd3629f1 100644 --- a/spec/controllers/groups/group_members_controller_spec.rb +++ b/spec/controllers/groups/group_members_controller_spec.rb @@ -139,6 +139,45 @@ RSpec.describe Groups::GroupMembersController do expect(group.users).not_to include group_user end end + + context 'access expiry date' do + before do + group.add_owner(user) + end + + subject do + post :create, params: { + group_id: group, + user_ids: group_user.id, + access_level: Gitlab::Access::GUEST, + expires_at: expires_at + } + end + + context 'when set to a date in the past' do + let(:expires_at) { 2.days.ago } + + it 'does not add user to members' do + subject + + expect(flash[:alert]).to include('Expires at cannot be a date in the past') + expect(response).to redirect_to(group_group_members_path(group)) + expect(group.users).not_to include group_user + end + end + + context 'when set to a date in the future' do + let(:expires_at) { 5.days.from_now } + + it 'adds user to members' do + subject + + expect(response).to set_flash.to 'Users were successfully added.' + expect(response).to redirect_to(group_group_members_path(group)) + expect(group.users).to include group_user + end + end + end end describe 'PUT update' do @@ -149,15 +188,49 @@ RSpec.describe Groups::GroupMembersController do sign_in(user) end - Gitlab::Access.options.each do |label, value| - it "can change the access level to #{label}" do - put :update, params: { - group_member: { access_level: value }, - group_id: group, - id: requester - }, xhr: true + context 'access level' do + Gitlab::Access.options.each do |label, value| + it "can change the access level to #{label}" do + put :update, params: { + group_member: { access_level: value }, + group_id: group, + id: requester + }, xhr: true - expect(requester.reload.human_access).to eq(label) + expect(requester.reload.human_access).to eq(label) + end + end + end + + context 'access expiry date' do + subject do + put :update, xhr: true, params: { + group_member: { + expires_at: expires_at + }, + group_id: group, + id: requester + } + end + + context 'when set to a date in the past' do + let(:expires_at) { 2.days.ago } + + it 'does not update the member' do + subject + + expect(requester.reload.expires_at).not_to eq(expires_at.to_date) + end + end + + context 'when set to a date in the future' do + let(:expires_at) { 5.days.from_now } + + it 'updates the member' do + subject + + expect(requester.reload.expires_at).to eq(expires_at.to_date) + end end end end diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb index 40a220d57a7..ae05e2d2631 100644 --- a/spec/controllers/projects/project_members_controller_spec.rb +++ b/spec/controllers/projects/project_members_controller_spec.rb @@ -129,6 +129,46 @@ RSpec.describe Projects::ProjectMembersController do expect(response).to redirect_to(project_project_members_path(project)) end end + + context 'access expiry date' do + before do + project.add_maintainer(user) + end + + subject do + post :create, params: { + namespace_id: project.namespace, + project_id: project, + user_ids: project_user.id, + access_level: Gitlab::Access::GUEST, + expires_at: expires_at + } + end + + context 'when set to a date in the past' do + let(:expires_at) { 2.days.ago } + + it 'does not add user to members' do + subject + + expect(flash[:alert]).to include('Expires at cannot be a date in the past') + expect(response).to redirect_to(project_project_members_path(project)) + expect(project.users).not_to include project_user + end + end + + context 'when set to a date in the future' do + let(:expires_at) { 5.days.from_now } + + it 'adds user to members' do + subject + + expect(response).to set_flash.to 'Users were successfully added.' + expect(response).to redirect_to(project_project_members_path(project)) + expect(project.users).to include project_user + end + end + end end describe 'PUT update' do @@ -139,16 +179,53 @@ RSpec.describe Projects::ProjectMembersController do sign_in(user) end - Gitlab::Access.options.each do |label, value| - it "can change the access level to #{label}" do - put :update, params: { - project_member: { access_level: value }, - namespace_id: project.namespace, - project_id: project, - id: requester - }, xhr: true + context 'access level' do + Gitlab::Access.options.each do |label, value| + it "can change the access level to #{label}" do + params = { + project_member: { access_level: value }, + namespace_id: project.namespace, + project_id: project, + id: requester + } + + put :update, params: params, xhr: true + + expect(requester.reload.human_access).to eq(label) + end + end + end + + context 'access expiry date' do + subject do + put :update, xhr: true, params: { + project_member: { + expires_at: expires_at + }, + namespace_id: project.namespace, + project_id: project, + id: requester + } + end - expect(requester.reload.human_access).to eq(label) + context 'when set to a date in the past' do + let(:expires_at) { 2.days.ago } + + it 'does not update the member' do + subject + + expect(requester.reload.expires_at).not_to eq(expires_at.to_date) + end + end + + context 'when set to a date in the future' do + let(:expires_at) { 5.days.from_now } + + it 'updates the member' do + subject + + expect(requester.reload.expires_at).to eq(expires_at.to_date) + end end end end diff --git a/spec/graphql/mutations/issues/set_confidential_spec.rb b/spec/graphql/mutations/issues/set_confidential_spec.rb index 820f9aa5e17..0b2fc0ecb93 100644 --- a/spec/graphql/mutations/issues/set_confidential_spec.rb +++ b/spec/graphql/mutations/issues/set_confidential_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe Mutations::Issues::SetConfidential do - let(:issue) { create(:issue) } + let(:project) { create(:project, :private) } + let(:issue) { create(:issue, project: project, assignees: [user]) } let(:user) { create(:user) } subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) } @@ -14,7 +15,7 @@ RSpec.describe Mutations::Issues::SetConfidential do let(:confidential) { true } let(:mutated_issue) { subject[:issue] } - subject { mutation.resolve(project_path: issue.project.full_path, iid: issue.iid, confidential: confidential) } + subject { mutation.resolve(project_path: project.full_path, iid: issue.iid, confidential: confidential) } it 'raises an error if the resource is not accessible to the user' do expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) @@ -22,7 +23,7 @@ RSpec.describe Mutations::Issues::SetConfidential do context 'when the user can update the issue' do before do - issue.project.add_developer(user) + project.add_developer(user) end it 'returns the issue as confidential' do @@ -39,5 +40,19 @@ RSpec.describe Mutations::Issues::SetConfidential do end end end + + context 'when guest user is an assignee' do + let(:project) { create(:project, :public) } + + before do + project.add_guest(user) + end + + it 'does not change issue confidentiality' do + expect(mutated_issue).to eq(issue) + expect(mutated_issue.confidential).to be_falsey + expect(subject[:errors]).to be_empty + end + end end end diff --git a/spec/graphql/mutations/merge_requests/set_milestone_spec.rb b/spec/graphql/mutations/merge_requests/set_milestone_spec.rb index 1c0d655ee83..ccb2d9bd132 100644 --- a/spec/graphql/mutations/merge_requests/set_milestone_spec.rb +++ b/spec/graphql/mutations/merge_requests/set_milestone_spec.rb @@ -3,31 +3,29 @@ require 'spec_helper' RSpec.describe Mutations::MergeRequests::SetMilestone do - let(:merge_request) { create(:merge_request) } let(:user) { create(:user) } + let(:project) { create(:project, :private) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project, assignees: [user]) } + let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) } + let(:milestone) { create(:milestone, project: project) } - subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) } + subject { mutation.resolve(project_path: project.full_path, iid: merge_request.iid, milestone: milestone) } specify { expect(described_class).to require_graphql_authorizations(:update_merge_request) } describe '#resolve' do - let(:milestone) { create(:milestone, project: merge_request.project) } - let(:mutated_merge_request) { subject[:merge_request] } - - subject { mutation.resolve(project_path: merge_request.project.full_path, iid: merge_request.iid, milestone: milestone) } - it 'raises an error if the resource is not accessible to the user' do expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) end context 'when the user can update the merge request' do before do - merge_request.project.add_developer(user) + project.add_developer(user) end it 'returns the merge request with the milestone' do - expect(mutated_merge_request).to eq(merge_request) - expect(mutated_merge_request.milestone).to eq(milestone) + expect(subject[:merge_request]).to eq(merge_request) + expect(subject[:merge_request].milestone).to eq(milestone) expect(subject[:errors]).to be_empty end @@ -43,13 +41,37 @@ RSpec.describe Mutations::MergeRequests::SetMilestone do let(:milestone) { nil } it 'removes the milestone' do - merge_request.update!(milestone: create(:milestone, project: merge_request.project)) + merge_request.update!(milestone: create(:milestone, project: project)) - expect(mutated_merge_request.milestone).to eq(nil) + expect(subject[:merge_request].milestone).to be_nil end it 'does not do anything if the MR already does not have a milestone' do - expect(mutated_merge_request.milestone).to eq(nil) + expect(subject[:merge_request].milestone).to be_nil + end + end + end + + context 'when issue assignee is a guest' do + let(:project) { create(:project, :public) } + + before do + project.add_guest(user) + end + + it 'does not update the milestone' do + expect(subject[:merge_request]).to eq(merge_request) + expect(subject[:merge_request].milestone).to be_nil + expect(subject[:errors]).to be_empty + end + + context 'when passing milestone_id as nil' do + let(:milestone) { nil } + + it 'does not remove the milestone' do + merge_request.update!(milestone: create(:milestone, project: project)) + + expect(subject[:merge_request].milestone).not_to be_nil end end end diff --git a/spec/migrations/insert_project_feature_flags_plan_limits_spec.rb b/spec/migrations/insert_project_feature_flags_plan_limits_spec.rb new file mode 100644 index 00000000000..1ad070de1ea --- /dev/null +++ b/spec/migrations/insert_project_feature_flags_plan_limits_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join( + 'db', + 'migrate', + '20200831222347_insert_project_feature_flags_plan_limits.rb' +) + +RSpec.describe InsertProjectFeatureFlagsPlanLimits do + let(:migration) { described_class.new } + let(:plans) { table(:plans) } + let(:plan_limits) { table(:plan_limits) } + let!(:default_plan) { plans.create!(name: 'default') } + let!(:free_plan) { plans.create!(name: 'free') } + let!(:bronze_plan) { plans.create!(name: 'bronze') } + let!(:silver_plan) { plans.create!(name: 'silver') } + let!(:gold_plan) { plans.create!(name: 'gold') } + let!(:default_plan_limits) do + plan_limits.create!(plan_id: default_plan.id, project_feature_flags: 200) + end + + context 'when on Gitlab.com' do + before do + expect(Gitlab).to receive(:com?).at_most(:twice).and_return(true) + end + + describe '#up' do + it 'updates the project_feature_flags plan limits' do + migration.up + + expect(plan_limits.pluck(:plan_id, :project_feature_flags)).to contain_exactly( + [default_plan.id, 200], + [free_plan.id, 50], + [bronze_plan.id, 100], + [silver_plan.id, 150], + [gold_plan.id, 200] + ) + end + end + + describe '#down' do + it 'removes the project_feature_flags plan limits' do + migration.up + migration.down + + expect(plan_limits.pluck(:plan_id, :project_feature_flags)).to contain_exactly( + [default_plan.id, 200], + [free_plan.id, 0], + [bronze_plan.id, 0], + [silver_plan.id, 0], + [gold_plan.id, 0] + ) + end + end + end + + context 'when on self-hosted' do + before do + expect(Gitlab).to receive(:com?).at_most(:twice).and_return(false) + end + + describe '#up' do + it 'does not change the plan limits' do + migration.up + + expect(plan_limits.pluck(:project_feature_flags)).to contain_exactly(200) + end + end + + describe '#down' do + it 'does not change the plan limits' do + migration.up + migration.down + + expect(plan_limits.pluck(:project_feature_flags)).to contain_exactly(200) + end + end + end +end diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb index de39c8c7c5c..f0bae3f29c0 100644 --- a/spec/models/active_session_spec.rb +++ b/spec/models/active_session_spec.rb @@ -23,7 +23,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do describe '#current?' do it 'returns true if the active session matches the current session' do - active_session = ActiveSession.new(session_id: rack_session) + active_session = ActiveSession.new(session_private_id: rack_session.private_id) expect(active_session.current?(session)).to be true end @@ -45,7 +45,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do describe '#public_id' do it 'returns an encrypted, url-encoded session id' do original_session_id = Rack::Session::SessionId.new("!*'();:@&\n=+$,/?%abcd#123[4567]8") - active_session = ActiveSession.new(session_id: original_session_id) + active_session = ActiveSession.new(session_id: original_session_id.public_id) encrypted_id = active_session.public_id derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id) @@ -106,8 +106,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do redis.sadd( "session:lookup:user:gitlab:#{user.id}", %w[ - 6919a6f1bb119dd7396fadc38fd18d0d - 59822c7d9fcdfa03725eff41782ad97d + 2::418729c72310bbf349a032f0bb6e3fce9f5a69df8f000d8ae0ac5d159d8f21ae + 2::d2ee6f70d6ef0e8701efa3f6b281cbe8e6bf3d109ef052a8b5ce88bfc7e71c26 ] ) end @@ -135,7 +135,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do redis.set("session:gitlab:#{rack_session.private_id}", Marshal.dump({ _csrf_token: 'abcd' })) end - expect(ActiveSession.sessions_from_ids([rack_session])).to eq [{ _csrf_token: 'abcd' }] + expect(ActiveSession.sessions_from_ids([rack_session.private_id])).to eq [{ _csrf_token: 'abcd' }] end it 'avoids a redis lookup for an empty array' do @@ -150,12 +150,11 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do redis = double(:redis) expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) - sessions = %w[session-a session-b session-c session-d] + sessions = %w[session-a session-b] mget_responses = sessions.map { |session| [Marshal.dump(session)]} - expect(redis).to receive(:mget).exactly(4).times.and_return(*mget_responses) + expect(redis).to receive(:mget).twice.times.and_return(*mget_responses) - session_ids = [1, 2].map { |id| Rack::Session::SessionId.new(id.to_s) } - expect(ActiveSession.sessions_from_ids(session_ids).map(&:to_s)).to eql(sessions) + expect(ActiveSession.sessions_from_ids([1, 2])).to eql(sessions) end end @@ -165,7 +164,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do Gitlab::Redis::SharedState.with do |redis| expect(redis.scan_each.to_a).to include( - "session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", + "session:user:gitlab:#{user.id}:2::418729c72310bbf349a032f0bb6e3fce9f5a69df8f000d8ae0ac5d159d8f21ae", "session:lookup:user:gitlab:#{user.id}" ) end @@ -208,13 +207,41 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do end end end + + context 'ActiveSession stored by deprecated rack_session_public_key' do + let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) } + let(:deprecated_active_session_lookup_key) { rack_session.public_id } + + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set("session:user:gitlab:#{user.id}:#{deprecated_active_session_lookup_key}", + '') + redis.sadd(described_class.lookup_key_name(user.id), + deprecated_active_session_lookup_key) + end + end + + it 'removes deprecated key and stores only new one' do + expected_session_keys = ["session:user:gitlab:#{user.id}:#{rack_session.private_id}", + "session:lookup:user:gitlab:#{user.id}"] + + ActiveSession.set(user, request) + + Gitlab::Redis::SharedState.with do |redis| + actual_session_keys = redis.scan_each(match: 'session:*').to_a + expect(actual_session_keys).to(match_array(expected_session_keys)) + + expect(redis.smembers("session:lookup:user:gitlab:#{user.id}")).to eq [rack_session.private_id] + end + end + end end - describe '.destroy' do + describe '.destroy_with_rack_session_id' do it 'gracefully handles a nil session ID' do expect(described_class).not_to receive(:destroy_sessions) - ActiveSession.destroy(user, nil) + ActiveSession.destroy_with_rack_session_id(user, nil) end it 'removes the entry associated with the currently killed user session' do @@ -224,7 +251,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do redis.set("session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358", '') end - ActiveSession.destroy(user, request.session.id) + ActiveSession.destroy_with_rack_session_id(user, request.session.id) Gitlab::Redis::SharedState.with do |redis| expect(redis.scan_each(match: "session:user:gitlab:*")).to match_array [ @@ -240,7 +267,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do redis.sadd("session:lookup:user:gitlab:#{user.id}", '6919a6f1bb119dd7396fadc38fd18d0d') end - ActiveSession.destroy(user, request.session.id) + ActiveSession.destroy_with_rack_session_id(user, request.session.id) Gitlab::Redis::SharedState.with do |redis| expect(redis.scan_each(match: "session:lookup:user:gitlab:#{user.id}").to_a).to be_empty @@ -249,12 +276,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do it 'removes the devise session' do Gitlab::Redis::SharedState.with do |redis| - redis.set("session:user:gitlab:#{user.id}:#{rack_session.public_id}", '') + redis.set("session:user:gitlab:#{user.id}:#{rack_session.private_id}", '') # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88 redis.set("session:gitlab:#{rack_session.private_id}", '') end - ActiveSession.destroy(user, request.session.id) + ActiveSession.destroy_with_rack_session_id(user, request.session.id) Gitlab::Redis::SharedState.with do |redis| expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty @@ -262,37 +289,83 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do end end - describe '.destroy_with_public_id' do - it 'receives a user and public id and destroys the associated session' do - ActiveSession.set(user, request) - session = ActiveSession.list(user).first + describe '.destroy_with_deprecated_encryption' do + shared_examples 'removes all session data' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set("session:user:gitlab:#{user.id}:#{active_session_lookup_key}", '') + # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88 + redis.set("session:gitlab:#{rack_session.private_id}", '') + + redis.set(described_class.key_name(user.id, active_session_lookup_key), + Marshal.dump(active_session)) + redis.sadd(described_class.lookup_key_name(user.id), + active_session_lookup_key) + end + end + + it 'removes the devise session' do + subject + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty + end + end - ActiveSession.destroy_with_public_id(user, session.public_id) + it 'removes the lookup entry' do + subject - total_sessions = ActiveSession.list(user).count - expect(total_sessions).to eq 0 + Gitlab::Redis::SharedState.with do |redis| + expect(redis.scan_each(match: "session:lookup:user:gitlab:#{user.id}").to_a).to be_empty + end + end + + it 'removes the ActiveSession' do + subject + + Gitlab::Redis::SharedState.with do |redis| + expect(redis.scan_each(match: "session:user:gitlab:*").to_a).to be_empty + end + end end - it 'handles invalid input for public id' do - expect do - ActiveSession.destroy_with_public_id(user, nil) - end.not_to raise_error + context 'destroy called with Rack::Session::SessionId#private_id' do + subject { ActiveSession.destroy_with_deprecated_encryption(user, rack_session.private_id) } + + it 'calls .destroy_sessions' do + expect(ActiveSession).to( + receive(:destroy_sessions) + .with(anything, user, [rack_session.private_id])) + + subject + end - expect do - ActiveSession.destroy_with_public_id(user, "") - end.not_to raise_error + context 'ActiveSession with session_private_id' do + let(:active_session) { ActiveSession.new(session_private_id: rack_session.private_id) } + let(:active_session_lookup_key) { rack_session.private_id } - expect do - ActiveSession.destroy_with_public_id(user, "aaaaaaaa") - end.not_to raise_error + include_examples 'removes all session data' + end end - it 'does not attempt to destroy session when given invalid input for public id' do - expect(ActiveSession).not_to receive(:destroy) + context 'destroy called with ActiveSession#public_id (deprecated)' do + let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) } + let(:encrypted_active_session_id) { active_session.public_id } + let(:active_session_lookup_key) { rack_session.public_id } + + subject { ActiveSession.destroy_with_deprecated_encryption(user, encrypted_active_session_id) } + + it 'calls .destroy_sessions' do + expect(ActiveSession).to( + receive(:destroy_sessions) + .with(anything, user, [active_session.public_id, rack_session.public_id, rack_session.private_id])) + + subject + end - ActiveSession.destroy_with_public_id(user, nil) - ActiveSession.destroy_with_public_id(user, "") - ActiveSession.destroy_with_public_id(user, "aaaaaaaa") + context 'ActiveSession with session_id (deprecated)' do + include_examples 'removes all session data' + end end end @@ -308,29 +381,43 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do before do Gitlab::Redis::SharedState.with do |redis| - redis.set(described_class.key_name(user.id, current_session_id), - Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new(current_session_id)))) - redis.set(described_class.key_name(user.id, '59822c7d9fcdfa03725eff41782ad97d'), - Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new('59822c7d9fcdfa03725eff41782ad97d')))) - redis.set(described_class.key_name(9999, '5c8611e4f9c69645ad1a1492f4131358'), - Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new('5c8611e4f9c69645ad1a1492f4131358')))) - redis.sadd(described_class.lookup_key_name(user.id), '59822c7d9fcdfa03725eff41782ad97d') - redis.sadd(described_class.lookup_key_name(user.id), current_session_id) - redis.sadd(described_class.lookup_key_name(9999), '5c8611e4f9c69645ad1a1492f4131358') + # setup for current user + [current_session_id, '59822c7d9fcdfa03725eff41782ad97d'].each do |session_public_id| + session_private_id = Rack::Session::SessionId.new(session_public_id).private_id + active_session = ActiveSession.new(session_private_id: session_private_id) + redis.set(described_class.key_name(user.id, session_private_id), + Marshal.dump(active_session)) + redis.sadd(described_class.lookup_key_name(user.id), + session_private_id) + end + + # setup for unrelated user + unrelated_user_id = 9999 + session_private_id = Rack::Session::SessionId.new('5c8611e4f9c69645ad1a1492f4131358').private_id + active_session = ActiveSession.new(session_private_id: session_private_id) + + redis.set(described_class.key_name(unrelated_user_id, session_private_id), + Marshal.dump(active_session)) + redis.sadd(described_class.lookup_key_name(unrelated_user_id), + session_private_id) end end it 'removes the entry associated with the all user sessions but current' do - expect { ActiveSession.destroy_all_but_current(user, request.session) }.to change { ActiveSession.session_ids_for_user(user.id).size }.from(2).to(1) + expect { ActiveSession.destroy_all_but_current(user, request.session) } + .to(change { ActiveSession.session_ids_for_user(user.id).size }.from(2).to(1)) expect(ActiveSession.session_ids_for_user(9999).size).to eq(1) end it 'removes the lookup entry of deleted sessions' do + session_private_id = Rack::Session::SessionId.new(current_session_id).private_id ActiveSession.destroy_all_but_current(user, request.session) Gitlab::Redis::SharedState.with do |redis| - expect(redis.smembers(described_class.lookup_key_name(user.id))).to eq [current_session_id] + expect( + redis.smembers(described_class.lookup_key_name(user.id)) + ).to eq([session_private_id]) end end @@ -464,5 +551,38 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do end end end + + context 'cleaning up old sessions stored by Rack::Session::SessionId#private_id' do + let(:max_number_of_sessions_plus_one) { ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS + 1 } + let(:max_number_of_sessions_plus_two) { ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS + 2 } + + before do + Gitlab::Redis::SharedState.with do |redis| + (1..max_number_of_sessions_plus_two).each do |number| + redis.set( + "session:user:gitlab:#{user.id}:#{number}", + Marshal.dump(ActiveSession.new(session_private_id: number.to_s, updated_at: number.days.ago)) + ) + redis.sadd( + "session:lookup:user:gitlab:#{user.id}", + "#{number}" + ) + end + end + end + + it 'removes obsolete active sessions entries' do + ActiveSession.cleanup(user) + + Gitlab::Redis::SharedState.with do |redis| + sessions = redis.scan_each(match: "session:user:gitlab:#{user.id}:*").to_a + + expect(sessions.count).to eq(ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS) + expect(sessions).not_to( + include("session:user:gitlab:#{user.id}:#{max_number_of_sessions_plus_one}", + "session:user:gitlab:#{user.id}:#{max_number_of_sessions_plus_two}")) + end + end + end end end diff --git a/spec/models/concerns/expirable_spec.rb b/spec/models/concerns/expirable_spec.rb index b20d759fc3f..5eb6530881e 100644 --- a/spec/models/concerns/expirable_spec.rb +++ b/spec/models/concerns/expirable_spec.rb @@ -4,9 +4,13 @@ require 'spec_helper' RSpec.describe Expirable do describe 'ProjectMember' do - let(:no_expire) { create(:project_member) } - let(:expire_later) { create(:project_member, expires_at: Time.current + 6.days) } - let(:expired) { create(:project_member, expires_at: Time.current - 6.days) } + let_it_be(:no_expire) { create(:project_member) } + let_it_be(:expire_later) { create(:project_member, expires_at: 8.days.from_now) } + let_it_be(:expired) { create(:project_member, expires_at: 1.day.from_now) } + + before do + travel_to(3.days.from_now) + end describe '.expired' do it { expect(ProjectMember.expired).to match_array([expired]) } diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 39807747cc0..90950d93db4 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -17,6 +17,13 @@ RSpec.describe Member do it { is_expected.to validate_presence_of(:user) } it { is_expected.to validate_presence_of(:source) } + context 'expires_at' do + it { is_expected.not_to allow_value(Date.yesterday).for(:expires_at) } + it { is_expected.to allow_value(Date.tomorrow).for(:expires_at) } + it { is_expected.to allow_value(Date.today).for(:expires_at) } + it { is_expected.to allow_value(nil).for(:expires_at) } + end + it_behaves_like 'an object with email-formated attributes', :invite_email do subject { build(:project_member) } end diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index f25f8933184..388d04c8012 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -44,8 +44,9 @@ RSpec.describe ProjectMember do let(:maintainer) { create(:project_member, project: project) } it "creates an expired event when left due to expiry" do - expired = create(:project_member, project: project, expires_at: Time.current - 6.days) - expired.destroy + expired = create(:project_member, project: project, expires_at: 1.day.from_now) + travel_to(2.days.from_now) { expired.destroy } + expect(Event.recent.first).to be_expired_action end diff --git a/spec/models/operations/feature_flag_spec.rb b/spec/models/operations/feature_flag_spec.rb index 83d6c6b95a3..db432e73355 100644 --- a/spec/models/operations/feature_flag_spec.rb +++ b/spec/models/operations/feature_flag_spec.rb @@ -7,6 +7,10 @@ RSpec.describe Operations::FeatureFlag do subject { create(:operations_feature_flag) } + it_behaves_like 'includes Limitable concern' do + subject { build(:operations_feature_flag, project: create(:project)) } + end + describe 'associations' do it { is_expected.to belong_to(:project) } it { is_expected.to have_many(:scopes) } diff --git a/spec/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index bd0426601db..7d637757f38 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -67,4 +67,29 @@ RSpec.describe API::API do end end end + + describe 'authentication with deploy token' do + context 'admin mode' do + let_it_be(:project) { create(:project, :public) } + let_it_be(:package) { create(:maven_package, project: project, name: project.full_path) } + let_it_be(:maven_metadatum) { package.maven_metadatum } + let_it_be(:package_file) { package.package_files.first } + let_it_be(:deploy_token) { create(:deploy_token) } + let(:headers_with_deploy_token) do + { + Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => deploy_token.token + } + end + + it 'does not bypass the session' do + expect(Gitlab::Auth::CurrentUserMode).not_to receive(:bypass_session!) + + get(api("/packages/maven/#{maven_metadatum.path}/#{package_file.file_name}"), + headers: headers_with_deploy_token) + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end + end + end end diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index de52087340c..55b2447fc68 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -244,13 +244,12 @@ RSpec.describe API::Members do it 'creates a new member' do expect do post api("/#{source_type.pluralize}/#{source.id}/members", maintainer), - params: { user_id: stranger.id, access_level: Member::DEVELOPER, expires_at: '2016-08-05' } + params: { user_id: stranger.id, access_level: Member::DEVELOPER } expect(response).to have_gitlab_http_status(:created) end.to change { source.members.count }.by(1) expect(json_response['id']).to eq(stranger.id) expect(json_response['access_level']).to eq(Member::DEVELOPER) - expect(json_response['expires_at']).to eq('2016-08-05') end end @@ -285,6 +284,40 @@ RSpec.describe API::Members do end end + context 'access expiry date' do + subject do + post api("/#{source_type.pluralize}/#{source.id}/members", maintainer), + params: { user_id: stranger.id, access_level: Member::DEVELOPER, expires_at: expires_at } + end + + context 'when set to a date in the past' do + let(:expires_at) { 2.days.ago.to_date } + + it 'does not create a member' do + expect do + subject + end.not_to change { source.members.count } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eq({ 'expires_at' => ['cannot be a date in the past'] }) + end + end + + context 'when set to a date in the future' do + let(:expires_at) { 2.days.from_now.to_date } + + it 'creates a member' do + expect do + subject + end.to change { source.members.count }.by(1) + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['id']).to eq(stranger.id) + expect(json_response['expires_at']).to eq(expires_at.to_s) + end + end + end + it "returns 409 if member already exists" do post api("/#{source_type.pluralize}/#{source.id}/members", maintainer), params: { user_id: maintainer.id, access_level: Member::MAINTAINER } @@ -369,12 +402,40 @@ RSpec.describe API::Members do context 'when authenticated as a maintainer/owner' do it 'updates the member' do put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", maintainer), - params: { access_level: Member::MAINTAINER, expires_at: '2016-08-05' } + params: { access_level: Member::MAINTAINER } expect(response).to have_gitlab_http_status(:ok) expect(json_response['id']).to eq(developer.id) expect(json_response['access_level']).to eq(Member::MAINTAINER) - expect(json_response['expires_at']).to eq('2016-08-05') + end + end + + context 'access expiry date' do + subject do + put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", maintainer), + params: { expires_at: expires_at, access_level: Member::MAINTAINER } + end + + context 'when set to a date in the past' do + let(:expires_at) { 2.days.ago.to_date } + + it 'does not update the member' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eq({ 'expires_at' => ['cannot be a date in the past'] }) + end + end + + context 'when set to a date in the future' do + let(:expires_at) { 2.days.from_now.to_date } + + it 'updates the member' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['expires_at']).to eq(expires_at.to_s) + end end end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index e09a7faece5..eeac7fb9923 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -25,6 +25,7 @@ RSpec.describe Issues::CreateService do assignee_ids: [assignee.id], label_ids: labels.map(&:id), milestone_id: milestone.id, + milestone: milestone, due_date: Date.tomorrow } end @@ -102,6 +103,12 @@ RSpec.describe Issues::CreateService do expect(issue.milestone).to be_nil expect(issue.due_date).to be_nil end + + it 'creates confidential issues' do + issue = described_class.new(project, guest, confidential: true).execute + + expect(issue.confidential).to be_truthy + end end it 'creates a pending todo for new assignee' do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index f0092c35fda..b3e8fba4e9a 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -10,6 +10,7 @@ RSpec.describe Issues::UpdateService, :mailer do let_it_be(:project, reload: true) { create(:project, :repository, group: group) } let_it_be(:label) { create(:label, project: project) } let_it_be(:label2) { create(:label, project: project) } + let_it_be(:milestone) { create(:milestone, project: project) } let(:issue) do create(:issue, title: 'Old title', @@ -53,7 +54,8 @@ RSpec.describe Issues::UpdateService, :mailer do label_ids: [label.id], due_date: Date.tomorrow, discussion_locked: true, - severity: 'low' + severity: 'low', + milestone_id: milestone.id } end @@ -70,6 +72,14 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.labels).to match_array [label] expect(issue.due_date).to eq Date.tomorrow expect(issue.discussion_locked).to be_truthy + expect(issue.confidential).to be_falsey + expect(issue.milestone).to eq milestone + end + + it 'updates issue milestone when passing `milestone` param' do + update_issue(milestone: milestone) + + expect(issue.milestone).to eq milestone end context 'when issue type is not incident' do @@ -128,6 +138,8 @@ RSpec.describe Issues::UpdateService, :mailer do expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, issue.id) update_issue(confidential: true) + + expect(issue.confidential).to be_truthy end it 'does not enqueue ConfidentialIssueWorker when an issue is made non confidential' do @@ -137,6 +149,8 @@ RSpec.describe Issues::UpdateService, :mailer do expect(TodosDestroyer::ConfidentialIssueWorker).not_to receive(:perform_in) update_issue(confidential: false) + + expect(issue.confidential).to be_falsey end context 'issue in incident type' do @@ -297,7 +311,7 @@ RSpec.describe Issues::UpdateService, :mailer do end it 'filters out params that cannot be set without the :admin_issue permission' do - described_class.new(project, guest, opts).execute(issue) + described_class.new(project, guest, opts.merge(confidential: true)).execute(issue) expect(issue).to be_valid expect(issue.title).to eq 'New title' @@ -307,6 +321,7 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.milestone).to be_nil expect(issue.due_date).to be_nil expect(issue.discussion_locked).to be_falsey + expect(issue.confidential).to be_falsey end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 6b7463d4996..3c3e10495d3 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -6,12 +6,13 @@ RSpec.describe MergeRequests::UpdateService, :mailer do include ProjectForksHelper let(:group) { create(:group, :public) } - let(:project) { create(:project, :repository, group: group) } + let(:project) { create(:project, :private, :repository, group: group) } let(:user) { create(:user) } let(:user2) { create(:user) } let(:user3) { create(:user) } let(:label) { create(:label, project: project) } let(:label2) { create(:label) } + let(:milestone) { create(:milestone, project: project) } let(:merge_request) do create(:merge_request, :simple, title: 'Old title', @@ -61,7 +62,8 @@ RSpec.describe MergeRequests::UpdateService, :mailer do } end - let(:service) { described_class.new(project, user, opts) } + let(:service) { described_class.new(project, current_user, opts) } + let(:current_user) { user } before do allow(service).to receive(:execute_hooks) @@ -85,6 +87,26 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(@merge_request.discussion_locked).to be_truthy end + context 'updating milestone' do + RSpec.shared_examples 'updates milestone' do + it 'sets milestone' do + expect(@merge_request.milestone).to eq milestone + end + end + + context 'when milestone_id param' do + let(:opts) { { milestone_id: milestone.id } } + + it_behaves_like 'updates milestone' + end + + context 'when milestone param' do + let(:opts) { { milestone: milestone } } + + it_behaves_like 'updates milestone' + end + end + it 'executes hooks with update action' do expect(service).to have_received(:execute_hooks) .with( @@ -152,6 +174,46 @@ RSpec.describe MergeRequests::UpdateService, :mailer do expect(note.note).to eq 'locked this merge request' end + context 'when current user cannot admin issues in the project' do + let(:guest) { create(:user) } + let(:current_user) { guest } + + before do + project.add_guest(guest) + end + + it 'filters out params that cannot be set without the :admin_merge_request permission' do + expect(@merge_request).to be_valid + expect(@merge_request.title).to eq('New title') + expect(@merge_request.assignees).to match_array([user3]) + expect(@merge_request).to be_opened + expect(@merge_request.labels.count).to eq(0) + expect(@merge_request.target_branch).to eq('target') + expect(@merge_request.discussion_locked).to be_falsey + expect(@merge_request.milestone).to be_nil + end + + context 'updating milestone' do + RSpec.shared_examples 'does not update milestone' do + it 'sets milestone' do + expect(@merge_request.milestone).to be_nil + end + end + + context 'when milestone_id param' do + let(:opts) { { milestone_id: milestone.id } } + + it_behaves_like 'does not update milestone' + end + + context 'when milestone param' do + let(:opts) { { milestone: milestone } } + + it_behaves_like 'does not update milestone' + end + end + end + context 'when not including source branch removal options' do before do opts.delete(:force_remove_source_branch) diff --git a/spec/validators/future_date_validator_spec.rb b/spec/validators/future_date_validator_spec.rb new file mode 100644 index 00000000000..6814ba7c820 --- /dev/null +++ b/spec/validators/future_date_validator_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe FutureDateValidator do + subject do + Class.new do + include ActiveModel::Model + include ActiveModel::Validations + attr_accessor :expires_at + validates :expires_at, future_date: true + end.new + end + + before do + subject.expires_at = date + end + + context 'past date' do + let(:date) { Date.yesterday } + + it { is_expected.not_to be_valid } + end + + context 'current date' do + let(:date) { Date.today } + + it { is_expected.to be_valid } + end + + context 'future date' do + let(:date) { Date.tomorrow } + + it { is_expected.to be_valid } + end +end diff --git a/spec/workers/remove_expired_members_worker_spec.rb b/spec/workers/remove_expired_members_worker_spec.rb index cbdd5a68698..8a34b41834b 100644 --- a/spec/workers/remove_expired_members_worker_spec.rb +++ b/spec/workers/remove_expired_members_worker_spec.rb @@ -7,9 +7,13 @@ RSpec.describe RemoveExpiredMembersWorker do describe '#perform' do context 'project members' do - let!(:expired_project_member) { create(:project_member, expires_at: 1.hour.ago, access_level: GroupMember::DEVELOPER) } - let!(:project_member_expiring_in_future) { create(:project_member, expires_at: 10.days.from_now, access_level: GroupMember::DEVELOPER) } - let!(:non_expiring_project_member) { create(:project_member, expires_at: nil, access_level: GroupMember::DEVELOPER) } + let_it_be(:expired_project_member) { create(:project_member, expires_at: 1.day.from_now, access_level: GroupMember::DEVELOPER) } + let_it_be(:project_member_expiring_in_future) { create(:project_member, expires_at: 10.days.from_now, access_level: GroupMember::DEVELOPER) } + let_it_be(:non_expiring_project_member) { create(:project_member, expires_at: nil, access_level: GroupMember::DEVELOPER) } + + before do + travel_to(3.days.from_now) + end it 'removes expired members' do expect { worker.perform }.to change { Member.count }.by(-1) @@ -28,9 +32,13 @@ RSpec.describe RemoveExpiredMembersWorker do end context 'group members' do - let!(:expired_group_member) { create(:group_member, expires_at: 1.hour.ago, access_level: GroupMember::DEVELOPER) } - let!(:group_member_expiring_in_future) { create(:group_member, expires_at: 10.days.from_now, access_level: GroupMember::DEVELOPER) } - let!(:non_expiring_group_member) { create(:group_member, expires_at: nil, access_level: GroupMember::DEVELOPER) } + let_it_be(:expired_group_member) { create(:group_member, expires_at: 1.day.from_now, access_level: GroupMember::DEVELOPER) } + let_it_be(:group_member_expiring_in_future) { create(:group_member, expires_at: 10.days.from_now, access_level: GroupMember::DEVELOPER) } + let_it_be(:non_expiring_group_member) { create(:group_member, expires_at: nil, access_level: GroupMember::DEVELOPER) } + + before do + travel_to(3.days.from_now) + end it 'removes expired members' do expect { worker.perform }.to change { Member.count }.by(-1) @@ -49,7 +57,11 @@ RSpec.describe RemoveExpiredMembersWorker do end context 'when the last group owner expires' do - let!(:expired_group_owner) { create(:group_member, expires_at: 1.hour.ago, access_level: GroupMember::OWNER) } + let_it_be(:expired_group_owner) { create(:group_member, expires_at: 1.day.from_now, access_level: GroupMember::OWNER) } + + before do + travel_to(3.days.from_now) + end it 'does not delete the owner' do worker.perform diff --git a/spec/workers/remove_unaccepted_member_invites_worker_spec.rb b/spec/workers/remove_unaccepted_member_invites_worker_spec.rb new file mode 100644 index 00000000000..96d7cf535ed --- /dev/null +++ b/spec/workers/remove_unaccepted_member_invites_worker_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RemoveUnacceptedMemberInvitesWorker do + let(:worker) { described_class.new } + + describe '#perform' do + context 'unaccepted members' do + before do + stub_const("#{described_class}::EXPIRATION_THRESHOLD", 1.day) + end + + it 'removes unaccepted members', :aggregate_failures do + unaccepted_group_invitee = create( + :group_member, invite_token: 't0ken', + invite_email: 'group_invitee@example.com', + user: nil, + created_at: Time.current - 5.days) + unaccepted_project_invitee = create( + :project_member, invite_token: 't0ken', + invite_email: 'project_invitee@example.com', + user: nil, + created_at: Time.current - 5.days) + + expect { worker.perform }.to change { Member.count }.by(-2) + + expect(Member.where(id: unaccepted_project_invitee.id)).not_to exist + expect(Member.where(id: unaccepted_group_invitee.id)).not_to exist + end + end + + context 'invited members still within expiration threshold' do + it 'leaves invited members', :aggregate_failures do + group_invitee = create( + :group_member, invite_token: 't0ken', + invite_email: 'group_invitee@example.com', + user: nil) + project_invitee = create( + :project_member, invite_token: 't0ken', + invite_email: 'project_invitee@example.com', + user: nil) + + expect { worker.perform }.not_to change { Member.count } + + expect(Member.where(id: group_invitee.id)).to exist + expect(Member.where(id: project_invitee.id)).to exist + end + end + + context 'accepted members' do + before do + stub_const("#{described_class}::EXPIRATION_THRESHOLD", 1.day) + end + + it 'leaves accepted members', :aggregate_failures do + user = create(:user) + accepted_group_invitee = create( + :group_member, invite_token: 't0ken', + invite_email: 'group_invitee@example.com', + user: user, + created_at: Time.current - 5.days) + accepted_project_invitee = create( + :project_member, invite_token: nil, + invite_email: 'project_invitee@example.com', + user: user, + created_at: Time.current - 5.days) + + expect { worker.perform }.not_to change { Member.count } + + expect(Member.where(id: accepted_group_invitee.id)).to exist + expect(Member.where(id: accepted_project_invitee.id)).to exist + end + end + end +end |