diff options
| author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-04 21:07:54 +0000 |
|---|---|---|
| committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-03-04 21:07:54 +0000 |
| commit | 2fd92f2dc784ade9cb4e1c33dd60cbfad7b86818 (patch) | |
| tree | 7779f36689db97a46e0268a4aec1d49f283eb0c8 | |
| parent | 42ca24aa5bbab7a2d43bc866d9bee9876941cea2 (diff) | |
| download | gitlab-ce-2fd92f2dc784ade9cb4e1c33dd60cbfad7b86818.tar.gz | |
Add latest changes from gitlab-org/gitlab@master
97 files changed, 1491 insertions, 280 deletions
diff --git a/.gitlab/ci/review.gitlab-ci.yml b/.gitlab/ci/review.gitlab-ci.yml index 2fafc1f1f50..12c1507da62 100644 --- a/.gitlab/ci/review.gitlab-ci.yml +++ b/.gitlab/ci/review.gitlab-ci.yml @@ -75,6 +75,7 @@ review-build-cng: name: review/${CI_COMMIT_REF_NAME} url: https://gitlab-${CI_ENVIRONMENT_SLUG}.${REVIEW_APPS_DOMAIN} on_stop: review-stop + auto_stop_in: 48 hours review-deploy: extends: diff --git a/CHANGELOG-EE.md b/CHANGELOG-EE.md index fe723f5d1ef..f2068d36ffc 100644 --- a/CHANGELOG-EE.md +++ b/CHANGELOG-EE.md @@ -1,5 +1,16 @@ Please view this file on the master branch, on stable branches it's out of date. +## 12.8.2 + +### Security (5 changes) + +- Don't show Contribution Analytics to users who are not group members. +- Update epic tree when group is transfered. +- Fix Service Side Request Forgery in JenkinsDeprecatedService. +- Enforce vulnerability feedback pipeline is in the same project. +- Enforce existing vulnerability feedback pipeline is in the same project. + + ## 12.8.1 ### Performance (1 change) diff --git a/app/assets/javascripts/error_tracking/components/error_details.vue b/app/assets/javascripts/error_tracking/components/error_details.vue index 093d993c3ad..43fa97e4095 100644 --- a/app/assets/javascripts/error_tracking/components/error_details.vue +++ b/app/assets/javascripts/error_tracking/components/error_details.vue @@ -108,16 +108,6 @@ export default { 'errorStatus', ]), ...mapGetters('details', ['stacktrace']), - reported() { - return sprintf( - __('Reported %{timeAgo} by %{reportedBy}'), - { - reportedBy: `<strong class="error-details-meta-culprit">${this.error.culprit}</strong>`, - timeAgo: this.timeFormatted(this.stacktraceData.date_received), - }, - false, - ); - }, firstReleaseLink() { return `${this.error.externalBaseUrl}/releases/${this.error.firstReleaseShortVersion}`; }, @@ -227,8 +217,19 @@ export default { </gl-alert> <div class="error-details-header d-flex py-2 justify-content-between"> - <div class="error-details-meta my-auto"> - <span v-if="!loadingStacktrace && stacktrace" v-html="reported"></span> + <div + v-if="!loadingStacktrace && stacktrace" + class="error-details-meta my-auto" + data-qa-selector="reported_text" + > + <gl-sprintf :message="__('Reported %{timeAgo} by %{reportedBy}')"> + <template #reportedBy> + <strong class="error-details-meta-culprit">{{ error.culprit }}</strong> + </template> + <template #timeAgo> + {{ timeFormatted(stacktraceData.date_received) }} + </template> + </gl-sprintf> </div> <div class="error-details-actions"> <div class="d-inline-flex bv-d-sm-down-none"> diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue index 57be97855e3..b1fb377e47a 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue @@ -1,5 +1,6 @@ <script> import { GlLoadingIcon } from '@gitlab/ui'; +import { escape } from 'lodash'; import simplePoll from '../../../lib/utils/simple_poll'; import eventHub from '../../event_hub'; import statusIcon from '../mr_widget_status_icon.vue'; @@ -44,11 +45,10 @@ export default { fastForwardMergeText() { return sprintf( __( - `Fast-forward merge is not possible. Rebase the source branch onto %{startTag}${this.mr.targetBranch}%{endTag} to allow this merge request to be merged.`, + 'Fast-forward merge is not possible. Rebase the source branch onto %{targetBranch} to allow this merge request to be merged.', ), { - startTag: '<span class="label-branch">', - endTag: '</span>', + targetBranch: `<span class="label-branch">${escape(this.mr.targetBranch)}</span>`, }, false, ); diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index 82bef91230e..8b0dd73c565 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -1090,6 +1090,20 @@ button.mini-pipeline-graph-dropdown-toggle { } } +.codequality-report { + .media { + padding: $gl-padding; + } + + .media-body { + flex-direction: row; + } + + .report-block-container { + height: auto !important; + } +} + .progress-bar.bg-primary { background-color: $blue-500 !important; } diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 655575e0944..549a443b1a8 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -9,6 +9,7 @@ module UploadsActions included do prepend_before_action :set_request_format_from_path_extension + rescue_from FileUploader::InvalidSecret, with: :render_404 end def create diff --git a/app/controllers/groups/group_links_controller.rb b/app/controllers/groups/group_links_controller.rb index d3360acd245..23daa29ac43 100644 --- a/app/controllers/groups/group_links_controller.rb +++ b/app/controllers/groups/group_links_controller.rb @@ -24,7 +24,7 @@ class Groups::GroupLinksController < Groups::ApplicationController end def update - @group_link.update(group_link_params) + Groups::GroupLinks::UpdateService.new(@group_link).execute(group_link_params) end def destroy diff --git a/app/graphql/types/diff_refs_type.rb b/app/graphql/types/diff_refs_type.rb index 03d080d784b..4049a204f66 100644 --- a/app/graphql/types/diff_refs_type.rb +++ b/app/graphql/types/diff_refs_type.rb @@ -8,7 +8,7 @@ module Types field :head_sha, GraphQL::STRING_TYPE, null: false, description: 'SHA of the HEAD at the time the comment was made' - field :base_sha, GraphQL::STRING_TYPE, null: false, + field :base_sha, GraphQL::STRING_TYPE, null: true, description: 'Merge base of the branch the comment was made on' field :start_sha, GraphQL::STRING_TYPE, null: false, description: 'SHA of the branch being compared against' diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index f9d7e8a2b7a..481e1807a78 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -6,6 +6,9 @@ class ApplicationSetting < ApplicationRecord include TokenAuthenticatable include ChronicDurationAttribute + GRAFANA_URL_ERROR_MESSAGE = 'Please check your Grafana URL setting in ' \ + 'Admin Area > Settings > Metrics and profiling > Metrics - Grafana' + add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required } add_authentication_token_field :health_check_access_token add_authentication_token_field :static_objects_external_storage_auth_token @@ -38,6 +41,14 @@ class ApplicationSetting < ApplicationRecord chronic_duration_attr_writer :archive_builds_in_human_readable, :archive_builds_in_seconds + validates :grafana_url, + system_hook_url: { + blocked_message: "is blocked: %{exception_message}. " + GRAFANA_URL_ERROR_MESSAGE + }, + if: :grafana_url_absolute? + + validate :validate_grafana_url + validates :uuid, presence: true validates :outbound_local_requests_whitelist, @@ -362,6 +373,19 @@ class ApplicationSetting < ApplicationRecord end after_commit :expire_performance_bar_allowed_user_ids_cache, if: -> { previous_changes.key?('performance_bar_allowed_group_id') } + def validate_grafana_url + unless parsed_grafana_url + self.errors.add( + :grafana_url, + "must be a valid relative or absolute URL. #{GRAFANA_URL_ERROR_MESSAGE}" + ) + end + end + + def grafana_url_absolute? + parsed_grafana_url&.absolute? + end + def sourcegraph_url_is_com? !!(sourcegraph_url =~ /\Ahttps:\/\/(www\.)?sourcegraph\.com/) end @@ -394,6 +418,12 @@ class ApplicationSetting < ApplicationRecord rescue RegexpError errors.add(:email_restrictions, _('is not a valid regular expression')) end + + private + + def parsed_grafana_url + @parsed_grafana_url ||= Gitlab::Utils.parse_url(grafana_url) + end end ApplicationSetting.prepend_if_ee('EE::ApplicationSetting') diff --git a/app/models/badge.rb b/app/models/badge.rb index eb351425e66..3400d6d407d 100644 --- a/app/models/badge.rb +++ b/app/models/badge.rb @@ -32,7 +32,9 @@ class Badge < ApplicationRecord end def rendered_image_url(project = nil) - build_rendered_url(image_url, project) + Gitlab::AssetProxy.proxy_url( + build_rendered_url(image_url, project) + ) end private diff --git a/app/models/group.rb b/app/models/group.rb index d6a4af5af15..e9b3e3c3369 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -516,18 +516,29 @@ class Group < Namespace group_group_links_query = GroupGroupLink.where(shared_group_id: self_and_ancestors_ids) cte = Gitlab::SQL::CTE.new(:group_group_links_cte, group_group_links_query) + cte_alias = cte.table.alias(GroupGroupLink.table_name) link = GroupGroupLink .with(cte.to_arel) + .select(smallest_value_arel([cte_alias[:group_access], group_member_table[:access_level]], + 'group_access')) .from([group_member_table, cte.alias_to(group_group_link_table)]) .where(group_member_table[:user_id].eq(user.id)) + .where(group_member_table[:requested_at].eq(nil)) .where(group_member_table[:source_id].eq(group_group_link_table[:shared_with_group_id])) + .where(group_member_table[:source_type].eq('Namespace')) .reorder(Arel::Nodes::Descending.new(group_group_link_table[:group_access])) .first link&.group_access end + def smallest_value_arel(args, column_alias) + Arel::Nodes::As.new( + Arel::Nodes::NamedFunction.new('LEAST', args), + Arel::Nodes::SqlLiteral.new(column_alias)) + end + def self.groups_including_descendants_by(group_ids) Gitlab::ObjectHierarchy .new(Group.where(id: group_ids)) diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index bdff9e28df1..bc3be67bd32 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -66,6 +66,7 @@ class GroupMember < Member def after_accept_invite notification_service.accept_group_invite(self) + update_two_factor_requirement super end diff --git a/app/models/user_detail.rb b/app/models/user_detail.rb index 1621f336111..5dc74421705 100644 --- a/app/models/user_detail.rb +++ b/app/models/user_detail.rb @@ -3,5 +3,5 @@ class UserDetail < ApplicationRecord belongs_to :user - validates :job_title, presence: true, length: { maximum: 200 } + validates :job_title, length: { maximum: 200 } end diff --git a/app/presenters/ci/pipeline_presenter.rb b/app/presenters/ci/pipeline_presenter.rb index 37abefb5664..ce9a3346b4b 100644 --- a/app/presenters/ci/pipeline_presenter.rb +++ b/app/presenters/ci/pipeline_presenter.rb @@ -134,7 +134,11 @@ module Ci def all_related_merge_requests strong_memoize(:all_related_merge_requests) do - pipeline.ref ? pipeline.all_merge_requests_by_recency.to_a : [] + if pipeline.ref && can?(current_user, :read_merge_request, pipeline.project) + pipeline.all_merge_requests_by_recency.to_a + else + [] + end end end end diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index 09a84950755..629c1cbdc5c 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -3,12 +3,24 @@ module Auth class ContainerRegistryAuthenticationService < BaseService AUDIENCE = 'container_registry' + REGISTRY_LOGIN_ABILITIES = [ + :read_container_image, + :create_container_image, + :destroy_container_image, + :update_container_image, + :admin_container_image, + :build_read_container_image, + :build_create_container_image, + :build_destroy_container_image + ].freeze def execute(authentication_abilities:) @authentication_abilities = authentication_abilities return error('UNAVAILABLE', status: 404, message: 'registry not enabled') unless registry.enabled + return error('DENIED', status: 403, message: 'access forbidden') unless has_registry_ability? + unless scopes.any? || current_user || project return error('DENIED', status: 403, message: 'access forbidden') end @@ -197,5 +209,11 @@ module Auth def has_authentication_ability?(capability) @authentication_abilities.to_a.include?(capability) end + + def has_registry_ability? + @authentication_abilities.any? do |ability| + REGISTRY_LOGIN_ABILITIES.include?(ability) + end + end end end diff --git a/app/services/groups/group_links/destroy_service.rb b/app/services/groups/group_links/destroy_service.rb index 29aa8de4e68..6835b6c4637 100644 --- a/app/services/groups/group_links/destroy_service.rb +++ b/app/services/groups/group_links/destroy_service.rb @@ -6,19 +6,17 @@ module Groups def execute(one_or_more_links) links = Array(one_or_more_links) - GroupGroupLink.transaction do - GroupGroupLink.delete(links) + if GroupGroupLink.delete(links) + Gitlab::AppLogger.info( + "GroupGroupLinks with ids: #{links.map(&:id)} have been deleted.") groups_to_refresh = links.map(&:shared_with_group) groups_to_refresh.uniq.each do |group| group.refresh_members_authorized_projects end - - Gitlab::AppLogger.info("GroupGroupLinks with ids: #{links.map(&:id)} have been deleted.") - rescue => ex - Gitlab::AppLogger.error(ex) - - raise + else + Gitlab::AppLogger.info( + "Failed to delete GroupGroupLinks with ids: #{links.map(&:id)}.") end end end diff --git a/app/services/groups/group_links/update_service.rb b/app/services/groups/group_links/update_service.rb new file mode 100644 index 00000000000..71b52cb616c --- /dev/null +++ b/app/services/groups/group_links/update_service.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Groups + module GroupLinks + class UpdateService < BaseService + def initialize(group_link, user = nil) + super(group_link.shared_group, user) + + @group_link = group_link + end + + def execute(group_link_params) + group_link.update!(group_link_params) + + if requires_authorization_refresh?(group_link_params) + group_link.shared_with_group.refresh_members_authorized_projects + end + end + + private + + attr_accessor :group_link + + def requires_authorization_refresh?(params) + params.include?(:group_access) + end + end + end +end diff --git a/app/services/projects/lfs_pointers/lfs_download_service.rb b/app/services/projects/lfs_pointers/lfs_download_service.rb index bd70012c76c..52c73bcff03 100644 --- a/app/services/projects/lfs_pointers/lfs_download_service.rb +++ b/app/services/projects/lfs_pointers/lfs_download_service.rb @@ -16,17 +16,14 @@ module Projects @lfs_download_object = lfs_download_object end - # rubocop: disable CodeReuse/ActiveRecord def execute return unless project&.lfs_enabled? && lfs_download_object return error("LFS file with oid #{lfs_oid} has invalid attributes") unless lfs_download_object.valid? - return if LfsObject.exists?(oid: lfs_oid) wrap_download_errors do download_lfs_file! end end - # rubocop: enable CodeReuse/ActiveRecord private @@ -39,14 +36,24 @@ module Projects def download_lfs_file! with_tmp_file do |tmp_file| download_and_save_file!(tmp_file) - project.lfs_objects << LfsObject.new(oid: lfs_oid, - size: lfs_size, - file: tmp_file) + + project.lfs_objects << find_or_create_lfs_object(tmp_file) success end end + def find_or_create_lfs_object(tmp_file) + lfs_obj = LfsObject.safe_find_or_create_by!( + oid: lfs_oid, + size: lfs_size + ) + + lfs_obj.update!(file: tmp_file) unless lfs_obj.file.file + + lfs_obj + end + def download_and_save_file!(file) digester = Digest::SHA256.new response = Gitlab::HTTP.get(lfs_sanitized_url, download_headers) do |fragment| diff --git a/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb b/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb index d6e6480bdad..75106297043 100644 --- a/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb +++ b/app/services/projects/lfs_pointers/lfs_object_download_list_service.rb @@ -26,12 +26,12 @@ module Projects return [] end - # Getting all Lfs pointers already in the database and linking them to the project - linked_oids = LfsLinkService.new(project).execute(lfs_pointers_in_repository.keys) - # Retrieving those oids not present in the database which we need to download - missing_oids = lfs_pointers_in_repository.except(*linked_oids) - # Downloading the required information and gathering it inside a LfsDownloadObject for each oid - LfsDownloadLinkListService.new(project, remote_uri: current_endpoint_uri).execute(missing_oids) + # Downloading the required information and gathering it inside an + # LfsDownloadObject for each oid + # + LfsDownloadLinkListService + .new(project, remote_uri: current_endpoint_uri) + .execute(lfs_pointers_in_repository) rescue LfsDownloadLinkListService::DownloadLinksError => e raise LfsObjectDownloadListError, "The LFS objects download list couldn't be imported. Error: #{e.message}" end diff --git a/app/services/web_hook_service.rb b/app/services/web_hook_service.rb index 514ba998d2c..178a321e20c 100644 --- a/app/services/web_hook_service.rb +++ b/app/services/web_hook_service.rb @@ -13,8 +13,14 @@ class WebHookService end end + GITLAB_EVENT_HEADER = 'X-Gitlab-Event' + attr_accessor :hook, :data, :hook_name, :request_options + def self.hook_to_event(hook_name) + hook_name.to_s.singularize.titleize + end + def initialize(hook, data, hook_name) @hook = hook @data = data @@ -112,7 +118,7 @@ class WebHookService @headers ||= begin { 'Content-Type' => 'application/json', - 'X-Gitlab-Event' => hook_name.singularize.titleize + GITLAB_EVENT_HEADER => self.class.hook_to_event(hook_name) }.tap do |hash| hash['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present? end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 0fc71d2e3f3..505b51c2006 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -16,6 +16,9 @@ class FileUploader < GitlabUploader MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}.freeze DYNAMIC_PATH_PATTERN = %r{.*(?<secret>\h{32})/(?<identifier>.*)}.freeze + VALID_SECRET_PATTERN = %r{\A\h{10,32}\z}.freeze + + InvalidSecret = Class.new(StandardError) after :remove, :prune_store_dir @@ -153,6 +156,10 @@ class FileUploader < GitlabUploader def secret @secret ||= self.class.generate_secret + + raise InvalidSecret unless @secret =~ VALID_SECRET_PATTERN + + @secret end # return a new uploader with a file copy on another project diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb index 300bd01ed22..99f503c3f06 100644 --- a/app/validators/addressable_url_validator.rb +++ b/app/validators/addressable_url_validator.rb @@ -23,7 +23,8 @@ # protect against Server-side Request Forgery (SSRF), or check for the right port. # # Configuration options: -# * <tt>message</tt> - A custom error message (default is: "must be a valid URL"). +# * <tt>message</tt> - A custom error message, used when the URL is blank. (default is: "must be a valid URL"). +# * <tt>blocked_message</tt> - A custom error message, used when the URL is blocked. Default: +'is blocked: %{exception_message}'+. # * <tt>schemes</tt> - Array of URI schemes. Default: +['http', 'https']+ # * <tt>allow_localhost</tt> - Allow urls pointing to +localhost+. Default: +true+ # * <tt>allow_local_network</tt> - Allow urls pointing to private network addresses. Default: +true+ @@ -59,7 +60,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator }.freeze DEFAULT_OPTIONS = BLOCKER_VALIDATE_OPTIONS.merge({ - message: 'must be a valid URL' + message: 'must be a valid URL', + blocked_message: 'is blocked: %{exception_message}' }).freeze def initialize(options) @@ -80,7 +82,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator Gitlab::UrlBlocker.validate!(value, blocker_args) rescue Gitlab::UrlBlocker::BlockedUrlError => e - record.errors.add(attribute, "is blocked: #{e.message}") + record.errors.add(attribute, options.fetch(:blocked_message) % { exception_message: e.message }) end private diff --git a/app/views/admin/application_settings/_grafana.html.haml b/app/views/admin/application_settings/_grafana.html.haml index b6e02bde895..700be7db54f 100644 --- a/app/views/admin/application_settings/_grafana.html.haml +++ b/app/views/admin/application_settings/_grafana.html.haml @@ -1,4 +1,4 @@ -= form_for @application_setting, url: admin_application_settings_path(anchor: 'js-grafana-settings'), html: { class: 'fieldset-form' } do |f| += form_for @application_setting, url: metrics_and_profiling_admin_application_settings_path(anchor: 'js-grafana-settings'), html: { class: 'fieldset-form' } do |f| = form_errors(@application_setting) %fieldset diff --git a/app/views/clusters/clusters/_form.html.haml b/app/views/clusters/clusters/_form.html.haml index a85b005b2b4..40fc7d110fc 100644 --- a/app/views/clusters/clusters/_form.html.haml +++ b/app/views/clusters/clusters/_form.html.haml @@ -3,7 +3,7 @@ .form-group %h5= s_('ClusterIntegration|Integration status') %label.append-bottom-0.js-cluster-enable-toggle-area - = render "shared/buttons/project_feature_toggle", is_checked: @cluster.enabled?, label: s_("ClusterIntegration|Toggle Kubernetes cluster"), disabled: !can?(current_user, :update_cluster, @cluster) do + = render "shared/buttons/project_feature_toggle", is_checked: @cluster.enabled?, label: s_("ClusterIntegration|Toggle Kubernetes cluster"), disabled: !can?(current_user, :update_cluster, @cluster), data: { qa_selector: 'integration_status_toggle' } do = field.hidden_field :enabled, { class: 'js-project-feature-toggle-input'} .form-text.text-muted= s_('ClusterIntegration|Enable or disable GitLab\'s connection to your Kubernetes cluster.') @@ -22,7 +22,7 @@ .form-group %h5= s_('ClusterIntegration|Base domain') - = field.text_field :base_domain, class: 'col-md-6 form-control js-select-on-focus qa-base-domain' + = field.text_field :base_domain, class: 'col-md-6 form-control js-select-on-focus', data: { qa_selector: 'base_domain_field' } .form-text.text-muted - auto_devops_url = help_page_path('topics/autodevops/index') - auto_devops_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: auto_devops_url } @@ -37,4 +37,4 @@ - if can?(current_user, :update_cluster, @cluster) .form-group - = field.submit _('Save changes'), class: 'btn btn-success qa-save-domain' + = field.submit _('Save changes'), class: 'btn btn-success', data: { qa_selector: 'save_changes_button' } diff --git a/app/views/clusters/clusters/user/_form.html.haml b/app/views/clusters/clusters/user/_form.html.haml index 39b6d74d9f9..ce226d29113 100644 --- a/app/views/clusters/clusters/user/_form.html.haml +++ b/app/views/clusters/clusters/user/_form.html.haml @@ -54,4 +54,4 @@ = render('clusters/clusters/namespace', platform_field: platform_kubernetes_field) .form-group - = field.submit s_('ClusterIntegration|Add Kubernetes cluster'), class: 'btn btn-success' + = field.submit s_('ClusterIntegration|Add Kubernetes cluster'), class: 'btn btn-success', data: { qa_selector: 'add_kubernetes_cluster_button' } diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml index 9f70124ba0d..78cd3f62dec 100644 --- a/app/views/layouts/nav/sidebar/_admin.html.haml +++ b/app/views/layouts/nav/sidebar/_admin.html.haml @@ -83,7 +83,7 @@ = _('Requests Profiles') - if Gitlab::CurrentSettings.current_application_settings.grafana_enabled? = nav_link do - = link_to Gitlab::CurrentSettings.current_application_settings.grafana_url, target: '_blank', title: _('Metrics Dashboard') do + = link_to Gitlab::CurrentSettings.current_application_settings.grafana_url, target: '_blank', title: _('Metrics Dashboard'), rel: 'noopener noreferrer' do %span = _('Metrics Dashboard') = render_if_exists 'layouts/nav/ee/admin/new_monitoring_sidebar' diff --git a/app/views/shared/issuable/form/_branch_chooser.html.haml b/app/views/shared/issuable/form/_branch_chooser.html.haml index 29ac17c43b9..8d9e5ddf065 100644 --- a/app/views/shared/issuable/form/_branch_chooser.html.haml +++ b/app/views/shared/issuable/form/_branch_chooser.html.haml @@ -8,7 +8,9 @@ .form-group.row.d-flex.gl-pl-3.gl-pr-3.branch-selector .align-self-center - %span= s_('From %{source_title} into').html_safe % { source_title: "<code>#{source_title}</code>".html_safe } + %span + = _('From <code>%{source_title}</code> into').html_safe % { source_title: source_title } + - if issuable.new_record? %code= target_title diff --git a/changelogs/unreleased/208675-add-package_name-as-option-to-packages-api.yml b/changelogs/unreleased/208675-add-package_name-as-option-to-packages-api.yml new file mode 100644 index 00000000000..3f7a0e3f62d --- /dev/null +++ b/changelogs/unreleased/208675-add-package_name-as-option-to-packages-api.yml @@ -0,0 +1,5 @@ +--- +title: Added package_name as filter parameter to packages API +merge_request: 26291 +author: +type: added diff --git a/changelogs/unreleased/georgekoltsov-27883-fix-import-pipeline-order.yml b/changelogs/unreleased/georgekoltsov-27883-fix-import-pipeline-order.yml new file mode 100644 index 00000000000..1ccabf0af1a --- /dev/null +++ b/changelogs/unreleased/georgekoltsov-27883-fix-import-pipeline-order.yml @@ -0,0 +1,5 @@ +--- +title: Fix reversed pipeline order on Project Import +merge_request: 26390 +author: +type: fixed diff --git a/config/initializers/8_devise.rb b/config/initializers/8_devise.rb index e1c37caaafd..6ed56598e15 100644 --- a/config/initializers/8_devise.rb +++ b/config/initializers/8_devise.rb @@ -80,8 +80,16 @@ Devise.setup do |config| # When allow_unconfirmed_access_for is zero, the user won't be able to sign in without confirming. # You can use this to let your user access some features of your application # without confirming the account, but blocking it after a certain period - # (ie 2 days). - config.allow_unconfirmed_access_for = 30.days + # (e.g. 3 days). + config.allow_unconfirmed_access_for = 3.days + + # A period that the user is allowed to confirm their account before their + # token becomes invalid. For example, if set to 1.day, the user can confirm + # their account within 1 days after the mail was sent, but on the second day + # their account can't be confirmed with the token any more. + # Default is nil, meaning there is no restriction on how long a user can take + # before confirming their account. + config.confirm_within = 1.day # Defines which key will be used when confirming an account # config.confirmation_keys = [ :email ] diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index 22536b60ca1..88e198116a0 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -36,15 +36,6 @@ use_sidekiq_legacy_memory_killer = !use_sidekiq_daemon_memory_killer use_request_store = ENV.fetch('SIDEKIQ_REQUEST_STORE', 1).to_i.nonzero? Sidekiq.configure_server do |config| - config.redis = queues_config_hash - - config.server_middleware(&Gitlab::SidekiqMiddleware.server_configurator({ - metrics: Settings.monitoring.sidekiq_exporter, - arguments_logger: ENV['SIDEKIQ_LOG_ARGUMENTS'] && !enable_json_logs, - memory_killer: enable_sidekiq_memory_killer && use_sidekiq_legacy_memory_killer, - request_store: use_request_store - })) - if enable_json_logs Sidekiq.logger.formatter = Gitlab::SidekiqLogging::JSONFormatter.new config.options[:job_logger] = Gitlab::SidekiqLogging::StructuredLogger @@ -54,6 +45,15 @@ Sidekiq.configure_server do |config| config.error_handlers << Gitlab::SidekiqLogging::ExceptionHandler.new end + config.redis = queues_config_hash + + config.server_middleware(&Gitlab::SidekiqMiddleware.server_configurator({ + metrics: Settings.monitoring.sidekiq_exporter, + arguments_logger: ENV['SIDEKIQ_LOG_ARGUMENTS'] && !enable_json_logs, + memory_killer: enable_sidekiq_memory_killer && use_sidekiq_legacy_memory_killer, + request_store: use_request_store + })) + config.client_middleware(&Gitlab::SidekiqMiddleware.client_configurator) config.on :startup do diff --git a/db/migrate/20200214085940_clean_grafana_url.rb b/db/migrate/20200214085940_clean_grafana_url.rb new file mode 100644 index 00000000000..7dff6532516 --- /dev/null +++ b/db/migrate/20200214085940_clean_grafana_url.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class CleanGrafanaUrl < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def up + execute( + <<-SQL + UPDATE + application_settings + SET + grafana_url = default + WHERE + position('javascript:' IN btrim(application_settings.grafana_url)) = 1 + SQL + ) + end + + def down + # no-op + end +end diff --git a/db/post_migrate/20200204113224_schedule_recalculate_project_authorizations_second_run.rb b/db/post_migrate/20200204113224_schedule_recalculate_project_authorizations_second_run.rb new file mode 100644 index 00000000000..8f4a347b5e2 --- /dev/null +++ b/db/post_migrate/20200204113224_schedule_recalculate_project_authorizations_second_run.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class ScheduleRecalculateProjectAuthorizationsSecondRun < ActiveRecord::Migration[5.1] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + MIGRATION = 'RecalculateProjectAuthorizationsWithMinMaxUserId' + BATCH_SIZE = 2_500 + DELAY_INTERVAL = 2.minutes.to_i + + disable_ddl_transaction! + + class User < ActiveRecord::Base + include ::EachBatch + + self.table_name = 'users' + end + + def up + say "Scheduling #{MIGRATION} jobs" + + User.each_batch(of: BATCH_SIZE) do |batch, index| + delay = index * DELAY_INTERVAL + range = batch.pluck('MIN(id)', 'MAX(id)').first + BackgroundMigrationWorker.perform_in(delay, MIGRATION, range) + end + end + + def down + end +end diff --git a/doc/administration/high_availability/README.md b/doc/administration/high_availability/README.md index 4f9e3b1a180..c8783b8ff5f 100644 --- a/doc/administration/high_availability/README.md +++ b/doc/administration/high_availability/README.md @@ -76,7 +76,7 @@ On different cloud vendors a best effort like for like can be used. - **Known Issues:** For the latest list of known performance issues head [here](https://gitlab.com/gitlab-org/gitlab/issues?label_name%5B%5D=Quality%3Aperformance-issues). -| Service | Nodes | Configuration | GCP type | +| Service | Nodes | Configuration[^8] | GCP type | | ----------------------------|-------|-----------------------|---------------| | GitLab Rails[^1] | 3 | 8 vCPU, 7.2GB Memory | n1-highcpu-8 | | PostgreSQL | 3 | 2 vCPU, 7.5GB Memory | n1-standard-2 | @@ -98,7 +98,7 @@ On different cloud vendors a best effort like for like can be used. - **Known Issues:** For the latest list of known performance issues head [here](https://gitlab.com/gitlab-org/gitlab/issues?label_name%5B%5D=Quality%3Aperformance-issues). -| Service | Nodes | Configuration | GCP type | +| Service | Nodes | Configuration[^8] | GCP type | | ----------------------------|-------|-----------------------|---------------| | GitLab Rails[^1] | 3 | 16 vCPU, 14.4GB Memory | n1-highcpu-16 | | PostgreSQL | 3 | 2 vCPU, 7.5GB Memory | n1-standard-2 | @@ -120,7 +120,7 @@ On different cloud vendors a best effort like for like can be used. - **Known Issues:** For the latest list of known performance issues head [here](https://gitlab.com/gitlab-org/gitlab/issues?label_name%5B%5D=Quality%3Aperformance-issues). -| Service | Nodes | Configuration | GCP type | +| Service | Nodes | Configuration[^8] | GCP type | | ----------------------------|-------|-----------------------|---------------| | GitLab Rails[^1] | 3 | 32 vCPU, 28.8GB Memory | n1-highcpu-32 | | PostgreSQL | 3 | 4 vCPU, 15GB Memory | n1-standard-4 | @@ -145,9 +145,9 @@ On different cloud vendors a best effort like for like can be used. - **Known Issues:** For the latest list of known performance issues head [here](https://gitlab.com/gitlab-org/gitlab/issues?label_name%5B%5D=Quality%3Aperformance-issues). -| Service | Nodes | Configuration | GCP type | +| Service | Nodes | Configuration[^8] | GCP type | | ----------------------------|-------|-----------------------|---------------| -| GitLab Rails[^1] | 7 | 32 vCPU, 28.8GB Memory | n1-highcpu-32 | +| GitLab Rails[^1] | 5 | 32 vCPU, 28.8GB Memory | n1-highcpu-32 | | PostgreSQL | 3 | 8 vCPU, 30GB Memory | n1-standard-8 | | PgBouncer | 3 | 2 vCPU, 1.8GB Memory | n1-highcpu-2 | | Gitaly[^2] [^5] [^7] | X | 32 vCPU, 120GB Memory | n1-standard-32 | @@ -170,9 +170,9 @@ On different cloud vendors a best effort like for like can be used. - **Known Issues:** For the latest list of known performance issues head [here](https://gitlab.com/gitlab-org/gitlab/issues?label_name%5B%5D=Quality%3Aperformance-issues). -| Service | Nodes | Configuration | GCP type | +| Service | Nodes | Configuration[^8] | GCP type | | ----------------------------|-------|-----------------------|---------------| -| GitLab Rails[^1] | 15 | 32 vCPU, 28.8GB Memory | n1-highcpu-32 | +| GitLab Rails[^1] | 12 | 32 vCPU, 28.8GB Memory | n1-highcpu-32 | | PostgreSQL | 3 | 16 vCPU, 60GB Memory | n1-standard-16 | | PgBouncer | 3 | 2 vCPU, 1.8GB Memory | n1-highcpu-2 | | Gitaly[^2] [^5] [^7] | X | 64 vCPU, 240GB Memory | n1-standard-64 | @@ -230,3 +230,9 @@ On different cloud vendors a best effort like for like can be used. as with time they may be adjusted higher or lower depending on the scale of your environment's workload. If you're running the environment on a Cloud provider you may need to refer to their documentation on how configure IOPS correctly. + +[^8]: The architectures were built and tested with the [Intel Xeon E5 v3 (Haswell)](https://cloud.google.com/compute/docs/cpu-platforms) + CPU platform on GCP. On different hardware you may find that adjustments, either lower + or higher, are required for your CPU or Node counts accordingly. For more info a + [Sysbench](https://github.com/akopytov/sysbench) benchmark of the CPU can be found + [here](https://gitlab.com/gitlab-org/quality/performance/-/wikis/Reference-Architectures/GCP-CPU-Benchmarks). diff --git a/doc/administration/monitoring/performance/grafana_configuration.md b/doc/administration/monitoring/performance/grafana_configuration.md index 12b69d10179..61b570aa42e 100644 --- a/doc/administration/monitoring/performance/grafana_configuration.md +++ b/doc/administration/monitoring/performance/grafana_configuration.md @@ -117,8 +117,9 @@ If you have set up Grafana, you can enable a link to access it easily from the s 1. Expand **Metrics - Grafana**. 1. Check the "Enable access to Grafana" checkbox. 1. If Grafana is enabled through Omnibus GitLab and on the same server, - leave "Grafana URL" unchanged. In any other case, enter the full URL - path of the Grafana instance. + leave **Grafana URL** unchanged. It should be `/-/grafana`. + + In any other case, enter the full URL of the Grafana instance. 1. Click **Save changes**. 1. The new link will be available in the **Admin Area > Monitoring > Metrics Dashboard**. diff --git a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md index d1aefb3219c..16eb3269ef0 100644 --- a/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md +++ b/doc/administration/troubleshooting/gitlab_rails_cheat_sheet.md @@ -262,14 +262,6 @@ p.each do |project| end ``` -### Identify un-indexed projects - -```ruby -Project.find_each do |project| - puts "id #{project.id}: #{project.namespace.name.to_s}/#{project.name.to_s}" if project.index_status.nil? -end -``` - ## Wikis ### Recreate @@ -761,12 +753,6 @@ Ci::Pipeline.where(project_id: p.id).where(status: 'pending').each {|p| p.cancel Ci::Pipeline.where(project_id: p.id).where(status: 'pending').count ``` -### Manually modify runner minutes - -```ruby -Namespace.find_by_full_path("user/proj").namespace_statistics.update(shared_runners_seconds: 27360) -``` - ### Remove artifacts more than a week old The Latest version of these steps can be found in the [job artifacts documentation](../job_artifacts.md) @@ -806,21 +792,6 @@ build.dependencies.each do |d| { puts "status: #{d.status}, finished at: #{d.fin completed: #{d.complete?}, artifacts_expired: #{d.artifacts_expired?}, erased: #{d.erased?}" } ``` -### Disable strict artifact checking (Introduced in GitLab 10.3.0) - -See [job artifacts documentation](../job_artifacts.md#validation-for-dependencies). - -```ruby -Feature.enable('ci_disable_validates_dependencies') -``` - -### Remove CI traces older than 6 months - -```ruby -current_user = User.find_by_email('cindy@gitlap.com') -Ci::Build.where("finished_at < ?", 6.months.ago.to_date).each {|b| puts b.id; b.erase(erased_by: current_user) if b.erasable?};nil -``` - ### Try CI service ```ruby @@ -965,12 +936,6 @@ end ## Sidekiq -### Size of a queue - -```ruby -Sidekiq::Queue.new('background_migration').size -``` - ### Kill a worker's Sidekiq jobs ```ruby @@ -1017,12 +982,6 @@ See <https://github.com/mperham/sidekiq/wiki/Signals#ttin>. /opt/gitlab/embedded/bin/redis-cli -s /var/opt/gitlab/redis/redis.socket ``` -### Connect to Redis (HA) - -```shell -/opt/gitlab/embedded/bin/redis-cli -h <host ip> -a <password> -``` - ## LFS ### Get info about LFS objects and associated project diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 7dda073d8ae..336c21ef6a1 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -1665,7 +1665,7 @@ type DiffRefs { """ Merge base of the branch the comment was made on """ - baseSha: String! + baseSha: String """ SHA of the HEAD at the time the comment was made diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 83b098dc73d..4d52f45d3e4 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -8546,13 +8546,9 @@ ], "type": { - "kind": "NON_NULL", - "name": null, - "ofType": { - "kind": "SCALAR", - "name": "String", - "ofType": null - } + "kind": "SCALAR", + "name": "String", + "ofType": null }, "isDeprecated": false, "deprecationReason": null diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 79be0c0d626..923cc7dff3d 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -286,7 +286,7 @@ Autogenerated return type of DestroySnippet | Name | Type | Description | | --- | ---- | ---------- | -| `baseSha` | String! | Merge base of the branch the comment was made on | +| `baseSha` | String | Merge base of the branch the comment was made on | | `headSha` | String! | SHA of the HEAD at the time the comment was made | | `startSha` | String! | SHA of the branch being compared against | diff --git a/doc/api/packages.md b/doc/api/packages.md index 097f9dff84e..b1b6969769c 100644 --- a/doc/api/packages.md +++ b/doc/api/packages.md @@ -21,6 +21,7 @@ GET /projects/:id/packages | `order_by`| string | no | The field to use as order. One of `created_at` (default), `name`, `version`, or `type`. | | `sort` | string | no | The direction of the order, either `asc` (default) for ascending order or `desc` for descending order. | | `package_type` | string | no | Filter the returned packages by type. One of `conan`, `maven`, `npm` or `nuget`. (_Introduced in GitLab 12.9_) +| `package_name` | string | no | Filter the project packages with a fuzzy search by name. (_Introduced in GitLab 12.9_) ```shell curl --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/:id/packages diff --git a/doc/development/testing_guide/review_apps.md b/doc/development/testing_guide/review_apps.md index 3307f29a98e..b8df3286da2 100644 --- a/doc/development/testing_guide/review_apps.md +++ b/doc/development/testing_guide/review_apps.md @@ -79,27 +79,38 @@ subgraph "CNG-mirror pipeline" **Additional notes:** - If the `review-deploy` job keep failing (note that we already retry it twice), - please post a message in the `#quality` channel and/or create a ~Quality ~bug + please post a message in the `#g_qe_engineering_productivity` channel and/or create a `~"Engineering Productivity"` `~"ep::review apps"` `~bug` issue with a link to your merge request. Note that the deployment failure can reveal an actual problem introduced in your merge request (i.e. this isn't necessarily a transient failure)! -- If the `review-qa-smoke` job keep failing (note that we already retry it twice), +- If the `review-qa-smoke` job keeps failing (note that we already retry it twice), please check the job's logs: you could discover an actual problem introduced in your merge request. You can also download the artifacts to see screenshots of the page at the time the failures occurred. If you don't find the cause of the failure or if it seems unrelated to your change, please post a message in the `#quality` channel and/or create a ~Quality ~bug issue with a link to your merge request. -- The manual [`review-stop`][gitlab-ci-yml] in the `test` stage can be used to +- The manual `review-stop` can be used to stop a Review App manually, and is also started by GitLab once a merge request's branch is deleted after being merged. -- Review Apps are cleaned up regularly via a pipeline schedule that runs - the [`schedule:review-cleanup`][gitlab-ci-yml] job. - The Kubernetes cluster is connected to the `gitlab-{ce,ee}` projects using [GitLab's Kubernetes integration][gitlab-k8s-integration]. This basically allows to have a link to the Review App directly from the merge request widget. +### Auto-stopping of Review Apps + +Review Apps are automatically stopped 2 days after the last deployment thanks to +the [Environment auto-stop](../../ci/environments.html#environments-auto-stop) feature. + +If you need your Review App to stay up for a longer time, you can +[pin its environment](../../ci/environments.html#auto-stop-example). + +The `review-cleanup` job that automatically runs in scheduled +pipelines (and is manual in merge request) stops stale Review Apps after 5 days, +deletes their environment after 6 days, and cleans up any dangling Helm releases +and Kubernetes resources after 7 days. + ## QA runs On every [pipeline][gitlab-pipeline] in the `qa` stage (which comes after the @@ -206,7 +217,7 @@ aids in identifying load spikes on the cluster, and if nodes are problematic or **Potential cause:** -That could be a sign that the [`schedule:review-cleanup`][gitlab-ci-yml] job is +That could be a sign that the `review-cleanup` job is failing to cleanup stale Review Apps and Kubernetes resources. **Where to look for further debugging:** @@ -270,7 +281,7 @@ kubectl get cm --sort-by='{.metadata.creationTimestamp}' | grep 'review-' | grep ### Using K9s -[K9s] is a powerful command line dashboard which allows you to filter by labels. This can help identify trends with apps exceeding the [review-app resource requests](https://gitlab.com/gitlab-org/gitlab/blob/master/scripts/review_apps/base-config.yaml). Kubernetes will schedule pods to nodes based on resource requests and allow for CPU usage up to the limits. +[K9s] is a powerful command line dashboard which allows you to filter by labels. This can help identify trends with apps exceeding the [review-app resource requests](https://gitlab.com/gitlab-org/gitlab/-/blob/master/scripts/review_apps/base-config.yaml). Kubernetes will schedule pods to nodes based on resource requests and allow for CPU usage up to the limits. - In K9s you can sort or add filters by typing the `/` character - `-lrelease=<review-app-slug>` - filters down to all pods for a release. This aids in determining what is having issues in a single deployment @@ -387,13 +398,11 @@ find a way to limit it to only us.** [helm-chart]: https://gitlab.com/gitlab-org/charts/gitlab/ [review-apps-ce]: https://console.cloud.google.com/kubernetes/clusters/details/us-central1-a/review-apps-ce?project=gitlab-review-apps [review-apps-ee]: https://console.cloud.google.com/kubernetes/clusters/details/us-central1-b/review-apps-ee?project=gitlab-review-apps -[review-apps.sh]: https://gitlab.com/gitlab-org/gitlab/blob/master/scripts/review_apps/review-apps.sh -[automated_cleanup.rb]: https://gitlab.com/gitlab-org/gitlab/blob/master/scripts/review_apps/automated_cleanup.rb -[Auto-DevOps.gitlab-ci.yml]: https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml -[gitlab-ci-yml]: https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab-ci.yml +[review-apps.sh]: https://gitlab.com/gitlab-org/gitlab/-/blob/master/scripts/review_apps/review-apps.sh +[automated_cleanup.rb]: https://gitlab.com/gitlab-org/gitlab/-/blob/master/scripts/review_apps/automated_cleanup.rb +[Auto-DevOps.gitlab-ci.yml]: https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Auto-DevOps.gitlab-ci.yml [gitlab-k8s-integration]: ../../user/project/clusters/index.md [K9s]: https://github.com/derailed/k9s -[password-bug]: https://gitlab.com/gitlab-org/gitlab-foss/issues/53621 --- diff --git a/doc/user/project/merge_requests/code_quality.md b/doc/user/project/merge_requests/code_quality.md index 91cdddc9300..b2b28c54350 100644 --- a/doc/user/project/merge_requests/code_quality.md +++ b/doc/user/project/merge_requests/code_quality.md @@ -260,11 +260,16 @@ GitLab. ## Code Quality reports -Once the Code Quality job has completed, GitLab: +Once the Code Quality job has completed: -- Checks the generated report. -- Compares the metrics between the source and target branches. -- Shows the information right on the merge request. +- The full list of code quality violations generated by a pipeline is available in the + Code Quality tab of the Pipeline Details page. +- Potential changes to code quality are shown directly in the merge request. + The Code Quality widget in the merge request compares the reports from the base and head of the branch, + then lists any violations that will be resolved or created when the branch is merged. +- The full JSON report is available as a + [downloadable artifact](../../project/pipelines/job_artifacts.html#downloading-artifacts) + for the `code_quality` job. If multiple jobs in a pipeline generate a code quality artifact, only the artifact from the last created job (the job with the largest job ID) is used. To avoid confusion, @@ -276,6 +281,10 @@ Code Quality job in your `.gitlab-ci.yml` for the very first time. Consecutive merge requests will have something to compare to and the Code Quality report will be shown properly. +These reports will only be available as long as the Code Quality artifact(s) required to generate +them are also available. See +[`artifacts:expire_in`](../../../ci/yaml/README.md#artifactsexpire_in) for more details. + <!-- ## Troubleshooting Include any troubleshooting steps that you can foresee. If you know beforehand what issues diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index ab83d84284f..76af29b2977 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -4,6 +4,8 @@ module API class Triggers < Grape::API include PaginationParams + HTTP_GITLAB_EVENT_HEADER = "HTTP_#{WebHookService::GITLAB_EVENT_HEADER}".underscore.upcase + params do requires :id, type: String, desc: 'The ID of a project' end @@ -19,6 +21,8 @@ module API post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42283') + forbidden! if gitlab_pipeline_hook_request? + # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } @@ -128,5 +132,11 @@ module API destroy_conditionally!(trigger) end end + + helpers do + def gitlab_pipeline_hook_request? + request.get_header(HTTP_GITLAB_EVENT_HEADER) == WebHookService.hook_to_event(:pipeline_hooks) + end + end end end diff --git a/lib/gitlab/asset_proxy.rb b/lib/gitlab/asset_proxy.rb new file mode 100644 index 00000000000..fd7c58ba68f --- /dev/null +++ b/lib/gitlab/asset_proxy.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +# This is based on https://github.com/jch/html-pipeline/blob/v2.12.2/lib/html/pipeline/camo_filter.rb +# and Banzai::Filter::AssetProxyFilter which we use to proxy images in Markdown + +module Gitlab + module AssetProxy + class << self + def proxy_url(url) + return url unless Gitlab.config.asset_proxy.enabled + return url if asset_host_whitelisted?(url) + + "#{Gitlab.config.asset_proxy.url}/#{asset_url_hash(url)}/#{hexencode(url)}" + end + + private + + def asset_host_whitelisted?(url) + parsed_url = URI.parse(url) + + Gitlab.config.asset_proxy.domain_regexp&.match?(parsed_url.host) + end + + def asset_url_hash(url) + OpenSSL::HMAC.hexdigest('sha1', Gitlab.config.asset_proxy.secret_key, url) + end + + def hexencode(str) + str.unpack1('H*') + end + end + end +end diff --git a/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id.rb b/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id.rb new file mode 100644 index 00000000000..b66fdfd5c65 --- /dev/null +++ b/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # rubocop:disable Style/Documentation + class RecalculateProjectAuthorizationsWithMinMaxUserId + def perform(min_user_id, max_user_id) + User.where(id: min_user_id..max_user_id).find_each do |user| + service = Users::RefreshAuthorizedProjectsService.new( + user, + incorrect_auth_found_callback: + ->(project_id, access_level) do + logger.info(message: 'Removing ProjectAuthorizations', + user_id: user.id, + project_id: project_id, + access_level: access_level) + end, + missing_auth_found_callback: + ->(project_id, access_level) do + logger.info(message: 'Creating ProjectAuthorizations', + user_id: user.id, + project_id: project_id, + access_level: access_level) + end + ) + + service.execute + end + end + + private + + def logger + @logger ||= Gitlab::BackgroundMigration::Logger.build + end + end + end +end diff --git a/lib/gitlab/dependency_linker/base_linker.rb b/lib/gitlab/dependency_linker/base_linker.rb index dd7ab92c6ae..a4e265eba88 100644 --- a/lib/gitlab/dependency_linker/base_linker.rb +++ b/lib/gitlab/dependency_linker/base_linker.rb @@ -7,6 +7,8 @@ module Gitlab GIT_INVALID_URL_REGEX = /^git\+#{URL_REGEX}/.freeze REPO_REGEX = %r{[^/'" ]+/[^/'" ]+}.freeze + include ActionView::Helpers::SanitizeHelper + class_attribute :file_type def self.support?(blob_name) @@ -62,7 +64,10 @@ module Gitlab end def link_tag(name, url) - %{<a href="#{ERB::Util.html_escape_once(url)}" rel="nofollow noreferrer noopener" target="_blank">#{ERB::Util.html_escape_once(name)}</a>}.html_safe + sanitize( + %{<a href="#{ERB::Util.html_escape_once(url)}" rel="nofollow noreferrer noopener" target="_blank">#{ERB::Util.html_escape_once(name)}</a>}, + attributes: %w[href rel target] + ) end # Links package names based on regex. diff --git a/lib/gitlab/import_export/relation_tree_restorer.rb b/lib/gitlab/import_export/relation_tree_restorer.rb index 1797bbad51a..e0d6ade51c2 100644 --- a/lib/gitlab/import_export/relation_tree_restorer.rb +++ b/lib/gitlab/import_export/relation_tree_restorer.rb @@ -26,6 +26,7 @@ module Gitlab ActiveRecord::Base.uncached do ActiveRecord::Base.no_touching do update_params! + update_relation_hashes! create_relations! end end @@ -217,6 +218,10 @@ module Gitlab excluded_keys: excluded_keys_for_relation(relation_key) } end + + def update_relation_hashes! + @tree_hash['ci_pipelines']&.sort_by! { |hash| hash['id'] } + end end end end diff --git a/lib/gitlab/project_authorizations.rb b/lib/gitlab/project_authorizations.rb index d65e8759ec9..ff90a009b2e 100644 --- a/lib/gitlab/project_authorizations.rb +++ b/lib/gitlab/project_authorizations.rb @@ -62,6 +62,7 @@ module Gitlab cte = Gitlab::SQL::RecursiveCTE.new(:namespaces_cte) members = Member.arel_table namespaces = Namespace.arel_table + group_group_links = GroupGroupLink.arel_table # Namespaces the user is a member of. cte << user.groups @@ -69,7 +70,10 @@ module Gitlab .except(:order) # Namespaces shared with any of the group - cte << Group.select([namespaces[:id], 'group_group_links.group_access AS access_level']) + cte << Group.select([namespaces[:id], + least(members[:access_level], + group_group_links[:group_access], + 'access_level')]) .joins(join_group_group_links) .joins(join_members_on_group_group_links) diff --git a/lib/gitlab/sidekiq_logging/json_formatter.rb b/lib/gitlab/sidekiq_logging/json_formatter.rb index e0b0d684bea..c20e929ae36 100644 --- a/lib/gitlab/sidekiq_logging/json_formatter.rb +++ b/lib/gitlab/sidekiq_logging/json_formatter.rb @@ -1,5 +1,8 @@ # frozen_string_literal: true +# This is needed for sidekiq-cluster +require 'json' + module Gitlab module SidekiqLogging class JSONFormatter diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index ae4c77255c5..fda2985307e 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -67,7 +67,13 @@ module Gitlab return false unless can_access_git? return false unless project - return false if !user.can?(:push_code, project) && !project.branch_allows_collaboration?(user, ref) + # Checking for an internal project to prevent an infinite loop: + # https://gitlab.com/gitlab-org/gitlab/issues/36805 + if project.internal? + return false unless user.can?(:push_code, project) + else + return false if !user.can?(:push_code, project) && !project.branch_allows_collaboration?(user, ref) + end if protected?(ProtectedBranch, project, ref) protected_branch_accessible_to?(ref, action: :push) diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb index 5727e73e725..ad6b213bb50 100644 --- a/lib/gitlab/utils.rb +++ b/lib/gitlab/utils.rb @@ -146,5 +146,14 @@ module Gitlab IPAddr.new(str) rescue IPAddr::InvalidAddressError end + + # Converts a string to an Addressable::URI object. + # If the string is not a valid URI, it returns nil. + # Param uri_string should be a String object. + # This method returns an Addressable::URI object or nil. + def parse_url(uri_string) + Addressable::URI.parse(uri_string) + rescue Addressable::URI::InvalidURIError, TypeError + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0a63aca949d..c6738dafa23 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -68,6 +68,11 @@ msgstr "" msgid "\"%{path}\" did not exist on \"%{ref}\"" msgstr "" +msgid "%d code quality issue" +msgid_plural "%d code quality issues" +msgstr[0] "" +msgstr[1] "" + msgid "%d comment" msgid_plural "%d comments" msgstr[0] "" @@ -4853,6 +4858,9 @@ msgstr "" msgid "Code Owners to the merge request changes." msgstr "" +msgid "Code Quality" +msgstr "" + msgid "Code Review" msgstr "" @@ -8378,6 +8386,9 @@ msgstr "" msgid "False positive" msgstr "" +msgid "Fast-forward merge is not possible. Rebase the source branch onto %{targetBranch} to allow this merge request to be merged." +msgstr "" + msgid "Fast-forward merge is not possible. Rebase the source branch onto the target branch or merge target branch into source branch to allow this merge request to be merged." msgstr "" @@ -8825,7 +8836,7 @@ msgstr "" msgid "From %{providerTitle}" msgstr "" -msgid "From %{source_title} into" +msgid "From <code>%{source_title}</code> into" msgstr "" msgid "From Bitbucket" @@ -23188,6 +23199,9 @@ msgstr "" msgid "ciReport|Fixed:" msgstr "" +msgid "ciReport|Found %{issuesWithCount}" +msgstr "" + msgid "ciReport|Investigate this vulnerability by creating an issue" msgstr "" @@ -23206,6 +23220,9 @@ msgstr "" msgid "ciReport|No changes to performance metrics" msgstr "" +msgid "ciReport|No code quality issues found" +msgstr "" + msgid "ciReport|Performance metrics" msgstr "" @@ -23236,6 +23253,9 @@ msgstr "" msgid "ciReport|There was an error dismissing the vulnerability. Please try again." msgstr "" +msgid "ciReport|There was an error fetching the codequality report." +msgstr "" + msgid "ciReport|There was an error reverting the dismissal. Please try again." msgstr "" @@ -429,6 +429,7 @@ module QA autoload :Gcloud, 'qa/service/cluster_provider/gcloud' autoload :Minikube, 'qa/service/cluster_provider/minikube' autoload :K3d, 'qa/service/cluster_provider/k3d' + autoload :K3s, 'qa/service/cluster_provider/k3s' end module DockerRun @@ -440,6 +441,7 @@ module QA autoload :GitlabRunner, 'qa/service/docker_run/gitlab_runner' autoload :MailHog, 'qa/service/docker_run/mail_hog' autoload :SamlIdp, 'qa/service/docker_run/saml_idp' + autoload :K3s, 'qa/service/docker_run/k3s' end end diff --git a/qa/qa/page/project/operations/kubernetes/add_existing.rb b/qa/qa/page/project/operations/kubernetes/add_existing.rb index 9f47841366e..c143b55d057 100644 --- a/qa/qa/page/project/operations/kubernetes/add_existing.rb +++ b/qa/qa/page/project/operations/kubernetes/add_existing.rb @@ -11,7 +11,7 @@ module QA element :api_url, 'url_field :api_url' # rubocop:disable QA/ElementWithPattern element :ca_certificate, 'text_area :ca_cert' # rubocop:disable QA/ElementWithPattern element :token, 'text_field :token' # rubocop:disable QA/ElementWithPattern - element :add_cluster_button, "submit s_('ClusterIntegration|Add Kubernetes cluster')" # rubocop:disable QA/ElementWithPattern + element :add_kubernetes_cluster_button element :rbac_checkbox end @@ -32,7 +32,7 @@ module QA end def add_cluster! - click_on 'Add Kubernetes cluster' + click_element :add_kubernetes_cluster_button, Page::Project::Operations::Kubernetes::Show end def uncheck_rbac! diff --git a/qa/qa/page/project/operations/kubernetes/show.rb b/qa/qa/page/project/operations/kubernetes/show.rb index 3d3eebdbec9..b639f867593 100644 --- a/qa/qa/page/project/operations/kubernetes/show.rb +++ b/qa/qa/page/project/operations/kubernetes/show.rb @@ -11,14 +11,20 @@ module QA end view 'app/views/clusters/clusters/_form.html.haml' do - element :base_domain - element :save_domain + element :integration_status_toggle, required: true + element :base_domain_field, required: true + element :save_changes_button, required: true + end + + view 'app/assets/javascripts/clusters/components/application_row.vue' do + element :install_button + element :uninstall_button end def install!(application_name) within_element(application_name) do has_element?(:install_button, application: application_name, wait: 30) - click_on 'Install' # TODO replace with click_element + click_element :install_button end end @@ -41,11 +47,11 @@ module QA end def set_domain(domain) - fill_element :base_domain, domain + fill_element :base_domain_field, domain end def save_domain - click_element :save_domain + click_element :save_changes_button, Page::Project::Operations::Kubernetes::Show end end end diff --git a/qa/qa/service/cluster_provider/k3s.rb b/qa/qa/service/cluster_provider/k3s.rb new file mode 100644 index 00000000000..165de795683 --- /dev/null +++ b/qa/qa/service/cluster_provider/k3s.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +module QA + module Service + module ClusterProvider + class K3s < Base + def validate_dependencies + Runtime::ApplicationSettings.set_application_settings(allow_local_requests_from_web_hooks_and_services: true) + end + + def setup + @k3s = Service::DockerRun::K3s.new.tap do |k3s| + k3s.register! + + shell "kubectl config set-cluster k3s --server https://#{k3s.host_name}:6443 --insecure-skip-tls-verify" + shell 'kubectl config set-credentials default --username=node --password=some-secret' + shell 'kubectl config set-context k3s --cluster=k3s --user=default' + shell 'kubectl config use-context k3s' + + wait_for_server(k3s.host_name) do + shell 'kubectl version' + + wait_for_namespaces do + # install local storage + shell 'kubectl apply -f https://raw.githubusercontent.com/rancher/local-path-provisioner/master/deploy/local-path-storage.yaml' + + # patch local storage + shell %(kubectl patch storageclass local-path -p '{"metadata": {"annotations":{"storageclass.kubernetes.io/is-default-class":"true"}}}') + end + end + end + end + + def teardown + Runtime::ApplicationSettings.set_application_settings(allow_local_requests_from_web_hooks_and_services: false) + + @k3s&.remove! + end + + def set_credentials(admin_user) + end + + # Fetch "real" certificate + # See https://github.com/rancher/k3s/issues/27 + def filter_credentials(credentials) + kubeconfig = YAML.safe_load(@k3s.kubeconfig) + ca_certificate = kubeconfig.dig('clusters', 0, 'cluster', 'certificate-authority-data') + + credentials.merge('data' => credentials['data'].merge('ca.crt' => ca_certificate)) + end + + private + + def wait_for_server(host_name) + print "Waiting for K3s server at `https://#{host_name}:6443` to become available " + + 60.times do + if service_available?('kubectl version') + return yield if block_given? + + return true + end + + sleep 1 + print '.' + end + + raise 'K3s server never came up' + end + + def wait_for_namespaces + print 'Waiting for k8s namespaces to populate' + + 60.times do + if service_available?('kubectl get pods --all-namespaces | grep --silent "Running"') + return yield if block_given? + + return true + end + + sleep 1 + print '.' + end + + raise 'K8s namespaces didnt populate correctly' + end + + def service_available?(command) + system("#{command} > /dev/null 2>&1") + end + end + end + end +end diff --git a/qa/qa/service/docker_run/base.rb b/qa/qa/service/docker_run/base.rb index 3f42c09ad2c..b02bbea8ff5 100644 --- a/qa/qa/service/docker_run/base.rb +++ b/qa/qa/service/docker_run/base.rb @@ -37,6 +37,10 @@ module QA def running? `docker ps -f name=#{@name}`.include?(@name) end + + def read_file(file_path) + `docker exec #{@name} /bin/cat #{file_path}` + end end end end diff --git a/qa/qa/service/docker_run/k3s.rb b/qa/qa/service/docker_run/k3s.rb new file mode 100644 index 00000000000..da254497ff0 --- /dev/null +++ b/qa/qa/service/docker_run/k3s.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module QA + module Service + module DockerRun + class K3s < Base + def initialize + @image = 'registry.gitlab.com/gitlab-org/cluster-integration/test-utils/k3s-gitlab-ci/releases/v0.6.1' + @name = 'k3s' + super + end + + def register! + pull + start_k3s + end + + def host_name + return 'localhost' unless Runtime::Env.running_in_ci? + + super + end + + def kubeconfig + read_file('/etc/rancher/k3s/k3s.yaml').chomp + end + + def start_k3s + command = <<~CMD.tr("\n", ' ') + docker run -d --rm + --network #{network} + --hostname #{host_name} + --name #{@name} + --publish 6443:6443 + --privileged + #{@image} server --cluster-secret some-secret + CMD + + command.gsub!("--network #{network} ", '') unless QA::Runtime::Env.running_in_ci? + + shell command + end + end + end + end +end diff --git a/qa/qa/specs/features/browser_ui/7_configure/kubernetes/kubernetes_integration_spec.rb b/qa/qa/specs/features/browser_ui/7_configure/kubernetes/kubernetes_integration_spec.rb index 73b5a579e08..728f22aed89 100644 --- a/qa/qa/specs/features/browser_ui/7_configure/kubernetes/kubernetes_integration_spec.rb +++ b/qa/qa/specs/features/browser_ui/7_configure/kubernetes/kubernetes_integration_spec.rb @@ -2,10 +2,9 @@ module QA context 'Configure' do - # This test requires GITLAB_QA_ADMIN_ACCESS_TOKEN to be specified - describe 'Kubernetes Cluster Integration', :orchestrated, :kubernetes, :requires_admin, :skip do + describe 'Kubernetes Cluster Integration', :orchestrated, :kubernetes, :requires_admin do context 'Project Clusters' do - let(:cluster) { Service::KubernetesCluster.new(provider_class: Service::ClusterProvider::K3d).create! } + let(:cluster) { Service::KubernetesCluster.new(provider_class: Service::ClusterProvider::K3s).create! } let(:project) do Resource::Project.fabricate_via_api! do |project| project.name = 'project-with-k8s' @@ -35,18 +34,6 @@ module QA expect(index).to have_cluster(cluster) end end - - it 'installs helm and tiller on a gitlab managed app' do - Resource::KubernetesCluster.fabricate_via_browser_ui! do |k8s_cluster| - k8s_cluster.project = project - k8s_cluster.cluster = cluster - k8s_cluster.install_helm_tiller = true - end - - Page::Project::Operations::Kubernetes::Show.perform do |show| - expect(show).to have_application_installed(:helm) - end - end end end end diff --git a/qa/spec/service/docker_run/k3s_spec.rb b/qa/spec/service/docker_run/k3s_spec.rb new file mode 100644 index 00000000000..0224b7d6704 --- /dev/null +++ b/qa/spec/service/docker_run/k3s_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module QA + describe Service::DockerRun::K3s do + describe '#host_name' do + context 'in CI' do + let(:name) { 'k3s-12345' } + let(:network) { 'thenet' } + + before do + allow(Runtime::Env).to receive(:running_in_ci?).and_return(true) + allow(subject).to receive(:network).and_return(network) + subject.instance_variable_set(:@name, name) + end + + it 'returns name.network' do + expect(subject.host_name).to eq("#{name}.#{network}") + end + end + + context 'not in CI' do + before do + allow(Runtime::Env).to receive(:running_in_ci?).and_return(false) + end + + it 'returns localhost if not running in a CI environment' do + expect(subject.host_name).to eq('localhost') + end + end + end + end +end diff --git a/scripts/review_apps/automated_cleanup.rb b/scripts/review_apps/automated_cleanup.rb index 8a04d8e00bc..323a1deedfc 100755 --- a/scripts/review_apps/automated_cleanup.rb +++ b/scripts/review_apps/automated_cleanup.rb @@ -54,7 +54,7 @@ class AutomatedCleanup end def perform_gitlab_environment_cleanup!(days_for_stop:, days_for_delete:) - puts "Checking for review apps not updated in the last #{days_for_stop} days..." + puts "Checking for Review Apps not updated in the last #{days_for_stop} days..." checked_environments = [] delete_threshold = threshold_time(days: days_for_delete) @@ -84,7 +84,7 @@ class AutomatedCleanup elsif deployed_at < stop_threshold stop_environment(environment, deployment) else - print_release_state(subject: 'Review app', release_name: environment.slug, release_date: last_deploy, action: 'leaving') + print_release_state(subject: 'Review App', release_name: environment.slug, release_date: last_deploy, action: 'leaving') end checked_environments << environment.slug @@ -94,9 +94,9 @@ class AutomatedCleanup end def perform_helm_releases_cleanup!(days:) - puts "Checking for Helm releases not updated in the last #{days} days..." + puts "Checking for Helm releases that are FAILED or not updated in the last #{days} days..." - threshold_day = threshold_time(days: days) + threshold = threshold_time(days: days) releases_to_delete = [] @@ -104,7 +104,7 @@ class AutomatedCleanup # Prevents deleting `dns-gitlab-review-app` releases or other unrelated releases next unless release.name.start_with?('review-') - if release.status == 'FAILED' || release.last_update < threshold_day + if release.status == 'FAILED' || release.last_update < threshold releases_to_delete << release else print_release_state(subject: 'Release', release_name: release.name, release_date: release.last_update, action: 'leaving') @@ -180,14 +180,14 @@ end automated_cleanup = AutomatedCleanup.new -timed('Review apps cleanup') do - automated_cleanup.perform_gitlab_environment_cleanup!(days_for_stop: 2, days_for_delete: 3) +timed('Review Apps cleanup') do + automated_cleanup.perform_gitlab_environment_cleanup!(days_for_stop: 5, days_for_delete: 6) end puts timed('Helm releases cleanup') do - automated_cleanup.perform_helm_releases_cleanup!(days: 3) + automated_cleanup.perform_helm_releases_cleanup!(days: 7) end exit(0) diff --git a/spec/controllers/groups/group_links_controller_spec.rb b/spec/controllers/groups/group_links_controller_spec.rb index c062de468fc..21169188386 100644 --- a/spec/controllers/groups/group_links_controller_spec.rb +++ b/spec/controllers/groups/group_links_controller_spec.rb @@ -6,9 +6,13 @@ describe Groups::GroupLinksController do let(:shared_with_group) { create(:group, :private) } let(:shared_group) { create(:group, :private) } let(:user) { create(:user) } + let(:group_member) { create(:user) } + let!(:project) { create(:project, group: shared_group) } before do sign_in(user) + + shared_with_group.add_developer(group_member) end describe '#create' do @@ -40,13 +44,9 @@ describe Groups::GroupLinksController do end context 'when user has correct access to both groups' do - let(:group_member) { create(:user) } - before do shared_with_group.add_developer(user) shared_group.add_owner(user) - - shared_with_group.add_developer(group_member) end context 'when default access level is requested' do @@ -56,6 +56,10 @@ describe Groups::GroupLinksController do context 'when owner access is requested' do let(:shared_group_access) { Gitlab::Access::OWNER } + before do + shared_with_group.add_owner(group_member) + end + include_examples 'creates group group link' it 'allows admin access for group member' do @@ -64,6 +68,10 @@ describe Groups::GroupLinksController do end end + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:read_project, project) }.from(false).to(true) + end + context 'when shared with group id is not present' do let(:shared_with_group_id) { nil } @@ -149,6 +157,7 @@ describe Groups::GroupLinksController do context 'when user has admin access to the shared group' do before do shared_group.add_owner(user) + shared_with_group.refresh_members_authorized_projects end it 'updates existing link' do @@ -162,6 +171,10 @@ describe Groups::GroupLinksController do expect(link.group_access).to eq(Gitlab::Access::GUEST) expect(link.expires_at).to eq(expiry_date) end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end end context 'when user does not have admin access to the shared group' do @@ -199,11 +212,16 @@ describe Groups::GroupLinksController do context 'when user has admin access to the shared group' do before do shared_group.add_owner(user) + shared_with_group.refresh_members_authorized_projects end it 'deletes existing link' do expect { subject }.to change(GroupGroupLink, :count).by(-1) end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end end context 'when user does not have admin access to the shared group' do diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index 16947f4d3be..a675014a77b 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -260,8 +260,10 @@ describe SnippetsController do context 'when the snippet description contains a file' do include FileMoverHelpers - let(:picture_file) { "/-/system/user/#{user.id}/secret56/picture.jpg" } - let(:text_file) { "/-/system/user/#{user.id}/secret78/text.txt" } + let(:picture_secret) { SecureRandom.hex } + let(:text_secret) { SecureRandom.hex } + let(:picture_file) { "/-/system/user/#{user.id}/#{picture_secret}/picture.jpg" } + let(:text_file) { "/-/system/user/#{user.id}/#{text_secret}/text.txt" } let(:description) do "Description with picture:  and "\ "text: [text.txt](/uploads#{text_file})" @@ -284,8 +286,8 @@ describe SnippetsController do snippet = subject expected_description = "Description with picture: "\ - " and "\ - "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/secret78/text.txt)" + " and "\ + "text: [text.txt](/uploads/-/system/personal_snippet/#{snippet.id}/#{text_secret}/text.txt)" expect(snippet.description).to eq(expected_description) end diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb index 0d24b02a64c..3bb70fdf376 100644 --- a/spec/features/issues/issue_detail_spec.rb +++ b/spec/features/issues/issue_detail_spec.rb @@ -28,7 +28,7 @@ describe 'Issue Detail', :js do visit project_issue_path(project, issue) end - it 'encodes the description to prevent xss issues' do + it 'encodes the description to prevent xss issues', quarantine: 'https://gitlab.com/gitlab-org/gitlab/issues/207951' do page.within('.issuable-details .detail-page-description') do image = find('img.js-lazy-loaded') diff --git a/spec/features/merge_request/user_creates_merge_request_spec.rb b/spec/features/merge_request/user_creates_merge_request_spec.rb index 67f6d8ebe32..86ee9fa5aa5 100644 --- a/spec/features/merge_request/user_creates_merge_request_spec.rb +++ b/spec/features/merge_request/user_creates_merge_request_spec.rb @@ -5,9 +5,9 @@ require "spec_helper" describe "User creates a merge request", :js do include ProjectForksHelper + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { create(:user) } let(:title) { "Some feature" } - let(:project) { create(:project, :repository) } - let(:user) { create(:user) } before do project.add_maintainer(user) @@ -38,6 +38,26 @@ describe "User creates a merge request", :js do end end + context "XSS branch name exists" do + before do + project.repository.create_branch("<img/src='x'/onerror=alert('oops')>", "master") + end + + it "doesn't execute the dodgy branch name" do + visit(project_new_merge_request_path(project)) + + find(".js-source-branch").click + click_link("<img/src='x'/onerror=alert('oops')>") + + find(".js-target-branch").click + click_link("feature") + + click_button("Compare branches") + + expect { page.driver.browser.switch_to.alert }.to raise_error(Selenium::WebDriver::Error::NoSuchAlertError) + end + end + context "to a forked project" do let(:forked_project) { fork_project(project, user, namespace: user.namespace, repository: true) } diff --git a/spec/features/projects/wiki/user_updates_wiki_page_spec.rb b/spec/features/projects/wiki/user_updates_wiki_page_spec.rb index d3a0c9b790b..9d9c83331fb 100644 --- a/spec/features/projects/wiki/user_updates_wiki_page_spec.rb +++ b/spec/features/projects/wiki/user_updates_wiki_page_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe 'User updates wiki page' do + include WikiHelpers + let(:user) { create(:user) } before do @@ -11,8 +13,11 @@ describe 'User updates wiki page' do end context 'when wiki is empty' do - before do + before do |example| visit(project_wikis_path(project)) + + wait_for_svg_to_be_loaded(example) + click_link "Create your first page" end diff --git a/spec/fixtures/lib/gitlab/import_export/complex/project.json b/spec/fixtures/lib/gitlab/import_export/complex/project.json index fae02540868..72ed8b818d8 100644 --- a/spec/fixtures/lib/gitlab/import_export/complex/project.json +++ b/spec/fixtures/lib/gitlab/import_export/complex/project.json @@ -6452,7 +6452,7 @@ ] }, { - "id": 37, + "id": 26, "project_id": 5, "ref": "master", "sha": "048721d90c449b244b7b4c53a9186b04330174ec", @@ -6744,7 +6744,7 @@ ] }, { - "id": 40, + "id": 19, "project_id": 5, "ref": "master", "sha": "2ea1f3dec713d940208fb5ce4a38765ecb5d3f73", @@ -6851,7 +6851,7 @@ ] }, { - "id": 42, + "id": 20, "project_id": 5, "ref": "master", "sha": "ce84140e8b878ce6e7c4d298c7202ff38170e3ac", diff --git a/spec/frontend/error_tracking/components/error_details_spec.js b/spec/frontend/error_tracking/components/error_details_spec.js index e43f9569ffc..2baec5f37fb 100644 --- a/spec/frontend/error_tracking/components/error_details_spec.js +++ b/spec/frontend/error_tracking/components/error_details_spec.js @@ -137,6 +137,28 @@ describe('ErrorDetails', () => { expect(wrapper.findAll(GlButton).length).toBe(3); }); + describe('unsafe chars for culprit field', () => { + const findReportedText = () => wrapper.find('[data-qa-selector="reported_text"]'); + const culprit = '<script>console.log("surprise!")</script>'; + beforeEach(() => { + store.state.details.loadingStacktrace = false; + wrapper.setData({ + error: { + culprit, + }, + }); + }); + + it('should not convert interpolated text to html entities', () => { + expect(findReportedText().findAll('script').length).toEqual(0); + expect(findReportedText().findAll('strong').length).toEqual(1); + }); + + it('should render text instead of converting to html entities', () => { + expect(findReportedText().text()).toContain(culprit); + }); + }); + describe('Badges', () => { it('should show language and error level badges', () => { wrapper.setData({ diff --git a/spec/graphql/types/diff_refs_type_spec.rb b/spec/graphql/types/diff_refs_type_spec.rb index 91017c827ad..85225e5809c 100644 --- a/spec/graphql/types/diff_refs_type_spec.rb +++ b/spec/graphql/types/diff_refs_type_spec.rb @@ -5,5 +5,9 @@ require 'spec_helper' describe GitlabSchema.types['DiffRefs'] do it { expect(described_class.graphql_name).to eq('DiffRefs') } - it { expect(described_class).to have_graphql_fields(:base_sha, :head_sha, :start_sha) } + it { is_expected.to have_graphql_fields(:head_sha, :base_sha, :start_sha).only } + + it { expect(described_class.fields['headSha'].type).to be_non_null } + it { expect(described_class.fields['baseSha'].type).not_to be_non_null } + it { expect(described_class.fields['startSha'].type).to be_non_null } end diff --git a/spec/lib/gitlab/asset_proxy_spec.rb b/spec/lib/gitlab/asset_proxy_spec.rb new file mode 100644 index 00000000000..f5aa1819982 --- /dev/null +++ b/spec/lib/gitlab/asset_proxy_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::AssetProxy do + context 'when asset proxy is disabled' do + before do + stub_asset_proxy_setting(enabled: false) + end + + it 'returns the original URL' do + url = 'http://example.com/test.png' + + expect(described_class.proxy_url(url)).to eq(url) + end + end + + context 'when asset proxy is enabled' do + before do + stub_asset_proxy_setting(whitelist: %w(gitlab.com *.mydomain.com)) + stub_asset_proxy_setting( + enabled: true, + url: 'https://assets.example.com', + secret_key: 'shared-secret', + domain_regexp: Banzai::Filter::AssetProxyFilter.compile_whitelist(Gitlab.config.asset_proxy.whitelist) + ) + end + + it 'returns a proxied URL' do + url = 'http://example.com/test.png' + proxied_url = 'https://assets.example.com/08df250eeeef1a8cf2c761475ac74c5065105612/687474703a2f2f6578616d706c652e636f6d2f746573742e706e67' + + expect(described_class.proxy_url(url)).to eq(proxied_url) + end + + context 'whitelisted domain' do + it 'returns original URL for single domain whitelist' do + url = 'http://gitlab.com/test.png' + + expect(described_class.proxy_url(url)).to eq(url) + end + + it 'returns original URL for wildcard subdomain whitelist' do + url = 'http://test.mydomain.com/test.png' + + expect(described_class.proxy_url(url)).to eq(url) + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb b/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb new file mode 100644 index 00000000000..14ba57eecbf --- /dev/null +++ b/spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::BackgroundMigration::RecalculateProjectAuthorizationsWithMinMaxUserId, :migration, schema: 20200204113224 do + let(:users_table) { table(:users) } + let(:min) { 1 } + let(:max) { 5 } + + before do + min.upto(max) do |i| + users_table.create!(id: i, email: "user#{i}@example.com", projects_limit: 10) + end + end + + describe '#perform' do + it 'initializes Users::RefreshAuthorizedProjectsService with correct users' do + min.upto(max) do |i| + user = User.find(i) + expect(Users::RefreshAuthorizedProjectsService).to( + receive(:new).with(user, any_args).and_call_original) + end + + described_class.new.perform(min, max) + end + + it 'executes Users::RefreshAuthorizedProjectsService' do + expected_call_counts = max - min + 1 + + service = instance_double(Users::RefreshAuthorizedProjectsService) + expect(Users::RefreshAuthorizedProjectsService).to( + receive(:new).exactly(expected_call_counts).times.and_return(service)) + expect(service).to receive(:execute).exactly(expected_call_counts).times + + described_class.new.perform(min, max) + end + end +end diff --git a/spec/lib/gitlab/dependency_linker/base_linker_spec.rb b/spec/lib/gitlab/dependency_linker/base_linker_spec.rb new file mode 100644 index 00000000000..1466ce2dfcc --- /dev/null +++ b/spec/lib/gitlab/dependency_linker/base_linker_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::DependencyLinker::BaseLinker do + let(:linker_class) do + Class.new(described_class) do + def link_dependencies + link_regex(%r{^(?<name>https?://[^ ]+)}, &:itself) + end + end + end + + let(:plain_content) do + <<~CONTENT + http://\\njavascript:alert(1) + https://gitlab.com/gitlab-org/gitlab + CONTENT + end + + let(:highlighted_content) do + <<~CONTENT + <span><span>http://</span><span>\\n</span><span>javascript:alert(1)</span></span> + <span><span>https://gitlab.com/gitlab-org/gitlab</span></span> + CONTENT + end + + let(:linker) { linker_class.new(plain_content, highlighted_content) } + + describe '#link' do + subject { linker.link } + + it 'only converts valid links' do + expect(subject).to eq( + <<~CONTENT + <span><span>#{link('http://')}</span><span>#{link('\n', url: '%5Cn')}</span><span>#{link('javascript:alert(1)', url: nil)}</span></span> + <span><span>#{link('https://gitlab.com/gitlab-org/gitlab')}</span></span> + CONTENT + ) + end + end + + def link(text, url: text) + attrs = [ + 'rel="nofollow noreferrer noopener"', + 'target="_blank"' + ] + + attrs.unshift(%{href="#{url}"}) if url + + %{<a #{attrs.join(' ')}>#{text}</a>} + end +end diff --git a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb index a46c6579670..7bc17b804df 100644 --- a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb @@ -104,6 +104,24 @@ describe Gitlab::ImportExport::Project::TreeRestorer do expect(pipeline.merge_request.source_branch).to eq('feature_conflict') end + it 'restores pipelines based on ascending id order' do + expected_ordered_shas = %w[ + 2ea1f3dec713d940208fb5ce4a38765ecb5d3f73 + ce84140e8b878ce6e7c4d298c7202ff38170e3ac + 048721d90c449b244b7b4c53a9186b04330174ec + sha-notes + 5f923865dde3436854e9ceb9cdb7815618d4e849 + d2d430676773caa88cdaf7c55944073b2fd5561a + 2ea1f3dec713d940208fb5ce4a38765ecb5d3f73 + ] + + project = Project.find_by_path('project') + + project.ci_pipelines.order(:id).each_with_index do |pipeline, i| + expect(pipeline['sha']).to eq expected_ordered_shas[i] + end + end + it 'preserves updated_at on issues' do issue = Issue.where(description: 'Aliquam enim illo et possimus.').first @@ -385,7 +403,7 @@ describe Gitlab::ImportExport::Project::TreeRestorer do it 'has the correct number of pipelines and statuses' do expect(@project.ci_pipelines.size).to eq(7) - @project.ci_pipelines.order(:id).zip([2, 2, 2, 2, 2, 0, 0]) + @project.ci_pipelines.order(:id).zip([2, 0, 2, 2, 2, 2, 0]) .each do |(pipeline, expected_status_size)| expect(pipeline.statuses.size).to eq(expected_status_size) end @@ -422,7 +440,7 @@ describe Gitlab::ImportExport::Project::TreeRestorer do end it 'restores external pull request for the restored pipeline' do - pipeline_with_external_pr = @project.ci_pipelines.order(:id).last + pipeline_with_external_pr = @project.ci_pipelines.where(source: 'external_pull_request_event').first expect(pipeline_with_external_pr.external_pull_request).to be_persisted end diff --git a/spec/lib/gitlab/project_authorizations_spec.rb b/spec/lib/gitlab/project_authorizations_spec.rb index 1c579128223..7b282433061 100644 --- a/spec/lib/gitlab/project_authorizations_spec.rb +++ b/spec/lib/gitlab/project_authorizations_spec.rb @@ -109,6 +109,20 @@ describe Gitlab::ProjectAuthorizations do end end + context 'with lower group access level than max access level for share' do + let(:user) { create(:user) } + + it 'creates proper authorizations' do + group.add_reporter(user) + + mapping = map_access_levels(authorizations) + + expect(mapping[project_parent.id]).to be_nil + expect(mapping[project.id]).to eq(Gitlab::Access::REPORTER) + expect(mapping[project_child.id]).to eq(Gitlab::Access::REPORTER) + end + end + context 'parent group user' do let(:user) { parent_group_user } diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index 8d13f377677..78370f0136c 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -30,6 +30,17 @@ describe Gitlab::UserAccess do end end + describe 'push to branch in an internal project' do + it 'will not infinitely loop when a project is internal' do + project.visibility_level = Gitlab::VisibilityLevel::INTERNAL + project.save! + + expect(project).not_to receive(:branch_allows_collaboration?) + + access.can_push_to_branch?('master') + end + end + describe 'push to empty project' do let(:empty_project) { create(:project_empty_repo) } let(:project_access) { described_class.new(user, project: empty_project) } diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb index 48fc2d826bc..d3780d22241 100644 --- a/spec/lib/gitlab/utils_spec.rb +++ b/spec/lib/gitlab/utils_spec.rb @@ -291,4 +291,18 @@ describe Gitlab::Utils do expect(described_class.string_to_ip_object('1:0:0:0:0:0:0:0/124')).to eq(IPAddr.new('1:0:0:0:0:0:0:0/124')) end end + + describe '.parse_url' do + it 'returns Addressable::URI object' do + expect(described_class.parse_url('http://gitlab.com')).to be_instance_of(Addressable::URI) + end + + it 'returns nil when URI cannot be parsed' do + expect(described_class.parse_url('://gitlab.com')).to be nil + end + + it 'returns nil with invalid parameter' do + expect(described_class.parse_url(1)).to be nil + end + end end diff --git a/spec/migrations/clean_grafana_url_spec.rb b/spec/migrations/clean_grafana_url_spec.rb new file mode 100644 index 00000000000..9f060fbaf7d --- /dev/null +++ b/spec/migrations/clean_grafana_url_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20200214085940_clean_grafana_url.rb') + +describe CleanGrafanaUrl, :migration do + let(:application_settings_table) { table(:application_settings) } + + [ + 'javascript:alert(window.opener.document.location)', + ' javascript:alert(window.opener.document.location)' + ].each do |grafana_url| + it "sets grafana_url back to its default value when grafana_url is '#{grafana_url}'" do + application_settings = application_settings_table.create!(grafana_url: grafana_url) + + migrate! + + expect(application_settings.reload.grafana_url).to eq('/-/grafana') + end + end + + ['/-/grafana', '/some/relative/url', 'http://localhost:9000'].each do |grafana_url| + it "does not modify grafana_url when grafana_url is '#{grafana_url}'" do + application_settings = application_settings_table.create!(grafana_url: grafana_url) + + migrate! + + expect(application_settings.reload.grafana_url).to eq(grafana_url) + end + end + + context 'when application_settings table has no rows' do + it 'does not fail' do + migrate! + end + end +end diff --git a/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb b/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb new file mode 100644 index 00000000000..04726f98c89 --- /dev/null +++ b/spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20200204113224_schedule_recalculate_project_authorizations_second_run.rb') + +describe ScheduleRecalculateProjectAuthorizationsSecondRun, :migration do + let(:users_table) { table(:users) } + + before do + stub_const("#{described_class}::BATCH_SIZE", 2) + + 1.upto(4) do |i| + users_table.create!(id: i, name: "user#{i}", email: "user#{i}@example.com", projects_limit: 1) + end + end + + it 'schedules background migration' do + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(BackgroundMigrationWorker.jobs.size).to eq(2) + expect(described_class::MIGRATION).to be_scheduled_migration(1, 2) + expect(described_class::MIGRATION).to be_scheduled_migration(3, 4) + end + end + end +end diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 82ece6b2b91..9dad6b1e766 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -19,6 +19,7 @@ describe ApplicationSetting do let(:http) { 'http://example.com' } let(:https) { 'https://example.com' } let(:ftp) { 'ftp://example.com' } + let(:javascript) { 'javascript:alert(window.opener.document.location)' } it { is_expected.to allow_value(nil).for(:home_page_url) } it { is_expected.to allow_value(http).for(:home_page_url) } @@ -81,6 +82,53 @@ describe ApplicationSetting do it { is_expected.not_to allow_value('abc').for(:minimum_password_length) } it { is_expected.to allow_value(10).for(:minimum_password_length) } + context 'grafana_url validations' do + before do + subject.instance_variable_set(:@parsed_grafana_url, nil) + end + + it { is_expected.to allow_value(http).for(:grafana_url) } + it { is_expected.to allow_value(https).for(:grafana_url) } + it { is_expected.not_to allow_value(ftp).for(:grafana_url) } + it { is_expected.not_to allow_value(javascript).for(:grafana_url) } + it { is_expected.to allow_value('/-/grafana').for(:grafana_url) } + it { is_expected.to allow_value('http://localhost:9000').for(:grafana_url) } + + context 'when local URLs are not allowed in system hooks' do + before do + stub_application_setting(allow_local_requests_from_system_hooks: false) + end + + it { is_expected.not_to allow_value('http://localhost:9000').for(:grafana_url) } + end + + context 'with invalid grafana URL' do + it 'adds an error' do + subject.grafana_url = ' ' + http + expect(subject.save).to be false + + expect(subject.errors[:grafana_url]).to eq([ + 'must be a valid relative or absolute URL. ' \ + 'Please check your Grafana URL setting in ' \ + 'Admin Area > Settings > Metrics and profiling > Metrics - Grafana' + ]) + end + end + + context 'with blocked grafana URL' do + it 'adds an error' do + subject.grafana_url = javascript + expect(subject.save).to be false + + expect(subject.errors[:grafana_url]).to eq([ + 'is blocked: Only allowed schemes are http, https. Please check your ' \ + 'Grafana URL setting in ' \ + 'Admin Area > Settings > Metrics and profiling > Metrics - Grafana' + ]) + end + end + end + context 'when snowplow is enabled' do before do setting.snowplow_enabled = true diff --git a/spec/models/badge_spec.rb b/spec/models/badge_spec.rb index 60ae579eb03..fba8f40e99b 100644 --- a/spec/models/badge_spec.rb +++ b/spec/models/badge_spec.rb @@ -91,6 +91,22 @@ describe Badge do let(:method) { :image_url } it_behaves_like 'rendered_links' + + context 'when asset proxy is enabled' do + let(:placeholder_url) { 'http://www.example.com/image' } + + before do + stub_asset_proxy_setting( + enabled: true, + url: 'https://assets.example.com', + secret_key: 'shared-secret' + ) + end + + it 'returns a proxied URL' do + expect(badge.rendered_image_url).to start_with('https://assets.example.com') + end + end end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index b5ed29189fd..576ac4393ed 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -563,6 +563,18 @@ describe Group do expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER) expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER) end + + context 'with lower group access level than max access level for share' do + let(:user) { create(:user) } + + it 'returns correct access level' do + group.add_reporter(user) + + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER) + end + end end context 'with user in the parent group' do @@ -584,6 +596,33 @@ describe Group do expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) end end + + context 'unrelated project owner' do + let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 } + let!(:group) { create(:group, id: common_id) } + let!(:unrelated_project) { create(:project, id: common_id) } + let(:user) { unrelated_project.owner } + + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + end + end + + context 'user without accepted access request' do + let!(:user) { create(:user) } + + before do + create(:group_member, :developer, :access_request, user: user, group: group) + end + + it 'returns correct access level' do + expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) + end + end end context 'when feature flag share_group_with_group is disabled' do diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index ad7dfac87af..9b5cce1aebf 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -65,10 +65,10 @@ describe GroupMember do end describe '#update_two_factor_requirement' do - let(:user) { build :user } - let(:group_member) { build :group_member, user: user } - it 'is called after creation and deletion' do + user = build :user + group_member = build :group_member, user: user + expect(user).to receive(:update_two_factor_requirement) group_member.save @@ -79,6 +79,21 @@ describe GroupMember do end end + describe '#after_accept_invite' do + it 'calls #update_two_factor_requirement' do + email = 'foo@email.com' + user = build(:user, email: email) + group = create(:group, require_two_factor_authentication: true) + group_member = create(:group_member, group: group, invite_token: '1234', invite_email: email) + + expect(user).to receive(:require_two_factor_authentication_from_group).and_call_original + + group_member.accept_invite!(user) + + expect(user.require_two_factor_authentication_from_group).to be_truthy + end + end + context 'access levels' do context 'with parent group' do it_behaves_like 'inherited access level as a member of entity' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index cbd55077316..56f4c68a913 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -4866,6 +4866,38 @@ describe Project do end end + context 'with cross internal project merge requests' do + let(:project) { create(:project, :repository, :internal) } + let(:forked_project) { fork_project(project, nil, repository: true) } + let(:user) { double(:user) } + + it "does not endlessly loop for internal projects with MRs to each other", :sidekiq_inline do + allow(user).to receive(:can?).and_return(true, false, true) + allow(user).to receive(:id).and_return(1) + + create( + :merge_request, + target_project: project, + target_branch: 'merge-test', + source_project: forked_project, + source_branch: 'merge-test', + allow_collaboration: true + ) + + create( + :merge_request, + target_project: forked_project, + target_branch: 'merge-test', + source_project: project, + source_branch: 'merge-test', + allow_collaboration: true + ) + + expect(user).to receive(:can?).at_most(5).times + project.branch_allows_collaboration?(user, "merge-test") + end + end + context 'with cross project merge requests' do let(:user) { create(:user) } let(:target_project) { create(:project, :repository) } diff --git a/spec/models/user_detail_spec.rb b/spec/models/user_detail_spec.rb index 3b6de5bb390..2b2bfff7be2 100644 --- a/spec/models/user_detail_spec.rb +++ b/spec/models/user_detail_spec.rb @@ -7,7 +7,7 @@ describe UserDetail do describe 'validations' do describe 'job_title' do - it { is_expected.to validate_presence_of(:job_title) } + it { is_expected.not_to validate_presence_of(:job_title) } it { is_expected.to validate_length_of(:job_title).is_at_most(200) } end end diff --git a/spec/presenters/ci/pipeline_presenter_spec.rb b/spec/presenters/ci/pipeline_presenter_spec.rb index 700d1f5cbb6..c9c4f567549 100644 --- a/spec/presenters/ci/pipeline_presenter_spec.rb +++ b/spec/presenters/ci/pipeline_presenter_spec.rb @@ -6,6 +6,7 @@ describe Ci::PipelinePresenter do include Gitlab::Routing let(:user) { create(:user) } + let(:current_user) { user } let(:project) { create(:project) } let(:pipeline) { create(:ci_pipeline, project: project) } @@ -15,7 +16,7 @@ describe Ci::PipelinePresenter do before do project.add_developer(user) - allow(presenter).to receive(:current_user) { user } + allow(presenter).to receive(:current_user) { current_user } end it 'inherits from Gitlab::View::Presenter::Delegated' do @@ -224,10 +225,90 @@ describe Ci::PipelinePresenter do describe '#all_related_merge_requests' do it 'memoizes the returned relation' do query_count = ActiveRecord::QueryRecorder.new do - 2.times { presenter.send(:all_related_merge_requests).count } + 3.times { presenter.send(:all_related_merge_requests).count } end.count - expect(query_count).to eq(1) + expect(query_count).to eq(2) + end + + context 'permissions' do + let!(:merge_request) do + create(:merge_request, project: project, source_project: project) + end + + subject(:all_related_merge_requests) do + presenter.send(:all_related_merge_requests) + end + + shared_examples 'private merge requests' do + context 'when not logged in' do + let(:current_user) {} + + it { is_expected.to be_empty } + end + + context 'when logged in as a non_member' do + let(:current_user) { create(:user) } + + it { is_expected.to be_empty } + end + + context 'when logged in as a guest' do + let(:current_user) { create(:user) } + + before do + project.add_guest(current_user) + end + + it { is_expected.to be_empty } + end + + context 'when logged in as a developer' do + it { is_expected.to contain_exactly(merge_request) } + end + + context 'when logged in as a maintainer' do + let(:current_user) { create(:user) } + + before do + project.add_maintainer(current_user) + end + + it { is_expected.to contain_exactly(merge_request) } + end + end + + context 'with a private project' do + it_behaves_like 'private merge requests' + end + + context 'with a public project with private merge requests' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + + project + .project_feature + .update!(merge_requests_access_level: ProjectFeature::PRIVATE) + end + + it_behaves_like 'private merge requests' + end + + context 'with a public project with public merge requests' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + + project + .project_feature + .update!(merge_requests_access_level: ProjectFeature::ENABLED) + end + + context 'when not logged in' do + let(:current_user) {} + + it { is_expected.to contain_exactly(merge_request) } + end + end end end diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 0309618e243..bcc1c6bc4d4 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -116,6 +116,18 @@ describe API::Triggers do end end end + + context 'when is triggered by a pipeline hook' do + it 'does not create a new pipeline' do + expect do + post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"), + params: { ref: 'refs/heads/other-branch' }, + headers: { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' } + end.not_to change(Ci::Pipeline, :count) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end describe 'GET /projects/:id/triggers' do diff --git a/spec/services/auth/container_registry_authentication_service_spec.rb b/spec/services/auth/container_registry_authentication_service_spec.rb index 5003dfcc951..84f4a7a4e7a 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -769,6 +769,15 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when deploy token has read_registry as a scope' do let(:current_user) { create(:deploy_token, projects: [project]) } + shared_examples 'able to login' do + context 'registry provides read_container_image authentication_abilities' do + let(:current_params) { {} } + let(:authentication_abilities) { [:read_container_image] } + + it_behaves_like 'an authenticated' + end + end + context 'for public project' do let(:project) { create(:project, :public) } @@ -783,6 +792,8 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'an inaccessible' end + + it_behaves_like 'able to login' end context 'for internal project' do @@ -799,6 +810,8 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'an inaccessible' end + + it_behaves_like 'able to login' end context 'for private project' do @@ -815,18 +828,38 @@ describe Auth::ContainerRegistryAuthenticationService do it_behaves_like 'an inaccessible' end + + it_behaves_like 'able to login' end end context 'when deploy token does not have read_registry scope' do let(:current_user) { create(:deploy_token, projects: [project], read_registry: false) } + shared_examples 'unable to login' do + context 'registry provides no container authentication_abilities' do + let(:current_params) { {} } + let(:authentication_abilities) { [] } + + it_behaves_like 'a forbidden' + end + + context 'registry provides inapplicable container authentication_abilities' do + let(:current_params) { {} } + let(:authentication_abilities) { [:download_code] } + + it_behaves_like 'a forbidden' + end + end + context 'for public project' do let(:project) { create(:project, :public) } context 'when pulling' do it_behaves_like 'a pullable' end + + it_behaves_like 'unable to login' end context 'for internal project' do @@ -835,6 +868,8 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pulling' do it_behaves_like 'an inaccessible' end + + it_behaves_like 'unable to login' end context 'for private project' do @@ -843,6 +878,15 @@ describe Auth::ContainerRegistryAuthenticationService do context 'when pulling' do it_behaves_like 'an inaccessible' end + + context 'when logging in' do + let(:current_params) { {} } + let(:authentication_abilities) { [] } + + it_behaves_like 'a forbidden' + end + + it_behaves_like 'unable to login' end end diff --git a/spec/services/groups/group_links/destroy_service_spec.rb b/spec/services/groups/group_links/destroy_service_spec.rb index 6f49b6eda94..284bcd0df2e 100644 --- a/spec/services/groups/group_links/destroy_service_spec.rb +++ b/spec/services/groups/group_links/destroy_service_spec.rb @@ -40,24 +40,11 @@ describe Groups::GroupLinks::DestroyService, '#execute' do end it 'updates project authorization once per group' do - expect(GroupGroupLink).to receive(:delete) + expect(GroupGroupLink).to receive(:delete).and_call_original expect(group).to receive(:refresh_members_authorized_projects).once expect(another_group).to receive(:refresh_members_authorized_projects).once subject.execute(links) end - - it 'rolls back changes when error happens' do - group.add_developer(user) - - expect(group).to receive(:refresh_members_authorized_projects).once.and_call_original - expect(another_group).to( - receive(:refresh_members_authorized_projects).and_raise('boom')) - - expect { subject.execute(links) }.to raise_error('boom') - - expect(GroupGroupLink.count).to eq(links.length) - expect(Ability.allowed?(user, :read_project, project)).to be_truthy - end end end diff --git a/spec/services/groups/group_links/update_service_spec.rb b/spec/services/groups/group_links/update_service_spec.rb new file mode 100644 index 00000000000..446364c9799 --- /dev/null +++ b/spec/services/groups/group_links/update_service_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Groups::GroupLinks::UpdateService, '#execute' do + let(:user) { create(:user) } + + let_it_be(:group) { create(:group, :private) } + let_it_be(:shared_group) { create(:group, :private) } + let_it_be(:project) { create(:project, group: shared_group) } + let(:group_member) { create(:user) } + let!(:link) { create(:group_group_link, shared_group: shared_group, shared_with_group: group) } + + let(:expiry_date) { 1.month.from_now.to_date } + let(:group_link_params) do + { group_access: Gitlab::Access::GUEST, + expires_at: expiry_date } + end + + subject { described_class.new(link).execute(group_link_params) } + + before do + group.add_developer(group_member) + end + + it 'updates existing link' do + expect(link.group_access).to eq(Gitlab::Access::DEVELOPER) + expect(link.expires_at).to be_nil + + subject + + link.reload + + expect(link.group_access).to eq(Gitlab::Access::GUEST) + expect(link.expires_at).to eq(expiry_date) + end + + it 'updates project permissions' do + expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false) + end + + it 'executes UserProjectAccessChangedService' do + expect_next_instance_of(UserProjectAccessChangedService) do |service| + expect(service).to receive(:execute) + end + + subject + end + + context 'with only param not requiring authorization refresh' do + let(:group_link_params) { { expires_at: Date.tomorrow } } + + it 'does not execute UserProjectAccessChangedService' do + expect(UserProjectAccessChangedService).not_to receive(:new) + + subject + end + end +end diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 21a139cdf3c..496d1fe67f2 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -134,6 +134,21 @@ describe Projects::LfsPointers::LfsDownloadService do end end + context 'when an lfs object with the same oid already exists' do + let!(:existing_lfs_object) { create(:lfs_object, oid: oid) } + + before do + stub_full_request(download_link).to_return(body: lfs_content) + end + + it_behaves_like 'no lfs object is created' + + it 'does not update the file attached to the existing LfsObject' do + expect { subject.execute } + .not_to change { existing_lfs_object.reload.file.file.file } + end + end + context 'when credentials present' do let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" } let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) } @@ -211,17 +226,5 @@ describe Projects::LfsPointers::LfsDownloadService do subject.execute end end - - context 'when an lfs object with the same oid already exists' do - before do - create(:lfs_object, oid: oid) - end - - it 'does not download the file' do - expect(subject).not_to receive(:download_lfs_file!) - - subject.execute - end - end end end diff --git a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb index 9dac29765a2..e94d8a85987 100644 --- a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb @@ -24,7 +24,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do describe '#execute' do context 'when no lfs pointer is linked' do before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([]) allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links) expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original end @@ -35,12 +34,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do subject.execute end - it 'links existent lfs objects to the project' do - expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute) - - subject.execute - end - it 'retrieves the download links of non existent objects' do expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids) @@ -48,32 +41,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do end end - context 'when some lfs objects are linked' do - before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys) - allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links) - end - - it 'retrieves the download links of non existent objects' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids) - - subject.execute - end - end - - context 'when all lfs objects are linked' do - before do - allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys) - allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute) - end - - it 'retrieves no download links' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original - - expect(subject.execute).to be_empty - end - end - context 'when lfsconfig file exists' do before do allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n") diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb index 63ebbcb93f9..3a306f80b3c 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -7,6 +7,7 @@ RSpec.shared_context 'GroupPolicy context' do let_it_be(:maintainer) { create(:user) } let_it_be(:owner) { create(:user) } let_it_be(:admin) { create(:admin) } + let_it_be(:non_group_member) { create(:user) } let_it_be(:group, refind: true) { create(:group, :private, :owner_subgroup_creation_only) } let(:guest_permissions) do diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 73087befad2..662c64647d6 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -69,13 +69,39 @@ RSpec.shared_examples 'handle uploads' do end describe "GET #show" do + let(:filename) { "rails_sample.jpg" } + + let(:upload_service) do + UploadService.new(model, jpg, uploader_class).execute + end + let(:show_upload) do - get :show, params: params.merge(secret: secret, filename: "rails_sample.jpg") + get :show, params: params.merge(secret: secret, filename: filename) end before do allow(FileUploader).to receive(:generate_secret).and_return(secret) - UploadService.new(model, jpg, uploader_class).execute + upload_service + end + + context 'when the secret is invalid' do + let(:secret) { "../../../../../../../../" } + let(:filename) { "Gemfile.lock" } + let(:upload_service) { nil } + + it 'responds with status 404' do + show_upload + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'is a working exploit without the validation' do + allow_any_instance_of(FileUploader).to receive(:secret) { secret } + + show_upload + + expect(response).to have_gitlab_http_status(:ok) + end end context 'when accessing a specific upload via different model' do diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index 9f053f03b0e..474515b537c 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -7,7 +7,7 @@ describe FileMover do let(:user) { create(:user) } let(:filename) { 'banana_sample.gif' } - let(:secret) { 'secret55' } + let(:secret) { SecureRandom.hex } let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", secret, filename) } let(:temp_description) do @@ -47,8 +47,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test  "\ - "same  ") + .to eq("test  "\ + "same  ") end it 'updates existing upload record' do @@ -75,8 +75,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test  "\ - "same  ") + .to eq("test  "\ + "same  ") end it 'does not change the upload record' do @@ -101,8 +101,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test  "\ - "same  ") + .to eq("test  "\ + "same  ") end it 'creates new target upload record an delete the old upload' do @@ -121,8 +121,8 @@ describe FileMover do subject expect(snippet.reload.description) - .to eq("test  "\ - "same  ") + .to eq("test  "\ + "same  ") end it 'does not change the upload record' do @@ -138,12 +138,8 @@ describe FileMover do let(:temp_file_path) { File.join("uploads/-/system/user/#{user.id}", '..', 'another_subdir_of_temp') } it 'does not trigger move if path is outside designated directory' do - stub_file_mover("uploads/-/system/user/#{user.id}/another_subdir_of_temp") expect(FileUtils).not_to receive(:move) - - subject - - expect(snippet.reload.description).to eq(temp_description) + expect { subject }.to raise_error(FileUploader::InvalidSecret) end end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index beb7aa3cf2c..efdbea85d4a 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -6,7 +6,8 @@ describe FileUploader do let(:group) { create(:group, name: 'awesome') } let(:project) { create(:project, :legacy_storage, namespace: group, name: 'project') } let(:uploader) { described_class.new(project, :avatar) } - let(:upload) { double(model: project, path: 'secret/foo.jpg') } + let(:upload) { double(model: project, path: "#{secret}/foo.jpg") } + let(:secret) { "55dc16aa0edd05693fd98b5051e83321" } # this would be nicer as SecureRandom.hex, but the shared_examples breaks subject { uploader } @@ -14,7 +15,7 @@ describe FileUploader do include_examples 'builds correct paths', store_dir: %r{awesome/project/\h+}, upload_path: %r{\h+/<filename>}, - absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg} + absolute_path: %r{#{described_class.root}/awesome/project/55dc16aa0edd05693fd98b5051e83321/foo.jpg} end context 'legacy storage' do @@ -51,11 +52,11 @@ describe FileUploader do end describe 'initialize' do - let(:uploader) { described_class.new(double, secret: 'secret') } + let(:uploader) { described_class.new(double, secret: secret) } it 'accepts a secret parameter' do expect(described_class).not_to receive(:generate_secret) - expect(uploader.secret).to eq('secret') + expect(uploader.secret).to eq(secret) end end @@ -154,8 +155,39 @@ describe FileUploader do describe '#secret' do it 'generates a secret if none is provided' do - expect(described_class).to receive(:generate_secret).and_return('secret') - expect(uploader.secret).to eq('secret') + expect(described_class).to receive(:generate_secret).and_return(secret) + expect(uploader.secret).to eq(secret) + expect(uploader.secret.size).to eq(32) + end + + context "validation" do + before do + uploader.instance_variable_set(:@secret, secret) + end + + context "32-byte hexadecimal" do + let(:secret) { SecureRandom.hex } + + it "returns the secret" do + expect(uploader.secret).to eq(secret) + end + end + + context "10-byte hexadecimal" do + let(:secret) { SecureRandom.hex(10) } + + it "returns the secret" do + expect(uploader.secret).to eq(secret) + end + end + + context "invalid secret supplied" do + let(:secret) { "%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2F%2E%2E%2Fgrafana%2Fconf%2F" } + + it "raises an exception" do + expect { uploader.secret }.to raise_error(described_class::InvalidSecret) + end + end end end diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb index ec652af222d..c211ec3607c 100644 --- a/spec/uploaders/personal_file_uploader_spec.rb +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -6,12 +6,13 @@ describe PersonalFileUploader do let(:model) { create(:personal_snippet) } let(:uploader) { described_class.new(model) } let(:upload) { create(:upload, :personal_snippet_upload) } + let(:secret) { SecureRandom.hex } subject { uploader } shared_examples '#base_dir' do before do - subject.instance_variable_set(:@secret, 'secret') + subject.instance_variable_set(:@secret, secret) end it 'is prefixed with uploads/-/system' do @@ -23,12 +24,12 @@ describe PersonalFileUploader do shared_examples '#to_h' do before do - subject.instance_variable_set(:@secret, 'secret') + subject.instance_variable_set(:@secret, secret) end it 'is correct' do allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name')) - expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name" + expected_url = "/uploads/-/system/personal_snippet/#{model.id}/#{secret}/file_name" expect(uploader.to_h).to eq( alt: 'file_name', diff --git a/spec/validators/addressable_url_validator_spec.rb b/spec/validators/addressable_url_validator_spec.rb index e8a44f7a12a..46b1bebb074 100644 --- a/spec/validators/addressable_url_validator_spec.rb +++ b/spec/validators/addressable_url_validator_spec.rb @@ -5,6 +5,9 @@ require 'spec_helper' describe AddressableUrlValidator do let!(:badge) { build(:badge, link_url: 'http://www.example.com') } + let(:validator) { described_class.new(validator_options.reverse_merge(attributes: [:link_url])) } + let(:validator_options) { {} } + subject { validator.validate(badge) } include_examples 'url validator examples', described_class::DEFAULT_OPTIONS[:schemes] @@ -114,6 +117,19 @@ describe AddressableUrlValidator do end end + context 'when blocked_message is set' do + let(:message) { 'is not allowed due to: %{exception_message}' } + let(:validator_options) { { blocked_message: message } } + + it 'blocks url with provided error message' do + badge.link_url = 'javascript:alert(window.opener.document.location)' + + subject + + expect(badge.errors.first[1]).to eq 'is not allowed due to: Only allowed schemes are http, https' + end + end + context 'when allow_nil is set to true' do let(:validator) { described_class.new(attributes: [:link_url], allow_nil: true) } |