diff --git a/CHANGELOG.md b/CHANGELOG.md index 037d3dd90d..e7a4857642 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,34 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 13.2.10 (2020-10-01) + +### Security (14 changes) + +- Do not store session id in Redis. +- Fix permission checks when updating confidentiality and milestone on issues or merge requests. +- Purge unaccepted member invitations older than 90 days. +- Adds feature flags plan limits. +- Prevent SVG XSS via Web IDE. +- Ensure user has no solo owned groups before triggering account deletion. +- Security fix safe params helper. +- Do not bypass admin mode when authenticated with deploy token. +- Fixes release asset link filepath ReDoS. +- Ensure global ID is of Annotation type in GraphQL destroy mutation. +- Validate that membership expiry dates are not in the past. +- Rate limit adding new email and re-sending email confirmation. +- Fix redaction of confidential Todos. +- Update GitLab Runner Helm Chart to 0.19.4. + + +## 13.2.9 (2020-09-04) + +### Fixed (2 changes) + +- Fix ActiveRecord::IrreversibleOrderError during restore from backup. !40789 +- Update the 2FA user update check to account for rounding errors. !41327 + + ## 13.2.8 (2020-09-02) ### Security (1 change) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index f9c1cf2f57..b9b445425c 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -13.2.8 +13.2.10 diff --git a/VERSION b/VERSION index f9c1cf2f57..b9b445425c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -13.2.8 +13.2.10 diff --git a/app/channels/application_cable/connection.rb b/app/channels/application_cable/connection.rb index 1361269f2a..b2d98d243f 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/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index fc0acd8f99..9cb1d16fbd 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -5,6 +5,7 @@ class Admin::UsersController < Admin::ApplicationController before_action :user, except: [:index, :new, :create] before_action :check_impersonation_availability, only: :impersonate + before_action :ensure_destroy_prerequisites_met, only: [:destroy] def index @users = User.filter_items(params[:filter]).order_name_asc @@ -168,7 +169,7 @@ class Admin::UsersController < Admin::ApplicationController end def destroy - user.delete_async(deleted_by: current_user, params: params.permit(:hard_delete)) + user.delete_async(deleted_by: current_user, params: destroy_params) respond_to do |format| format.html { redirect_to admin_users_path, status: :found, notice: _("The user is being deleted.") } @@ -197,6 +198,24 @@ class Admin::UsersController < Admin::ApplicationController user == current_user end + def destroy_params + params.permit(:hard_delete) + end + + def ensure_destroy_prerequisites_met + return if hard_delete? + + if user.solo_owned_groups.present? + message = s_('AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account') + + redirect_to admin_user_path(user), status: :see_other, alert: message + end + end + + def hard_delete? + destroy_params[:hard_delete] + end + def user @user ||= find_routable!(User, params[:id]) end diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb index d9e4a3a3ae..d61546fd5d 100644 --- a/app/controllers/concerns/authenticates_with_two_factor.rb +++ b/app/controllers/concerns/authenticates_with_two_factor.rb @@ -125,6 +125,10 @@ module AuthenticatesWithTwoFactor def user_changed?(user) return false unless session[:user_updated_at] - user.updated_at != session[:user_updated_at] + # See: https://gitlab.com/gitlab-org/gitlab/-/issues/244638 + # Rounding errors happen when the user is updated, as the Rails ActiveRecord + # object has higher precision than what is stored in the database, therefore + # using .to_i to force truncation to the timestamp + user.updated_at.to_i != session[:user_updated_at].to_i end end diff --git a/app/controllers/profiles/active_sessions_controller.rb b/app/controllers/profiles/active_sessions_controller.rb index d9ec3195fd..e4cd5d65e1 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/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index f666a1150a..da553e34ef 100644 --- a/app/controllers/profiles/emails_controller.rb +++ b/app/controllers/profiles/emails_controller.rb @@ -2,6 +2,8 @@ class Profiles::EmailsController < Profiles::ApplicationController before_action :find_email, only: [:destroy, :resend_confirmation_instructions] + before_action -> { rate_limit!(:profile_add_new_email) }, only: [:create] + before_action -> { rate_limit!(:profile_resend_email_confirmation) }, only: [:resend_confirmation_instructions] def index @primary_email = current_user.email @@ -38,6 +40,16 @@ class Profiles::EmailsController < Profiles::ApplicationController private + def rate_limit!(action) + rate_limiter = ::Gitlab::ApplicationRateLimiter + + if rate_limiter.throttled?(action, scope: current_user) + rate_limiter.log_request(request, action, current_user) + + redirect_back_or_default(options: { alert: _('This action has been performed too many times. Try again later.') }) + end + end + def email_params params.require(:email).permit(:email) end diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb index 69a3898af5..29f1e4bfd4 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -12,6 +12,7 @@ class Projects::RawController < Projects::ApplicationController before_action :authorize_download_code! before_action :show_rate_limit, only: [:show], unless: :external_storage_request? before_action :assign_ref_vars + before_action :no_cache_headers, only: [:show] before_action :redirect_to_external_storage, only: :show, if: :static_objects_external_storage_enabled? def show diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index b1c1fe3ba7..f054be50e2 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -10,7 +10,7 @@ class RegistrationsController < Devise::RegistrationsController skip_before_action :required_signup_info, :check_two_factor_requirement, only: [:welcome, :update_registration] prepend_before_action :check_captcha, only: :create - before_action :whitelist_query_limiting, only: [:destroy] + before_action :whitelist_query_limiting, :ensure_destroy_prerequisites_met, only: [:destroy] before_action :ensure_terms_accepted, if: -> { action_name == 'create' && Gitlab::CurrentSettings.current_application_settings.enforce_terms? } before_action :load_recaptcha, only: :new @@ -125,6 +125,14 @@ class RegistrationsController < Devise::RegistrationsController private + def ensure_destroy_prerequisites_met + if current_user.solo_owned_groups.present? + redirect_to profile_account_path, + status: :see_other, + alert: s_('Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account') + end + end + def user_created_message(confirmed: false) "User Created: username=#{resource.username} email=#{resource.email} ip=#{request.remote_ip} confirmed:#{confirmed}" end diff --git a/app/graphql/mutations/metrics/dashboard/annotations/base.rb b/app/graphql/mutations/metrics/dashboard/annotations/base.rb index 3126267da6..ad52f84378 100644 --- a/app/graphql/mutations/metrics/dashboard/annotations/base.rb +++ b/app/graphql/mutations/metrics/dashboard/annotations/base.rb @@ -9,7 +9,7 @@ module Mutations # This method is defined here in order to be used by `authorized_find!` in the subclasses. def find_object(id:) - GitlabSchema.object_from_id(id) + GitlabSchema.object_from_id(id, expected_type: ::Metrics::Dashboard::Annotation) end end end diff --git a/app/helpers/active_sessions_helper.rb b/app/helpers/active_sessions_helper.rb index 8fb23f99cb..2256bd006e 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, size: 16, 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/helpers/safe_params_helper.rb b/app/helpers/safe_params_helper.rb index 72bf1377b0..e9f0d82bb2 100644 --- a/app/helpers/safe_params_helper.rb +++ b/app/helpers/safe_params_helper.rb @@ -5,7 +5,7 @@ module SafeParamsHelper # Use this helper when generating links with `params.merge(...)` def safe_params if params.respond_to?(:permit!) - params.except(:host, :port, :protocol).permit! + params.except(*ActionDispatch::Routing::RouteSet::RESERVED_OPTIONS).permit! else params end diff --git a/app/models/active_session.rb b/app/models/active_session.rb index 4908290e06..dded0eb1dc 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,34 +83,6 @@ class ActiveSession end end - def self.destroy(user, session_id) - return unless session_id - - Gitlab::Redis::SharedState.with do |redis| - destroy_sessions(redis, user, [session_id]) - end - end - - def self.destroy_with_public_id(user, public_id) - decrypted_id = decrypt_public_id(public_id) - - return if decrypted_id.nil? - - session_id = Rack::Session::SessionId.new(decrypted_id) - destroy(user, session_id) - end - - def self.destroy_sessions(redis, user, session_ids) - key_names = session_ids.map { |session_id| key_name(user.id, session_id.public_id) } - - redis.srem(lookup_key_name(user.id), session_ids.map(&:public_id)) - - Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do - redis.del(key_names) - redis.del(rack_session_keys(session_ids)) - end - end - def self.cleanup(user) Gitlab::Redis::SharedState.with do |redis| clean_up_old_sessions(redis, user) @@ -105,12 +90,52 @@ class ActiveSession 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 + # 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 Gitlab::Redis::SharedState.with do |redis| - destroy_sessions(redis, user, session_ids.map(&:session_id)) if session_ids.any? + 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) } + + redis.srem(lookup_key_name(user.id), session_ids) + + Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do + redis.del(key_names) + redis.del(rack_session_keys(session_ids)) + end + end + + # 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| + destroy_sessions(redis, user, [session_id, decrypted_session_id, rack_session_private_id].compact) + end + end + + 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| + 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 - - # 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/application_setting.rb b/app/models/application_setting.rb index 25b81aef45..d3432d99fc 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -426,6 +426,8 @@ class ApplicationSetting < ApplicationRecord end def self.create_from_defaults + check_schema! + transaction(requires_new: true) do super end @@ -434,6 +436,22 @@ class ApplicationSetting < ApplicationRecord current_without_cache end + # Due to the frequency with which settings are accessed, it is + # likely that during a backup restore a running GitLab process + # will insert a new `application_settings` row before the + # constraints have been added to the table. This would add an + # extra row with ID 1 and prevent the primary key constraint from + # being added, which made ActiveRecord throw a + # IrreversibleOrderError anytime the settings were accessed + # (https://gitlab.com/gitlab-org/gitlab/-/issues/36405). To + # prevent this from happening, we do a sanity check that the + # primary key constraint is present before inserting a new entry. + def self.check_schema! + return if ActiveRecord::Base.connection.primary_key(self.table_name).present? + + raise "The `#{self.table_name}` table is missing a primary key constraint in the database schema" + end + # By default, the backend is Rails.cache, which uses # ActiveSupport::Cache::RedisStore. Since loading ApplicationSetting # can cause a significant amount of load on Redis, let's cache it in diff --git a/app/models/clusters/applications/runner.rb b/app/models/clusters/applications/runner.rb index 5548651ebe..71d007ba28 100644 --- a/app/models/clusters/applications/runner.rb +++ b/app/models/clusters/applications/runner.rb @@ -3,7 +3,7 @@ module Clusters module Applications class Runner < ApplicationRecord - VERSION = '0.18.3' + VERSION = '0.19.4' self.table_name = 'clusters_applications_runners' diff --git a/app/models/member.rb b/app/models/member.rb index 64453e6f24..34360fc62f 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/releases/link.rb b/app/models/releases/link.rb index dc7e78a85a..82272f4857 100644 --- a/app/models/releases/link.rb +++ b/app/models/releases/link.rb @@ -6,7 +6,9 @@ module Releases belongs_to :release - FILEPATH_REGEX = /\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 65a73dadc2..e309ea7d67 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 8d22f0edcd..a8cd516acd 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/services/members/base_service.rb b/app/services/members/base_service.rb index 5d69418fb7..3f55f661d9 100644 --- a/app/services/members/base_service.rb +++ b/app/services/members/base_service.rb @@ -7,6 +7,11 @@ module Members def initialize(current_user = nil, params = {}) @current_user = current_user @params = params + + # could be a string, force to an integer, part of fix + # https://gitlab.com/gitlab-org/gitlab/-/issues/219496 + # Allow the ArgumentError to be raised if it can't be converted to an integer. + @params[:access_level] = Integer(@params[:access_level]) if @params[:access_level] end def after_execute(args) diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb index 4743e9b02c..0c0548a17a 100644 --- a/app/services/todos/destroy/entity_leave_service.rb +++ b/app/services/todos/destroy/entity_leave_service.rb @@ -52,7 +52,14 @@ module Todos # rubocop: disable CodeReuse/ActiveRecord def remove_project_todos - Todo.where(project_id: non_authorized_projects, user_id: user.id).delete_all + # Issues are viewable by guests (even in private projects), so remove those todos + # from projects without guest access + Todo.where(project_id: non_authorized_guest_projects, user_id: user.id) + .delete_all + + # MRs require reporter access, so remove those todos that are not authorized + Todo.where(project_id: non_authorized_reporter_projects, target_type: MergeRequest.name, user_id: user.id) + .delete_all end # rubocop: enable CodeReuse/ActiveRecord @@ -68,7 +75,7 @@ module Todos when Project { id: entity.id } when Namespace - { namespace_id: non_member_groups } + { namespace_id: non_authorized_reporter_groups } end Project.where(condition) @@ -76,8 +83,32 @@ module Todos # rubocop: enable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord - def non_authorized_projects - projects.where('id NOT IN (?)', user.authorized_projects.select(:id)) + def authorized_reporter_projects + user.authorized_projects(Gitlab::Access::REPORTER).select(:id) + end + # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def authorized_guest_projects + user.authorized_projects(Gitlab::Access::GUEST).select(:id) + end + # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def non_authorized_reporter_projects + projects.where('id NOT IN (?)', authorized_reporter_projects) + end + # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def non_authorized_guest_projects + projects.where('id NOT IN (?)', authorized_guest_projects) + end + # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def authorized_reporter_groups + GroupsFinder.new(user, min_access_level: Gitlab::Access::REPORTER).execute.select(:id) end # rubocop: enable CodeReuse/ActiveRecord @@ -91,9 +122,9 @@ module Todos # rubocop: enable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord - def non_member_groups + def non_authorized_reporter_groups entity.self_and_descendants.select(:id) - .where('id NOT IN (?)', user.membership_groups.select(:id)) + .where('id NOT IN (?)', authorized_reporter_groups) end # rubocop: enable CodeReuse/ActiveRecord @@ -106,8 +137,6 @@ module Todos # rubocop: disable CodeReuse/ActiveRecord def confidential_issues assigned_ids = IssueAssignee.select(:issue_id).where(user_id: user.id) - authorized_reporter_projects = user - .authorized_projects(Gitlab::Access::REPORTER).select(:id) Issue.where(project_id: projects, confidential: true) .where('project_id NOT IN(?)', authorized_reporter_projects) diff --git a/app/validators/future_date_validator.rb b/app/validators/future_date_validator.rb new file mode 100644 index 0000000000..1ba4a1e273 --- /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 c4b3fa80d7..97f13a55de 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 5148772c88..59a949fade 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -291,6 +291,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 0000000000..4b75b43791 --- /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/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index b7432c4cbe..1bba665c91 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -440,6 +440,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 d8a4da8cdf..a83d83d9fb 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 0000000000..d4bd2431d9 --- /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 0000000000..2c91016d9d --- /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 0000000000..3b0e4b99eb --- /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/20200916120837 b/db/schema_migrations/20200916120837 new file mode 100644 index 0000000000..41ce56533b --- /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 3c8a6a74c8..ecc4ec7b95 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13891,7 +13891,8 @@ CREATE TABLE public.plan_limits ( ci_max_artifact_size_requirements integer DEFAULT 0 NOT NULL, ci_max_artifact_size_coverage_fuzzing integer DEFAULT 0 NOT NULL, ci_max_artifact_size_browser_performance integer DEFAULT 0 NOT NULL, - ci_max_artifact_size_load_performance integer DEFAULT 0 NOT NULL + ci_max_artifact_size_load_performance integer DEFAULT 0 NOT NULL, + project_feature_flags integer DEFAULT 200 NOT NULL ); CREATE SEQUENCE public.plan_limits_id_seq @@ -18580,6 +18581,8 @@ CREATE INDEX idx_jira_connect_subscriptions_on_installation_id ON public.jira_co CREATE UNIQUE INDEX idx_jira_connect_subscriptions_on_installation_id_namespace_id ON public.jira_connect_subscriptions USING btree (jira_connect_installation_id, namespace_id); +CREATE INDEX idx_members_created_at_user_id_invite_token ON public.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 public.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 public.merge_requests USING btree (source_project_id, source_branch) WHERE (state_id = 1); @@ -23869,5 +23872,8 @@ COPY "schema_migrations" (version) FROM STDIN; 20200716120419 20200717040735 20200728182311 +20200831204646 +20200831222347 +20200916120837 \. diff --git a/doc/.vale/gitlab/Acronyms.yml b/doc/.vale/gitlab/Acronyms.yml index d166e71491..d6f5b66567 100644 --- a/doc/.vale/gitlab/Acronyms.yml +++ b/doc/.vale/gitlab/Acronyms.yml @@ -61,6 +61,7 @@ exceptions: - RSA - RSS - SAML + - SCIM - SCP - SCSS - SHA diff --git a/doc/administration/pages/index.md b/doc/administration/pages/index.md index 4efd92eaa0..f90184eefd 100644 --- a/doc/administration/pages/index.md +++ b/doc/administration/pages/index.md @@ -409,7 +409,7 @@ pages: ### Using a custom Certificate Authority (CA) NOTE: **Note:** -[Before 13.2](https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/4289), when using Omnibus, a [workaround was required](https://docs.gitlab.com/13.1/ee/administration/pages/index.html#using-a-custom-certificate-authority-ca). +[Before 13.3](https://gitlab.com/gitlab-org/omnibus-gitlab/-/merge_requests/4411), when using Omnibus, a [workaround was required](https://docs.gitlab.com/13.1/ee/administration/pages/index.html#using-a-custom-certificate-authority-ca). When using certificates issued by a custom CA, [Access Control](../../user/project/pages/pages_access_control.md#gitlab-pages-access-control) and the [online view of HTML job artifacts](../../ci/pipelines/job_artifacts.md#browsing-artifacts) diff --git a/doc/api/protected_branches.md b/doc/api/protected_branches.md index a1f42958c3..b9bb24cf31 100644 --- a/doc/api/protected_branches.md +++ b/doc/api/protected_branches.md @@ -248,7 +248,7 @@ Example response: ### Example with user / group level access **(STARTER)** Elements in the `allowed_to_push` / `allowed_to_merge` / `allowed_to_unprotect` array should take the -form `{user_id: integer}`, `{group_id: integer}` or `{access_level: integer}`. Each user must have access to the project and each group must [have this project shared](../user/project/members/share_project_with_groups.md). These access levels allow [more granular control over protected branch access](../user/project/protected_branches.md#restricting-push-and-merge-access-to-certain-users-starter) and were [added to the API in](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/3516) GitLab 10.3 EE. +form `{user_id: integer}`, `{group_id: integer}` or `{access_level: integer}`. Each user must have access to the project and each group must [have this project shared](../user/project/members/share_project_with_groups.md). These access levels allow [more granular control over protected branch access](../user/project/protected_branches.md#restricting-push-and-merge-access-to-certain-users-starter) and were [added to the API](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/3516) in GitLab 10.3 EE. ```shell curl --request POST --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/projects/5/protected_branches?name=*-stable&allowed_to_push%5B%5D%5Buser_id%5D=1" diff --git a/doc/api/scim.md b/doc/api/scim.md index bd86408ee6..1e86a3ce53 100644 --- a/doc/api/scim.md +++ b/doc/api/scim.md @@ -2,7 +2,7 @@ > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/9388) in [GitLab Silver](https://about.gitlab.com/pricing/) 11.10. -The SCIM API implements [the RFC7644 protocol](https://tools.ietf.org/html/rfc7644). +The SCIM API implements the [RFC7644 protocol](https://tools.ietf.org/html/rfc7644). CAUTION: **Caution:** This API is for internal system use for connecting with a SCIM provider. While it can be used directly, it is subject to change without notice. diff --git a/doc/development/changelog.md b/doc/development/changelog.md index 00a0573a8b..28d8b51c51 100644 --- a/doc/development/changelog.md +++ b/doc/development/changelog.md @@ -41,8 +41,8 @@ the `author` field. GitLab team members **should not**. a changelog entry regardless of these guidelines if the contributor wants one. Example: "Fixed a typo on the search results page." - Any docs-only changes **should not** have a changelog entry. -- Any change behind a feature flag **should not** have a changelog entry - unless - the feature flag has been defaulted to true. +- Any change behind a disabled feature flag **should not** have a changelog entry. +- Any change behind an enabled feature flag **should** have a changelog entry. - A change that [removes a feature flag](feature_flags/development.md) **should** have a changelog entry - only if the feature flag did not default to true already. - A fix for a regression introduced and then fixed in the same release (i.e., diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md index ef9fd748db..d0dcbda4a6 100644 --- a/doc/development/testing_guide/frontend_testing.md +++ b/doc/development/testing_guide/frontend_testing.md @@ -638,6 +638,38 @@ unit tests. Instead of `setImmediate`, use `jest.runAllTimers` or `jest.runOnlyPendingTimers` to run pending timers. The latter is useful when you have `setInterval` in the code. **Remember:** our Jest configuration uses fake timers. +## Avoid non-deterministic specs + +Non-determinism is the breeding ground for flaky and brittle specs. Such specs end up breaking the CI pipeline, interrupting the work flow of other contributors. + +1. Make sure your test subject's collaborators (e.g., axios, apollo, lodash helpers) and test environment (e.g., Date) behave consistently across systems and over time. +1. Make sure tests are focused and not doing "extra work" (e.g., needlessly creating the test subject more than once in an individual test) + +### Faking `Date` for determinism + +Consider using `useFakeDate` to ensure a consistent value is returned with every `new Date()` or `Date.now()`. + +```javascript +import { useFakeDate } from 'helpers/fake_date'; + +describe('cool/component', () => { + useFakeDate(); + + // ... +}); +``` + +### Faking `Math.random` for determinism + +Consider replacing `Math.random` with a fake when the test subject depends on it. + +```javascript +beforeEach(() => { + // https://xkcd.com/221/ + jest.spyOn(Math, 'random').mockReturnValue(0.4); +}); +``` + ## Factories TBU diff --git a/doc/user/admin_area/custom_project_templates.md b/doc/user/admin_area/custom_project_templates.md index d194ca4221..8d044b94fb 100644 --- a/doc/user/admin_area/custom_project_templates.md +++ b/doc/user/admin_area/custom_project_templates.md @@ -15,7 +15,8 @@ templates are sourced. Every project directly under the group namespace will be available to the user if they have access to them. For example: -- Public project in the group will be available to every logged in user. +- Public projects, in the group will be available to every signed-in user, if all enabled [project features](../project/settings/index.md#sharing-and-permissions) + are set to **Everyone With Access**. - Private projects will be available only if the user is a member of the project. Repository and database information that are copied over to each new project are diff --git a/doc/user/clusters/management_project.md b/doc/user/clusters/management_project.md index 892d2bce18..f7241444a6 100644 --- a/doc/user/clusters/management_project.md +++ b/doc/user/clusters/management_project.md @@ -73,10 +73,10 @@ configure cluster: name: production ``` -### Setting the environment scope **(PREMIUM)** +### Setting the environment scope [Environment -scopes](../project/clusters/index.md#setting-the-environment-scope-premium) +scopes](../project/clusters/index.md#setting-the-environment-scope) are usable when associating multiple clusters to the same management project. diff --git a/doc/user/group/custom_project_templates.md b/doc/user/group/custom_project_templates.md index ebeacda24c..7c02d80d08 100644 --- a/doc/user/group/custom_project_templates.md +++ b/doc/user/group/custom_project_templates.md @@ -20,9 +20,8 @@ GitLab administrators can [set project templates for an entire GitLab instance](../admin_area/custom_project_templates.md). Within this section, you can configure the group where all the custom project -templates are sourced. Every project directly under the group namespace will be -available to the user if they have access to them. For example, every public -project in the group will be available to every logged in user. +templates are sourced. Every project _template_ directly under the group namespace is +available to every signed-in user, if all enabled [project features](../project/settings/index.md#sharing-and-permissions) are set to **Everyone With Access**. However, private projects will be available only if the user is a member of the project. diff --git a/doc/user/project/clusters/add_eks_clusters.md b/doc/user/project/clusters/add_eks_clusters.md index b11483a744..f58e8bf0f9 100644 --- a/doc/user/project/clusters/add_eks_clusters.md +++ b/doc/user/project/clusters/add_eks_clusters.md @@ -135,7 +135,7 @@ To create and add a new Kubernetes cluster to your project, group, or instance: 1. Click **Authenticate with AWS**. 1. Choose your cluster's settings: - **Kubernetes cluster name** - The name you wish to give the cluster. - - **Environment scope** - The [associated environment](index.md#setting-the-environment-scope-premium) to this cluster. + - **Environment scope** - The [associated environment](index.md#setting-the-environment-scope) to this cluster. - **Kubernetes version** - The Kubernetes version to use. Currently the only version supported is 1.14. - **Role name** - Select the [IAM role](https://docs.aws.amazon.com/eks/latest/userguide/service_IAM_role.html) to allow Amazon EKS and the Kubernetes control plane to manage AWS resources on your behalf. This IAM role is separate diff --git a/doc/user/project/clusters/add_gke_clusters.md b/doc/user/project/clusters/add_gke_clusters.md index 2746076bef..a0180fbecf 100644 --- a/doc/user/project/clusters/add_gke_clusters.md +++ b/doc/user/project/clusters/add_gke_clusters.md @@ -55,7 +55,7 @@ To create and add a new Kubernetes cluster to your project, group, or instance: **Sign in with Google** button. 1. Choose your cluster's settings: - **Kubernetes cluster name** - The name you wish to give the cluster. - - **Environment scope** - The [associated environment](index.md#setting-the-environment-scope-premium) to this cluster. + - **Environment scope** - The [associated environment](index.md#setting-the-environment-scope) to this cluster. - **Google Cloud Platform project** - Choose the project you created in your GCP console that will host the Kubernetes cluster. Learn more about [Google Cloud Platform projects](https://cloud.google.com/resource-manager/docs/creating-managing-projects). diff --git a/doc/user/project/clusters/add_remove_clusters.md b/doc/user/project/clusters/add_remove_clusters.md index 65f1c59f4c..abfb7745ce 100644 --- a/doc/user/project/clusters/add_remove_clusters.md +++ b/doc/user/project/clusters/add_remove_clusters.md @@ -169,7 +169,7 @@ To add a Kubernetes cluster to your project, group, or instance: 1. Click the **Add existing cluster** tab and fill in the details: 1. **Kubernetes cluster name** (required) - The name you wish to give the cluster. 1. **Environment scope** (required) - The - [associated environment](index.md#setting-the-environment-scope-premium) to this cluster. + [associated environment](index.md#setting-the-environment-scope) to this cluster. 1. **API URL** (required) - It's the URL that GitLab uses to access the Kubernetes API. Kubernetes exposes several APIs, we want the "base" URL that is common to all of them. diff --git a/doc/user/project/clusters/index.md b/doc/user/project/clusters/index.md index ddcfd376d8..efbc9d8762 100644 --- a/doc/user/project/clusters/index.md +++ b/doc/user/project/clusters/index.md @@ -74,10 +74,10 @@ project. That way you can have different clusters for different environments, like dev, staging, production, and so on. Simply add another cluster, like you did the first time, and make sure to -[set an environment scope](#setting-the-environment-scope-premium) that will +[set an environment scope](#setting-the-environment-scope) that will differentiate the new cluster with the rest. -#### Setting the environment scope **(PREMIUM)** +#### Setting the environment scope When adding more than one Kubernetes cluster to your project, you need to differentiate them with an environment scope. The environment scope associates clusters with [environments](../../../ci/environments/index.md) similar to how the diff --git a/doc/user/project/members/index.md b/doc/user/project/members/index.md index 3eb411aef1..d8579c4cd8 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 8c4c466cd7..8633df4cb7 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -53,8 +53,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/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index 4eeec534fc..6173918b45 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -31,7 +31,9 @@ module Gitlab group_export: { threshold: -> { application_settings.group_export_limit }, interval: 1.minute }, group_download_export: { threshold: -> { application_settings.group_download_export_limit }, interval: 1.minute }, group_import: { threshold: -> { application_settings.group_import_limit }, interval: 1.minute }, - group_testing_hook: { threshold: 5, interval: 1.minute } + group_testing_hook: { threshold: 5, interval: 1.minute }, + profile_add_new_email: { threshold: 5, interval: 1.minute }, + profile_resend_email_confirmation: { threshold: 5, interval: 1.minute } }.freeze end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 81f7b67b2a..fa20e5373f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1942,6 +1942,9 @@ msgstr "" msgid "AdminUsers|You cannot remove your own admin rights." msgstr "" +msgid "AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account" +msgstr "" + msgid "Administration" msgstr "" @@ -17980,6 +17983,9 @@ msgstr "" msgid "Profiles|You don't have access to delete this user." msgstr "" +msgid "Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account" +msgstr "" + msgid "Profiles|You must transfer ownership or delete these groups before you can delete your account." msgstr "" @@ -23972,6 +23978,9 @@ msgstr "" msgid "This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention." msgstr "" +msgid "This action has been performed too many times. Try again later." +msgstr "" + msgid "This also resolves all related threads" msgstr "" @@ -27623,6 +27632,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/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 08a1d7c9fa..9e7f24ff7e 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -36,7 +36,7 @@ RSpec.describe Admin::UsersController do end end - describe 'DELETE #user with projects', :sidekiq_might_not_need_inline do + describe 'DELETE #destroy', :sidekiq_might_not_need_inline do let(:project) { create(:project, namespace: user.namespace) } let!(:issue) { create(:issue, author: user) } @@ -59,6 +59,41 @@ RSpec.describe Admin::UsersController do expect(User.exists?(user.id)).to be_falsy expect(Issue.exists?(issue.id)).to be_falsy end + + context 'prerequisites for account deletion' do + context 'solo-owned groups' do + let(:group) { create(:group) } + + context 'if the user is the sole owner of at least one group' do + before do + create(:group_member, :owner, group: group, user: user) + end + + context 'soft-delete' do + it 'fails' do + delete :destroy, params: { id: user.username } + + message = s_('AdminUsers|You must transfer ownership or delete the groups owned by this user before you can delete their account') + + expect(flash[:alert]).to eq(message) + expect(response).to have_gitlab_http_status(:see_other) + expect(response).to redirect_to admin_user_path(user) + expect(User.exists?(user.id)).to be_truthy + end + end + + context 'hard-delete' do + it 'succeeds' do + delete :destroy, params: { id: user.username, hard_delete: true } + + expect(response).to redirect_to(admin_users_path) + expect(flash[:notice]).to eq(_('The user is being deleted.')) + expect(User.exists?(user.id)).to be_falsy + end + end + end + end + end end describe 'PUT #activate' do diff --git a/spec/controllers/groups/group_members_controller_spec.rb b/spec/controllers/groups/group_members_controller_spec.rb index 85f1b247ee..4b9dd3629f 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/profiles/emails_controller_spec.rb b/spec/controllers/profiles/emails_controller_spec.rb index 246f8a6cd7..950120ae56 100644 --- a/spec/controllers/profiles/emails_controller_spec.rb +++ b/spec/controllers/profiles/emails_controller_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Profiles::EmailsController do - let(:user) { create(:user) } + let_it_be(:user) { create(:user) } before do sign_in(user) @@ -15,37 +15,75 @@ RSpec.describe Profiles::EmailsController do end end - describe '#create' do - context 'when email address is valid' do - let(:email_params) { { email: "add_email@example.com" } } + shared_examples_for 'respects the rate limit' do + context 'after the rate limit is exceeded' do + before do + allowed_threshold = Gitlab::ApplicationRateLimiter.rate_limits[action][:threshold] - it 'sends an email confirmation' do - expect { post(:create, params: { email: email_params }) }.to change { ActionMailer::Base.deliveries.size } + allow(Gitlab::ApplicationRateLimiter) + .to receive(:increment) + .and_return(allowed_threshold + 1) end + + it 'does not send any email' do + expect { subject }.not_to change { ActionMailer::Base.deliveries.size } + end + + it 'displays an alert' do + subject + + expect(response).to have_gitlab_http_status(:redirect) + expect(flash[:alert]).to eq(_('This action has been performed too many times. Try again later.')) + end + end + end + + describe '#create' do + let(:email) { 'add_email@example.com' } + let(:params) { { email: { email: email } } } + + subject { post(:create, params: params) } + + it 'sends an email confirmation' do + expect { subject }.to change { ActionMailer::Base.deliveries.size } end context 'when email address is invalid' do - let(:email_params) { { email: "test.@example.com" } } + let(:email) { 'invalid.@example.com' } it 'does not send an email confirmation' do - expect { post(:create, params: { email: email_params }) }.not_to change { ActionMailer::Base.deliveries.size } + expect { subject }.not_to change { ActionMailer::Base.deliveries.size } end end + + it_behaves_like 'respects the rate limit' do + let(:action) { :profile_add_new_email } + end end describe '#resend_confirmation_instructions' do - let(:email_params) { { email: "add_email@example.com" } } + let_it_be(:email) { create(:email, user: user) } + let(:params) { { id: email.id } } + + subject { put(:resend_confirmation_instructions, params: params) } it 'resends an email confirmation' do - email = user.emails.create(email: 'add_email@example.com') + expect { subject }.to change { ActionMailer::Base.deliveries.size } - expect { put(:resend_confirmation_instructions, params: { id: email }) }.to change { ActionMailer::Base.deliveries.size } - expect(ActionMailer::Base.deliveries.last.to).to eq [email_params[:email]] - expect(ActionMailer::Base.deliveries.last.subject).to match "Confirmation instructions" + expect(ActionMailer::Base.deliveries.last.to).to eq [email.email] + expect(ActionMailer::Base.deliveries.last.subject).to match 'Confirmation instructions' end - it 'unable to resend an email confirmation' do - expect { put(:resend_confirmation_instructions, params: { id: 1 }) }.not_to change { ActionMailer::Base.deliveries.size } + context 'email does not exist' do + let(:params) { { id: non_existing_record_id } } + + it 'does not send an email confirmation' do + expect { subject }.not_to change { ActionMailer::Base.deliveries.size } + end + end + + it_behaves_like 'respects the rate limit' do + let(:action) { :profile_resend_email_confirmation } end end end diff --git a/spec/controllers/projects/project_members_controller_spec.rb b/spec/controllers/projects/project_members_controller_spec.rb index 40a220d57a..ae05e2d263 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 + } - expect(requester.reload.human_access).to eq(label) + 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 + + 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/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index 5f10343eb7..b3921164c8 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -33,6 +33,11 @@ RSpec.describe Projects::RawController do it_behaves_like 'project cache control headers' it_behaves_like 'content disposition headers' + it_behaves_like 'uncached response' do + before do + subject + end + end end context 'image header' do diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 66caa58666..c1afc75a6f 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -384,6 +384,24 @@ RSpec.describe RegistrationsController do expect_success end end + + context 'prerequisites for account deletion' do + context 'solo-owned groups' do + let(:group) { create(:group) } + + context 'if the user is the sole owner of at least one group' do + before do + create(:group_member, :owner, group: group, user: user) + end + + it 'fails' do + delete :destroy, params: { password: '12345678' } + + expect_failure(s_('Profiles|You must transfer ownership or delete groups you are an owner of before you can delete your account')) + end + end + end + end end describe '#update_registration' do diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index e4b53186ea..216991c56c 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -31,6 +31,7 @@ FactoryBot.define do pages_access_level do visibility_level == Gitlab::VisibilityLevel::PUBLIC ? ProjectFeature::ENABLED : ProjectFeature::PRIVATE end + metrics_dashboard_access_level { ProjectFeature::PRIVATE } # we can't assign the delegated `#ci_cd_settings` attributes directly, as the # `#ci_cd_settings` relation needs to be created first @@ -54,7 +55,9 @@ FactoryBot.define do issues_access_level: evaluator.issues_access_level, forking_access_level: evaluator.forking_access_level, merge_requests_access_level: merge_requests_access_level, - repository_access_level: evaluator.repository_access_level + repository_access_level: evaluator.repository_access_level, + pages_access_level: evaluator.pages_access_level, + metrics_dashboard_access_level: evaluator.metrics_dashboard_access_level } if ActiveRecord::Migrator.current_version >= PAGES_ACCESS_LEVEL_SCHEMA_VERSION @@ -300,6 +303,9 @@ FactoryBot.define do trait(:pages_enabled) { pages_access_level { ProjectFeature::ENABLED } } trait(:pages_disabled) { pages_access_level { ProjectFeature::DISABLED } } trait(:pages_private) { pages_access_level { ProjectFeature::PRIVATE } } + trait(:metrics_dashboard_enabled) { metrics_dashboard_access_level { ProjectFeature::ENABLED } } + trait(:metrics_dashboard_disabled) { metrics_dashboard_access_level { ProjectFeature::DISABLED } } + trait(:metrics_dashboard_private) { metrics_dashboard_access_level { ProjectFeature::PRIVATE } } trait :auto_devops do association :auto_devops, factory: :project_auto_devops diff --git a/spec/frontend/helpers/fake_date.js b/spec/frontend/helpers/fake_date.js new file mode 100644 index 0000000000..8417b1c520 --- /dev/null +++ b/spec/frontend/helpers/fake_date.js @@ -0,0 +1,49 @@ +// Frida Kahlo's birthday (6 = July) +export const DEFAULT_ARGS = [2020, 6, 6]; + +const RealDate = Date; + +const isMocked = val => Boolean(val.mock); + +export const createFakeDateClass = ctorDefault => { + const FakeDate = new Proxy(RealDate, { + construct: (target, argArray) => { + const ctorArgs = argArray.length ? argArray : ctorDefault; + + return new RealDate(...ctorArgs); + }, + apply: (target, thisArg, argArray) => { + const ctorArgs = argArray.length ? argArray : ctorDefault; + + return RealDate(...ctorArgs); + }, + // We want to overwrite the default 'now', but only if it's not already mocked + get: (target, prop) => { + if (prop === 'now' && !isMocked(target[prop])) { + return () => new RealDate(...ctorDefault).getTime(); + } + + return target[prop]; + }, + getPrototypeOf: target => { + return target.prototype; + }, + // We need to be able to set props so that `jest.spyOn` will work. + set: (target, prop, value) => { + // eslint-disable-next-line no-param-reassign + target[prop] = value; + return true; + }, + }); + + return FakeDate; +}; + +export const useFakeDate = (...args) => { + const FakeDate = createFakeDateClass(args.length ? args : DEFAULT_ARGS); + global.Date = FakeDate; +}; + +export const useRealDate = () => { + global.Date = RealDate; +}; diff --git a/spec/frontend/helpers/fake_date_spec.js b/spec/frontend/helpers/fake_date_spec.js new file mode 100644 index 0000000000..8afc8225f9 --- /dev/null +++ b/spec/frontend/helpers/fake_date_spec.js @@ -0,0 +1,33 @@ +import { createFakeDateClass, DEFAULT_ARGS, useRealDate } from './fake_date'; + +describe('spec/helpers/fake_date', () => { + describe('createFakeDateClass', () => { + let FakeDate; + + beforeAll(() => { + useRealDate(); + }); + + beforeEach(() => { + FakeDate = createFakeDateClass(DEFAULT_ARGS); + }); + + it('should use default args', () => { + expect(new FakeDate()).toEqual(new Date(...DEFAULT_ARGS)); + expect(FakeDate()).toEqual(Date(...DEFAULT_ARGS)); + }); + + it('should have deterministic now()', () => { + expect(FakeDate.now()).not.toBe(Date.now()); + expect(FakeDate.now()).toBe(new Date(...DEFAULT_ARGS).getTime()); + }); + + it('should be instanceof Date', () => { + expect(new FakeDate()).toBeInstanceOf(Date); + }); + + it('should be instanceof self', () => { + expect(new FakeDate()).toBeInstanceOf(FakeDate); + }); + }); +}); diff --git a/spec/graphql/mutations/issues/set_confidential_spec.rb b/spec/graphql/mutations/issues/set_confidential_spec.rb index 820f9aa5e1..0b2fc0ecb9 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 1c0d655ee8..ccb2d9bd13 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/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index fd4e8bc1cd..786db23ffc 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/spec/lib/gitlab/current_settings_spec.rb @@ -115,6 +115,16 @@ RSpec.describe Gitlab::CurrentSettings do expect(settings).to have_attributes(settings_from_defaults) end + context 'when ApplicationSettings does not have a primary key' do + before do + allow(ActiveRecord::Base.connection).to receive(:primary_key).with('application_settings').and_return(nil) + end + + it 'raises an exception if ApplicationSettings does not have a primary key' do + expect { described_class.current_application_settings }.to raise_error(/table is missing a primary key constraint/) + end + end + context 'with pending migrations' do let(:current_settings) { described_class.current_application_settings } 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 0000000000..1ad070de1e --- /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 de39c8c7c5..f0bae3f29c 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}", '') - ActiveSession.destroy_with_public_id(user, session.public_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 - total_sessions = ActiveSession.list(user).count - expect(total_sessions).to eq 0 + 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 + + it 'removes the lookup entry' do + subject + + 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) } - expect do - ActiveSession.destroy_with_public_id(user, "") - end.not_to raise_error + it 'calls .destroy_sessions' do + expect(ActiveSession).to( + receive(:destroy_sessions) + .with(anything, user, [rack_session.private_id])) - expect do - ActiveSession.destroy_with_public_id(user, "aaaaaaaa") - end.not_to raise_error + subject + end + + 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 } + + 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 } - ActiveSession.destroy_with_public_id(user, nil) - ActiveSession.destroy_with_public_id(user, "") - ActiveSession.destroy_with_public_id(user, "aaaaaaaa") + 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 + + 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/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 5723b0d072..deed1abeff 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -654,6 +654,16 @@ RSpec.describe ApplicationSetting do end end + context 'when ApplicationSettings does not have a primary key' do + before do + allow(ActiveRecord::Base.connection).to receive(:primary_key).with(described_class.table_name).and_return(nil) + end + + it 'raises an exception' do + expect { described_class.create_from_defaults }.to raise_error(/table is missing a primary key constraint/) + end + end + describe '#disabled_oauth_sign_in_sources=' do before do allow(Devise).to receive(:omniauth_providers).and_return([:github]) diff --git a/spec/models/concerns/expirable_spec.rb b/spec/models/concerns/expirable_spec.rb index b20d759fc3..5eb6530881 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 4323263855..d1b81d9986 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -18,6 +18,13 @@ RSpec.describe Member do it { is_expected.to validate_presence_of(:source) } it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.all_values) } + 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 f25f893318..388d04c801 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/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index bd0426601d..7d637757f3 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/files_spec.rb b/spec/requests/api/files_spec.rb index b50f63ed67..9187777619 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -502,16 +502,13 @@ RSpec.describe API::Files do expect(response).to have_gitlab_http_status(:ok) end - it 'sets no-cache headers' do - url = route('.gitignore') + "/raw" - expect(Gitlab::Workhorse).to receive(:send_git_blob) + it_behaves_like 'uncached response' do + before do + url = route('.gitignore') + "/raw" + expect(Gitlab::Workhorse).to receive(:send_git_blob) - get api(url, current_user), params: params - - expect(response.headers["Cache-Control"]).to include("no-store") - expect(response.headers["Cache-Control"]).to include("no-cache") - expect(response.headers["Pragma"]).to eq("no-cache") - expect(response.headers["Expires"]).to eq("Fri, 01 Jan 1990 00:00:00 GMT") + get api(url, current_user), params: params + end end context 'when mandatory params are not given' do diff --git a/spec/requests/api/graphql/mutations/metrics/dashboard/annotations/delete_spec.rb b/spec/requests/api/graphql/mutations/metrics/dashboard/annotations/delete_spec.rb index 2459a6f382..2d4ed3ff06 100644 --- a/spec/requests/api/graphql/mutations/metrics/dashboard/annotations/delete_spec.rb +++ b/spec/requests/api/graphql/mutations/metrics/dashboard/annotations/delete_spec.rb @@ -9,13 +9,9 @@ RSpec.describe Mutations::Metrics::Dashboard::Annotations::Delete do let_it_be(:project) { create(:project, :private, :repository) } let_it_be(:environment) { create(:environment, project: project) } let_it_be(:annotation) { create(:metrics_dashboard_annotation, environment: environment) } - let(:mutation) do - variables = { - id: GitlabSchema.id_from_object(annotation).to_s - } - graphql_mutation(:delete_annotation, variables) - end + let(:variables) { { id: GitlabSchema.id_from_object(annotation).to_s } } + let(:mutation) { graphql_mutation(:delete_annotation, variables) } def mutation_response graphql_mutation_response(:delete_annotation) @@ -37,15 +33,11 @@ RSpec.describe Mutations::Metrics::Dashboard::Annotations::Delete do end context 'with invalid params' do - let(:mutation) do - variables = { - id: 'invalid_id' - } + let(:variables) { { id: GitlabSchema.id_from_object(project).to_s } } - graphql_mutation(:delete_annotation, variables) + it_behaves_like 'a mutation that returns top-level errors' do + let(:match_errors) { eq(["#{variables[:id]} is not a valid id for #{annotation.class}."]) } end - - it_behaves_like 'a mutation that returns top-level errors', errors: ['invalid_id is not a valid GitLab id.'] end context 'when the delete fails' do diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 23889912d7..549853db88 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 fdf2326b75..0a9e2fd1bc 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 @@ -59,6 +60,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 77bd540e22..b115879a72 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', @@ -52,7 +53,8 @@ RSpec.describe Issues::UpdateService, :mailer do state_event: 'close', label_ids: [label.id], due_date: Date.tomorrow, - discussion_locked: true + discussion_locked: true, + milestone_id: milestone.id } end @@ -69,6 +71,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 it 'refreshes the number of open issues when the issue is made confidential', :use_clean_rails_memory_store_caching do @@ -82,6 +92,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 @@ -91,6 +103,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 it 'updates open issue counter for assignees when issue is reassigned' do @@ -157,7 +171,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' @@ -167,6 +181,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/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index f510916558..2d4457f3f6 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -31,17 +31,35 @@ RSpec.describe Members::UpdateService do end context 'when member is downgraded to guest' do - let(:params) do - { access_level: Gitlab::Access::GUEST } + shared_examples 'schedules to delete confidential todos' do + it do + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + + updated_member = described_class.new(current_user, params).execute(member, permission: permission) + + expect(updated_member).to be_valid + expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + end end - it 'schedules to delete confidential todos' do - expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + context 'with Gitlab::Access::GUEST level as a string' do + let(:params) { { access_level: Gitlab::Access::GUEST.to_s } } - updated_member = described_class.new(current_user, params).execute(member, permission: permission) + it_behaves_like 'schedules to delete confidential todos' + end - expect(updated_member).to be_valid - expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + context 'with Gitlab::Access::GUEST level as an integer' do + let(:params) { { access_level: Gitlab::Access::GUEST } } + + it_behaves_like 'schedules to delete confidential todos' + end + end + + context 'when access_level is invalid' do + let(:params) { { access_level: 'invalid' } } + + it 'raises an error' do + expect { described_class.new(current_user, params) }.to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"') end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index c3433c8c9d..5cbfb92263 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', @@ -60,7 +61,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) @@ -83,6 +85,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( @@ -150,6 +172,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/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb index ccafe3bb7a..921037bd5d 100644 --- a/spec/services/todos/destroy/entity_leave_service_spec.rb +++ b/spec/services/todos/destroy/entity_leave_service_spec.rb @@ -3,78 +3,146 @@ require 'spec_helper' RSpec.describe Todos::Destroy::EntityLeaveService do - let(:group) { create(:group, :private) } - let(:project) { create(:project, group: group) } - let(:user) { create(:user) } - let(:user2) { create(:user) } - let(:issue) { create(:issue, project: project, confidential: true) } - let(:mr) { create(:merge_request, source_project: project) } + let_it_be(:user, reload: true) { create(:user) } + let_it_be(:user2, reload: true) { create(:user) } - let!(:todo_mr_user) { create(:todo, user: user, target: mr, project: project) } - let!(:todo_issue_user) { create(:todo, user: user, target: issue, project: project) } - let!(:todo_group_user) { create(:todo, user: user, group: group) } - let!(:todo_issue_user2) { create(:todo, user: user2, target: issue, project: project) } - let!(:todo_group_user2) { create(:todo, user: user2, group: group) } + let(:group) { create(:group, :private) } + let(:project) { create(:project, :private, group: group) } + let(:issue) { create(:issue, project: project) } + let(:issue_c) { create(:issue, project: project, confidential: true) } + let!(:todo_group_user) { create(:todo, user: user, group: group) } + let!(:todo_group_user2) { create(:todo, user: user2, group: group) } + + let(:mr) { create(:merge_request, source_project: project) } + let!(:todo_mr_user) { create(:todo, user: user, target: mr, project: project) } + let!(:todo_issue_user) { create(:todo, user: user, target: issue, project: project) } + let!(:todo_issue_c_user) { create(:todo, user: user, target: issue_c, project: project) } + let!(:todo_issue_c_user2) { create(:todo, user: user2, target: issue_c, project: project) } + + shared_examples 'using different access permissions' do |access_table| + using RSpec::Parameterized::TableSyntax + + where(:group_access, :project_access, :c_todos, :mr_todos, :method, &access_table) + + with_them do + before do + set_access(project, user, project_access) if project_access + set_access(group, user, group_access) if group_access + end + + it "#{params[:method].to_s.humanize(capitalize: false)}" do + send(method) + end + end + end + + shared_examples 'does not remove any todos' do + it { does_not_remove_any_todos } + end + + shared_examples 'removes only confidential issues todos' do + it { removes_only_confidential_issues_todos } + end + + def does_not_remove_any_todos + expect { subject }.not_to change { Todo.count } + end + + def removes_only_confidential_issues_todos + expect { subject }.to change { Todo.count }.from(6).to(5) + end + + def removes_confidential_issues_and_merge_request_todos + expect { subject }.to change { Todo.count }.from(6).to(4) + expect(user.todos).to match_array([todo_issue_user, todo_group_user]) + end + + def set_access(object, user, access_name) + case access_name + when :developer + object.add_developer(user) + when :reporter + object.add_reporter(user) + when :guest + object.add_guest(user) + end + end describe '#execute' do - context 'when a user leaves a project' do + describe 'updating a Project' do subject { described_class.new(user.id, project.id, 'Project').execute } + # a private project in a private group is valid context 'when project is private' do - it 'removes project todos for the provided user' do - expect { subject }.to change { Todo.count }.from(5).to(3) + context 'when user is not a member of the project' do + it 'removes project todos for the provided user' do + expect { subject }.to change { Todo.count }.from(6).to(3) - expect(user.todos).to match_array([todo_group_user]) - expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2]) - end - - context 'when the user is member of the project' do - before do - project.add_developer(user) - end - - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } + expect(user.todos).to match_array([todo_group_user]) + expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) end end - context 'when the user is a project guest' do - before do - project.add_guest(user) - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + PRIVATE_PROJECT_PRIVATE_GROUP_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end - end - - context 'when the user is member of a parent group' do - before do - group.add_developer(user) - end - - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end - end - - context 'when the user is guest of a parent group' do - before do - project.add_guest(user) - end - - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end + it_behaves_like 'using different access permissions', PRIVATE_PROJECT_PRIVATE_GROUP_ACCESS_TABLE end end - context 'when project is not private' do - before do - group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + # a private project in an internal/public group is valid + context 'when project is private in an internal/public group' do + let(:group) { create(:group, :internal) } + + context 'when user is not a member of the project' do + it 'removes project todos for the provided user' do + expect { subject }.to change { Todo.count }.from(6).to(3) + + expect(user.todos).to match_array([todo_group_user]) + expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) + end end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + PRIVATE_PROJECT_INTERNAL_GROUP_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration + + it_behaves_like 'using different access permissions', PRIVATE_PROJECT_INTERNAL_GROUP_ACCESS_TABLE + end + end + + # an internal project in an internal/public group is valid + context 'when project is not private' do + let(:group) { create(:group, :internal) } + let(:project) { create(:project, :internal, group: group) } + let(:issue) { create(:issue, project: project) } + let(:issue_c) { create(:issue, project: project, confidential: true) } + it 'enqueues the PrivateFeaturesWorker' do expect(TodosDestroyer::PrivateFeaturesWorker) .to receive(:perform_async).with(project.id, user.id) @@ -84,50 +152,42 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'confidential issues' do context 'when a user is not an author of confidential issue' do - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end + it_behaves_like 'removes only confidential issues todos' end context 'when a user is an author of confidential issue' do before do - issue.update!(author: user) + issue_c.update!(author: user) end - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'does not remove any todos' end context 'when a user is an assignee of confidential issue' do before do - issue.assignees << user + issue_c.assignees << user end - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'does not remove any todos' end - context 'when a user is a project guest' do - before do - project.add_guest(user) - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + INTERNAL_PROJECT_INTERNAL_GROUP_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :keep, :removes_only_confidential_issues_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :keep, :removes_only_confidential_issues_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :keep, :removes_only_confidential_issues_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end - end - - context 'when a user is a project guest but group developer' do - before do - project.add_guest(user) - group.add_developer(user) - end - - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'using different access permissions', INTERNAL_PROJECT_INTERNAL_GROUP_ACCESS_TABLE end end @@ -138,42 +198,43 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'removes only users issue todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) + expect { subject }.to change { Todo.count }.from(6).to(5) end end end end end - context 'when a user leaves a group' do + describe 'updating a Group' do subject { described_class.new(user.id, group.id, 'Group').execute } context 'when group is private' do - it 'removes group and subproject todos for the user' do - expect { subject }.to change { Todo.count }.from(5).to(2) + context 'when a user leaves a group' do + it 'removes group and subproject todos for the user' do + expect { subject }.to change { Todo.count }.from(6).to(2) - expect(user.todos).to be_empty - expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2]) - end - - context 'when the user is member of the group' do - before do - group.add_developer(user) - end - - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } + expect(user.todos).to be_empty + expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2]) end end - context 'when the user is member of the group project but not the group' do - before do - project.add_developer(user) - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + PRIVATE_GROUP_PRIVATE_PROJECT_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :delete, :removes_confidential_issues_and_merge_request_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :delete, :removes_confidential_issues_and_merge_request_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'using different access permissions', PRIVATE_GROUP_PRIVATE_PROJECT_ACCESS_TABLE end context 'with nested groups' do @@ -191,12 +252,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do context 'when the user is not a member of any groups/projects' do it 'removes todos for the user including subprojects todos' do - expect { subject }.to change { Todo.count }.from(11).to(4) + expect { subject }.to change { Todo.count }.from(12).to(4) expect(user.todos).to be_empty expect(user2.todos) .to match_array( - [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] ) end end @@ -208,9 +269,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do parent_group.add_developer(user) end - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'does not remove any todos' end context 'when the user is member of a subgroup' do @@ -219,12 +278,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'does not remove group and subproject todos' do - expect { subject }.to change { Todo.count }.from(11).to(7) + expect { subject }.to change { Todo.count }.from(12).to(7) expect(user.todos).to match_array([todo_group_user, todo_subgroup_user, todo_subproject_user]) expect(user2.todos) .to match_array( - [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] ) end end @@ -235,12 +294,12 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end it 'does not remove subproject and group todos' do - expect { subject }.to change { Todo.count }.from(11).to(7) + expect { subject }.to change { Todo.count }.from(12).to(7) expect(user.todos).to match_array([todo_subgroup_user, todo_group_user, todo_subproject_user]) expect(user2.todos) .to match_array( - [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] + [todo_issue_c_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2] ) end end @@ -248,10 +307,10 @@ RSpec.describe Todos::Destroy::EntityLeaveService do end context 'when group is not private' do - before do - group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) - end + let(:group) { create(:group, :internal) } + let(:project) { create(:project, :internal, group: group) } + let(:issue) { create(:issue, project: project) } + let(:issue_c) { create(:issue, project: project, confidential: true) } it 'enqueues the PrivateFeaturesWorker' do expect(TodosDestroyer::PrivateFeaturesWorker) @@ -260,31 +319,24 @@ RSpec.describe Todos::Destroy::EntityLeaveService do subject end - context 'when user is not member' do - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end - end + context 'access permissions' do + # rubocop:disable RSpec/LeakyConstantDeclaration + INTERNAL_GROUP_INTERNAL_PROJECT_ACCESS_TABLE = + lambda do |_| + [ + # :group_access, :project_access, :c_todos, :mr_todos, :method + [nil, nil, :delete, :keep, :removes_only_confidential_issues_todos], + [nil, :reporter, :keep, :keep, :does_not_remove_any_todos], + [nil, :guest, :delete, :keep, :removes_only_confidential_issues_todos], + [:reporter, nil, :keep, :keep, :does_not_remove_any_todos], + [:guest, nil, :delete, :keep, :removes_only_confidential_issues_todos], + [:guest, :reporter, :keep, :keep, :does_not_remove_any_todos], + [:guest, :guest, :delete, :keep, :removes_only_confidential_issues_todos] + ] + end + # rubocop:enable RSpec/LeakyConstantDeclaration - context 'when user is a project guest' do - before do - project.add_guest(user) - end - - it 'removes only confidential issues todos' do - expect { subject }.to change { Todo.count }.from(5).to(4) - end - end - - context 'when user is a project guest & group developer' do - before do - project.add_guest(user) - group.add_developer(user) - end - - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } - end + it_behaves_like 'using different access permissions', INTERNAL_GROUP_INTERNAL_PROJECT_ACCESS_TABLE end end end diff --git a/spec/support/shared_examples/cached_response_shared_examples.rb b/spec/support/shared_examples/cached_response_shared_examples.rb new file mode 100644 index 0000000000..34e5f741b4 --- /dev/null +++ b/spec/support/shared_examples/cached_response_shared_examples.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true +# +# Negates lib/gitlab/no_cache_headers.rb +# + +RSpec.shared_examples 'cached response' do + it 'defines a cached header response' do + expect(response.headers["Cache-Control"]).not_to include("no-store", "no-cache") + expect(response.headers["Pragma"]).not_to eq("no-cache") + expect(response.headers["Expires"]).not_to eq("Fri, 01 Jan 1990 00:00:00 GMT") + end +end diff --git a/spec/validators/future_date_validator_spec.rb b/spec/validators/future_date_validator_spec.rb new file mode 100644 index 0000000000..6814ba7c82 --- /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 cbdd5a6869..8a34b41834 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 0000000000..96d7cf535e --- /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