diff options
41 files changed, 408 insertions, 164 deletions
@@ -33,7 +33,7 @@ gem 'omniauth-auth0', '~> 2.0.0' gem 'omniauth-azure-oauth2', '~> 0.0.9' gem 'omniauth-cas3', '~> 1.1.4' gem 'omniauth-facebook', '~> 4.0.0' -gem 'omniauth-github', '~> 1.3' +gem 'omniauth-github', '~> 1.4' gem 'omniauth-gitlab', '~> 1.0.2' gem 'omniauth-google-oauth2', '~> 0.6.0' gem 'omniauth-kerberos', '~> 0.3.0', group: :kerberos diff --git a/Gemfile.lock b/Gemfile.lock index a3fb732021f..9e261018f03 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -688,7 +688,7 @@ GEM omniauth (~> 1.2) omniauth-facebook (4.0.0) omniauth-oauth2 (~> 1.2) - omniauth-github (1.3.0) + omniauth-github (1.4.0) omniauth (~> 1.5) omniauth-oauth2 (>= 1.4.0, < 2.0) omniauth-gitlab (1.0.3) @@ -1304,7 +1304,7 @@ DEPENDENCIES omniauth-azure-oauth2 (~> 0.0.9) omniauth-cas3 (~> 1.1.4) omniauth-facebook (~> 4.0.0) - omniauth-github (~> 1.3) + omniauth-github (~> 1.4) omniauth-gitlab (~> 1.0.2) omniauth-google-oauth2 (~> 0.6.0) omniauth-kerberos (~> 0.3.0) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 21253e004ef..41f3603506f 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -158,6 +158,27 @@ } } +// Temporary hack until `gitlab-ui` issue is fixed. +// https://gitlab.com/gitlab-org/gitlab-ui/issues/164 +.gl-dropdown .dropdown-menu-toggle { + .gl-dropdown-caret { + position: absolute; + right: $gl-padding-8; + top: $gl-padding-8; + } + + // Add some child to the button so that the default height kicks in + // when there's no text (since the caret is now aboslute) + &::after { + border: 0; + content: ' '; + display: inline-block; + margin: 0; + padding: 0; + position: relative; + } +} + @mixin dropdown-item-hover { background-color: $gray-darker; color: $gl-text-color; diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index e9bba904dab..9ec8f930a78 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -11,7 +11,7 @@ module SpammableActions end def mark_as_spam - if Spam::MarkAsSpamService.new(target: spammable).execute + if Spam::MarkAsSpamService.new(spammable: spammable).execute redirect_to spammable_path, notice: _("%{spammable_titlecase} was submitted to Akismet successfully.") % { spammable_titlecase: spammable.spammable_entity_type.titlecase } else redirect_to spammable_path, alert: _('Error with Akismet. Please check the logs for more info.') @@ -42,7 +42,7 @@ module SpammableActions end format.json do - locals = { target: spammable, script: false, has_submit: false } + locals = { spammable: spammable, script: false, has_submit: false } recaptcha_html = render_to_string(partial: 'shared/recaptcha_form', formats: :html, locals: locals) render json: { recaptcha_html: recaptcha_html } diff --git a/app/graphql/mutations/snippets/mark_as_spam.rb b/app/graphql/mutations/snippets/mark_as_spam.rb index 3171f4645ca..8cfbbae7c08 100644 --- a/app/graphql/mutations/snippets/mark_as_spam.rb +++ b/app/graphql/mutations/snippets/mark_as_spam.rb @@ -24,7 +24,7 @@ module Mutations private def mark_as_spam(snippet) - Spam::MarkAsSpamService.new(target: snippet).execute + Spam::MarkAsSpamService.new(spammable: snippet).execute end def authorized_resource?(snippet) diff --git a/app/models/label.rb b/app/models/label.rb index 938ecb323e2..632207701d8 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -42,6 +42,22 @@ class Label < ApplicationRecord scope :order_name_desc, -> { reorder(title: :desc) } scope :subscribed_by, ->(user_id) { joins(:subscriptions).where(subscriptions: { user_id: user_id, subscribed: true }) } + scope :top_labels_by_target, -> (target_relation) { + label_id_column = arel_table[:id] + + # Window aggregation to count labels + count_by_id = Arel::Nodes::Over.new( + Arel::Nodes::NamedFunction.new('count', [label_id_column]), + Arel::Nodes::Window.new.partition(label_id_column) + ).as('count_by_id') + + select(arel_table[Arel.star], count_by_id) + .joins(:label_links) + .merge(LabelLink.where(target: target_relation)) + .reorder(count_by_id: :desc) + .distinct + } + def self.prioritized(project) joins(:priorities) .where(label_priorities: { project_id: project }) diff --git a/app/models/user.rb b/app/models/user.rb index 11bfa485ae9..ec9bc7ae01e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1226,7 +1226,8 @@ class User < ApplicationRecord { name: name, username: username, - avatar_url: avatar_url(only_path: false) + avatar_url: avatar_url(only_path: false), + email: email } end diff --git a/app/services/concerns/akismet_methods.rb b/app/services/concerns/akismet_methods.rb index 4e554ddac4c..105b79785bd 100644 --- a/app/services/concerns/akismet_methods.rb +++ b/app/services/concerns/akismet_methods.rb @@ -1,15 +1,15 @@ # frozen_string_literal: true module AkismetMethods - def target_owner - @user ||= User.find(target.author_id) + def spammable_owner + @user ||= User.find(spammable.author_id) end def akismet @akismet ||= Spam::AkismetService.new( - target_owner.name, - target_owner.email, - target.try(:spammable_text) || target&.text, + spammable_owner.name, + spammable_owner.email, + spammable.try(:spammable_text) || spammable&.text, options ) end diff --git a/app/services/concerns/spam_check_methods.rb b/app/services/concerns/spam_check_methods.rb index 62836881099..695bdf92b49 100644 --- a/app/services/concerns/spam_check_methods.rb +++ b/app/services/concerns/spam_check_methods.rb @@ -2,7 +2,7 @@ # SpamCheckMethods # -# Provide helper methods for checking if a given target spammable object has +# Provide helper methods for checking if a given spammable object has # potential spam data. # # Dependencies: @@ -18,13 +18,13 @@ module SpamCheckMethods end # rubocop:enable Gitlab/ModuleWithInstanceVariables - # In order to be proceed to the spam check process, @target has to be + # In order to be proceed to the spam check process, @spammable has to be # a dirty instance, which means it should be already assigned with the new # attribute values. # rubocop:disable Gitlab/ModuleWithInstanceVariables def spam_check(spammable, user) Spam::SpamCheckService.new( - target: spammable, + spammable: spammable, request: @request ).execute( api: @api, diff --git a/app/services/post_receive_service.rb b/app/services/post_receive_service.rb new file mode 100644 index 00000000000..e3818e76c4c --- /dev/null +++ b/app/services/post_receive_service.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +# PostReceiveService class +# +# Used for scheduling related jobs after a push action has been performed +class PostReceiveService + attr_reader :user, :project, :params + + def initialize(user, project, params) + @user = user + @project = project + @params = params + end + + def execute + response = Gitlab::InternalPostReceive::Response.new + + push_options = Gitlab::PushOptions.new(params[:push_options]) + + response.reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease + + PostReceive.perform_async(params[:gl_repository], params[:identifier], + params[:changes], push_options.as_json) + + mr_options = push_options.get(:merge_request) + if mr_options.present? + message = process_mr_push_options(mr_options, project, user, params[:changes]) + response.add_alert_message(message) + end + + broadcast_message = BroadcastMessage.current&.last&.message + response.add_alert_message(broadcast_message) + + response.add_merge_request_urls(merge_request_urls) + + # Neither User nor Project are guaranteed to be returned; an orphaned write deploy + # key could be used + if user && project + redirect_message = Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id) + project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user.id, project.id) + + response.add_basic_message(redirect_message) + response.add_basic_message(project_created_message) + end + + response + end + + def process_mr_push_options(push_options, project, user, changes) + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/61359') + + service = ::MergeRequests::PushOptionsHandlerService.new( + project, user, changes, push_options + ).execute + + if service.errors.present? + push_options_warning(service.errors.join("\n\n")) + end + end + + def push_options_warning(warning) + options = Array.wrap(params[:push_options]).map { |p| "'#{p}'" }.join(' ') + "WARNINGS:\nError encountered with push options #{options}: #{warning}" + end + + def merge_request_urls + ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) + end +end diff --git a/app/services/spam/ham_service.rb b/app/services/spam/ham_service.rb index cedb9f02f3d..d0f53eea90c 100644 --- a/app/services/spam/ham_service.rb +++ b/app/services/spam/ham_service.rb @@ -4,23 +4,25 @@ module Spam class HamService include AkismetMethods - attr_accessor :target, :options + attr_accessor :spam_log, :options - def initialize(target) - @target = target - @user = target.user + def initialize(spam_log) + @spam_log = spam_log + @user = spam_log.user @options = { - ip_address: target.source_ip, - user_agent: target.user_agent + ip_address: spam_log.source_ip, + user_agent: spam_log.user_agent } end def execute if akismet.submit_ham - target.update_attribute(:submitted_as_ham, true) + spam_log.update_attribute(:submitted_as_ham, true) else false end end + + alias_method :spammable, :spam_log end end diff --git a/app/services/spam/mark_as_spam_service.rb b/app/services/spam/mark_as_spam_service.rb index ed5e674d8e9..0ebcf17927a 100644 --- a/app/services/spam/mark_as_spam_service.rb +++ b/app/services/spam/mark_as_spam_service.rb @@ -4,21 +4,21 @@ module Spam class MarkAsSpamService include ::AkismetMethods - attr_accessor :target, :options + attr_accessor :spammable, :options - def initialize(target:) - @target = target + def initialize(spammable:) + @spammable = spammable @options = {} - @options[:ip_address] = @target.ip_address - @options[:user_agent] = @target.user_agent + @options[:ip_address] = @spammable.ip_address + @options[:user_agent] = @spammable.user_agent end def execute - return unless target.submittable_as_spam? + return unless spammable.submittable_as_spam? return unless akismet.submit_spam - target.user_agent_detail.update_attribute(:submitted, true) + spammable.user_agent_detail.update_attribute(:submitted, true) end end end diff --git a/app/services/spam/spam_check_service.rb b/app/services/spam/spam_check_service.rb index a922b0d96e7..d19ef03976f 100644 --- a/app/services/spam/spam_check_service.rb +++ b/app/services/spam/spam_check_service.rb @@ -4,11 +4,11 @@ module Spam class SpamCheckService include AkismetMethods - attr_accessor :target, :request, :options + attr_accessor :spammable, :request, :options attr_reader :spam_log - def initialize(target:, request:) - @target = target + def initialize(spammable:, request:) + @spammable = spammable @request = request @options = {} @@ -17,8 +17,8 @@ module Spam @options[:user_agent] = @request.env['HTTP_USER_AGENT'] @options[:referrer] = @request.env['HTTP_REFERRER'] else - @options[:ip_address] = @target.ip_address - @options[:user_agent] = @target.user_agent + @options[:ip_address] = @spammable.ip_address + @options[:user_agent] = @spammable.user_agent end end @@ -29,10 +29,10 @@ module Spam SpamLog.verify_recaptcha!(user_id: user_id, id: spam_log_id) else # Otherwise, it goes to Akismet for spam check. - # If so, it assigns target spammable object as "spam" and creates a SpamLog record. + # If so, it assigns spammable object as "spam" and creates a SpamLog record. possible_spam = check(api) - target.spam = possible_spam unless target.allow_possible_spam? - target.spam_log = spam_log + spammable.spam = possible_spam unless spammable.allow_possible_spam? + spammable.spam_log = spam_log end end @@ -48,18 +48,18 @@ module Spam end def check_for_spam? - target.check_for_spam? + spammable.check_for_spam? end def create_spam_log(api) @spam_log = SpamLog.create!( { - user_id: target.author_id, - title: target.spam_title, - description: target.spam_description, + user_id: spammable.author_id, + title: spammable.spam_title, + description: spammable.spam_description, source_ip: options[:ip_address], user_agent: options[:user_agent], - noteable_type: target.class.to_s, + noteable_type: spammable.class.to_s, via_api: api } ) diff --git a/changelogs/unreleased/24992-add-avatar_url-and-email_user-in_web_hook.yml b/changelogs/unreleased/24992-add-avatar_url-and-email_user-in_web_hook.yml new file mode 100644 index 00000000000..b362235aebd --- /dev/null +++ b/changelogs/unreleased/24992-add-avatar_url-and-email_user-in_web_hook.yml @@ -0,0 +1,5 @@ +--- +title: add avatar_url in job webhook, and email in pipeline webhook +merge_request: 24992 +author: Guillaume Micouin +type: added diff --git a/changelogs/unreleased/37297-monitor-dashboard-dropdown-controls-caret-icon-is-misaligned.yml b/changelogs/unreleased/37297-monitor-dashboard-dropdown-controls-caret-icon-is-misaligned.yml new file mode 100644 index 00000000000..06e49b00612 --- /dev/null +++ b/changelogs/unreleased/37297-monitor-dashboard-dropdown-controls-caret-icon-is-misaligned.yml @@ -0,0 +1,5 @@ +--- +title: Fix dropdown caret not being positioned correctly +merge_request: 24273 +author: +type: fixed diff --git a/changelogs/unreleased/limit-grape-param-log-size.yml b/changelogs/unreleased/limit-grape-param-log-size.yml new file mode 100644 index 00000000000..cf61bde91d5 --- /dev/null +++ b/changelogs/unreleased/limit-grape-param-log-size.yml @@ -0,0 +1,5 @@ +--- +title: Limit size of params array in JSON logs to 10 KiB +merge_request: 25158 +author: +type: changed diff --git a/changelogs/unreleased/weimeng-upgrade-omniauth-github.yml b/changelogs/unreleased/weimeng-upgrade-omniauth-github.yml new file mode 100644 index 00000000000..aa2cdec2a6b --- /dev/null +++ b/changelogs/unreleased/weimeng-upgrade-omniauth-github.yml @@ -0,0 +1,5 @@ +--- +title: Upgrade omniauth-github gem to fix GitHub API deprecation notice +merge_request: 24928 +author: +type: fixed diff --git a/config/initializers/lograge.rb b/config/initializers/lograge.rb index e7bab1c8fa3..fb93c3a6e12 100644 --- a/config/initializers/lograge.rb +++ b/config/initializers/lograge.rb @@ -28,7 +28,7 @@ unless Gitlab::Runtime.sidekiq? payload = { time: Time.now.utc.iso8601(3), - params: params, + params: Gitlab::Utils::LogLimitedArray.log_limited_array(params), remote_ip: event.payload[:remote_ip], user_id: event.payload[:user_id], username: event.payload[:username], diff --git a/db/migrate/20200203173508_add_confirmed_attributes_to_vulnerabilities.rb b/db/migrate/20200203173508_add_confirmed_attributes_to_vulnerabilities.rb new file mode 100644 index 00000000000..43a375c011f --- /dev/null +++ b/db/migrate/20200203173508_add_confirmed_attributes_to_vulnerabilities.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddConfirmedAttributesToVulnerabilities < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :vulnerabilities, :confirmed_by_id, :bigint + add_column :vulnerabilities, :confirmed_at, :datetime_with_timezone + end +end diff --git a/db/migrate/20200203183508_add_index_for_vulnerability_confirmed_by.rb b/db/migrate/20200203183508_add_index_for_vulnerability_confirmed_by.rb new file mode 100644 index 00000000000..dd50e7be893 --- /dev/null +++ b/db/migrate/20200203183508_add_index_for_vulnerability_confirmed_by.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddIndexForVulnerabilityConfirmedBy < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :vulnerabilities, :confirmed_by_id + add_concurrent_foreign_key :vulnerabilities, :users, column: :confirmed_by_id, on_delete: :nullify + end + + def down + remove_foreign_key :vulnerabilities, column: :confirmed_by_id + remove_concurrent_index :vulnerabilities, :confirmed_by_id + end +end diff --git a/db/schema.rb b/db/schema.rb index bce47ba1c72..f95662582ec 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -4356,8 +4356,11 @@ ActiveRecord::Schema.define(version: 2020_02_13_204737) do t.datetime_with_timezone "resolved_at" t.integer "report_type", limit: 2, null: false t.integer "cached_markdown_version" + t.bigint "confirmed_by_id" + t.datetime_with_timezone "confirmed_at" t.index ["author_id"], name: "index_vulnerabilities_on_author_id" t.index ["closed_by_id"], name: "index_vulnerabilities_on_closed_by_id" + t.index ["confirmed_by_id"], name: "index_vulnerabilities_on_confirmed_by_id" t.index ["due_date_sourcing_milestone_id"], name: "index_vulnerabilities_on_due_date_sourcing_milestone_id" t.index ["epic_id"], name: "index_vulnerabilities_on_epic_id" t.index ["last_edited_by_id"], name: "index_vulnerabilities_on_last_edited_by_id" @@ -5014,6 +5017,7 @@ ActiveRecord::Schema.define(version: 2020_02_13_204737) do add_foreign_key "vulnerabilities", "projects", name: "fk_efb96ab1e2", on_delete: :cascade add_foreign_key "vulnerabilities", "users", column: "author_id", name: "fk_b1de915a15", on_delete: :nullify add_foreign_key "vulnerabilities", "users", column: "closed_by_id", name: "fk_cf5c60acbf", on_delete: :nullify + add_foreign_key "vulnerabilities", "users", column: "confirmed_by_id", name: "fk_959d40ad0a", on_delete: :nullify add_foreign_key "vulnerabilities", "users", column: "last_edited_by_id", name: "fk_1302949740", on_delete: :nullify add_foreign_key "vulnerabilities", "users", column: "resolved_by_id", name: "fk_76bc5f5455", on_delete: :nullify add_foreign_key "vulnerabilities", "users", column: "updated_by_id", name: "fk_7ac31eacb9", on_delete: :nullify diff --git a/doc/user/project/integrations/webhooks.md b/doc/user/project/integrations/webhooks.md index acc187d1e6b..5c9a21e2fbb 100644 --- a/doc/user/project/integrations/webhooks.md +++ b/doc/user/project/integrations/webhooks.md @@ -1054,7 +1054,8 @@ X-Gitlab-Event: Pipeline Hook "user":{ "name": "Administrator", "username": "root", - "avatar_url": "http://www.gravatar.com/avatar/e32bd13e2add097461cb96824b7a829c?s=80\u0026d=identicon" + "avatar_url": "http://www.gravatar.com/avatar/e32bd13e2add097461cb96824b7a829c?s=80\u0026d=identicon", + "email": "user_email@gitlab.com" }, "project":{ "id": 1, @@ -1243,7 +1244,8 @@ X-Gitlab-Event: Job Hook "user": { "id": 3, "name": "User", - "email": "user@gitlab.com" + "email": "user@gitlab.com", + "avatar_url": "http://www.gravatar.com/avatar/e32bd13e2add097461cb96824b7a829c?s=80\u0026d=identicon" }, "commit": { "id": 2366, diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index b17cb2633d8..ab43096a1de 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -55,30 +55,6 @@ module API ::Users::ActivityService.new(actor).execute if commands.include?(params[:action]) end - def merge_request_urls - ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) - end - - def process_mr_push_options(push_options, project, user, changes) - Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/61359') - - service = ::MergeRequests::PushOptionsHandlerService.new( - project, - user, - changes, - push_options - ).execute - - if service.errors.present? - push_options_warning(service.errors.join("\n\n")) - end - end - - def push_options_warning(warning) - options = Array.wrap(params[:push_options]).map { |p| "'#{p}'" }.join(' ') - "WARNINGS:\nError encountered with push options #{options}: #{warning}" - end - def redis_ping result = Gitlab::Redis::SharedState.with { |redis| redis.ping } diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 9de3644daa8..382bbeb66de 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -212,40 +212,7 @@ module API post '/post_receive' do status 200 - response = Gitlab::InternalPostReceive::Response.new - - # Try to load the project and users so we have the application context - # available for logging before we schedule any jobs. - user = actor.user - project - - push_options = Gitlab::PushOptions.new(params[:push_options]) - - response.reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease - - PostReceive.perform_async(params[:gl_repository], params[:identifier], - params[:changes], push_options.as_json) - - mr_options = push_options.get(:merge_request) - if mr_options.present? - message = process_mr_push_options(mr_options, project, user, params[:changes]) - response.add_alert_message(message) - end - - broadcast_message = BroadcastMessage.current&.last&.message - response.add_alert_message(broadcast_message) - - response.add_merge_request_urls(merge_request_urls) - - # Neither User nor Project are guaranteed to be returned; an orphaned write deploy - # key could be used - if user && project - redirect_message = Gitlab::Checks::ProjectMoved.fetch_message(user.id, project.id) - project_created_message = Gitlab::Checks::ProjectCreated.fetch_message(user.id, project.id) - - response.add_basic_message(redirect_message) - response.add_basic_message(project_created_message) - end + response = PostReceiveService.new(actor.user, project, params).execute ee_post_receive_response_hook(response) diff --git a/lib/gitlab/data_builder/build.rb b/lib/gitlab/data_builder/build.rb index f7b7db50b2f..e6702c5a38b 100644 --- a/lib/gitlab/data_builder/build.rb +++ b/lib/gitlab/data_builder/build.rb @@ -38,11 +38,7 @@ module Gitlab project_id: project.id, project_name: project.full_name, - user: { - id: user.try(:id), - name: user.try(:name), - email: user.try(:email) - }, + user: user.try(:hook_attrs), commit: { # note: commit.id is actually the pipeline id diff --git a/lib/gitlab/grape_logging/formatters/lograge_with_timestamp.rb b/lib/gitlab/grape_logging/formatters/lograge_with_timestamp.rb index 9bb1e8fc7a2..837473d47cd 100644 --- a/lib/gitlab/grape_logging/formatters/lograge_with_timestamp.rb +++ b/lib/gitlab/grape_logging/formatters/lograge_with_timestamp.rb @@ -25,9 +25,12 @@ module Gitlab def process_params(data) return [] unless data.has_key?(:params) - data[:params] - .each_pair - .map { |k, v| { key: k, value: utf8_encode_values(v) } } + params_array = + data[:params] + .each_pair + .map { |k, v| { key: k, value: utf8_encode_values(v) } } + + Gitlab::Utils::LogLimitedArray.log_limited_array(params_array) end def utf8_encode_values(data) diff --git a/lib/gitlab/sidekiq_logging/structured_logger.rb b/lib/gitlab/sidekiq_logging/structured_logger.rb index 8e7626b8eb6..6dacca5529b 100644 --- a/lib/gitlab/sidekiq_logging/structured_logger.rb +++ b/lib/gitlab/sidekiq_logging/structured_logger.rb @@ -6,8 +6,6 @@ require 'active_record/log_subscriber' module Gitlab module SidekiqLogging class StructuredLogger - MAXIMUM_JOB_ARGUMENTS_LENGTH = 10.kilobytes - def call(job, queue) started_time = get_time base_payload = parse_job(job) @@ -85,7 +83,7 @@ module Gitlab job['pid'] = ::Process.pid job.delete('args') unless ENV['SIDEKIQ_LOG_ARGUMENTS'] - job['args'] = limited_job_args(job['args']) if job['args'] + job['args'] = Gitlab::Utils::LogLimitedArray.log_limited_array(job['args']) if job['args'] job end @@ -108,21 +106,6 @@ module Gitlab def current_time Gitlab::Metrics::System.monotonic_time end - - def limited_job_args(args) - return unless args.is_a?(Array) - - total_length = 0 - limited_args = args.take_while do |arg| - total_length += arg.to_json.length - - total_length <= MAXIMUM_JOB_ARGUMENTS_LENGTH - end - - limited_args.push('...') if total_length > MAXIMUM_JOB_ARGUMENTS_LENGTH - - limited_args - end end end end diff --git a/lib/gitlab/utils/log_limited_array.rb b/lib/gitlab/utils/log_limited_array.rb new file mode 100644 index 00000000000..fe8aadf9020 --- /dev/null +++ b/lib/gitlab/utils/log_limited_array.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + module LogLimitedArray + MAXIMUM_ARRAY_LENGTH = 10.kilobytes + + # Prepare an array for logging by limiting its JSON representation + # to around 10 kilobytes. Once we hit the limit, add "..." as the + # last item in the returned array. + def self.log_limited_array(array) + return [] unless array.is_a?(Array) + + total_length = 0 + limited_array = array.take_while do |arg| + total_length += arg.to_json.length + + total_length <= MAXIMUM_ARRAY_LENGTH + end + + limited_array.push('...') if total_length > MAXIMUM_ARRAY_LENGTH + + limited_array + end + end + end +end diff --git a/spec/controllers/projects/deployments_controller_spec.rb b/spec/controllers/projects/deployments_controller_spec.rb index bf80346c63a..37dcfa78772 100644 --- a/spec/controllers/projects/deployments_controller_spec.rb +++ b/spec/controllers/projects/deployments_controller_spec.rb @@ -59,7 +59,7 @@ describe Projects::DeploymentsController do end end - it 'returns a empty response 204 resposne' do + it 'returns an empty 204 response' do get :metrics, params: deployment_params(id: deployment.to_param) expect(response).to have_gitlab_http_status(:no_content) expect(response.body).to eq('') diff --git a/spec/db/schema_spec.rb b/spec/db/schema_spec.rb index 482e0fbe7ce..f2f7d6cbafc 100644 --- a/spec/db/schema_spec.rb +++ b/spec/db/schema_spec.rb @@ -55,7 +55,7 @@ describe 'Database schema' do members: %w[source_id created_by_id], merge_requests: %w[last_edited_by_id state_id], namespaces: %w[owner_id parent_id], - notes: %w[author_id commit_id noteable_id updated_by_id resolved_by_id discussion_id], + notes: %w[author_id commit_id noteable_id updated_by_id resolved_by_id confirmed_by_id discussion_id], notification_settings: %w[source_id], oauth_access_grants: %w[resource_owner_id application_id], oauth_access_tokens: %w[resource_owner_id application_id], diff --git a/spec/initializers/lograge_spec.rb b/spec/initializers/lograge_spec.rb index 7c61d034ac9..15165c6db98 100644 --- a/spec/initializers/lograge_spec.rb +++ b/spec/initializers/lograge_spec.rb @@ -5,16 +5,37 @@ require 'spec_helper' describe 'lograge', type: :request do let(:headers) { { 'X-Request-ID' => 'new-correlation-id' } } - context 'for API requests' do - subject { get("/api/v4/endpoint", params: {}, headers: headers) } + let(:large_params) do + half_limit = Gitlab::Utils::LogLimitedArray::MAXIMUM_ARRAY_LENGTH / 2 + + { + a: 'a', + b: 'b' * half_limit, + c: 'c' * half_limit, + d: 'd' + } + end + + let(:limited_params) do + large_params.slice(:a, :b).map { |k, v| { key: k.to_s, value: v } } + ['...'] + end + context 'for API requests' do it 'logs to api_json log' do # we assert receiving parameters by grape logger expect_any_instance_of(Gitlab::GrapeLogging::Formatters::LogrageWithTimestamp).to receive(:call) .with(anything, anything, anything, a_hash_including("correlation_id" => "new-correlation-id")) .and_call_original - subject + get("/api/v4/endpoint", params: {}, headers: headers) + end + + it 'limits param size' do + expect(Lograge.formatter).to receive(:call) + .with(a_hash_including(params: limited_params)) + .and_call_original + + get("/api/v4/endpoint", params: large_params, headers: headers) end end @@ -67,6 +88,14 @@ describe 'lograge', type: :request do subject end + + it 'limits param size' do + expect(Lograge.formatter).to receive(:call) + .with(a_hash_including(params: limited_params)) + .and_call_original + + get("/", params: large_params, headers: headers) + end end context 'with a log subscriber' do diff --git a/spec/lib/gitlab/data_builder/build_spec.rb b/spec/lib/gitlab/data_builder/build_spec.rb index fdb855de786..da27125c9a6 100644 --- a/spec/lib/gitlab/data_builder/build_spec.rb +++ b/spec/lib/gitlab/data_builder/build_spec.rb @@ -4,7 +4,8 @@ require 'spec_helper' describe Gitlab::DataBuilder::Build do let(:runner) { create(:ci_runner, :instance) } - let(:build) { create(:ci_build, :running, runner: runner) } + let(:user) { create(:user) } + let(:build) { create(:ci_build, :running, runner: runner, user: user) } describe '.build' do let(:data) do @@ -22,6 +23,15 @@ describe Gitlab::DataBuilder::Build do it { expect(data[:project_id]).to eq(build.project.id) } it { expect(data[:project_name]).to eq(build.project.full_name) } it { expect(data[:pipeline_id]).to eq(build.pipeline.id) } + it { + expect(data[:user]).to eq( + { + name: user.name, + username: user.username, + avatar_url: user.avatar_url(only_path: false), + email: user.email + }) + } it { expect(data[:commit][:id]).to eq(build.pipeline.id) } it { expect(data[:runner][:id]).to eq(build.runner.id) } it { expect(data[:runner][:description]).to eq(build.runner.description) } diff --git a/spec/lib/gitlab/data_builder/pipeline_spec.rb b/spec/lib/gitlab/data_builder/pipeline_spec.rb index 86ab7f888ca..da22da8de0f 100644 --- a/spec/lib/gitlab/data_builder/pipeline_spec.rb +++ b/spec/lib/gitlab/data_builder/pipeline_spec.rb @@ -11,7 +11,8 @@ describe Gitlab::DataBuilder::Pipeline do project: project, status: 'success', sha: project.commit.sha, - ref: project.default_branch) + ref: project.default_branch, + user: user) end let!(:build) { create(:ci_build, pipeline: pipeline) } @@ -37,6 +38,12 @@ describe Gitlab::DataBuilder::Pipeline do expect(build_data[:allow_failure]).to eq(build.allow_failure) expect(project_data).to eq(project.hook_attrs(backward: false)) expect(data[:merge_request]).to be_nil + expect(data[:user]).to eq({ + name: user.name, + username: user.username, + avatar_url: user.avatar_url(only_path: false), + email: user.email + }) end context 'pipeline without variables' do diff --git a/spec/lib/gitlab/sidekiq_logging/structured_logger_spec.rb b/spec/lib/gitlab/sidekiq_logging/structured_logger_spec.rb index 43cdb998091..ac4cf1734a5 100644 --- a/spec/lib/gitlab/sidekiq_logging/structured_logger_spec.rb +++ b/spec/lib/gitlab/sidekiq_logging/structured_logger_spec.rb @@ -99,13 +99,8 @@ describe Gitlab::SidekiqLogging::StructuredLogger do context 'when the job args are bigger than the maximum allowed' do it 'keeps args from the front until they exceed the limit' do Timecop.freeze(timestamp) do - job['args'] = [ - 1, - 2, - 'a' * (described_class::MAXIMUM_JOB_ARGUMENTS_LENGTH / 2), - 'b' * (described_class::MAXIMUM_JOB_ARGUMENTS_LENGTH / 2), - 3 - ] + half_limit = Gitlab::Utils::LogLimitedArray::MAXIMUM_ARRAY_LENGTH / 2 + job['args'] = [1, 2, 'a' * half_limit, 'b' * half_limit, 3] expected_args = job['args'].take(3) + ['...'] diff --git a/spec/lib/gitlab/utils/log_limited_array_spec.rb b/spec/lib/gitlab/utils/log_limited_array_spec.rb new file mode 100644 index 00000000000..2729b2c7b6f --- /dev/null +++ b/spec/lib/gitlab/utils/log_limited_array_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +describe Gitlab::Utils::LogLimitedArray do + describe '.log_limited_array' do + context 'when the argument is not an array' do + it 'returns an empty array' do + expect(described_class.log_limited_array('aa')).to eq([]) + end + end + + context 'when the argument is an array' do + context 'when the array is under the limit' do + it 'returns the array unchanged' do + expect(described_class.log_limited_array(%w(a b))).to eq(%w(a b)) + end + end + + context 'when the array exceeds the limit' do + it 'replaces arguments after the limit with an ellipsis string' do + half_limit = described_class::MAXIMUM_ARRAY_LENGTH / 2 + long_array = ['a' * half_limit, 'b' * half_limit, 'c'] + + expect(described_class.log_limited_array(long_array)) + .to eq(long_array.take(1) + ['...']) + end + end + + context 'when the array contains arrays and hashes' do + it 'calculates the size based on the JSON representation' do + long_array = [ + 'a', + ['b'] * 10, + { c: 'c' * 10 }, + # Each character in the array takes up four characters: the + # character itself, the two quotes, and the comma (closing + # square bracket for the last item) + ['d'] * (described_class::MAXIMUM_ARRAY_LENGTH / 4), + 'e' + ] + + expect(described_class.log_limited_array(long_array)) + .to eq(long_array.take(3) + ['...']) + end + end + end + end +end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index baf2cfeab0c..dc878c2d3c0 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -183,6 +183,31 @@ describe Label do end end + describe '.top_labels_by_target' do + let(:label) { create(:label) } + let(:popular_label) { create(:label) } + let(:merge_request1) { create(:merge_request) } + let(:merge_request2) { create(:merge_request) } + + before do + merge_request1.labels = [label, popular_label] + merge_request2.labels = [popular_label] + end + + it 'returns distinct labels, ordered by usage in the given target relation' do + top_labels = described_class.top_labels_by_target(MergeRequest.all) + + expect(top_labels).to match_array([popular_label, label]) + end + + it 'excludes labels that are not assigned to any records in the given target relation' do + merge_requests = MergeRequest.where(id: merge_request2.id) + top_labels = described_class.top_labels_by_target(merge_requests) + + expect(top_labels).to match_array([popular_label]) + end + end + describe '.optionally_subscribed_by' do let!(:user) { create(:user) } let!(:label) { create(:label) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 90795e0a8e4..36ddb624cba 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -4200,4 +4200,17 @@ describe User, :do_not_mock_admin_mode do expect(described_class.bots).to match_array([bot]) end end + + describe '#hook_attrs' do + it 'includes name, username, avatar_url, and email' do + user = create(:user) + user_attributes = { + name: user.name, + username: user.username, + avatar_url: user.avatar_url(only_path: false), + email: user.email + } + expect(user.hook_attrs).to eq(user_attributes) + end + end end diff --git a/spec/services/spam/ham_service_spec.rb b/spec/services/spam/ham_service_spec.rb index a4583108dba..9848f48def2 100644 --- a/spec/services/spam/ham_service_spec.rb +++ b/spec/services/spam/ham_service_spec.rb @@ -4,10 +4,10 @@ require 'spec_helper' describe Spam::HamService do let_it_be(:user) { create(:user) } - let!(:target) { create(:spam_log, user: user, submitted_as_ham: false) } + let!(:spam_log) { create(:spam_log, user: user, submitted_as_ham: false) } let(:fake_akismet_service) { double(:akismet_service) } - subject { described_class.new(target) } + subject { described_class.new(spam_log) } before do allow(Spam::AkismetService).to receive(:new).and_return fake_akismet_service @@ -24,23 +24,23 @@ describe Spam::HamService do end it 'does not update the record' do - expect { subject.execute }.not_to change { target.submitted_as_ham } + expect { subject.execute }.not_to change { spam_log.submitted_as_ham } end context 'if spam log record has already been marked as spam' do before do - target.update_attribute(:submitted_as_ham, true) + spam_log.update_attribute(:submitted_as_ham, true) end it 'does not update the record' do - expect { subject.execute }.not_to change { target.submitted_as_ham } + expect { subject.execute }.not_to change { spam_log.submitted_as_ham } end end end context 'Akismet ham submission is successful' do before do - target.update_attribute(:submitted_as_ham, false) + spam_log.update_attribute(:submitted_as_ham, false) allow(fake_akismet_service).to receive(:submit_ham).and_return true end @@ -49,7 +49,7 @@ describe Spam::HamService do end it 'updates the record' do - expect { subject.execute }.to change { target.submitted_as_ham }.from(false).to(true) + expect { subject.execute }.to change { spam_log.submitted_as_ham }.from(false).to(true) end end end diff --git a/spec/services/spam/mark_as_spam_service_spec.rb b/spec/services/spam/mark_as_spam_service_spec.rb index 3824a8244bb..cba9d6a39cb 100644 --- a/spec/services/spam/mark_as_spam_service_spec.rb +++ b/spec/services/spam/mark_as_spam_service_spec.rb @@ -4,19 +4,19 @@ require 'spec_helper' describe Spam::MarkAsSpamService do let(:user_agent_detail) { build(:user_agent_detail) } - let(:target) { build(:issue, user_agent_detail: user_agent_detail) } + let(:spammable) { build(:issue, user_agent_detail: user_agent_detail) } let(:fake_akismet_service) { double(:akismet_service, submit_spam: true) } - subject { described_class.new(target: target) } + subject { described_class.new(spammable: spammable) } describe '#execute' do before do allow(subject).to receive(:akismet).and_return(fake_akismet_service) end - context 'when the target object is not submittable' do + context 'when the spammable object is not submittable' do before do - allow(target).to receive(:submittable_as_spam?).and_return false + allow(spammable).to receive(:submittable_as_spam?).and_return false end it 'does not submit as spam' do @@ -26,7 +26,7 @@ describe Spam::MarkAsSpamService do context 'spam is submitted successfully' do before do - allow(target).to receive(:submittable_as_spam?).and_return true + allow(spammable).to receive(:submittable_as_spam?).and_return true allow(fake_akismet_service).to receive(:submit_spam).and_return true end @@ -34,14 +34,14 @@ describe Spam::MarkAsSpamService do expect(subject.execute).to be_truthy end - it "updates the target object's user agent detail as being submitted as spam" do + it "updates the spammable object's user agent detail as being submitted as spam" do expect(user_agent_detail).to receive(:update_attribute) subject.execute end context 'when Akismet does not consider it spam' do - it 'does not update the target object as spam' do + it 'does not update the spammable object as spam' do allow(fake_akismet_service).to receive(:submit_spam).and_return false expect(subject.execute).to be_falsey diff --git a/spec/services/spam/spam_check_service_spec.rb b/spec/services/spam/spam_check_service_spec.rb index 376afc78b5f..732b64b52a0 100644 --- a/spec/services/spam/spam_check_service_spec.rb +++ b/spec/services/spam/spam_check_service_spec.rb @@ -22,12 +22,12 @@ describe Spam::SpamCheckService do end describe '#initialize' do - subject { described_class.new(target: issue, request: request) } + subject { described_class.new(spammable: issue, request: request) } context 'when the request is nil' do let(:request) { nil } - it 'assembles the options with information from the target' do + it 'assembles the options with information from the spammable' do aggregate_failures do expect(subject.options[:ip_address]).to eq(issue.ip_address) expect(subject.options[:user_agent]).to eq(issue.user_agent) @@ -39,7 +39,7 @@ describe Spam::SpamCheckService do context 'when the request is present' do let(:request) { double(:request, env: env) } - it 'assembles the options with information from the target' do + it 'assembles the options with information from the spammable' do aggregate_failures do expect(subject.options[:ip_address]).to eq(fake_ip) expect(subject.options[:user_agent]).to eq(fake_user_agent) @@ -55,7 +55,7 @@ describe Spam::SpamCheckService do let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) } subject do - described_service = described_class.new(target: issue, request: request) + described_service = described_class.new(spammable: issue, request: request) described_service.execute(user_id: user.id, api: nil, recaptcha_verified: recaptcha_verified, spam_log_id: existing_spam_log.id) end @@ -81,7 +81,7 @@ describe Spam::SpamCheckService do context 'when recaptcha was not verified' do let(:recaptcha_verified) { false } - context 'when target attributes have not changed' do + context 'when spammable attributes have not changed' do before do issue.closed_at = Time.zone.now @@ -98,7 +98,7 @@ describe Spam::SpamCheckService do end end - context 'when target attributes have changed' do + context 'when spammable attributes have changed' do before do issue.description = 'SPAM!' end diff --git a/spec/support/shared_examples/spam_check_shared_examples.rb b/spec/support/shared_examples/spam_check_shared_examples.rb index a16934b7d77..3ecae16db39 100644 --- a/spec/support/shared_examples/spam_check_shared_examples.rb +++ b/spec/support/shared_examples/spam_check_shared_examples.rb @@ -2,7 +2,7 @@ shared_examples 'akismet spam' do context 'when request is missing' do - subject { described_class.new(target: issue, request: nil) } + subject { described_class.new(spammable: issue, request: nil) } it "doesn't check as spam" do subject |