diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb index 16590e66d6142d21259b42eea671c75c89be985c..07760fb5c8f627a69246768c2bee09a98e45edcc 100644 --- a/app/controllers/admin/applications_controller.rb +++ b/app/controllers/admin/applications_controller.rb @@ -1,5 +1,6 @@ class Admin::ApplicationsController < Admin::ApplicationController include OauthApplications + prepend EE::Admin::ApplicationsController before_action :set_application, only: [:show, :edit, :update, :destroy] before_action :load_scopes, only: [:new, :create, :edit, :update] @@ -22,8 +23,7 @@ def create @application = Doorkeeper::Application.new(application_params) if @application.save - flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create]) - redirect_to admin_application_url(@application) + redirect_to_admin_page else render :new end @@ -42,6 +42,13 @@ def destroy redirect_to admin_applications_url, status: 302, notice: 'Application was successfully destroyed.' end + protected + + def redirect_to_admin_page + flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create]) + redirect_to admin_application_url(@application) + end + private def set_application diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 8e914c29a9f75917ce5670b729d8fb586c4ea619..36e33e7c690c35a39a51a4a15b894ee8dfde4de5 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -128,7 +128,7 @@ def update end respond_to do |format| - result = Users::UpdateService.new(user, user_params_with_pass).execute do |user| + result = Users::UpdateService.new(current_user, user_params_with_pass.merge(user: user)).execute do |user| user.skip_reconfirmation! end @@ -155,7 +155,7 @@ def destroy def remove_email email = user.emails.find(params[:email_id]) - success = Emails::DestroyService.new(user, email: email.email).execute + success = Emails::DestroyService.new(current_user, user: user, email: email.email).execute respond_to do |format| if success @@ -226,7 +226,7 @@ def user_params_ee end def update_user(&block) - result = Users::UpdateService.new(user).execute(&block) + result = Users::UpdateService.new(current_user, user: user).execute(&block) result[:status] == :success end diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb index 9849aa93fa61a29830c77d95ff368ced63edac7c..f7054da00190b6b3fc9ba130cc2c91365743c3b3 100644 --- a/app/controllers/concerns/oauth_applications.rb +++ b/app/controllers/concerns/oauth_applications.rb @@ -1,5 +1,6 @@ module OauthApplications extend ActiveSupport::Concern + prepend ::EE::Concerns::OauthApplications included do before_action :prepare_scopes, only: [:create, :update] diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index 306afb65f10df40ff56eabc2b802a66dd48bd740..c061d5da0425bed3f7fbe11a4605ab84054d0bb1 100644 --- a/app/controllers/confirmations_controller.rb +++ b/app/controllers/confirmations_controller.rb @@ -1,4 +1,6 @@ class ConfirmationsController < Devise::ConfirmationsController + prepend ::EE::ConfirmationsController + def almost_there flash[:notice] = nil render layout: "devise_empty" @@ -12,10 +14,14 @@ def after_resending_confirmation_instructions_path_for(resource) def after_confirmation_path_for(resource_name, resource) if signed_in?(resource_name) - after_sign_in_path_for(resource) + after_sign_in(resource) else flash[:notice] += " Please sign in." new_session_path(resource_name) end end + + def after_sign_in(resource) + after_sign_in_path_for(resource) + end end diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index 2ae4785b12cc1b43c1336240d0b5948bb75f4e96..e20804fcaa0ed29d287c7d2aca8f4501507a091b 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -3,6 +3,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController include Gitlab::GonHelper include PageLayoutHelper include OauthApplications + prepend ::EE::Oauth::ApplicationsController before_action :verify_user_oauth_applications_enabled before_action :authenticate_user! @@ -21,14 +22,20 @@ def create @application.owner = current_user if @application.save - flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create]) - redirect_to oauth_application_url(@application) + redirect_to_oauth_application_page else set_index_vars render :index end end + protected + + def redirect_to_oauth_application_page + flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create]) + redirect_to oauth_application_url(@application) + end + private def verify_user_oauth_applications_enabled diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index fda944adecdf589abdbcbafdcca9b2272598a5d4..8185014e11392711136aff12e705e6cb27e2f39f 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -3,6 +3,8 @@ class PasswordsController < Devise::PasswordsController before_action :prevent_ldap_reset, only: [:create] before_action :throttle_reset, only: [:create] + prepend EE::PasswordsController + def edit super reset_password_token = Devise.token_generator.digest( diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb index 408650aac548c0324dc9004f0edfd7abb0e49f36..39b9f8a84d1fb5fd4375f5a1d77413f6652d24b0 100644 --- a/app/controllers/profiles/avatars_controller.rb +++ b/app/controllers/profiles/avatars_controller.rb @@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController def destroy @user = current_user - Users::UpdateService.new(@user).execute { |user| user.remove_avatar! } + Users::UpdateService.new(current_user, user: @user).execute { |user| user.remove_avatar! } redirect_to profile_path, status: 302 end diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index ddb67d1c4d11f79aafe4705dd4eaa94f6e5ecdc8..97db84b92d42dfe69621549763e7065d6e7a4961 100644 --- a/app/controllers/profiles/emails_controller.rb +++ b/app/controllers/profiles/emails_controller.rb @@ -5,7 +5,7 @@ def index end def create - @email = Emails::CreateService.new(current_user, email_params).execute + @email = Emails::CreateService.new(current_user, email_params.merge(user: current_user)).execute if @email.errors.blank? NotificationService.new.new_email(@email) @@ -19,7 +19,7 @@ def create def destroy @email = current_user.emails.find(params[:id]) - Emails::DestroyService.new(current_user, email: @email.email).execute + Emails::DestroyService.new(current_user, user: current_user, email: @email.email).execute respond_to do |format| format.html { redirect_to profile_emails_url, status: 302 } diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb index 4393c65261770222d57784033adb3afff7ad0a80..76a128a1215f43b1f1f53a6070bd1a39924ca79e 100644 --- a/app/controllers/profiles/keys_controller.rb +++ b/app/controllers/profiles/keys_controller.rb @@ -1,4 +1,6 @@ class Profiles::KeysController < Profiles::ApplicationController + prepend ::EE::Profiles::KeysController + skip_before_action :authenticate_user!, only: [:get_keys] def index @@ -14,7 +16,7 @@ def create @key = Keys::CreateService.new(current_user, key_params).execute if @key.persisted? - redirect_to profile_key_path(@key) + redirect_to_profile_key_path else @keys = current_user.keys.select(&:persisted?) render :index @@ -50,6 +52,12 @@ def get_keys end end + protected + + def redirect_to_profile_key_path + redirect_to profile_key_path(@key) + end + private def key_params diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 960b75126023c4361ade50ac7853ae07dc427292..8a38ba65d4ccc09684946338e6baef7e95363b62 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -7,7 +7,7 @@ def show end def update - result = Users::UpdateService.new(current_user, user_params).execute + result = Users::UpdateService.new(current_user, user_params.merge(user: current_user)).execute if result[:status] == :success flash[:notice] = "Notification settings saved" diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index 7beb52dd8e86b84075b203c662d2d89ddba176cb..dcfcb855ab55a89b56f4d98878f575bbeb8bc355 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -21,10 +21,10 @@ def create password_automatically_set: false } - result = Users::UpdateService.new(@user, password_attributes).execute + result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute if result[:status] == :success - Users::UpdateService.new(@user, password_expires_at: nil).execute + Users::UpdateService.new(current_user, user: @user, password_expires_at: nil).execute redirect_to root_path, notice: 'Password successfully changed' else @@ -46,7 +46,7 @@ def update return end - result = Users::UpdateService.new(@user, password_attributes).execute + result = Users::UpdateService.new(current_user, password_attributes.merge(user: @user)).execute if result[:status] == :success flash[:notice] = "Password was successfully updated. Please login with it" diff --git a/app/controllers/profiles/preferences_controller.rb b/app/controllers/profiles/preferences_controller.rb index cce2a847b53890385e4be6a99cabffe447c6fc64..ed0f98179ebbb4dae8288f62cdb0af4785e70186 100644 --- a/app/controllers/profiles/preferences_controller.rb +++ b/app/controllers/profiles/preferences_controller.rb @@ -6,7 +6,7 @@ def show def update begin - result = Users::UpdateService.new(user, preferences_params).execute + result = Users::UpdateService.new(current_user, preferences_params.merge(user: user)).execute if result[:status] == :success flash[:notice] = 'Preferences saved.' diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index 1a4f77639e7293d89559d1bfc6ec4827a3051a3d..aa9789f8a0f3ca88133f2fd34f3fa1ecbb8de210 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -10,7 +10,7 @@ def show current_user.otp_grace_period_started_at = Time.current end - Users::UpdateService.new(current_user).execute! + Users::UpdateService.new(current_user, user: current_user).execute! if two_factor_authentication_required? && !current_user.two_factor_enabled? two_factor_authentication_reason( @@ -41,7 +41,7 @@ def show def create if current_user.validate_and_consume_otp!(params[:pin_code]) - Users::UpdateService.new(current_user, otp_required_for_login: true).execute! do |user| + Users::UpdateService.new(current_user, user: current_user, otp_required_for_login: true).execute! do |user| @codes = user.generate_otp_backup_codes! end @@ -70,7 +70,7 @@ def create_u2f end def codes - Users::UpdateService.new(current_user).execute! do |user| + Users::UpdateService.new(current_user, user: current_user).execute! do |user| @codes = user.generate_otp_backup_codes! end end diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index d83824fef06e35500e6b1e8c91d5c879add8d267..5d87037f01232dc17a39a33677af7c3e9c24e92e 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -10,7 +10,7 @@ def show def update respond_to do |format| - result = Users::UpdateService.new(@user, user_params).execute + result = Users::UpdateService.new(current_user, user_params.merge(user: @user)).execute if result[:status] == :success message = "Profile was successfully updated" @@ -25,7 +25,7 @@ def update end def reset_private_token - Users::UpdateService.new(@user).execute! do |user| + Users::UpdateService.new(current_user, user: @user).execute! do |user| user.reset_authentication_token! end @@ -35,7 +35,7 @@ def reset_private_token end def reset_incoming_email_token - Users::UpdateService.new(@user).execute! do |user| + Users::UpdateService.new(current_user, user: @user).execute! do |user| user.reset_incoming_email_token! end @@ -45,7 +45,7 @@ def reset_incoming_email_token end def reset_rss_token - Users::UpdateService.new(@user).execute! do |user| + Users::UpdateService.new(current_user, user: @user).execute! do |user| user.reset_rss_token! end @@ -61,7 +61,7 @@ def audit_log end def update_username - result = Users::UpdateService.new(@user, username: user_params[:username]).execute + result = Users::UpdateService.new(current_user, user: @user, username: user_params[:username]).execute options = if result[:status] == :success { notice: "Username successfully changed" } diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 89c05aa35dc1f652305fcdf5694d8490b6db9812..8267644b565220bb1e2d8b2557032d927b08fe22 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -57,7 +57,7 @@ def check_initial_setup return unless user && user.require_password_creation? - Users::UpdateService.new(user).execute do |user| + Users::UpdateService.new(current_user, user: user).execute do |user| @token = user.generate_reset_token end diff --git a/app/models/license.rb b/app/models/license.rb index 1b9fec0a33fabb328addd8ca4352950238c67de2..e7ad62ba6a00af4f1e7d48ebea4b4b91359b723d 100644 --- a/app/models/license.rb +++ b/app/models/license.rb @@ -38,6 +38,7 @@ class License < ActiveRecord::Base cross_project_pipelines db_load_balancing deploy_board + extended_audit_events file_locks geo group_issue_boards @@ -107,6 +108,7 @@ class License < ActiveRecord::Base auditor_user db_load_balancing elastic_search + extended_audit_events geo ldap_extras object_storage diff --git a/app/models/user.rb b/app/models/user.rb index 4d1e98fd29c1661bb11d6f8b79da0dbebf5bd0d8..21a43c5be9ec96a1e7aa230087fbadb3b6a511cc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -63,7 +63,7 @@ def update_tracked_fields!(request) lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i) return unless lease.try_obtain - Users::UpdateService.new(self).execute(validate: false) + Users::UpdateService.new(self, user: self).execute(validate: false) end attr_accessor :force_random_password @@ -545,8 +545,8 @@ def owns_public_email def update_emails_with_primary_email primary_email_record = emails.find_by(email: email) if primary_email_record - Emails::DestroyService.new(self, email: email).execute - Emails::CreateService.new(self, email: email_was).execute + Emails::DestroyService.new(self, user: self, email: email).execute + Emails::CreateService.new(self, user: self, email: email_was).execute end end @@ -1023,7 +1023,7 @@ def increment_failed_attempts! if attempts_exceeded? lock_access! unless access_locked? else - Users::UpdateService.new(self).execute(validate: false) + Users::UpdateService.new(self, user: self).execute(validate: false) end end @@ -1209,7 +1209,7 @@ def self.create_unique_internal(scope, username, email_pattern, &creation_block) &creation_block ) - Users::UpdateService.new(user).execute(validate: false) + Users::UpdateService.new(user, user: user).execute(validate: false) user ensure Gitlab::ExclusiveLease.cancel(lease_key, uuid) diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb index ace498890978f5b23eee1efe37afc0391242ea55..7f591c89411d1fe4d6efb349cc7878f6c280e6d2 100644 --- a/app/services/emails/base_service.rb +++ b/app/services/emails/base_service.rb @@ -1,7 +1,8 @@ module Emails class BaseService - def initialize(user, opts) - @user = user + def initialize(current_user, opts) + @current_user = current_user + @user = opts.delete(:user) @email = opts[:email] end end diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb index b6491ee98049f41dc5638f536acfd1588ecd6989..e1ceb68bfe8aa5591171036cbf8c8cf13cf6f6a3 100644 --- a/app/services/emails/create_service.rb +++ b/app/services/emails/create_service.rb @@ -1,5 +1,7 @@ module Emails class CreateService < ::Emails::BaseService + prepend ::EE::Emails::CreateService + def execute @user.emails.create(email: @email) end diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb index d586b9dfe0c89efdf577882b4a93c1eda3d0aeeb..88055c25ae43035f77e3174cf9f12192f663a16a 100644 --- a/app/services/emails/destroy_service.rb +++ b/app/services/emails/destroy_service.rb @@ -1,13 +1,15 @@ module Emails class DestroyService < ::Emails::BaseService + prepend ::EE::Emails::DestroyService + def execute - Email.find_by_email!(@email).destroy && update_secondary_emails! + update_secondary_emails! if Email.find_by_email!(@email).destroy end private def update_secondary_emails! - result = ::Users::UpdateService.new(@user).execute do |user| + result = ::Users::UpdateService.new(@current_user, user: @user).execute do |user| user.update_secondary_emails! end diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index 6188b8a43493a30ced5e3241a0f422509a3f7e64..62cceb0cb7e6595a9f31275426452330b0b18059 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -1,9 +1,11 @@ module Users class UpdateService < BaseService include NewUserNotifier + prepend EE::Users::UpdateService - def initialize(user, params = {}) - @user = user + def initialize(current_user, params = {}) + @current_user = current_user + @user = params.delete(:user) @params = params.dup end @@ -15,9 +17,7 @@ def execute(validate: true, &block) user_exists = @user.persisted? if @user.save(validate: validate) - notify_new_user(@user, nil) unless user_exists - - success + notify_success(user_exists) else error(@user.errors.full_messages.uniq.join('. ')) end @@ -31,6 +31,14 @@ def execute!(*args, &block) true end + protected + + def notify_success(user_exists) + notify_new_user(@user, nil) unless user_exists + + success + end + private def assign_attributes(&block) diff --git a/ee/app/controllers/ee/admin/applications_controller.rb b/ee/app/controllers/ee/admin/applications_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..6420a156ddd49fcb6d9ead4ba4b1ab437eb0a9bb --- /dev/null +++ b/ee/app/controllers/ee/admin/applications_controller.rb @@ -0,0 +1,15 @@ +module EE + module Admin + module ApplicationsController + protected + + def redirect_to_admin_page + raise NotImplementedError unless defined?(super) + + log_audit_event + + super + end + end + end +end diff --git a/ee/app/controllers/ee/concerns/oauth_applications.rb b/ee/app/controllers/ee/concerns/oauth_applications.rb new file mode 100644 index 0000000000000000000000000000000000000000..bcdd03d5f4cfca754e5fef1b7255aaa1273ddd23 --- /dev/null +++ b/ee/app/controllers/ee/concerns/oauth_applications.rb @@ -0,0 +1,16 @@ +module EE + module Concerns + module OauthApplications + extend ActiveSupport::Concern + + def log_audit_event + ::AuditEventService.new(current_user, + current_user, + action: :custom, + custom_message: 'OAuth access granted', + ip_address: request.remote_ip) + .for_user(@application.name).security_event + end + end + end +end diff --git a/ee/app/controllers/ee/confirmations_controller.rb b/ee/app/controllers/ee/confirmations_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..49562ea4a83329bd48b87893cc4936dba63d2141 --- /dev/null +++ b/ee/app/controllers/ee/confirmations_controller.rb @@ -0,0 +1,15 @@ +module EE + module ConfirmationsController + include EE::Audit::Changes + + protected + + def after_sign_in(resource) + raise NotImplementedError unless defined?(super) + + audit_changes(:email, as: 'email address', model: resource) + + super(resource) + end + end +end diff --git a/ee/app/controllers/ee/oauth/applications_controller.rb b/ee/app/controllers/ee/oauth/applications_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..385e41cb6c9a3828a27ce40911d00a03259278f6 --- /dev/null +++ b/ee/app/controllers/ee/oauth/applications_controller.rb @@ -0,0 +1,15 @@ +module EE + module Oauth + module ApplicationsController + protected + + def redirect_to_oauth_application_page + raise NotImplementedError unless defined?(super) + + log_audit_event + + super + end + end + end +end diff --git a/ee/app/controllers/ee/passwords_controller.rb b/ee/app/controllers/ee/passwords_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..2c9921c03b2848a5a822a0c19236189c0ca58bbe --- /dev/null +++ b/ee/app/controllers/ee/passwords_controller.rb @@ -0,0 +1,20 @@ +module EE + module PasswordsController + extend ActiveSupport::Concern + + prepended do + before_action :log_audit_event, only: [:create] + end + + private + + def log_audit_event + ::AuditEventService.new(current_user, + resource, + action: :custom, + custom_message: 'Ask for password reset', + ip_address: request.remote_ip) + .for_user(resource_params[:email]).unauth_security_event + end + end +end diff --git a/ee/app/controllers/ee/profiles/keys_controller.rb b/ee/app/controllers/ee/profiles/keys_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..ebcd96edb404ad8664810b2849bfd6378e5cd9b2 --- /dev/null +++ b/ee/app/controllers/ee/profiles/keys_controller.rb @@ -0,0 +1,26 @@ +module EE + module Profiles + module KeysController + protected + + def redirect_to_profile_key_path + raise NotImplementedError unless defined?(super) + + log_audit_event + + super + end + + private + + def log_audit_event + ::AuditEventService.new(current_user, + current_user, + action: :custom, + custom_message: 'Added SSH key', + ip_address: request.remote_ip) + .for_user(@key.title).security_event + end + end + end +end diff --git a/ee/app/services/ee/audit_event_service.rb b/ee/app/services/ee/audit_event_service.rb index 705381230a98238b32e3ce7f6d551b182d0e262e..0a25547dee1fbd75336fed8ac0bd2851a8fa3d58 100644 --- a/ee/app/services/ee/audit_event_service.rb +++ b/ee/app/services/ee/audit_event_service.rb @@ -41,33 +41,6 @@ def for_member(member) self end - def for_deploy_key(key_title) - action = @details[:action] - author_name = @author.name - - @details = - case action - when :destroy - { - remove: "deploy_key", - author_name: author_name, - target_id: key_title, - target_type: "DeployKey", - target_details: key_title - } - when :create - { - add: "deploy_key", - author_name: author_name, - target_id: key_title, - target_type: "DeployKey", - target_details: key_title - } - end - - self - end - def for_failed_login ip = @details[:ip_address] auth = @details[:with] || 'STANDARD' @@ -82,6 +55,20 @@ def for_failed_login self end + def for_changes + @details = + { + change: @details[:as] || @details[:column], + from: @details[:from], + to: @details[:to], + author_name: @author.name, + target_id: @entity.id, + target_type: @entity.class, + target_details: @entity.name + } + self + end + def security_event if admin_audit_log_enabled? add_security_event_admin_details! @@ -89,35 +76,92 @@ def security_event return super end - super if audit_events_enabled? + super if audit_events_enabled? || entity_audit_events_enabled? end - def add_security_event_admin_details! - @details.merge!(ip_address: @author.current_sign_in_ip, - entity_path: @entity.full_path) + def unauth_security_event + return unless audit_events_enabled? + + @details.delete(:ip_address) unless admin_audit_log_enabled? + @details[:entity_path] = @entity&.full_path if admin_audit_log_enabled? + + SecurityEvent.create( + author_id: @author.respond_to?(:id) ? @author.id : -1, + entity_id: @entity.respond_to?(:id) ? @entity.id : -1, + entity_type: 'User', + details: @details + ) end - def audit_events_enabled? - return true unless @entity.respond_to?(:feature_available?) + def entity_audit_events_enabled? + @entity.respond_to?(:feature_available?) && @entity.feature_available?(:audit_events) + end - @entity.feature_available?(:audit_events) + def audit_events_enabled? + # Always log auth events. Log all other events if `extended_audit_events` is enabled + @details[:with] || License.feature_available?(:extended_audit_events) end def admin_audit_log_enabled? License.feature_available?(:admin_audit_log) end - def unauth_security_event - return unless audit_events_enabled? + def method_missing(method_sym, *arguments, &block) + super(method_sym, *arguments, &block) unless respond_to?(method_sym) - @details.delete(:ip_address) unless admin_audit_log_enabled? + for_custom_model(method_sym.to_s.split('for_').last, *arguments) + end - SecurityEvent.create( - author_id: -1, - entity_id: -1, - entity_type: 'User', - details: @details - ) + def respond_to?(method, include_private = false) + method.to_s.start_with?('for_') || super + end + + private + + def for_custom_model(model, key_title) + action = @details[:action] + model_class = model.camelize + custom_message = @details[:custom_message] + + @details = + case action + when :destroy + { + remove: model, + author_name: @author.name, + target_id: key_title, + target_type: model_class, + target_details: key_title + } + when :create + { + add: model, + author_name: @author.name, + target_id: key_title, + target_type: model_class, + target_details: key_title + } + when :custom + { + custom_message: custom_message, + author_name: @author&.name, + target_id: key_title, + target_type: model_class, + target_details: key_title, + ip_address: @details[:ip_address] + } + end + + self + end + + def ip_address + @author&.current_sign_in_ip || @details[:ip_address] + end + + def add_security_event_admin_details! + @details.merge!(ip_address: ip_address, + entity_path: @entity.full_path) end end end diff --git a/ee/app/services/ee/emails/base_service.rb b/ee/app/services/ee/emails/base_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..69cdfb0825ab775029730f8249105706c34d1e21 --- /dev/null +++ b/ee/app/services/ee/emails/base_service.rb @@ -0,0 +1,12 @@ +module EE + module Emails + module BaseService + private + + def log_audit_event(options = {}) + ::AuditEventService.new(@current_user, @user, options) + .for_email(@email).security_event + end + end + end +end diff --git a/ee/app/services/ee/emails/create_service.rb b/ee/app/services/ee/emails/create_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..c5ce8be46a39ebeda3ad0799ae3d3bb998fab6d9 --- /dev/null +++ b/ee/app/services/ee/emails/create_service.rb @@ -0,0 +1,13 @@ +module EE + module Emails + module CreateService + include ::EE::Emails::BaseService + + def execute + super.tap do |email| + log_audit_event(action: :create) if email.persisted? + end + end + end + end +end diff --git a/ee/app/services/ee/emails/destroy_service.rb b/ee/app/services/ee/emails/destroy_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..eb55b7f73d28e5a7bf764111cb7a9c9ec4246853 --- /dev/null +++ b/ee/app/services/ee/emails/destroy_service.rb @@ -0,0 +1,13 @@ +module EE + module Emails + module DestroyService + include ::EE::Emails::BaseService + + def execute + super.tap do + log_audit_event(action: :destroy) + end + end + end + end +end diff --git a/ee/app/services/ee/users/update_service.rb b/ee/app/services/ee/users/update_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..6cf2e401dfb92be745d7e91b7e901afd48daaaed --- /dev/null +++ b/ee/app/services/ee/users/update_service.rb @@ -0,0 +1,22 @@ +module EE + module Users + module UpdateService + include EE::Audit::Changes + + private + + def notify_success(user_exists) + notify_new_user(@user, nil) unless user_exists + + audit_changes(:email, as: 'email address') + audit_changes(:encrypted_password, as: 'password', skip_changes: true) + + success + end + + def model + @user + end + end + end +end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index d30544a7a145ae8e885e03ccc1f332251b429adb..8276ca48a663e0a1fd03e213465b3d39cf3b6513 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -149,7 +149,7 @@ class Internal < Grape::API codes = nil - ::Users::UpdateService.new(user).execute! do |user| + ::Users::UpdateService.new(current_user, user: user).execute! do |user| codes = user.generate_otp_backup_codes! end diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb index bcc0833aa5c0ba60e20c25ac9d18b9e2a2438204..0266bf2f71715efbdd6b98b9eaa57ac7a7d6cfc0 100644 --- a/lib/api/notification_settings.rb +++ b/lib/api/notification_settings.rb @@ -35,7 +35,7 @@ class NotificationSettings < Grape::API new_notification_email = params.delete(:notification_email) if new_notification_email - ::Users::UpdateService.new(current_user, notification_email: new_notification_email).execute + ::Users::UpdateService.new(current_user, user: current_user, notification_email: new_notification_email).execute end notification_setting.update(declared_params(include_missing: false)) diff --git a/lib/api/users.rb b/lib/api/users.rb index 49b7d9139b968ff28f4c60db3b347b449bb07aa9..dce47f3a854d2e3bc27bcea20181392f033860f2 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -172,7 +172,7 @@ def find_user(params) user_params[:password_expires_at] = Time.now if user_params[:password].present? - result = ::Users::UpdateService.new(user, user_params.except(:extern_uid, :provider)).execute + result = ::Users::UpdateService.new(current_user, user_params.except(:extern_uid, :provider).merge(user: user)).execute if result[:status] == :success present user, with: Entities::UserPublic @@ -332,7 +332,7 @@ def find_user(params) user = User.find_by(id: params.delete(:id)) not_found!('User') unless user - email = Emails::CreateService.new(user, declared_params(include_missing: false)).execute + email = Emails::CreateService.new(current_user, declared_params(include_missing: false).merge(user: user)).execute if email.errors.blank? NotificationService.new.new_email(email) @@ -373,7 +373,7 @@ def find_user(params) not_found!('Email') unless email destroy_conditionally!(email) do |email| - Emails::DestroyService.new(current_user, email: email.email).execute + Emails::DestroyService.new(current_user, user: user, email: email.email).execute end user.update_secondary_emails! @@ -678,7 +678,7 @@ def find_impersonation_token requires :email, type: String, desc: 'The new email' end post "emails" do - email = Emails::CreateService.new(current_user, declared_params).execute + email = Emails::CreateService.new(current_user, declared_params.merge(user: current_user)).execute if email.errors.blank? NotificationService.new.new_email(email) @@ -697,7 +697,7 @@ def find_impersonation_token not_found!('Email') unless email destroy_conditionally!(email) do |email| - Emails::DestroyService.new(current_user, email: email.email).execute + Emails::DestroyService.new(current_user, user: current_user, email: email.email).execute end current_user.update_secondary_emails! diff --git a/lib/audit/details.rb b/lib/audit/details.rb index 8fefdbc9418d5a9dfcc277db9e67fd464c18ecb4..42ead21b00ea9e8322ba12f78548ea167ed4a749 100644 --- a/lib/audit/details.rb +++ b/lib/audit/details.rb @@ -1,6 +1,6 @@ module Audit class Details - ACTIONS = %i[add remove failed_login change].freeze + ACTIONS = %i[add remove failed_login change custom_message].freeze def self.humanize(*args) new(*args).humanize @@ -31,8 +31,17 @@ def action_text "Removed #{value}" when :failed_login "Failed to login with #{Gitlab::OAuth::Provider.label_for(value).upcase} authentication" + when :custom_message + value else - "Changed #{value} from #{@details[:from]} to #{@details[:to]}" + text_for_change(value) + end + end + + def text_for_change(value) + "Changed #{value}".tap do |changed_string| + changed_string << " from #{@details[:from]}" if @details[:from] + changed_string << " to #{@details[:to]}" if @details[:to] end end end diff --git a/lib/ee/audit/changes.rb b/lib/ee/audit/changes.rb new file mode 100644 index 0000000000000000000000000000000000000000..888876e614b513e037bf438a86accaa60d55ebd5 --- /dev/null +++ b/lib/ee/audit/changes.rb @@ -0,0 +1,47 @@ +module EE + module Audit + module Changes + def audit_changes(column, options = {}) + column = options[:column] || column + @model = options[:model] + + return unless changed?(column) + + audit_event(parse_options(column, options)) + end + + protected + + def model + @model + end + + private + + def changed?(column) + model.previous_changes.has_key?(column) + end + + def changes(column) + model.previous_changes[column] + end + + def parse_options(column, options) + options.tap do |options_hash| + options_hash[:column] = column + options_hash[:action] = :update + + unless options[:skip_changes] + options_hash[:from] = changes(column).first + options_hash[:to] = changes(column).last + end + end + end + + def audit_event(options) + ::AuditEventService.new(@current_user, model, options) + .for_changes.security_event + end + end + end +end diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 58264af840a26aabfe1fbf60e253f1907bcb8400..95d39fce52f08d66f3070988ae36a0e0f84b1f8d 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -20,7 +20,7 @@ def self.allowed?(user, options = {}) # permissions to keep things clean if access.allowed? access.update_user - Users::UpdateService.new(user, last_credential_check_at: Time.now).execute + Users::UpdateService.new(user, user: user, last_credential_check_at: Time.now).execute true else @@ -172,8 +172,9 @@ def update_email return false if user.email == ldap_email - user.skip_reconfirmation! - user.update(email: ldap_email) + Users::UpdateService.new(user, user: user, email: ldap_email).execute do |user| + user.skip_reconfirmation! + end end def update_identity diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 04ff854e26f6fa2e4d6ea57aaa9e13337506b72c..619241502a2eea5bf86f3c3479d1c9f1a762db98 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -34,7 +34,7 @@ def save(provider = 'OAuth') block_after_save = needs_blocking? - Users::UpdateService.new(gl_user).execute! + Users::UpdateService.new(gl_user, user: gl_user).execute! gl_user.block if block_after_save diff --git a/spec/controllers/admin/applications_controller_spec.rb b/spec/controllers/admin/applications_controller_spec.rb index 7bd6c0e6117ae58caa52bf4fa9c38419cf219c76..c7441bb7904da8afd35d049bca7c74b93a3246ad 100644 --- a/spec/controllers/admin/applications_controller_spec.rb +++ b/spec/controllers/admin/applications_controller_spec.rb @@ -28,6 +28,8 @@ describe 'POST #create' do it 'creates the application' do + stub_licensed_features(extended_audit_events: true) + create_params = attributes_for(:application, trusted: true) expect do @@ -38,6 +40,7 @@ expect(response).to redirect_to(admin_application_path(application)) expect(application).to have_attributes(create_params.except(:uid, :owner_type)) + expect(SecurityEvent.count).to eq(1) end it 'renders the application form on errors' do diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb index 552899eb36ce4f360455556fb21bbd94b4fc16c4..ba35dc360eb26a3c403af515b2e419feebaef3a8 100644 --- a/spec/controllers/oauth/applications_controller_spec.rb +++ b/spec/controllers/oauth/applications_controller_spec.rb @@ -25,5 +25,18 @@ expect(response).to redirect_to(profile_path) end end + + describe 'POST #create' do + it 'logs the audit event' do + stub_licensed_features(extended_audit_events: true) + + sign_in(user) + + application = build(:oauth_application) + application_attributes = application.attributes.merge(scopes: []) + + expect { post :create, doorkeeper_application: application_attributes }.to change { SecurityEvent.count }.by(1) + end + end end end diff --git a/spec/controllers/profiles/keys_controller_spec.rb b/spec/controllers/profiles/keys_controller_spec.rb index 363ed410bc03c949bdb2727675817e8a29394ead..aa8dbb66ccb0acd174a925e2d3e17742f74efc4c 100644 --- a/spec/controllers/profiles/keys_controller_spec.rb +++ b/spec/controllers/profiles/keys_controller_spec.rb @@ -66,4 +66,16 @@ end end end + + describe '#create' do + it 'logs the audit event' do + stub_licensed_features(extended_audit_events: true) + + sign_in(user) + + key = build(:key) + + expect { post :create, key: key.attributes }.to change { SecurityEvent.count }.by(1) + end + end end diff --git a/spec/ee/spec/services/ee/audit_event_service_spec.rb b/spec/ee/spec/services/ee/audit_event_service_spec.rb index 1dedce22891e4a9de10adb706e747a4a9296ad10..6e82246dc0bc5c9cb3f741dfdf9cf426c38736cf 100644 --- a/spec/ee/spec/services/ee/audit_event_service_spec.rb +++ b/spec/ee/spec/services/ee/audit_event_service_spec.rb @@ -7,6 +7,10 @@ let(:service) { described_class.new(author_name, nil, ip_address: ip_address) } let(:event) { service.for_failed_login.unauth_security_event } + before do + stub_licensed_features(extended_audit_events: true) + end + it 'has the right type' do expect(event.entity_type).to eq('User') end diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index 0c897d32e3104c417e23ebf45f1159adb6d51124..8769b2c5a638a7a0ff29ff0ca5b48a3bd622de4c 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -155,6 +155,8 @@ def enter_code(code) it 'creates a security event after failed OAuth login' do stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [mock_saml_config]) + stub_licensed_features(extended_audit_events: true) + user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') gitlab_sign_in_via('saml', user, 'wrong-uid') @@ -177,6 +179,8 @@ def enter_code(code) end it 'blocks invalid login' do + stub_licensed_features(extended_audit_events: true) + user = create(:user, password: 'not-the-default') gitlab_sign_in(user) diff --git a/spec/lib/audit/details_spec.rb b/spec/lib/audit/details_spec.rb index 9716ed8c7b11d486da110377d40566c1bbc8e62a..0f9a811ee014d135d3cd535d3984efb00f7e43e1 100644 --- a/spec/lib/audit/details_spec.rb +++ b/spec/lib/audit/details_spec.rb @@ -75,5 +75,23 @@ expect(described_class.humanize(removal_action)).to eq('Removed deploy key') end end + + context 'change email' do + let(:action) do + { + change: 'email', + from: 'a@b.com', + to: 'c@b.com', + author_name: 'author', + target_id: '', + target_type: 'Email', + target_details: 'Email' + } + end + + it 'humanizes the removal action' do + expect(described_class.humanize(action)).to eq('Changed email from a@b.com to c@b.com') + end + end end end diff --git a/spec/lib/ee/audit/changes_spec.rb b/spec/lib/ee/audit/changes_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..a18afc02f5b420a7dff725e41cbaa386096ad4e3 --- /dev/null +++ b/spec/lib/ee/audit/changes_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' + +describe EE::Audit::Changes do + describe '.audit_changes' do + let(:user) { create(:user) } + let(:foo_instance) { Class.new { include EE::Audit::Changes }.new } + + before do + stub_licensed_features(extended_audit_events: true) + + foo_instance.instance_variable_set(:@current_user, user) + foo_instance.instance_variable_set(:@user, user) + + allow(foo_instance).to receive(:model).and_return(user) + end + + describe 'non audit changes' do + it 'does not call the audit event service' do + user.update!(name: 'new name') + + expect { foo_instance.audit_changes(:email) }.not_to change { SecurityEvent.count } + end + end + + describe 'audit changes' do + it 'calls the audit event service' do + user.update!(name: 'new name') + + expect { foo_instance.audit_changes(:name) }.to change { SecurityEvent.count }.by(1) + end + end + end +end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 2e4d753007c85cfda46f5f79c97edbcf22baf74f..d04d6f2f0e7725345f8ab8e823a12dc832ff717a 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -474,10 +474,13 @@ end it "updates user with new password and forces reset on next login" do + stub_licensed_features(extended_audit_events: true) + put api("/users/#{user.id}", admin), password: '12345678' expect(response).to have_http_status(200) expect(user.reload.password_expires_at).to be <= Time.now + expect(AuditEvent.count).to eq(1) end it "updates user with organization" do diff --git a/spec/services/audit_event_service_spec.rb b/spec/services/audit_event_service_spec.rb index 4543277e05b040ff42e981b0916544b5089219f9..35c2b904a572ddc53e0690ca6bcd1f0e3d2d4183 100644 --- a/spec/services/audit_event_service_spec.rb +++ b/spec/services/audit_event_service_spec.rb @@ -56,20 +56,20 @@ end end - describe '#audit_events_enabled?' do + describe '#entity_audit_events_enabled??' do context 'entity is a project' do let(:service) { described_class.new(user, project, { action: :destroy }) } it 'returns false when project is unlicensed' do stub_licensed_features(audit_events: false) - expect(service.audit_events_enabled?).to be_falsy + expect(service.entity_audit_events_enabled?).to be_falsy end it 'returns true when project is licensed' do stub_licensed_features(audit_events: true) - expect(service.audit_events_enabled?).to be_truthy + expect(service.entity_audit_events_enabled?).to be_truthy end end @@ -80,19 +80,35 @@ it 'returns false when group is unlicensed' do stub_licensed_features(audit_events: false) - expect(service.audit_events_enabled?).to be_falsy + expect(service.entity_audit_events_enabled?).to be_falsy end it 'returns true when group is licensed' do stub_licensed_features(audit_events: true) - expect(service.audit_events_enabled?).to be_truthy + expect(service.entity_audit_events_enabled?).to be_truthy end end context 'entity is a user' do let(:service) { described_class.new(user, user, { action: :destroy }) } + it 'returns false when unlicensed' do + stub_licensed_features(audit_events: false, admin_audit_log: false) + + expect(service.audit_events_enabled?).to be_falsey + end + + it 'returns true when licensed with extended events' do + stub_licensed_features(extended_audit_events: true) + + expect(service.audit_events_enabled?).to be_truthy + end + end + + context 'auth event' do + let(:service) { described_class.new(user, user, { with: 'auth' }) } + it 'returns true when unlicensed' do stub_licensed_features(audit_events: false, admin_audit_log: false) diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb index 641d5538de8b0867ea1865382dcb7ac804bffaa2..544f5b228bf2bfd072388265ef4094f014e2caf2 100644 --- a/spec/services/emails/create_service_spec.rb +++ b/spec/services/emails/create_service_spec.rb @@ -2,11 +2,15 @@ describe Emails::CreateService do let(:user) { create(:user) } - let(:opts) { { email: 'new@email.com' } } + let(:opts) { { email: 'new@email.com', user: user } } subject(:service) { described_class.new(user, opts) } describe '#execute' do + before do + stub_licensed_features(extended_audit_events: true) + end + it 'creates an email with valid attributes' do expect { service.execute }.to change { Email.count }.by(1) expect(Email.where(opts)).not_to be_empty @@ -17,5 +21,9 @@ expect(user.emails).to eq(Email.where(opts)) end + + it 'registers a security event' do + expect { service.execute }.to change { SecurityEvent.count }.by(1) + end end end diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb index 1f4294dd90529f4000bfb7db3bbcb9b772de84db..a69279e3df0d055c0af6f899f4006ebaed76e5fc 100644 --- a/spec/services/emails/destroy_service_spec.rb +++ b/spec/services/emails/destroy_service_spec.rb @@ -4,11 +4,19 @@ let!(:user) { create(:user) } let!(:email) { create(:email, user: user) } - subject(:service) { described_class.new(user, email: email.email) } + subject(:service) { described_class.new(user, user: user, email: email.email) } + + before do + stub_licensed_features(extended_audit_events: true) + end describe '#execute' do it 'removes an email' do expect { service.execute }.to change { user.emails.count }.by(-1) end + + it 'registers a security event' do + expect { service.execute }.to change { SecurityEvent.count }.by(1) + end end end diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb index 6ee35a33b2d18f33ffde9839c39154b6d1f9c8ad..f8d4a47b212083f0cff17bf0f0d6712edf2f24ea 100644 --- a/spec/services/users/update_service_spec.rb +++ b/spec/services/users/update_service_spec.rb @@ -31,13 +31,13 @@ end def update_user(user, opts) - described_class.new(user, opts).execute + described_class.new(user, opts.merge(user: user)).execute end end describe '#execute!' do it 'updates the name' do - service = described_class.new(user, name: 'New Name') + service = described_class.new(user, user: user, name: 'New Name') expect(service).not_to receive(:notify_new_user) result = service.execute! @@ -55,7 +55,7 @@ def update_user(user, opts) it 'fires system hooks when a new user is saved' do system_hook_service = spy(:system_hook_service) user = build(:user) - service = described_class.new(user, name: 'John Doe') + service = described_class.new(user, user: user, name: 'John Doe') expect(service).to receive(:notify_new_user).and_call_original expect(service).to receive(:system_hook_service).and_return(system_hook_service) @@ -65,7 +65,7 @@ def update_user(user, opts) end def update_user(user, opts) - described_class.new(user, opts).execute! + described_class.new(user, opts.merge(user: user)).execute! end end end