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 | |
| parent | c90be62bdefdb6bb67c73a9c4a6d164c9f78a28d (diff) | |
| download | gitlab-ce-516fba52cf280b9d5bad08dce9f0150f859b6cea.tar.gz | |
Add latest changes from gitlab-org/security/gitlab@13-4-stable-ee
52 files changed, 1069 insertions, 162 deletions
diff --git a/app/channels/application_cable/connection.rb b/app/channels/application_cable/connection.rb index 1361269f2a2..b2d98d243f9 100644 --- a/app/channels/application_cable/connection.rb +++ b/app/channels/application_cable/connection.rb @@ -15,7 +15,7 @@ module ApplicationCable private def find_user_from_session_store - session = ActiveSession.sessions_from_ids([session_id]).first + session = ActiveSession.sessions_from_ids([session_id.private_id]).first Warden::SessionSerializer.new('rack.session' => session).fetch(:user) end diff --git a/app/controllers/profiles/active_sessions_controller.rb b/app/controllers/profiles/active_sessions_controller.rb index d9ec3195fd1..e4cd5d65e1a 100644 --- a/app/controllers/profiles/active_sessions_controller.rb +++ b/app/controllers/profiles/active_sessions_controller.rb @@ -6,7 +6,9 @@ class Profiles::ActiveSessionsController < Profiles::ApplicationController end def destroy - ActiveSession.destroy_with_public_id(current_user, params[:id]) + # params[:id] can be either an Rack::Session::SessionId#private_id + # or an encrypted Rack::Session::SessionId#public_id + ActiveSession.destroy_with_deprecated_encryption(current_user, params[:id]) current_user.forget_me! respond_to do |format| diff --git a/app/helpers/active_sessions_helper.rb b/app/helpers/active_sessions_helper.rb index b80152777a8..322c5b3b16d 100644 --- a/app/helpers/active_sessions_helper.rb +++ b/app/helpers/active_sessions_helper.rb @@ -22,4 +22,13 @@ module ActiveSessionsHelper sprite_icon(icon_name, css_class: 'gl-mt-2') end + + def revoke_session_path(active_session) + if active_session.session_private_id + profile_active_session_path(active_session.session_private_id) + else + # TODO: remove in 13.7 + profile_active_session_path(active_session.public_id) + end + end end diff --git a/app/models/active_session.rb b/app/models/active_session.rb index 4908290e06b..dded0eb1dc3 100644 --- a/app/models/active_session.rb +++ b/app/models/active_session.rb @@ -9,14 +9,14 @@ class ActiveSession attr_accessor :created_at, :updated_at, :ip_address, :browser, :os, :device_name, :device_type, - :is_impersonated, :session_id + :is_impersonated, :session_id, :session_private_id - def current?(session) - return false if session_id.nil? || session.id.nil? + def current?(rack_session) + return false if session_private_id.nil? || rack_session.id.nil? # Rack v2.0.8+ added private_id, which uses the hash of the # public_id to avoid timing attacks. - session_id.private_id == session.id.private_id + session_private_id == rack_session.id.private_id end def human_device_type @@ -25,13 +25,14 @@ class ActiveSession # This is not the same as Rack::Session::SessionId#public_id, but we # need to preserve this for backwards compatibility. + # TODO: remove in 13.7 def public_id - Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id.public_id) + Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id) end def self.set(user, request) Gitlab::Redis::SharedState.with do |redis| - session_id = request.session.id.public_id + session_private_id = request.session.id.private_id client = DeviceDetector.new(request.user_agent) timestamp = Time.current @@ -43,25 +44,37 @@ class ActiveSession device_type: client.device_type, created_at: user.current_sign_in_at || timestamp, updated_at: timestamp, - session_id: session_id, + # TODO: remove in 13.7 + session_id: request.session.id.public_id, + session_private_id: session_private_id, is_impersonated: request.session[:impersonator_id].present? ) redis.pipelined do redis.setex( - key_name(user.id, session_id), + key_name(user.id, session_private_id), Settings.gitlab['session_expire_delay'] * 60, Marshal.dump(active_user_session) ) redis.sadd( lookup_key_name(user.id), - session_id + session_private_id ) + + # We remove the ActiveSession stored by using public_id to avoid + # duplicate entries + remove_deprecated_active_sessions_with_public_id(redis, user.id, request.session.id.public_id) end end end + # TODO: remove in 13.7 + private_class_method def self.remove_deprecated_active_sessions_with_public_id(redis, user_id, rack_session_public_id) + redis.srem(lookup_key_name(user_id), rack_session_public_id) + redis.del(key_name(user_id, rack_session_public_id)) + end + def self.list(user) Gitlab::Redis::SharedState.with do |redis| cleaned_up_lookup_entries(redis, user).map do |raw_session| @@ -70,27 +83,29 @@ class ActiveSession end end - def self.destroy(user, session_id) - return unless session_id - + def self.cleanup(user) Gitlab::Redis::SharedState.with do |redis| - destroy_sessions(redis, user, [session_id]) + clean_up_old_sessions(redis, user) + cleaned_up_lookup_entries(redis, user) end end - def self.destroy_with_public_id(user, public_id) - decrypted_id = decrypt_public_id(public_id) + # TODO: remove in 13.7 + # After upgrade there might be a duplicate ActiveSessions: + # - one with the public_id stored in #session_id + # - another with private_id stored in #session_private_id + def self.destroy_with_rack_session_id(user, rack_session_id) + return unless rack_session_id - return if decrypted_id.nil? - - session_id = Rack::Session::SessionId.new(decrypted_id) - destroy(user, session_id) + Gitlab::Redis::SharedState.with do |redis| + destroy_sessions(redis, user, [rack_session_id.public_id, rack_session_id.private_id]) + end end def self.destroy_sessions(redis, user, session_ids) - key_names = session_ids.map { |session_id| key_name(user.id, session_id.public_id) } + key_names = session_ids.map { |session_id| key_name(user.id, session_id) } - redis.srem(lookup_key_name(user.id), session_ids.map(&:public_id)) + redis.srem(lookup_key_name(user.id), session_ids) Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do redis.del(key_names) @@ -98,19 +113,29 @@ class ActiveSession end end - def self.cleanup(user) + # TODO: remove in 13.7 + # After upgrade, .destroy might be called with the session id encrypted + # by .public_id. + def self.destroy_with_deprecated_encryption(user, session_id) + return unless session_id + + decrypted_session_id = decrypt_public_id(session_id) + rack_session_private_id = if decrypted_session_id + Rack::Session::SessionId.new(decrypted_session_id).private_id + end + Gitlab::Redis::SharedState.with do |redis| - clean_up_old_sessions(redis, user) - cleaned_up_lookup_entries(redis, user) + destroy_sessions(redis, user, [session_id, decrypted_session_id, rack_session_private_id].compact) end end - def self.destroy_all_but_current(user, current_session) - session_ids = not_impersonated(user) - session_ids.reject! { |session| session.current?(current_session) } if current_session + def self.destroy_all_but_current(user, current_rack_session) + sessions = not_impersonated(user) + sessions.reject! { |session| session.current?(current_rack_session) } if current_rack_session Gitlab::Redis::SharedState.with do |redis| - destroy_sessions(redis, user, session_ids.map(&:session_id)) if session_ids.any? + session_ids = (sessions.map(&:session_id) | sessions.map(&:session_private_id)).compact + destroy_sessions(redis, user, session_ids) if session_ids.any? end end @@ -132,17 +157,16 @@ class ActiveSession # Lists the relevant session IDs for the user. # - # Returns an array of Rack::Session::SessionId objects + # Returns an array of strings def self.session_ids_for_user(user_id) Gitlab::Redis::SharedState.with do |redis| - session_ids = redis.smembers(lookup_key_name(user_id)) - session_ids.map { |id| Rack::Session::SessionId.new(id) } + redis.smembers(lookup_key_name(user_id)) end end # Lists the session Hash objects for the given session IDs. # - # session_ids - An array of Rack::Session::SessionId objects + # session_ids - An array of strings # # Returns an array of ActiveSession objects def self.sessions_from_ids(session_ids) @@ -168,27 +192,12 @@ class ActiveSession # Returns an ActiveSession object def self.load_raw_session(raw_session) # rubocop:disable Security/MarshalLoad - session = Marshal.load(raw_session) + Marshal.load(raw_session) # rubocop:enable Security/MarshalLoad + end - # Older ActiveSession models serialize `session_id` as strings, To - # avoid breaking older sessions, we keep backwards compatibility - # with older Redis keys and initiate Rack::Session::SessionId here. - session.session_id = Rack::Session::SessionId.new(session.session_id) if session.try(:session_id).is_a?(String) - session - end - - def self.rack_session_keys(session_ids) - session_ids.each_with_object([]) do |session_id, arr| - # This is a redis-rack implementation detail - # (https://github.com/redis-store/redis-rack/blob/master/lib/rack/session/redis.rb#L88) - # - # We need to delete session keys based on the legacy public key name - # and the newer private ID keys, but there's no well-defined interface - # so we have to do it directly. - arr << "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id.public_id}" - arr << "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id.private_id}" - end + def self.rack_session_keys(rack_session_ids) + rack_session_ids.map { |session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" } end def self.raw_active_session_entries(redis, session_ids, user_id) @@ -220,7 +229,7 @@ class ActiveSession sessions = active_session_entries(session_ids, user.id, redis) sessions.sort_by! {|session| session.updated_at }.reverse! destroyable_sessions = sessions.drop(ALLOWED_NUMBER_OF_ACTIVE_SESSIONS) - destroyable_session_ids = destroyable_sessions.map { |session| session.session_id } + destroyable_session_ids = destroyable_sessions.flat_map { |session| [session.session_id, session.session_private_id] }.compact destroy_sessions(redis, user, destroyable_session_ids) if destroyable_session_ids.any? end @@ -244,6 +253,7 @@ class ActiveSession entries.compact end + # TODO: remove in 13.7 private_class_method def self.decrypt_public_id(public_id) Gitlab::CryptoHelper.aes256_gcm_decrypt(public_id) rescue diff --git a/app/models/member.rb b/app/models/member.rb index 5a084a3a2e6..7ea9caa45d3 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -5,6 +5,7 @@ class Member < ApplicationRecord include AfterCommitQueue include Sortable include Importable + include CreatedAtFilterable include Expirable include Gitlab::Access include Presentable @@ -20,6 +21,7 @@ class Member < ApplicationRecord delegate :name, :username, :email, to: :user, prefix: true + validates :expires_at, allow_blank: true, future_date: true validates :user, presence: true, unless: :invite? validates :source, presence: true validates :user_id, uniqueness: { scope: [:source_type, :source_id], diff --git a/app/models/operations/feature_flag.rb b/app/models/operations/feature_flag.rb index 586e9d689a1..104338b80d1 100644 --- a/app/models/operations/feature_flag.rb +++ b/app/models/operations/feature_flag.rb @@ -4,8 +4,11 @@ module Operations class FeatureFlag < ApplicationRecord include AtomicInternalId include IidRoutes + include Limitable self.table_name = 'operations_feature_flags' + self.limit_scope = :project + self.limit_name = 'project_feature_flags' belongs_to :project diff --git a/app/models/releases/link.rb b/app/models/releases/link.rb index e1dc3b904b9..82272f4857a 100644 --- a/app/models/releases/link.rb +++ b/app/models/releases/link.rb @@ -6,7 +6,9 @@ module Releases belongs_to :release - FILEPATH_REGEX = %r{\A/(?:[\-\.\w]+/?)*[\da-zA-Z]+\z}.freeze + # See https://gitlab.com/gitlab-org/gitlab/-/issues/218753 + # Regex modified to prevent catastrophic backtracking + FILEPATH_REGEX = %r{\A\/[^\/](?!.*\/\/.*)[\-\.\w\/]+[\da-zA-Z]+\z}.freeze validates :url, presence: true, addressable_url: { schemes: %w(http https ftp) }, uniqueness: { scope: :release } validates :name, presence: true, uniqueness: { scope: :release } diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 56bcef0c562..1672ba2830a 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -19,6 +19,7 @@ class IssuableBaseService < BaseService def filter_params(issuable) unless can_admin_issuable?(issuable) + params.delete(:milestone) params.delete(:milestone_id) params.delete(:labels) params.delete(:add_label_ids) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index ce21b2e0275..b9832400302 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -3,6 +3,7 @@ module Issues class UpdateService < Issues::BaseService include SpamCheckMethods + extend ::Gitlab::Utils::Override def execute(issue) handle_move_between_ids(issue) @@ -17,6 +18,17 @@ module Issues super end + override :filter_params + def filter_params(issue) + super + + # filter confidential in `Issues::UpdateService` and not in `IssuableBaseService#filtr_params` + # because we do allow users that cannot admin issues to set confidential flag when creating an issue + unless can_admin_issuable?(issue) + params.delete(:confidential) + end + end + def before_update(issue, skip_spam_check: false) spam_check(issue, current_user, action: :update) unless skip_spam_check end diff --git a/app/validators/future_date_validator.rb b/app/validators/future_date_validator.rb new file mode 100644 index 00000000000..1ba4a1e273d --- /dev/null +++ b/app/validators/future_date_validator.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +# FutureDateValidator + +# Validates that a date is in the future. +# +# Example: +# +# class Member < ActiveRecord::Base +# validates :expires_at, allow_blank: true, future_date: true +# end + +class FutureDateValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + record.errors.add(attribute, _('cannot be a date in the past')) if value < Date.current + end +end diff --git a/app/views/profiles/active_sessions/_active_session.html.haml b/app/views/profiles/active_sessions/_active_session.html.haml index c4b3fa80d75..97f13a55dea 100644 --- a/app/views/profiles/active_sessions/_active_session.html.haml +++ b/app/views/profiles/active_sessions/_active_session.html.haml @@ -27,6 +27,9 @@ - unless is_current_session .float-right - = link_to profile_active_session_path(active_session.public_id), data: { confirm: _('Are you sure? The device will be signed out of GitLab and all remember me tokens revoked.') }, method: :delete, class: "btn btn-danger gl-ml-3" do + = link_to(revoke_session_path(active_session), + { data: { confirm: _('Are you sure? The device will be signed out of GitLab and all remember me tokens revoked.') }, + method: :delete, + class: "btn btn-danger gl-ml-3" }) do %span.sr-only= _('Revoke') = _('Revoke') diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 451decce9fb..11bf797fb90 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -315,6 +315,14 @@ :weight: 1 :idempotent: :tags: [] +- :name: cronjob:remove_unaccepted_member_invites + :feature_category: :authentication_and_authorization + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: cronjob:remove_unreferenced_lfs_objects :feature_category: :git_lfs :has_external_dependencies: diff --git a/app/workers/remove_unaccepted_member_invites_worker.rb b/app/workers/remove_unaccepted_member_invites_worker.rb new file mode 100644 index 00000000000..4b75b43791e --- /dev/null +++ b/app/workers/remove_unaccepted_member_invites_worker.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class RemoveUnacceptedMemberInvitesWorker # rubocop:disable Scalability/IdempotentWorker + include ApplicationWorker + include CronjobQueue # rubocop:disable Scalability/CronWorkerContext + + feature_category :authentication_and_authorization + urgency :low + idempotent! + + EXPIRATION_THRESHOLD = 90.days + BATCH_SIZE = 10_000 + + def perform + # We need to check for user_id IS NULL because we have accepted invitations + # in the database where we did not clear the invite_token. We do not + # want to accidentally delete those members. + loop do + # rubocop: disable CodeReuse/ActiveRecord + inner_query = Member + .select(:id) + .invite + .created_before(EXPIRATION_THRESHOLD.ago) + .where(user_id: nil) + .limit(BATCH_SIZE) + + records_deleted = Member.where(id: inner_query).delete_all + # rubocop: enable CodeReuse/ActiveRecord + + break if records_deleted == 0 + end + end +end diff --git a/changelogs/unreleased/17817-hashed_session_ids_in_redis.yml b/changelogs/unreleased/17817-hashed_session_ids_in_redis.yml new file mode 100644 index 00000000000..0c274f33f36 --- /dev/null +++ b/changelogs/unreleased/17817-hashed_session_ids_in_redis.yml @@ -0,0 +1,5 @@ +--- +title: Do not store session id in Redis +merge_request: +author: +type: security diff --git a/changelogs/unreleased/195327-update-confidentiality-and-milestone.yml b/changelogs/unreleased/195327-update-confidentiality-and-milestone.yml new file mode 100644 index 00000000000..1f883523353 --- /dev/null +++ b/changelogs/unreleased/195327-update-confidentiality-and-milestone.yml @@ -0,0 +1,6 @@ +--- +title: Fix permission checks when updating confidentiality and milestone on issues + or merge requests +merge_request: +author: +type: security diff --git a/changelogs/unreleased/222349-purge_unaccepted_member_invitations.yml b/changelogs/unreleased/222349-purge_unaccepted_member_invitations.yml new file mode 100644 index 00000000000..988ebe9f0c8 --- /dev/null +++ b/changelogs/unreleased/222349-purge_unaccepted_member_invitations.yml @@ -0,0 +1,5 @@ +--- +title: Purge unaccepted member invitations older than 90 days +merge_request: +author: +type: security diff --git a/changelogs/unreleased/feature-flag-plan-limits.yml b/changelogs/unreleased/feature-flag-plan-limits.yml new file mode 100644 index 00000000000..cac5e0847e4 --- /dev/null +++ b/changelogs/unreleased/feature-flag-plan-limits.yml @@ -0,0 +1,5 @@ +--- +title: Adds feature flags plan limits +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fix_session_bypassing_for_admin_mode_in_api.yml b/changelogs/unreleased/security-fix_session_bypassing_for_admin_mode_in_api.yml new file mode 100644 index 00000000000..bf86f177cd3 --- /dev/null +++ b/changelogs/unreleased/security-fix_session_bypassing_for_admin_mode_in_api.yml @@ -0,0 +1,5 @@ +--- +title: Do not bypass admin mode when authenticated with deploy token +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-fixes-release-asset-link-filepath-ReDoS.yml b/changelogs/unreleased/security-fixes-release-asset-link-filepath-ReDoS.yml new file mode 100644 index 00000000000..e48c3ff963c --- /dev/null +++ b/changelogs/unreleased/security-fixes-release-asset-link-filepath-ReDoS.yml @@ -0,0 +1,5 @@ +--- +title: Fixes release asset link filepath ReDoS +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-members-expiry-date-should-be-in-future.yml b/changelogs/unreleased/security-members-expiry-date-should-be-in-future.yml new file mode 100644 index 00000000000..42418f24345 --- /dev/null +++ b/changelogs/unreleased/security-members-expiry-date-should-be-in-future.yml @@ -0,0 +1,5 @@ +--- +title: Validate that membership expiry dates are not in the past +merge_request: +author: +type: security diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 6ccd027dd5d..15c9fd427ff 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -445,6 +445,9 @@ Settings.cron_jobs['remove_expired_members_worker']['job_class'] = 'RemoveExpire Settings.cron_jobs['remove_expired_group_links_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['remove_expired_group_links_worker']['cron'] ||= '10 0 * * *' Settings.cron_jobs['remove_expired_group_links_worker']['job_class'] = 'RemoveExpiredGroupLinksWorker' +Settings.cron_jobs['remove_unaccepted_member_invites_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['remove_unaccepted_member_invites_worker']['cron'] ||= '10 15 * * *' +Settings.cron_jobs['remove_unaccepted_member_invites_worker']['job_class'] = 'RemoveUnacceptedMemberInvitesWorker' Settings.cron_jobs['prune_old_events_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['prune_old_events_worker']['cron'] ||= '0 */6 * * *' Settings.cron_jobs['prune_old_events_worker']['job_class'] = 'PruneOldEventsWorker' diff --git a/config/initializers/warden.rb b/config/initializers/warden.rb index 84bda81a33a..3cfab52efd1 100644 --- a/config/initializers/warden.rb +++ b/config/initializers/warden.rb @@ -40,7 +40,8 @@ Rails.application.configure do |config| activity = Gitlab::Auth::Activity.new(opts) tracker = Gitlab::Auth::BlockedUserTracker.new(user, auth) - ActiveSession.destroy(user, auth.request.session.id) + # TODO: switch to `auth.request.session.id.private_id` in 13.7 + ActiveSession.destroy_with_rack_session_id(user, auth.request.session.id) activity.user_session_destroyed! ## diff --git a/db/migrate/20200831204646_add_project_feature_flags_to_plan_limits.rb b/db/migrate/20200831204646_add_project_feature_flags_to_plan_limits.rb new file mode 100644 index 00000000000..d4bd2431d9c --- /dev/null +++ b/db/migrate/20200831204646_add_project_feature_flags_to_plan_limits.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddProjectFeatureFlagsToPlanLimits < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column(:plan_limits, :project_feature_flags, :integer, default: 200, null: false) + end +end diff --git a/db/migrate/20200831222347_insert_project_feature_flags_plan_limits.rb b/db/migrate/20200831222347_insert_project_feature_flags_plan_limits.rb new file mode 100644 index 00000000000..2c91016d9dd --- /dev/null +++ b/db/migrate/20200831222347_insert_project_feature_flags_plan_limits.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class InsertProjectFeatureFlagsPlanLimits < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + return unless Gitlab.com? + + create_or_update_plan_limit('project_feature_flags', 'free', 50) + create_or_update_plan_limit('project_feature_flags', 'bronze', 100) + create_or_update_plan_limit('project_feature_flags', 'silver', 150) + create_or_update_plan_limit('project_feature_flags', 'gold', 200) + end + + def down + return unless Gitlab.com? + + create_or_update_plan_limit('project_feature_flags', 'free', 0) + create_or_update_plan_limit('project_feature_flags', 'bronze', 0) + create_or_update_plan_limit('project_feature_flags', 'silver', 0) + create_or_update_plan_limit('project_feature_flags', 'gold', 0) + end +end diff --git a/db/migrate/20200916120837_add_index_to_members_for_unaccepted_invitations.rb b/db/migrate/20200916120837_add_index_to_members_for_unaccepted_invitations.rb new file mode 100644 index 00000000000..3b0e4b99eb5 --- /dev/null +++ b/db/migrate/20200916120837_add_index_to_members_for_unaccepted_invitations.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class AddIndexToMembersForUnacceptedInvitations < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + INDEX_NAME = 'idx_members_created_at_user_id_invite_token' + INDEX_SCOPE = 'invite_token IS NOT NULL AND user_id IS NULL' + + disable_ddl_transaction! + + def up + add_concurrent_index(:members, :created_at, where: INDEX_SCOPE, name: INDEX_NAME) + end + + def down + remove_concurrent_index(:members, :created_at, where: INDEX_SCOPE, name: INDEX_NAME) + end +end diff --git a/db/schema_migrations/20200831204646 b/db/schema_migrations/20200831204646 new file mode 100644 index 00000000000..ff4f71e8fc7 --- /dev/null +++ b/db/schema_migrations/20200831204646 @@ -0,0 +1 @@ +06e87d83d5520e6ffbfb839d15a6dd02ad2caf5737136441d496e29e03a07e64
\ No newline at end of file diff --git a/db/schema_migrations/20200831222347 b/db/schema_migrations/20200831222347 new file mode 100644 index 00000000000..6a01ea5cdc3 --- /dev/null +++ b/db/schema_migrations/20200831222347 @@ -0,0 +1 @@ +00af22b19af29b453f0022ded835bd9246c602c63a04a51ef93cbedd47047753
\ No newline at end of file diff --git a/db/schema_migrations/20200916120837 b/db/schema_migrations/20200916120837 new file mode 100644 index 00000000000..41ce56533b2 --- /dev/null +++ b/db/schema_migrations/20200916120837 @@ -0,0 +1 @@ +ff246eb2761c4504b67b7d7b197990a671626038e50f1b82d6b3e4739a1ec3d4
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 99a070a3838..6e921810cfb 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -14362,7 +14362,8 @@ CREATE TABLE plan_limits ( npm_max_file_size bigint DEFAULT 524288000 NOT NULL, nuget_max_file_size bigint DEFAULT 524288000 NOT NULL, pypi_max_file_size bigint DEFAULT '3221225472'::bigint NOT NULL, - generic_packages_max_file_size bigint DEFAULT '5368709120'::bigint NOT NULL + generic_packages_max_file_size bigint DEFAULT '5368709120'::bigint NOT NULL, + project_feature_flags integer DEFAULT 200 NOT NULL ); CREATE SEQUENCE plan_limits_id_seq @@ -19312,6 +19313,8 @@ CREATE INDEX idx_jira_connect_subscriptions_on_installation_id ON jira_connect_s CREATE UNIQUE INDEX idx_jira_connect_subscriptions_on_installation_id_namespace_id ON jira_connect_subscriptions USING btree (jira_connect_installation_id, namespace_id); +CREATE INDEX idx_members_created_at_user_id_invite_token ON members USING btree (created_at) WHERE ((invite_token IS NOT NULL) AND (user_id IS NULL)); + CREATE INDEX idx_merge_requests_on_id_and_merge_jid ON merge_requests USING btree (id, merge_jid) WHERE ((merge_jid IS NOT NULL) AND (state_id = 4)); CREATE INDEX idx_merge_requests_on_source_project_and_branch_state_opened ON merge_requests USING btree (source_project_id, source_branch) WHERE (state_id = 1); diff --git a/doc/user/project/members/index.md b/doc/user/project/members/index.md index 3eb411aef18..d8579c4cd8e 100644 --- a/doc/user/project/members/index.md +++ b/doc/user/project/members/index.md @@ -93,6 +93,9 @@ invitation, change their access level, or even delete them. Once the user accepts the invitation, they will be prompted to create a new GitLab account using the same e-mail address the invitation was sent to. +Note: **Note:** +Unaccepted invites are automatically deleted after 90 days. + ## Project membership and requesting access Project owners can : diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index 46b69214877..bf5044b4832 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -54,8 +54,10 @@ module API user = find_user_from_sources return unless user - # Sessions are enforced to be unavailable for API calls, so ignore them for admin mode - Gitlab::Auth::CurrentUserMode.bypass_session!(user.id) if Feature.enabled?(:user_mode_in_session) + if user.is_a?(User) && Feature.enabled?(:user_mode_in_session) + # Sessions are enforced to be unavailable for API calls, so ignore them for admin mode + Gitlab::Auth::CurrentUserMode.bypass_session!(user.id) + end unless api_access_allowed?(user) forbidden!(api_access_denied_message(user)) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 46b7dac4745..439c26a0e99 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -29646,6 +29646,9 @@ msgstr "" msgid "by %{user}" msgstr "" +msgid "cannot be a date in the past" +msgstr "" + msgid "cannot be changed if a personal project has container registry tags." msgstr "" 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 diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100755..100644 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100755..100644 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |