From 35b93f9d223e132a66501920ba84d5ece1013aab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 2 Nov 2015 15:14:34 +0000 Subject: [PATCH 001/134] Add notice about offline migrations --- doc/development/migration_style_guide.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 4fa1961fde9..4e108c17871 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -8,6 +8,8 @@ In addition, having to take a server offline for a an upgrade small or big is a big burden for most organizations. For this reason it is important that your migrations are written carefully, can be applied online and adhere to the style guide below. +It's advised to have offline migrations only in major GitLab releases. + When writing your migrations, also consider that databases might have stale data or inconsistencies and guard for that. Try to make as little assumptions as possible about the state of the database. @@ -33,6 +35,8 @@ It is always preferable to have a migration run online. If you expect the migrat to take particularly long (for instance, if it loops through all notes), this is valuable information to add. +If you don't provide the information it means that a migration is safe to run online. + ### Reversibility Your migration should be reversible. This is very important, as it should @@ -85,4 +89,4 @@ select_all("SELECT name, COUNT(id) as cnt FROM tags GROUP BY name HAVING COUNT(i execute("UPDATE taggings SET tag_id = #{origin_tag_id} WHERE tag_id IN(#{duplicate_ids.join(",")})") execute("DELETE FROM tags WHERE id IN(#{duplicate_ids.join(",")})") end -``` +``` \ No newline at end of file From 055afab5c7d33d061d339c270bd258ed847450f3 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Mon, 1 Feb 2016 23:58:04 +0100 Subject: [PATCH 002/134] Make the CI permission model simpler This MR simplifies CI permission model: - read_build: allows to read a list of builds, artifacts and trace - update_build: allows to cancel and retry builds - create_build: allows to create builds from gitlab-ci.yml (not yet implemented) - admin_build: allows to manage triggers, runners and variables - read_commit_status: allows to read a list of commit statuses (including the overall of builds) - create_commit_status: allows to create a new commit status using API Remove all extra methods to manage permission. Made all controllers to use explicitly the new permissions. --- app/controllers/ci/application_controller.rb | 2 +- .../projects/artifacts_controller.rb | 12 +------ app/controllers/projects/builds_controller.rb | 9 ++--- app/controllers/projects/commit_controller.rb | 12 ++----- .../projects/runner_projects_controller.rb | 2 +- .../projects/runners_controller.rb | 2 +- .../projects/triggers_controller.rb | 2 +- .../projects/variables_controller.rb | 2 +- app/helpers/projects_helper.rb | 2 +- app/models/ability.rb | 34 ++++++++++++++----- app/views/admin/builds/_build.html.haml | 6 ++-- app/views/projects/builds/index.html.haml | 2 +- app/views/projects/builds/show.html.haml | 5 ++- app/views/projects/commit/_builds.html.haml | 2 +- .../commit_statuses/_commit_status.html.haml | 6 ++-- lib/api/builds.rb | 29 +++++++++++----- lib/api/commit_statuses.rb | 2 +- lib/api/triggers.rb | 8 ++--- lib/api/variables.rb | 2 +- spec/requests/api/builds_spec.rb | 8 ++--- 20 files changed, 78 insertions(+), 71 deletions(-) diff --git a/app/controllers/ci/application_controller.rb b/app/controllers/ci/application_controller.rb index c420b59c3a2..59c77653509 100644 --- a/app/controllers/ci/application_controller.rb +++ b/app/controllers/ci/application_controller.rb @@ -13,7 +13,7 @@ module Ci end def authorize_manage_builds! - unless can?(current_user, :manage_builds, project) + unless can?(current_user, :update_build, project) return page_404 end end diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index f159a6d6dc6..cfea1266516 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -1,6 +1,6 @@ class Projects::ArtifactsController < Projects::ApplicationController layout 'project' - before_action :authorize_read_build_artifacts! + before_action :authorize_read_build! def download unless artifacts_file.file_storage? @@ -43,14 +43,4 @@ class Projects::ArtifactsController < Projects::ApplicationController def artifacts_file @artifacts_file ||= build.artifacts_file end - - def authorize_read_build_artifacts! - unless can?(current_user, :read_build_artifacts, @project) - if current_user.nil? - return authenticate_user! - else - return render_404 - end - end - end end diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb index 92d9699fe84..9e89296e71d 100644 --- a/app/controllers/projects/builds_controller.rb +++ b/app/controllers/projects/builds_controller.rb @@ -1,7 +1,8 @@ class Projects::BuildsController < Projects::ApplicationController before_action :build, except: [:index, :cancel_all] - before_action :authorize_manage_builds!, except: [:index, :show, :status] + before_action :authorize_read_build!, except: [:cancel, :cancel_all, :retry] + before_action :authorize_update_build!, except: [:index, :show, :status] layout "project" @@ -69,10 +70,4 @@ class Projects::BuildsController < Projects::ApplicationController def build_path(build) namespace_project_build_path(build.project.namespace, build.project, build) end - - def authorize_manage_builds! - unless can?(current_user, :manage_builds, project) - return render_404 - end - end end diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index f5a169e5aa9..2bf367d2a25 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -4,10 +4,10 @@ class Projects::CommitController < Projects::ApplicationController # Authorize before_action :require_non_empty_project - before_action :authorize_download_code!, except: [:cancel_builds] - before_action :authorize_manage_builds!, only: [:cancel_builds] + before_action :authorize_download_code!, except: [:cancel_builds, :retry_builds] + before_action :authorize_update_build!, only: [:cancel_builds, :retry_builds] + before_action :authorize_read_commit_status!, only: [:builds] before_action :commit - before_action :authorize_manage_builds!, only: [:cancel_builds, :retry_builds] before_action :define_show_vars, only: [:show, :builds] def show @@ -77,10 +77,4 @@ class Projects::CommitController < Projects::ApplicationController @statuses = ci_commit.statuses if ci_commit end - - def authorize_manage_builds! - unless can?(current_user, :manage_builds, project) - return render_404 - end - end end diff --git a/app/controllers/projects/runner_projects_controller.rb b/app/controllers/projects/runner_projects_controller.rb index e2785caa2fb..bedeb4a295c 100644 --- a/app/controllers/projects/runner_projects_controller.rb +++ b/app/controllers/projects/runner_projects_controller.rb @@ -1,5 +1,5 @@ class Projects::RunnerProjectsController < Projects::ApplicationController - before_action :authorize_admin_project! + before_action :authorize_admin_build! layout 'project_settings' diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 4993b2648a5..0dd2d6a99be 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -1,6 +1,6 @@ class Projects::RunnersController < Projects::ApplicationController + before_action :authorize_admin_build! before_action :set_runner, only: [:edit, :update, :destroy, :pause, :resume, :show] - before_action :authorize_admin_project! layout 'project_settings' diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 30adfad1daa..92359745cec 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -1,5 +1,5 @@ class Projects::TriggersController < Projects::ApplicationController - before_action :authorize_admin_project! + before_action :authorize_admin_build! layout 'project_settings' diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 10efafea9db..00234654578 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -1,5 +1,5 @@ class Projects::VariablesController < Projects::ApplicationController - before_action :authorize_admin_project! + before_action :authorize_admin_build! layout 'project_settings' diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 8c8b355028c..4543eff0d63 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -126,7 +126,7 @@ module ProjectsHelper nav_tabs << :merge_requests end - if project.builds_enabled? && can?(current_user, :read_build, project) + if can?(current_user, :read_build, project) nav_tabs << :builds end diff --git a/app/models/ability.rb b/app/models/ability.rb index ab59a3506a2..e58e7a40273 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -52,10 +52,15 @@ class Ability :read_project_member, :read_merge_request, :read_note, + :read_commit_status, :read_build, :download_code ] + if project.restrict_builds? + rules -= :read_build + end + rules - project_disabled_features_rules(project) else [] @@ -113,6 +118,10 @@ class Ability if project.public? || project.internal? rules.push(*public_project_rules) + + if team.guest?(user) && project.restrict_builds? + rules -= named_abilities('build') + end end if project.owner == user || user.admin? @@ -134,7 +143,9 @@ class Ability def public_project_rules @public_project_rules ||= project_guest_rules + [ :download_code, - :fork_project + :fork_project, + :read_commit_status, + :read_build, ] end @@ -149,7 +160,7 @@ class Ability :read_project_member, :read_merge_request, :read_note, - :read_build, + :read_commit_status, :create_project, :create_issue, :create_note @@ -158,24 +169,25 @@ class Ability def project_report_rules @project_report_rules ||= project_guest_rules + [ - :create_commit_status, - :read_commit_statuses, - :read_build_artifacts, :download_code, :fork_project, :create_project_snippet, :update_issue, :admin_issue, - :admin_label + :admin_label, + :read_build, ] end def project_dev_rules @project_dev_rules ||= project_report_rules + [ :admin_merge_request, + :create_commit_status, + :update_commit_status, + :create_build, + :update_build, :create_merge_request, :create_wiki, - :manage_builds, :push_code ] end @@ -201,7 +213,9 @@ class Ability :admin_merge_request, :admin_note, :admin_wiki, - :admin_project + :admin_project, + :admin_commit_status, + :admin_build ] end @@ -240,6 +254,10 @@ class Ability rules += named_abilities('wiki') end + unless project.builds_enabled + rules += named_abilities('build') + end + rules end diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index c395bd908c3..bd7fb0b36f4 100644 --- a/app/views/admin/builds/_build.html.haml +++ b/app/views/admin/builds/_build.html.haml @@ -4,7 +4,7 @@ = ci_status_with_icon(build.status) %td.build-link - - if build.target_url + - if can?(current_user, :read_build, project) && build.target_url = link_to build.target_url do %strong Build ##{build.id} - else @@ -60,10 +60,10 @@ %td .pull-right - - if current_user && can?(current_user, :read_build_artifacts, project) && build.artifacts? + - if can?(current_user, :read_build, project) && build.artifacts? = link_to build.artifacts_download_url, title: 'Download artifacts' do %i.fa.fa-download - - if current_user && can?(current_user, :manage_builds, build.project) + - if current_user && can?(current_user, :update_build, build.project) - if build.active? - if build.cancel_url = link_to build.cancel_url, method: :post, title: 'Cancel' do diff --git a/app/views/projects/builds/index.html.haml b/app/views/projects/builds/index.html.haml index bbb6944a65a..2a2a4745bed 100644 --- a/app/views/projects/builds/index.html.haml +++ b/app/views/projects/builds/index.html.haml @@ -3,7 +3,7 @@ .project-issuable-filter .controls - - if can?(current_user, :manage_builds, @project) + - if can?(current_user, :update_build, @project) .pull-left.hidden-xs - if @all_builds.running_or_pending.any? = link_to 'Cancel running', cancel_all_namespace_project_builds_path(@project.namespace, @project), diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml index ba1fdc6f0e7..c43d6c3b427 100644 --- a/app/views/projects/builds/show.html.haml +++ b/app/views/projects/builds/show.html.haml @@ -89,8 +89,7 @@ Test coverage %h1 #{@build.coverage}% - - if current_user && can?(current_user, :read_build_artifacts, @project) && @build.artifacts? - + - if can?(current_user, :read_build, @project) && @build.artifacts? .build-widget.artifacts %h4.title Build artifacts .center @@ -102,7 +101,7 @@ .build-widget %h4.title Build ##{@build.id} - - if current_user && can?(current_user, :manage_builds, @project) + - if current_user && can?(current_user, :update_build, @project) .pull-right - if @build.cancel_url = link_to "Cancel", @build.cancel_url, class: 'btn btn-sm btn-danger', method: :post diff --git a/app/views/projects/commit/_builds.html.haml b/app/views/projects/commit/_builds.html.haml index 329aaa0bb8b..befad27666c 100644 --- a/app/views/projects/commit/_builds.html.haml +++ b/app/views/projects/commit/_builds.html.haml @@ -1,6 +1,6 @@ .gray-content-block.middle-block .pull-right - - if can?(current_user, :manage_builds, @ci_commit.project) + - if can?(current_user, :update_build, @ci_commit.project) - if @ci_commit.builds.latest.failed.any?(&:retryable?) = link_to "Retry failed", retry_builds_namespace_project_commit_path(@ci_commit.project.namespace, @ci_commit.project, @ci_commit.sha), class: 'btn btn-grouped btn-primary', method: :post diff --git a/app/views/projects/commit_statuses/_commit_status.html.haml b/app/views/projects/commit_statuses/_commit_status.html.haml index 2e3c956ddc4..fba4405cb7d 100644 --- a/app/views/projects/commit_statuses/_commit_status.html.haml +++ b/app/views/projects/commit_statuses/_commit_status.html.haml @@ -8,7 +8,7 @@ = ci_status_with_icon(commit_status.status) %td.commit_status-link - - if commit_status.target_url + - if can?(current_user, :read_build, commit_status.project) && commit_status.target_url = link_to commit_status.target_url do %strong ##{commit_status.id} - else @@ -66,10 +66,10 @@ %td .pull-right - - if current_user && can?(current_user, :read_build_artifacts, commit_status.project) && commit_status.artifacts_download_url + - if can?(current_user, :read_build, commit_status.project) && commit_status.artifacts_download_url = link_to commit_status.artifacts_download_url, title: 'Download artifacts' do %i.fa.fa-download - - if current_user && can?(current_user, :manage_builds, commit_status.project) + - if can?(current_user, :update_build, commit_status.project) - if commit_status.active? - if commit_status.cancel_url = link_to commit_status.cancel_url, method: :post, title: 'Cancel' do diff --git a/lib/api/builds.rb b/lib/api/builds.rb index d293f988165..a8bd3842ce4 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -13,11 +13,12 @@ module API # Example Request: # GET /projects/:id/builds get ':id/builds' do + builds = user_project.builds.order('id DESC') builds = filter_builds(builds, params[:scope]) present paginate(builds), with: Entities::Build, - user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) + user_can_download_artifacts: can?(current_user, :read_build, user_project) end # Get builds for a specific commit of a project @@ -30,6 +31,8 @@ module API # Example Request: # GET /projects/:id/repository/commits/:sha/builds get ':id/repository/commits/:sha/builds' do + authorize_read_builds! + commit = user_project.ci_commits.find_by_sha(params[:sha]) return not_found! unless commit @@ -37,7 +40,7 @@ module API builds = filter_builds(builds, params[:scope]) present paginate(builds), with: Entities::Build, - user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) + user_can_download_artifacts: can?(current_user, :read_build, user_project) end # Get a specific build of a project @@ -48,11 +51,13 @@ module API # Example Request: # GET /projects/:id/builds/:build_id get ':id/builds/:build_id' do + authorize_read_builds! + build = get_build(params[:build_id]) return not_found!(build) unless build present build, with: Entities::Build, - user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) + user_can_download_artifacts: can?(current_user, :read_build, user_project) end # Get a trace of a specific build of a project @@ -67,6 +72,8 @@ module API # is saved in the DB instead of file). But before that, we need to consider how to replace the value of # `runners_token` with some mask (like `xxxxxx`) when sending trace file directly by workhorse. get ':id/builds/:build_id/trace' do + authorize_read_builds! + build = get_build(params[:build_id]) return not_found!(build) unless build @@ -86,7 +93,7 @@ module API # example request: # post /projects/:id/build/:build_id/cancel post ':id/builds/:build_id/cancel' do - authorize_manage_builds! + authorize_update_builds! build = get_build(params[:build_id]) return not_found!(build) unless build @@ -94,7 +101,7 @@ module API build.cancel present build, with: Entities::Build, - user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) + user_can_download_artifacts: can?(current_user, :read_build, user_project) end # Retry a specific build of a project @@ -105,7 +112,7 @@ module API # example request: # post /projects/:id/build/:build_id/retry post ':id/builds/:build_id/retry' do - authorize_manage_builds! + authorize_update_builds! build = get_build(params[:build_id]) return forbidden!('Build is not retryable') unless build && build.retryable? @@ -113,7 +120,7 @@ module API build = Ci::Build.retry(build) present build, with: Entities::Build, - user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project) + user_can_download_artifacts: can?(current_user, :read_build, user_project) end end @@ -141,8 +148,12 @@ module API builds.where(status: available_statuses && scope) end - def authorize_manage_builds! - authorize! :manage_builds, user_project + def authorize_read_builds! + authorize! :read_build, user_project + end + + def authorize_update_builds! + authorize! :update_build, user_project end end end diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 1162271f5fc..9422d438d21 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -18,7 +18,7 @@ module API # Examples: # GET /projects/:id/repository/commits/:sha/statuses get ':id/repository/commits/:sha/statuses' do - authorize! :read_commit_statuses, user_project + authorize! :read_commit_status, user_project sha = params[:sha] ci_commit = user_project.ci_commit(sha) not_found! 'Commit' unless ci_commit diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 5e4964f446c..d1d07394e92 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -54,7 +54,7 @@ module API # GET /projects/:id/triggers get ':id/triggers' do authenticate! - authorize_admin_project + authorize! :admin_build, user_project triggers = user_project.triggers.includes(:trigger_requests) triggers = paginate(triggers) @@ -71,7 +71,7 @@ module API # GET /projects/:id/triggers/:token get ':id/triggers/:token' do authenticate! - authorize_admin_project + authorize! :admin_build, user_project trigger = user_project.triggers.find_by(token: params[:token].to_s) return not_found!('Trigger') unless trigger @@ -87,7 +87,7 @@ module API # POST /projects/:id/triggers post ':id/triggers' do authenticate! - authorize_admin_project + authorize! :admin_build, user_project trigger = user_project.triggers.create @@ -103,7 +103,7 @@ module API # DELETE /projects/:id/triggers/:token delete ':id/triggers/:token' do authenticate! - authorize_admin_project + authorize! :admin_build, user_project trigger = user_project.triggers.find_by(token: params[:token].to_s) return not_found!('Trigger') unless trigger diff --git a/lib/api/variables.rb b/lib/api/variables.rb index d9a055f6c92..f6495071a11 100644 --- a/lib/api/variables.rb +++ b/lib/api/variables.rb @@ -2,7 +2,7 @@ module API # Projects variables API class Variables < Grape::API before { authenticate! } - before { authorize_admin_project } + before { authorize! :admin_build, user_project } resource :projects do # Get project variables diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index 8c9f5a382b7..6c07802db8b 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -113,7 +113,7 @@ describe API::API, api: true do describe 'POST /projects/:id/builds/:build_id/cancel' do context 'authorized user' do - context 'user with :manage_builds persmission' do + context 'user with :update_build persmission' do it 'should cancel running or pending build' do post api("/projects/#{project.id}/builds/#{build.id}/cancel", user) @@ -122,7 +122,7 @@ describe API::API, api: true do end end - context 'user without :manage_builds permission' do + context 'user without :update_build permission' do it 'should not cancel build' do post api("/projects/#{project.id}/builds/#{build.id}/cancel", user2) @@ -142,7 +142,7 @@ describe API::API, api: true do describe 'POST /projects/:id/builds/:build_id/retry' do context 'authorized user' do - context 'user with :manage_builds persmission' do + context 'user with :update_build persmission' do it 'should retry non-running build' do post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user) @@ -152,7 +152,7 @@ describe API::API, api: true do end end - context 'user without :manage_builds permission' do + context 'user without :update_build permission' do it 'should not retry build' do post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user2) From 627909c2a4a938c6387afa459ef4dc815fe9fb5a Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 2 Feb 2016 17:59:37 +0100 Subject: [PATCH 003/134] Add CI setting: allow_guest_to_access_builds Add the `read_build` ability if user is anonymous or guest and allow_guest_to_access_builds is enabled. --- app/controllers/projects_controller.rb | 1 + app/models/ability.rb | 14 ++++---- app/views/projects/edit.html.haml | 8 +++-- ...dd_allow_guest_to_access_builds_project.rb | 5 +++ db/schema.rb | 35 ++++++++++--------- doc/permissions/permissions.md | 9 +++++ 6 files changed, 47 insertions(+), 25 deletions(-) create mode 100644 db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 4df5095bd94..153e7caaae3 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -227,6 +227,7 @@ class ProjectsController < ApplicationController :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id, :default_branch, :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar, :builds_enabled, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex, + :allow_guest_to_access_builds, ) end diff --git a/app/models/ability.rb b/app/models/ability.rb index e58e7a40273..313c6f049b7 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -53,12 +53,11 @@ class Ability :read_merge_request, :read_note, :read_commit_status, - :read_build, :download_code ] - if project.restrict_builds? - rules -= :read_build + if project.allow_guest_to_access_builds? + rules += :read_build end rules - project_disabled_features_rules(project) @@ -114,13 +113,17 @@ class Ability elsif team.guest?(user) rules.push(*project_guest_rules) + + if project.allow_guest_to_access_builds? + rules += :read_build + end end if project.public? || project.internal? rules.push(*public_project_rules) - if team.guest?(user) && project.restrict_builds? - rules -= named_abilities('build') + if project.allow_guest_to_access_builds? + rules += :read_build end end @@ -145,7 +148,6 @@ class Ability :download_code, :fork_project, :read_commit_status, - :read_build, ] end diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 8a99aceef7f..e3165caad05 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -157,8 +157,12 @@ %li phpunit --coverage-text --colors=never (PHP) - %code ^\s*Lines:\s*\d+.\d+\% - - + .form-group + .col-sm-offset-2.col-sm-10 + .checkbox + = f.label :allow_guest_to_access_builds do + = f.check_box :allow_guest_to_access_builds + Allow guest to access builds (including build logs and artifacts) %fieldset.features %legend Advanced settings diff --git a/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb new file mode 100644 index 00000000000..69ce8d08bba --- /dev/null +++ b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb @@ -0,0 +1,5 @@ +class AddAllowGuestToAccessBuildsProject < ActiveRecord::Migration + def change + add_column :projects, :allow_guest_to_access_builds, :boolean, default: true, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 2ad2c23fba5..a04e812ae22 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160128233227) do +ActiveRecord::Schema.define(version: 20160202164642) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -650,34 +650,35 @@ ActiveRecord::Schema.define(version: 20160128233227) do t.datetime "created_at" t.datetime "updated_at" t.integer "creator_id" - t.boolean "issues_enabled", default: true, null: false - t.boolean "wall_enabled", default: true, null: false - t.boolean "merge_requests_enabled", default: true, null: false - t.boolean "wiki_enabled", default: true, null: false + t.boolean "issues_enabled", default: true, null: false + t.boolean "wall_enabled", default: true, null: false + t.boolean "merge_requests_enabled", default: true, null: false + t.boolean "wiki_enabled", default: true, null: false t.integer "namespace_id" - t.string "issues_tracker", default: "gitlab", null: false + t.string "issues_tracker", default: "gitlab", null: false t.string "issues_tracker_id" - t.boolean "snippets_enabled", default: true, null: false + t.boolean "snippets_enabled", default: true, null: false t.datetime "last_activity_at" t.string "import_url" - t.integer "visibility_level", default: 0, null: false - t.boolean "archived", default: false, null: false + t.integer "visibility_level", default: 0, null: false + t.boolean "archived", default: false, null: false t.string "avatar" t.string "import_status" - t.float "repository_size", default: 0.0 - t.integer "star_count", default: 0, null: false + t.float "repository_size", default: 0.0 + t.integer "star_count", default: 0, null: false t.string "import_type" t.string "import_source" - t.integer "commit_count", default: 0 + t.integer "commit_count", default: 0 t.text "import_error" t.integer "ci_id" - t.boolean "builds_enabled", default: true, null: false - t.boolean "shared_runners_enabled", default: true, null: false + t.boolean "builds_enabled", default: true, null: false + t.boolean "shared_runners_enabled", default: true, null: false t.string "runners_token" t.string "build_coverage_regex" - t.boolean "build_allow_git_fetch", default: true, null: false - t.integer "build_timeout", default: 3600, null: false - t.boolean "pending_delete", default: false + t.boolean "build_allow_git_fetch", default: true, null: false + t.integer "build_timeout", default: 3600, null: false + t.boolean "pending_delete", default: false + t.boolean "allow_guest_to_access_builds", default: true, null: false end add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree diff --git a/doc/permissions/permissions.md b/doc/permissions/permissions.md index 1be78ac1823..168e7d143ee 100644 --- a/doc/permissions/permissions.md +++ b/doc/permissions/permissions.md @@ -18,6 +18,9 @@ documentation](../workflow/add-user/add-user.md). |---------------------------------------|---------|------------|-------------|----------|--------| | Create new issue | ✓ | ✓ | ✓ | ✓ | ✓ | | Leave comments | ✓ | ✓ | ✓ | ✓ | ✓ | +| See a list of builds | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | +| See a build log | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | +| Download and browse build artifacts | ✓ [^1] | ✓ | ✓ | ✓ | ✓ | | Pull project code | | ✓ | ✓ | ✓ | ✓ | | Download project | | ✓ | ✓ | ✓ | ✓ | | Create code snippets | | ✓ | ✓ | ✓ | ✓ | @@ -31,6 +34,7 @@ documentation](../workflow/add-user/add-user.md). | Remove non-protected branches | | | ✓ | ✓ | ✓ | | Add tags | | | ✓ | ✓ | ✓ | | Write a wiki | | | ✓ | ✓ | ✓ | +| Cancel and retry builds | | | ✓ | ✓ | ✓ | | Create new milestones | | | | ✓ | ✓ | | Add new team members | | | | ✓ | ✓ | | Push to protected branches | | | | ✓ | ✓ | @@ -40,12 +44,17 @@ documentation](../workflow/add-user/add-user.md). | Edit project | | | | ✓ | ✓ | | Add deploy keys to project | | | | ✓ | ✓ | | Configure project hooks | | | | ✓ | ✓ | +| Manage runners | | | | ✓ | ✓ | +| Manage build triggers | | | | ✓ | ✓ | +| Manage variables | | | | ✓ | ✓ | | Switch visibility level | | | | | ✓ | | Transfer project to another namespace | | | | | ✓ | | Remove project | | | | | ✓ | | Force push to protected branches | | | | | | | Remove protected branches | | | | | | +[^1]: If **Allow guest to access builds** is enabled in CI settings + ## Group In order for a group to appear as public and be browsable, it must contain at From 8670411ae7acb93b5113634a3ae5e476ef6d2aee Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 3 Feb 2016 11:24:14 +0100 Subject: [PATCH 004/134] Clean Ci::ApplicationController from unused permission related code --- app/controllers/ci/application_controller.rb | 47 -------------------- app/controllers/ci/projects_controller.rb | 5 +-- 2 files changed, 2 insertions(+), 50 deletions(-) diff --git a/app/controllers/ci/application_controller.rb b/app/controllers/ci/application_controller.rb index 59c77653509..5bb7d499cdc 100644 --- a/app/controllers/ci/application_controller.rb +++ b/app/controllers/ci/application_controller.rb @@ -3,52 +3,5 @@ module Ci def self.railtie_helpers_paths "app/helpers/ci" end - - private - - def authorize_access_project! - unless can?(current_user, :read_project, project) - return page_404 - end - end - - def authorize_manage_builds! - unless can?(current_user, :update_build, project) - return page_404 - end - end - - def authenticate_admin! - return render_404 unless current_user.is_admin? - end - - def authorize_manage_project! - unless can?(current_user, :admin_project, project) - return page_404 - end - end - - def page_404 - render file: "#{Rails.root}/public/404.html", status: 404, layout: false - end - - def default_headers - headers['X-Frame-Options'] = 'DENY' - headers['X-XSS-Protection'] = '1; mode=block' - end - - # JSON for infinite scroll via Pager object - def pager_json(partial, count) - html = render_to_string( - partial, - layout: false, - formats: [:html] - ) - - render json: { - html: html, - count: count - } - end end end diff --git a/app/controllers/ci/projects_controller.rb b/app/controllers/ci/projects_controller.rb index 3004c2d27f0..711c2847d5e 100644 --- a/app/controllers/ci/projects_controller.rb +++ b/app/controllers/ci/projects_controller.rb @@ -1,8 +1,7 @@ module Ci class ProjectsController < Ci::ApplicationController - before_action :project, except: [:index] - before_action :authenticate_user!, except: [:index, :build, :badge] - before_action :authorize_access_project!, except: [:index, :badge] + before_action :project + before_action :authorize_read_project!, except: [:badge] before_action :no_cache, only: [:badge] protect_from_forgery From e80c79e3ad0efe505541ebac0b149b750cd1cc60 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 3 Feb 2016 12:41:16 +0100 Subject: [PATCH 005/134] Fix build errors --- app/models/ability.rb | 6 +++--- app/views/projects/edit.html.haml | 6 +++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 313c6f049b7..a9246dd3dd5 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -57,7 +57,7 @@ class Ability ] if project.allow_guest_to_access_builds? - rules += :read_build + rules << :read_build end rules - project_disabled_features_rules(project) @@ -115,7 +115,7 @@ class Ability rules.push(*project_guest_rules) if project.allow_guest_to_access_builds? - rules += :read_build + rules << :read_build end end @@ -123,7 +123,7 @@ class Ability rules.push(*public_project_rules) if project.allow_guest_to_access_builds? - rules += :read_build + rules << :read_build end end diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index e3165caad05..fd61ce6a99a 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -130,6 +130,7 @@ %strong git fetch %br %span.descr Faster + .form-group = f.label :build_timeout_in_minutes, 'Timeout', class: 'control-label' .col-sm-10 @@ -157,12 +158,15 @@ %li phpunit --coverage-text --colors=never (PHP) - %code ^\s*Lines:\s*\d+.\d+\% + .form-group .col-sm-offset-2.col-sm-10 .checkbox = f.label :allow_guest_to_access_builds do = f.check_box :allow_guest_to_access_builds - Allow guest to access builds (including build logs and artifacts) + %strong Guests can see builds + .help-block Allow guests and anonymous users to access builds including build trace and artifacts + %fieldset.features %legend Advanced settings From c3d897a9a382b0b3354d29726add5af8c322beb4 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 3 Feb 2016 13:33:47 +0100 Subject: [PATCH 006/134] Properly handle commit status permissions (for a build) --- app/models/ability.rb | 22 +++++++++++++++++++ .../commit_statuses/_commit_status.html.haml | 8 +++---- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index a9246dd3dd5..bf24749b173 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -5,6 +5,12 @@ class Ability return [] unless user.is_a?(User) return [] if user.blocked? + if subject.is_a?(CommitStatus) + rules = project_abilities(user, subject) + rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) + return rules + end + case subject.class.name when "Project" then project_abilities(user, subject) when "Issue" then issue_abilities(user, subject) @@ -25,6 +31,10 @@ class Ability case true when subject.is_a?(PersonalSnippet) anonymous_personal_snippet_abilities(subject) + when subject.is_a?(CommitStatus) + rules = anonymous_project_abilities(subject) + rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) + rules when subject.is_a?(Project) || subject.respond_to?(:project) anonymous_project_abilities(subject) when subject.is_a?(Group) || subject.respond_to?(:group) @@ -396,6 +406,18 @@ class Ability rules end + def filter_build_abilities(rules) + # If we can't read build we should also not have that + # ability when looking at this in context of commit_status + unless rules.include?(:read_build) + rules -= [:read_commit_status] + end + unless rules.include?(:update_build) + rules -= [:update_commit_status] + end + rules + end + def abilities @abilities ||= begin abilities = Six.new diff --git a/app/views/projects/commit_statuses/_commit_status.html.haml b/app/views/projects/commit_statuses/_commit_status.html.haml index fba4405cb7d..c02c5983ac8 100644 --- a/app/views/projects/commit_statuses/_commit_status.html.haml +++ b/app/views/projects/commit_statuses/_commit_status.html.haml @@ -1,6 +1,6 @@ %tr.commit_status %td.status - - if commit_status.target_url + - if can?(current_user, :read_commit_status, commit_status) && commit_status.target_url = link_to commit_status.target_url, class: "ci-status ci-#{commit_status.status}" do = ci_icon_for_status(commit_status.status) = commit_status.status @@ -8,7 +8,7 @@ = ci_status_with_icon(commit_status.status) %td.commit_status-link - - if can?(current_user, :read_build, commit_status.project) && commit_status.target_url + - if can?(current_user, :read_commit_status, commit_status) && commit_status.target_url = link_to commit_status.target_url do %strong ##{commit_status.id} - else @@ -66,10 +66,10 @@ %td .pull-right - - if can?(current_user, :read_build, commit_status.project) && commit_status.artifacts_download_url + - if can?(current_user, :read_commit_status, commit_status) && commit_status.artifacts_download_url = link_to commit_status.artifacts_download_url, title: 'Download artifacts' do %i.fa.fa-download - - if can?(current_user, :update_build, commit_status.project) + - if can?(current_user, :update_commit_status, commit_status) - if commit_status.active? - if commit_status.cancel_url = link_to commit_status.cancel_url, method: :post, title: 'Cancel' do From 020623f3bbdb25c07e31985f0fe072988ba0eff2 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 2 Feb 2016 15:51:48 +0100 Subject: [PATCH 007/134] Improve CI API specs related to operations on build Conflicts: spec/factories/ci/builds.rb --- spec/factories/ci/builds.rb | 15 ++++++++ spec/requests/ci/api/builds_spec.rb | 57 ++++++++++++++++++----------- 2 files changed, 50 insertions(+), 22 deletions(-) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index d2db77f6286..f0ae3a18561 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -65,5 +65,20 @@ FactoryGirl.define do build.trace = 'BUILD TRACE' end end + + trait :artifacts do + after(:create) do |build, _| + build.artifacts_file = + fixture_file_upload(Rails.root + + 'spec/fixtures/ci_build_artifacts.zip', + 'application/zip') + + build.artifacts_metadata = + fixture_file_upload(Rails.root + + 'spec/fixtures/ci_build_artifacts_metadata.gz', + 'application/x-gzip') + build.save! + end + end end end diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 244947762dd..e1b6f981538 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -151,8 +151,8 @@ describe Ci::API::API do context "Artifacts" do let(:file_upload) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') } let(:file_upload2) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'image/gif') } - let(:commit) { FactoryGirl.create(:ci_commit, project: project) } - let(:build) { FactoryGirl.create(:ci_build, commit: commit, runner_id: runner.id) } + let(:commit) { create(:ci_commit, project: project) } + let(:build) { create(:ci_build, commit: commit, runner_id: runner.id) } let(:authorize_url) { ci_api("/builds/#{build.id}/artifacts/authorize") } let(:post_url) { ci_api("/builds/#{build.id}/artifacts") } let(:delete_url) { ci_api("/builds/#{build.id}/artifacts") } @@ -349,33 +349,46 @@ describe Ci::API::API do end end - describe "DELETE /builds/:id/artifacts" do - before do - build.run! - post delete_url, token: build.token, file: file_upload + describe 'DELETE /builds/:id/artifacts' do + let(:build) { create(:ci_build, :artifacts) } + before { delete delete_url, token: build.token } + + it 'should respond valid status' do + expect(response.status).to eq(200) end - it "should delete artifact build" do - build.success - delete delete_url, token: build.token - expect(response.status).to eq(200) + it 'should remove artifacts file' do + expect(build.artifacts_file.exists?).to be_falsy + end + + it 'should remove artifacts metadata' do + expect(build.artifacts_metadata.exists?).to be_falsy end end - describe "GET /builds/:id/artifacts" do - before do - build.run! + describe 'GET /builds/:id/artifacts' do + before { get get_url, token: build.token } + + context 'build has artifacts' do + let(:build) { create(:ci_build, :artifacts) } + let(:download_headers) do + { 'Content-Transfer-Encoding'=>'binary', + 'Content-Disposition'=>'attachment; filename=ci_build_artifacts.zip' } + end + + it 'should respond with valid status' do + expect(response.status).to eq(200) + end + + it 'should download artifact' do + expect(response.headers).to include download_headers + end end - it "should download artifact" do - build.update_attributes(artifacts_file: file_upload) - get get_url, token: build.token - expect(response.status).to eq(200) - end - - it "should fail to download if no artifact uploaded" do - get get_url, token: build.token - expect(response.status).to eq(404) + context 'build does not has artifacts' do + it 'should respond with not found' do + expect(response.status).to eq(404) + end end end end From 53c917a6c6b410dbc56c75f3282ced8f95829d57 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 4 Feb 2016 10:16:21 +0100 Subject: [PATCH 008/134] Remove unmaintainable db schema comment from build factory --- spec/factories/ci/builds.rb | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index f0ae3a18561..c1b6ecd329a 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -1,30 +1,3 @@ -# == Schema Information -# -# Table name: builds -# -# id :integer not null, primary key -# project_id :integer -# status :string(255) -# finished_at :datetime -# trace :text -# created_at :datetime -# updated_at :datetime -# started_at :datetime -# runner_id :integer -# commit_id :integer -# coverage :float -# commands :text -# job_id :integer -# name :string(255) -# deploy :boolean default(FALSE) -# options :text -# allow_failure :boolean default(FALSE), not null -# stage :string(255) -# trigger_request_id :integer -# - -# Read about factories at https://github.com/thoughtbot/factory_girl - FactoryGirl.define do factory :ci_build, class: Ci::Build do name 'test' From bf6d69483725a99d20a88479e469f55567c7b9fd Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 4 Feb 2016 10:24:14 +0100 Subject: [PATCH 009/134] Extract shared context level up in build specs Also improve performance of specs by joining some of examples. --- spec/requests/ci/api/builds_spec.rb | 73 +++++++---------------------- 1 file changed, 17 insertions(+), 56 deletions(-) diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index e1b6f981538..01b369720ca 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/spec/requests/ci/api/builds_spec.rb @@ -160,12 +160,10 @@ describe Ci::API::API do let(:headers) { { "GitLab-Workhorse" => "1.0" } } let(:headers_with_token) { headers.merge(Ci::API::Helpers::BUILD_TOKEN_HEADER => build.token) } + before { build.run! } + describe "POST /builds/:id/artifacts/authorize" do context "should authorize posting artifact to running build" do - before do - build.run! - end - it "using token as parameter" do post authorize_url, { token: build.token }, headers expect(response.status).to eq(200) @@ -180,10 +178,6 @@ describe Ci::API::API do end context "should fail to post too large artifact" do - before do - build.run! - end - it "using token as parameter" do stub_application_setting(max_artifacts_size: 0) post authorize_url, { token: build.token, filesize: 100 }, headers @@ -197,8 +191,8 @@ describe Ci::API::API do end end - context "should get denied" do - it do + context 'token is invalid' do + it 'should respond with forbidden'do post authorize_url, { token: 'invalid', filesize: 100 } expect(response.status).to eq(403) end @@ -206,17 +200,13 @@ describe Ci::API::API do end describe "POST /builds/:id/artifacts" do - context "Disable sanitizer" do + context "disable sanitizer" do before do # by configuring this path we allow to pass temp file from any path allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/') end context "should post artifact to running build" do - before do - build.run! - end - it "uses regual file post" do upload_artifacts(file_upload, headers_with_token, false) expect(response.status).to eq(201) @@ -244,10 +234,7 @@ describe Ci::API::API do let(:stored_artifacts_file) { build.reload.artifacts_file.file } let(:stored_metadata_file) { build.reload.artifacts_metadata.file } - before do - build.run! - post(post_url, post_data, headers_with_token) - end + before { post(post_url, post_data, headers_with_token) } context 'post data accelerated by workhorse is correct' do let(:post_data) do @@ -257,11 +244,8 @@ describe Ci::API::API do 'metadata.name' => metadata.original_filename } end - it 'responds with valid status' do - expect(response.status).to eq(201) - end - it 'stores artifacts and artifacts metadata' do + expect(response.status).to eq(201) expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename) expect(stored_metadata_file.original_filename).to eq(metadata.original_filename) end @@ -282,56 +266,42 @@ describe Ci::API::API do end end - - context "should fail to post too large artifact" do - before do - build.run! - end - - it do + context "artifacts file is too large" do + it "should fail to post too large artifact" do stub_application_setting(max_artifacts_size: 0) upload_artifacts(file_upload, headers_with_token) expect(response.status).to eq(413) end end - context "should fail to post artifacts without file" do - before do - build.run! - end - - it do + context "artifacts post request does not contain file" do + it "should fail to post artifacts without file" do post post_url, {}, headers_with_token expect(response.status).to eq(400) end end - context "should fail to post artifacts without GitLab-Workhorse" do - before do - build.run! - end - - it do + context 'GitLab Workhorse is not configured' do + it "should fail to post artifacts without GitLab-Workhorse" do post post_url, { token: build.token }, {} expect(response.status).to eq(403) end end end - context "should fail to post artifacts for outside of tmp path" do + context "artifacts are being stored outside of tmp path" do before do # by configuring this path we allow to pass file from @tmpdir only # but all temporary files are stored in system tmp directory @tmpdir = Dir.mktmpdir allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return(@tmpdir) - build.run! end after do FileUtils.remove_entry @tmpdir end - it do + it "should fail to post artifacts for outside of tmp path" do upload_artifacts(file_upload, headers_with_token) expect(response.status).to eq(400) end @@ -353,15 +323,9 @@ describe Ci::API::API do let(:build) { create(:ci_build, :artifacts) } before { delete delete_url, token: build.token } - it 'should respond valid status' do + it 'should remove build artifacts' do expect(response.status).to eq(200) - end - - it 'should remove artifacts file' do expect(build.artifacts_file.exists?).to be_falsy - end - - it 'should remove artifacts metadata' do expect(build.artifacts_metadata.exists?).to be_falsy end end @@ -376,11 +340,8 @@ describe Ci::API::API do 'Content-Disposition'=>'attachment; filename=ci_build_artifacts.zip' } end - it 'should respond with valid status' do - expect(response.status).to eq(200) - end - it 'should download artifact' do + expect(response.status).to eq(200) expect(response.headers).to include download_headers end end From 5f7be11aa6d5d9edc31baa09b50ac21ee80533aa Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 11:09:42 +0100 Subject: [PATCH 010/134] Simplify abilities --- app/models/ability.rb | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index bf24749b173..e1767ed8dd1 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -5,10 +5,9 @@ class Ability return [] unless user.is_a?(User) return [] if user.blocked? + # We check with `is_a?`, because CommitStatus uses inheritance if subject.is_a?(CommitStatus) - rules = project_abilities(user, subject) - rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) - return rules + return commit_status_abilities(user, subject) end case subject.class.name @@ -32,9 +31,7 @@ class Ability when subject.is_a?(PersonalSnippet) anonymous_personal_snippet_abilities(subject) when subject.is_a?(CommitStatus) - rules = anonymous_project_abilities(subject) - rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) - rules + anonymous_commit_status_abilities(subject) when subject.is_a?(Project) || subject.respond_to?(:project) anonymous_project_abilities(subject) when subject.is_a?(Group) || subject.respond_to?(:group) @@ -66,9 +63,8 @@ class Ability :download_code ] - if project.allow_guest_to_access_builds? - rules << :read_build - end + # Allow to read builds by anonymous user if guests are allowed + rules << :read_build if project.allow_guest_to_access_builds? rules - project_disabled_features_rules(project) else @@ -76,6 +72,13 @@ class Ability end end + def anonymous_commit_status_abilities(subject) + rules = anonymous_project_abilities(subject.project) + # If subject is Ci::Build which inherits from CommitStatus filter the abilities + rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) + rules + end + def anonymous_group_abilities(subject) group = if subject.is_a?(Group) subject @@ -123,18 +126,15 @@ class Ability elsif team.guest?(user) rules.push(*project_guest_rules) - - if project.allow_guest_to_access_builds? - rules << :read_build - end end if project.public? || project.internal? rules.push(*public_project_rules) + end - if project.allow_guest_to_access_builds? - rules << :read_build - end + # Allow to read builds if guests are allowed + if team.guest?(user) || project.public? || project.internal? + rules << :read_build if project.allow_guest_to_access_builds? end if project.owner == user || user.admin? @@ -406,6 +406,13 @@ class Ability rules end + def commit_status_abilities(user, subject) + rules = project_abilities(user, subject.project) + # If subject is Ci::Build which inherits from CommitStatus filter the abilities + rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) + rules + end + def filter_build_abilities(rules) # If we can't read build we should also not have that # ability when looking at this in context of commit_status From 6a5a175d9fd1d72cdaab033eefc4191561e619cc Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 11:14:12 +0100 Subject: [PATCH 011/134] Expose allow_guest_to_access_builds in GitLab API --- doc/api/projects.md | 10 ++++++++-- lib/api/entities.rb | 1 + lib/api/projects.rb | 12 +++++++++--- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/doc/api/projects.md b/doc/api/projects.md index 3f372c955d2..873927f0e74 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -80,7 +80,9 @@ Parameters: "avatar_url": "http://example.com/uploads/project/avatar/4/uploads/avatar.png", "shared_runners_enabled": true, "forks_count": 0, - "star_count": 0 + "star_count": 0, + "runners_token": "b8547b1dc37721d05889db52fa2f02", + "allow_guest_to_access_builds": true }, { "id": 6, @@ -137,7 +139,8 @@ Parameters: "shared_runners_enabled": true, "forks_count": 0, "star_count": 0, - "runners_token": "b8547b1dc37721d05889db52fa2f02" + "runners_token": "b8547b1dc37721d05889db52fa2f02", + "allow_guest_to_access_builds": true } ] ``` @@ -424,6 +427,7 @@ Parameters: - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) - `import_url` (optional) +- `allow_guest_to_access_builds` (optional) ### Create project for user @@ -446,6 +450,7 @@ Parameters: - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) - `import_url` (optional) +- `allow_guest_to_access_builds` (optional) ### Edit project @@ -469,6 +474,7 @@ Parameters: - `snippets_enabled` (optional) - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) +- `allow_guest_to_access_builds` (optional) On success, method returns 200 with the updated project. If parameters are invalid, 400 is returned. diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 82a75734de0..3a68b2c7801 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -72,6 +72,7 @@ module API expose :star_count, :forks_count expose :open_issues_count, if: lambda { |project, options| project.issues_enabled? && project.default_issues_tracker? } expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] } + expose :allow_guest_to_access_builds end class ProjectMember < UserBasic diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 1f991e600e3..f68598e991b 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -99,6 +99,7 @@ module API # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) - 0 by default # import_url (optional) + # allow_guest_to_access_builds (optional) # Example Request # POST /projects post do @@ -115,7 +116,8 @@ module API :namespace_id, :public, :visibility_level, - :import_url] + :import_url, + :allow_guest_to_access_builds] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(current_user, attrs).execute if @project.saved? @@ -145,6 +147,7 @@ module API # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) # import_url (optional) + # allow_guest_to_access_builds (optional) # Example Request # POST /projects/user/:user_id post "user/:user_id" do @@ -161,7 +164,8 @@ module API :shared_runners_enabled, :public, :visibility_level, - :import_url] + :import_url, + :allow_guest_to_access_builds] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(user, attrs).execute if @project.saved? @@ -205,6 +209,7 @@ module API # shared_runners_enabled (optional) # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) - visibility level of a project + # allow_guest_to_access_builds (optional) # Example Request # PUT /projects/:id put ':id' do @@ -219,7 +224,8 @@ module API :snippets_enabled, :shared_runners_enabled, :public, - :visibility_level] + :visibility_level, + :allow_guest_to_access_builds] attrs = map_public_to_visibility_level(attrs) authorize_admin_project authorize! :rename_project, user_project if attrs[:name].present? From b4c36130cc285ac25caef842040e44898eaf858d Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 12:57:46 +0100 Subject: [PATCH 012/134] Rename allow_guest_to_access_builds to public_builds --- app/controllers/projects_controller.rb | 2 +- app/models/ability.rb | 10 +- app/views/projects/edit.html.haml | 8 +- ...dd_allow_guest_to_access_builds_project.rb | 2 +- db/schema.rb | 34 ++-- doc/api/projects.md | 10 +- features/steps/shared/project.rb | 8 + lib/api/entities.rb | 4 +- lib/api/projects.rb | 12 +- spec/features/builds_spec.rb | 2 +- spec/features/commits_spec.rb | 162 +++++++++++------- .../security/project/public_access_spec.rb | 54 ++++++ 12 files changed, 207 insertions(+), 101 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 153e7caaae3..14ca7426c2f 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -227,7 +227,7 @@ class ProjectsController < ApplicationController :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id, :default_branch, :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar, :builds_enabled, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex, - :allow_guest_to_access_builds, + :public_builds, ) end diff --git a/app/models/ability.rb b/app/models/ability.rb index e1767ed8dd1..a6862f83158 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -64,7 +64,7 @@ class Ability ] # Allow to read builds by anonymous user if guests are allowed - rules << :read_build if project.allow_guest_to_access_builds? + rules << :read_build if project.public_builds? rules - project_disabled_features_rules(project) else @@ -132,9 +132,9 @@ class Ability rules.push(*public_project_rules) end - # Allow to read builds if guests are allowed - if team.guest?(user) || project.public? || project.internal? - rules << :read_build if project.allow_guest_to_access_builds? + # Allow to read builds for internal projects + if project.public? || project.internal? + rules << :read_build if project.public_builds? end if project.owner == user || user.admin? @@ -172,7 +172,6 @@ class Ability :read_project_member, :read_merge_request, :read_note, - :read_commit_status, :create_project, :create_issue, :create_note @@ -187,6 +186,7 @@ class Ability :update_issue, :admin_issue, :admin_label, + :read_commit_status, :read_build, ] end diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index fd61ce6a99a..fdcb6987471 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -162,10 +162,10 @@ .form-group .col-sm-offset-2.col-sm-10 .checkbox - = f.label :allow_guest_to_access_builds do - = f.check_box :allow_guest_to_access_builds - %strong Guests can see builds - .help-block Allow guests and anonymous users to access builds including build trace and artifacts + = f.label :public_builds do + = f.check_box :public_builds + %strong Public builds + .help-block Allow everyone to access builds for Public and Internal projects %fieldset.features %legend diff --git a/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb index 69ce8d08bba..793984343b4 100644 --- a/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb +++ b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb @@ -1,5 +1,5 @@ class AddAllowGuestToAccessBuildsProject < ActiveRecord::Migration def change - add_column :projects, :allow_guest_to_access_builds, :boolean, default: true, null: false + add_column :projects, :public_builds, :boolean, default: true, null: false end end diff --git a/db/schema.rb b/db/schema.rb index a04e812ae22..4669a777103 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -650,35 +650,35 @@ ActiveRecord::Schema.define(version: 20160202164642) do t.datetime "created_at" t.datetime "updated_at" t.integer "creator_id" - t.boolean "issues_enabled", default: true, null: false - t.boolean "wall_enabled", default: true, null: false - t.boolean "merge_requests_enabled", default: true, null: false - t.boolean "wiki_enabled", default: true, null: false + t.boolean "issues_enabled", default: true, null: false + t.boolean "wall_enabled", default: true, null: false + t.boolean "merge_requests_enabled", default: true, null: false + t.boolean "wiki_enabled", default: true, null: false t.integer "namespace_id" - t.string "issues_tracker", default: "gitlab", null: false + t.string "issues_tracker", default: "gitlab", null: false t.string "issues_tracker_id" - t.boolean "snippets_enabled", default: true, null: false + t.boolean "snippets_enabled", default: true, null: false t.datetime "last_activity_at" t.string "import_url" - t.integer "visibility_level", default: 0, null: false - t.boolean "archived", default: false, null: false + t.integer "visibility_level", default: 0, null: false + t.boolean "archived", default: false, null: false t.string "avatar" t.string "import_status" - t.float "repository_size", default: 0.0 - t.integer "star_count", default: 0, null: false + t.float "repository_size", default: 0.0 + t.integer "star_count", default: 0, null: false t.string "import_type" t.string "import_source" - t.integer "commit_count", default: 0 + t.integer "commit_count", default: 0 t.text "import_error" t.integer "ci_id" - t.boolean "builds_enabled", default: true, null: false - t.boolean "shared_runners_enabled", default: true, null: false + t.boolean "builds_enabled", default: true, null: false + t.boolean "shared_runners_enabled", default: true, null: false t.string "runners_token" t.string "build_coverage_regex" - t.boolean "build_allow_git_fetch", default: true, null: false - t.integer "build_timeout", default: 3600, null: false - t.boolean "pending_delete", default: false - t.boolean "allow_guest_to_access_builds", default: true, null: false + t.boolean "build_allow_git_fetch", default: true, null: false + t.integer "build_timeout", default: 3600, null: false + t.boolean "pending_delete", default: false + t.boolean "public_builds", default: true, null: false end add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree diff --git a/doc/api/projects.md b/doc/api/projects.md index 873927f0e74..9e9486cd87a 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -82,7 +82,7 @@ Parameters: "forks_count": 0, "star_count": 0, "runners_token": "b8547b1dc37721d05889db52fa2f02", - "allow_guest_to_access_builds": true + "public_builds": true }, { "id": 6, @@ -140,7 +140,7 @@ Parameters: "forks_count": 0, "star_count": 0, "runners_token": "b8547b1dc37721d05889db52fa2f02", - "allow_guest_to_access_builds": true + "public_builds": true } ] ``` @@ -427,7 +427,7 @@ Parameters: - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) - `import_url` (optional) -- `allow_guest_to_access_builds` (optional) +- `public_builds` (optional) ### Create project for user @@ -450,7 +450,7 @@ Parameters: - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) - `import_url` (optional) -- `allow_guest_to_access_builds` (optional) +- `public_builds` (optional) ### Edit project @@ -474,7 +474,7 @@ Parameters: - `snippets_enabled` (optional) - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) -- `allow_guest_to_access_builds` (optional) +- `public_builds` (optional) On success, method returns 200 with the updated project. If parameters are invalid, 400 is returned. diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index d9c75d12238..1200eb319d7 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -240,6 +240,14 @@ module SharedProject end end + step 'public access for builds is enabled' do + @project.update(public_builds: true) + end + + step 'public access for builds is disabled' do + @project.update(public_builds: false) + end + def user_owns_project(user_name:, project_name:, visibility: :private) user = user_exists(user_name, username: user_name.gsub(/\s/, '').underscore) project = Project.find_by(name: project_name) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 3a68b2c7801..1aa6570bd06 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -72,7 +72,7 @@ module API expose :star_count, :forks_count expose :open_issues_count, if: lambda { |project, options| project.issues_enabled? && project.default_issues_tracker? } expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] } - expose :allow_guest_to_access_builds + expose :public_builds end class ProjectMember < UserBasic @@ -384,7 +384,7 @@ module API # for downloading of artifacts (see: https://gitlab.com/gitlab-org/gitlab-ce/issues/4255) expose :download_url do |repo_obj, options| if options[:user_can_download_artifacts] - repo_obj.download_url + repo_obj.artifacts_download_url end end expose :commit, with: RepoCommit do |repo_obj, _options| diff --git a/lib/api/projects.rb b/lib/api/projects.rb index f68598e991b..6067c8b4a5e 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -99,7 +99,7 @@ module API # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) - 0 by default # import_url (optional) - # allow_guest_to_access_builds (optional) + # public_builds (optional) # Example Request # POST /projects post do @@ -117,7 +117,7 @@ module API :public, :visibility_level, :import_url, - :allow_guest_to_access_builds] + :public_builds] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(current_user, attrs).execute if @project.saved? @@ -147,7 +147,7 @@ module API # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) # import_url (optional) - # allow_guest_to_access_builds (optional) + # public_builds (optional) # Example Request # POST /projects/user/:user_id post "user/:user_id" do @@ -165,7 +165,7 @@ module API :public, :visibility_level, :import_url, - :allow_guest_to_access_builds] + :public_builds] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(user, attrs).execute if @project.saved? @@ -209,7 +209,7 @@ module API # shared_runners_enabled (optional) # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) - visibility level of a project - # allow_guest_to_access_builds (optional) + # public_builds (optional) # Example Request # PUT /projects/:id put ':id' do @@ -225,7 +225,7 @@ module API :shared_runners_enabled, :public, :visibility_level, - :allow_guest_to_access_builds] + :public_builds] attrs = map_public_to_visibility_level(attrs) authorize_admin_project authorize! :rename_project, user_project if attrs[:name].present? diff --git a/spec/features/builds_spec.rb b/spec/features/builds_spec.rb index d37bd103714..22e38151bd8 100644 --- a/spec/features/builds_spec.rb +++ b/spec/features/builds_spec.rb @@ -8,7 +8,7 @@ describe "Builds" do @commit = FactoryGirl.create :ci_commit @build = FactoryGirl.create :ci_build, commit: @commit @project = @commit.project - @project.team << [@user, :master] + @project.team << [@user, :developer] end describe "GET /:project/builds" do diff --git a/spec/features/commits_spec.rb b/spec/features/commits_spec.rb index 5a62da10619..dacaa96d760 100644 --- a/spec/features/commits_spec.rb +++ b/spec/features/commits_spec.rb @@ -8,7 +8,6 @@ describe 'Commits' do describe 'CI' do before do login_as :user - project.team << [@user, :master] stub_ci_commit_to_return_yaml_file end @@ -19,6 +18,10 @@ describe 'Commits' do context 'commit status is Generic Commit Status' do let!(:status) { FactoryGirl.create :generic_commit_status, commit: commit } + before do + project.team << [@user, :reporter] + end + describe 'Commit builds' do before do visit ci_status_path(commit) @@ -37,83 +40,124 @@ describe 'Commits' do context 'commit status is Ci Build' do let!(:build) { FactoryGirl.create :ci_build, commit: commit } + let(:artifacts_file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') } - describe 'Project commits' do + context 'when logged as developer' do before do - visit namespace_project_commits_path(project.namespace, project, :master) + project.team << [@user, :developer] end - it 'should show build status' do - page.within("//li[@id='commit-#{commit.short_sha}']") do - expect(page).to have_css(".ci-status-link") + describe 'Project commits' do + before do + visit namespace_project_commits_path(project.namespace, project, :master) + end + + it 'should show build status' do + page.within("//li[@id='commit-#{commit.short_sha}']") do + expect(page).to have_css(".ci-status-link") + end + end + end + + describe 'Commit builds' do + before do + visit ci_status_path(commit) + end + + it { expect(page).to have_content commit.sha[0..7] } + it { expect(page).to have_content commit.git_commit_message } + it { expect(page).to have_content commit.git_author_name } + end + + context 'Download artifacts' do + before do + build.update_attributes(artifacts_file: artifacts_file) + end + + it do + visit ci_status_path(commit) + click_on 'Download artifacts' + expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) + end + end + + describe 'Cancel all builds' do + it 'cancels commit' do + visit ci_status_path(commit) + click_on 'Cancel running' + expect(page).to have_content 'canceled' + end + end + + describe 'Cancel build' do + it 'cancels build' do + visit ci_status_path(commit) + click_on 'Cancel' + expect(page).to have_content 'canceled' + end + end + + describe '.gitlab-ci.yml not found warning' do + context 'ci builds enabled' do + it "does not show warning" do + visit ci_status_path(commit) + expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' + end + + it 'shows warning' do + stub_ci_commit_yaml_file(nil) + visit ci_status_path(commit) + expect(page).to have_content '.gitlab-ci.yml not found in this commit' + end + end + + context 'ci builds disabled' do + before do + stub_ci_builds_disabled + stub_ci_commit_yaml_file(nil) + visit ci_status_path(commit) + end + + it 'does not show warning' do + expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' + end end end end - describe 'Commit builds' do - before do - visit ci_status_path(commit) - end - - it { expect(page).to have_content commit.sha[0..7] } - it { expect(page).to have_content commit.git_commit_message } - it { expect(page).to have_content commit.git_author_name } - end - - context 'Download artifacts' do - let(:artifacts_file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') } - + context "when logged as reporter" do before do + project.team << [@user, :reporter] build.update_attributes(artifacts_file: artifacts_file) + visit ci_status_path(commit) end it do - visit ci_status_path(commit) - click_on 'Download artifacts' - expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) + expect(page).to have_content commit.sha[0..7] + expect(page).to have_content commit.git_commit_message + expect(page).to have_content commit.git_author_name + expect(page).to have_link('Download artifacts') + expect(page).to_not have_link('Cancel running') + expect(page).to_not have_link('Retry failed') end end - describe 'Cancel all builds' do - it 'cancels commit' do + context 'when accessing internal project with disallowed access' do + before do + project.update( + visibility_level: Gitlab::VisibilityLevel::INTERNAL, + public_builds: false) + build.update_attributes(artifacts_file: artifacts_file) visit ci_status_path(commit) - click_on 'Cancel running' - expect(page).to have_content 'canceled' - end - end - - describe 'Cancel build' do - it 'cancels build' do - visit ci_status_path(commit) - click_on 'Cancel' - expect(page).to have_content 'canceled' - end - end - - describe '.gitlab-ci.yml not found warning' do - context 'ci builds enabled' do - it "does not show warning" do - visit ci_status_path(commit) - expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' - end - - it 'shows warning' do - stub_ci_commit_yaml_file(nil) - visit ci_status_path(commit) - expect(page).to have_content '.gitlab-ci.yml not found in this commit' - end end - context 'ci builds disabled' do - before do - stub_ci_builds_disabled - stub_ci_commit_yaml_file(nil) - visit ci_status_path(commit) - end - - it 'does not show warning' do - expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' - end + it do + expect(page).to have_content commit.sha[0..7] + expect(page).to have_content commit.git_commit_message + expect(page).to have_content commit.git_author_name + expect(page).to_not have_link('Download artifacts') + expect(page).to_not have_link('Cancel running') + expect(page).to_not have_link('Retry failed') end end end diff --git a/spec/features/security/project/public_access_spec.rb b/spec/features/security/project/public_access_spec.rb index 655d2c8b7d9..b98476f854e 100644 --- a/spec/features/security/project/public_access_spec.rb +++ b/spec/features/security/project/public_access_spec.rb @@ -96,6 +96,60 @@ describe "Public Project Access", feature: true do it { is_expected.to be_denied_for :visitor } end + describe "GET /:project_path/builds" do + subject { namespace_project_builds_path(project.namespace, project) } + + context "when allowed for public" do + before { project.update(public_builds: true) } + + it { is_expected.to be_allowed_for master } + it { is_expected.to be_allowed_for reporter } + it { is_expected.to be_allowed_for :admin } + it { is_expected.to be_allowed_for guest } + it { is_expected.to be_allowed_for :user } + it { is_expected.to be_allowed_for :visitor } + end + + context "when disallowed for public" do + before { project.update(public_builds: false) } + + it { is_expected.to be_allowed_for master } + it { is_expected.to be_allowed_for reporter } + it { is_expected.to be_allowed_for :admin } + it { is_expected.to be_denied_for guest } + it { is_expected.to be_denied_for :user } + it { is_expected.to be_denied_for :visitor } + end + end + + describe "GET /:project_path/builds/:id" do + let(:commit) { create(:ci_commit, project: project) } + let(:build) { create(:ci_build, commit: commit) } + subject { namespace_project_build_path(project.namespace, project, build.id) } + + context "when allowed for public" do + before { project.update(public_builds: true) } + + it { is_expected.to be_allowed_for master } + it { is_expected.to be_allowed_for reporter } + it { is_expected.to be_allowed_for :admin } + it { is_expected.to be_allowed_for guest } + it { is_expected.to be_allowed_for :user } + it { is_expected.to be_allowed_for :visitor } + end + + context "when disallowed for public" do + before { project.update(public_builds: false) } + + it { is_expected.to be_allowed_for master } + it { is_expected.to be_allowed_for reporter } + it { is_expected.to be_allowed_for :admin } + it { is_expected.to be_denied_for guest } + it { is_expected.to be_denied_for :user } + it { is_expected.to be_denied_for :visitor } + end + end + describe "GET /:project_path/blob" do before do commit = project.repository.commit From d231b6b9182ce9f68f267af0a073136c898f6892 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 13:13:01 +0100 Subject: [PATCH 013/134] Add behaviour tests for build permissions --- features/project/builds/permissions.feature | 35 +++++++++++++++++++++ features/steps/project/builds/summary.rb | 8 ----- features/steps/shared/builds.rb | 17 ++++++++++ features/steps/shared/project.rb | 4 +++ 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/features/project/builds/permissions.feature b/features/project/builds/permissions.feature index 1193bcd74f6..3c7f72335d9 100644 --- a/features/project/builds/permissions.feature +++ b/features/project/builds/permissions.feature @@ -5,6 +5,41 @@ Feature: Project Builds Permissions And project has CI enabled And project has a recent build + Scenario: I try to visit build details as guest + Given I am member of a project with a guest role + When I visit recent build details page + Then page status code should be 404 + + Scenario: I try to visit project builds page as guest + Given I am member of a project with a guest role + When I visit project builds page + Then page status code should be 404 + + Scenario: I try to visit build details of internal project without access to builds + Given The project is internal + And public access for builds is disabled + When I visit recent build details page + Then page status code should be 404 + + Scenario: I try to visit internal project builds page without access to builds + Given The project is internal + And public access for builds is disabled + When I visit project builds page + Then page status code should be 404 + + Scenario: I try to visit build details of internal project with access to builds + Given The project is internal + And public access for builds is enabled + When I visit recent build details page + Then I see details of a build + And I see build trace + + Scenario: I try to visit internal project builds page with access to builds + Given The project is internal + And public access for builds is enabled + When I visit project builds page + Then I see the build + Scenario: I try to download build artifacts as guest Given I am member of a project with a guest role And recent build has artifacts available diff --git a/features/steps/project/builds/summary.rb b/features/steps/project/builds/summary.rb index 036bc0a499e..433253dd44b 100644 --- a/features/steps/project/builds/summary.rb +++ b/features/steps/project/builds/summary.rb @@ -4,14 +4,6 @@ class Spinach::Features::ProjectBuildsSummary < Spinach::FeatureSteps include SharedBuilds include RepoHelpers - step 'I see details of a build' do - expect(page).to have_content "Build ##{@build.id}" - end - - step 'I see build trace' do - expect(page).to have_css '#build-trace' - end - step 'I see button to CI Lint' do page.within('.controls') do ci_lint_tool_link = page.find_link('CI Lint') diff --git a/features/steps/shared/builds.rb b/features/steps/shared/builds.rb index 92bf362879b..726e2e814ad 100644 --- a/features/steps/shared/builds.rb +++ b/features/steps/shared/builds.rb @@ -38,4 +38,21 @@ module SharedBuilds step 'I access artifacts download page' do visit download_namespace_project_build_artifacts_path(@project.namespace, @project, @build) end + + step 'I see details of a build' do + expect(page).to have_content "Build ##{@build.id}" + end + + step 'I see build trace' do + expect(page).to have_css '#build-trace' + end + + step 'I see the build' do + page.within('.commit_status') do + expect(page).to have_content "##{@build.id}" + expect(page).to have_content @build.sha[0..7] + expect(page).to have_content @build.ref + expect(page).to have_content @build.name + end + end end diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index 1200eb319d7..b13e82f276b 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -240,6 +240,10 @@ module SharedProject end end + step 'The project is internal' do + @project.update(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + end + step 'public access for builds is enabled' do @project.update(public_builds: true) end From c64c106091ee5526e9413b9929205134c0ce114f Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 14:43:52 +0100 Subject: [PATCH 014/134] Update ability model after comments --- app/models/ability.rb | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index a6862f83158..00f5f3a93b3 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -5,22 +5,18 @@ class Ability return [] unless user.is_a?(User) return [] if user.blocked? - # We check with `is_a?`, because CommitStatus uses inheritance - if subject.is_a?(CommitStatus) - return commit_status_abilities(user, subject) - end - - case subject.class.name - when "Project" then project_abilities(user, subject) - when "Issue" then issue_abilities(user, subject) - when "Note" then note_abilities(user, subject) - when "ProjectSnippet" then project_snippet_abilities(user, subject) - when "PersonalSnippet" then personal_snippet_abilities(user, subject) - when "MergeRequest" then merge_request_abilities(user, subject) - when "Group" then group_abilities(user, subject) - when "Namespace" then namespace_abilities(user, subject) - when "GroupMember" then group_member_abilities(user, subject) - when "ProjectMember" then project_member_abilities(user, subject) + case subject + when CommitStatus then commit_status_abilities(user, subject) + when Project then project_abilities(user, subject) + when Issue then issue_abilities(user, subject) + when Note then note_abilities(user, subject) + when ProjectSnippet then project_snippet_abilities(user, subject) + when PersonalSnippet then personal_snippet_abilities(user, subject) + when MergeRequest then merge_request_abilities(user, subject) + when Group then group_abilities(user, subject) + when Namespace then namespace_abilities(user, subject) + when GroupMember then group_member_abilities(user, subject) + when ProjectMember then project_member_abilities(user, subject) else [] end.concat(global_abilities(user)) end @@ -130,10 +126,8 @@ class Ability if project.public? || project.internal? rules.push(*public_project_rules) - end - # Allow to read builds for internal projects - if project.public? || project.internal? + # Allow to read builds for internal projects rules << :read_build if project.public_builds? end @@ -416,11 +410,8 @@ class Ability def filter_build_abilities(rules) # If we can't read build we should also not have that # ability when looking at this in context of commit_status - unless rules.include?(:read_build) - rules -= [:read_commit_status] - end - unless rules.include?(:update_build) - rules -= [:update_commit_status] + %w(read create update admin).each do |rule| + rules -= [:"#{rule}_commit_status"] unless rules.include?(:"#{rule}_build") end rules end From a7c441aa17ba64eb702b583ac9198cdace599d2e Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 16:32:41 +0100 Subject: [PATCH 015/134] Use `delete` instead of assignment operator when filtering build abilities --- app/models/ability.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 00f5f3a93b3..a866eadeebb 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -411,7 +411,7 @@ class Ability # If we can't read build we should also not have that # ability when looking at this in context of commit_status %w(read create update admin).each do |rule| - rules -= [:"#{rule}_commit_status"] unless rules.include?(:"#{rule}_build") + rules.delete(:"#{rule}_commit_status") unless rules.include?(:"#{rule}_build") end rules end From 9b925d79c9bfcd1be46bd52e275570a1005a3a79 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 4 Feb 2016 18:36:16 +0100 Subject: [PATCH 016/134] WIP - fix and spec for cross reference issue with forks --- app/services/system_note_service.rb | 4 ++-- spec/services/system_note_service_spec.rb | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 1083bcec054..34b6636c39f 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -291,8 +291,8 @@ class SystemNoteService notes = notes.where(noteable_id: noteable.id) end - gfm_reference = mentioner.gfm_reference(noteable.project) - notes = notes.where(note: cross_reference_note_content(gfm_reference)) + gfm_reference = mentioner.gfm_reference(nil) + notes = notes.where('note LIKE ?', "#{cross_reference_note_prefix}%#{gfm_reference}") notes.count > 0 end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index d3364a71022..a3d3147a79b 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -424,6 +424,22 @@ describe SystemNoteService, services: true do to be_falsey end end + + context 'commit from fork' do + let(:author2) { create(:user) } + let(:forked_project) { Projects::ForkService.new(project, author2).execute } + let(:service) { CreateCommitBuildsService.new } + let(:commit2) { forked_project.commit } + + before do + described_class.cross_reference(commit0, commit2, author2) + end + + it 'is falsey when is a fork mentioning an external issue' do + expect(described_class.cross_reference_exists?(commit0, commit2)). + to be_falsey + end + end end include JiraServiceHelper From c663ca434fb6c8a2251e4251c6f0f9d13660ccbe Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 4 Feb 2016 18:38:11 +0100 Subject: [PATCH 017/134] remove unnecessary lower function on SQL --- .../20160129135155_remove_dot_atom_path_ending_of_projects.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20160129135155_remove_dot_atom_path_ending_of_projects.rb b/db/migrate/20160129135155_remove_dot_atom_path_ending_of_projects.rb index 091de54978b..d3ea956952e 100644 --- a/db/migrate/20160129135155_remove_dot_atom_path_ending_of_projects.rb +++ b/db/migrate/20160129135155_remove_dot_atom_path_ending_of_projects.rb @@ -48,7 +48,7 @@ class RemoveDotAtomPathEndingOfProjects < ActiveRecord::Migration end def projects_with_dot_atom - select_all("SELECT p.id, p.path, n.path as namespace_path, n.id as namespace_id FROM projects p inner join namespaces n on n.id = p.namespace_id WHERE lower(p.path) LIKE '%.atom'") + select_all("SELECT p.id, p.path, n.path as namespace_path, n.id as namespace_id FROM projects p inner join namespaces n on n.id = p.namespace_id WHERE p.path LIKE '%.atom'") end def up From 5f38dbc026add6b4afd2d74b4b664d2c63e12222 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Thu, 4 Feb 2016 13:40:42 -0600 Subject: [PATCH 018/134] Increase project import timeout from 4 minutes to 15 minutes Many users were experiencing timeouts when we only allowed 4 minutes before a timeout. A 15 minute timeout is more than reasonable and prevents us from having to add a configuration for this. Fixes gitlab-org/gitlab-ee#246 --- CHANGELOG | 1 + app/views/shared/_import_form.html.haml | 2 +- lib/gitlab/backend/shell.rb | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index b7e8822fdd6..76e776742b4 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -14,6 +14,7 @@ v 8.5.0 (unreleased) - Fix diff comments loaded by AJAX to load comment with diff in discussion tab - Whitelist raw "abbr" elements when parsing Markdown (Benedict Etzel) - Don't vendor minified JS + - Increase project import timeout to 15 minutes - Display 404 error on group not found - Track project import failure - Support Two-factor Authentication for LDAP users diff --git a/app/views/shared/_import_form.html.haml b/app/views/shared/_import_form.html.haml index 285af56ad73..627814bcfae 100644 --- a/app/views/shared/_import_form.html.haml +++ b/app/views/shared/_import_form.html.haml @@ -11,6 +11,6 @@ %li If your HTTP repository is not publicly accessible, add authentication information to the URL: https://username:password@gitlab.company.com/group/project.git. %li - The import will time out after 4 minutes. For big repositories, use a clone/push combination. + The import will time out after 15 minutes. For repositories that take longer, use a clone/push combination. %li To migrate an SVN repository, check out #{link_to "this document", "http://doc.gitlab.com/ce/workflow/importing/migrating_from_svn.html"}. diff --git a/lib/gitlab/backend/shell.rb b/lib/gitlab/backend/shell.rb index f751458ac66..b9bb6e76081 100644 --- a/lib/gitlab/backend/shell.rb +++ b/lib/gitlab/backend/shell.rb @@ -36,7 +36,7 @@ module Gitlab # import_repository("gitlab/gitlab-ci", "https://github.com/randx/six.git") # def import_repository(name, url) - output, status = Popen::popen([gitlab_shell_projects_path, 'import-project', "#{name}.git", url, '240']) + output, status = Popen::popen([gitlab_shell_projects_path, 'import-project', "#{name}.git", url, '900']) raise Error, output unless status.zero? true end From be3399868cbcfdce85daa6fdf53a079bc163056a Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 2 Nov 2015 19:02:08 +0100 Subject: [PATCH 019/134] Minor refactoring on Gitlab::Git --- lib/gitlab/git.rb | 7 ++++--- spec/lib/gitlab/git_spec.rb | 9 +++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) create mode 100644 spec/lib/gitlab/git_spec.rb diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index f065cc5e9e9..a33daeb119f 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -1,8 +1,9 @@ module Gitlab module Git - BLANK_SHA = '0' * 40 - TAG_REF_PREFIX = "refs/tags/" - BRANCH_REF_PREFIX = "refs/heads/" + # '0' * 40 -- this was easyer to freeze + BLANK_SHA = "0000000000000000000000000000000000000000".freeze + TAG_REF_PREFIX = "refs/tags/".freeze + BRANCH_REF_PREFIX = "refs/heads/".freeze class << self def ref_name(ref) diff --git a/spec/lib/gitlab/git_spec.rb b/spec/lib/gitlab/git_spec.rb new file mode 100644 index 00000000000..3b4052fa549 --- /dev/null +++ b/spec/lib/gitlab/git_spec.rb @@ -0,0 +1,9 @@ +require 'spec_helper' + +describe Gitlab::Git do + describe "BLANK_SHA" do + it "is a string of 40 zero's" do + expect(Gitlab::Git::BLANK_SHA).to eq('0' * 40) + end + end +end From 5ffec2c953478fbad35d7e7f4fc4b6eb119d7918 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 3 Nov 2015 13:09:18 +0100 Subject: [PATCH 020/134] Freeze the expression instead of the literal Also remove the spec for it --- lib/gitlab/git.rb | 3 +-- spec/lib/gitlab/git_spec.rb | 9 --------- 2 files changed, 1 insertion(+), 11 deletions(-) delete mode 100644 spec/lib/gitlab/git_spec.rb diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index a33daeb119f..191bea86ac3 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -1,7 +1,6 @@ module Gitlab module Git - # '0' * 40 -- this was easyer to freeze - BLANK_SHA = "0000000000000000000000000000000000000000".freeze + BLANK_SHA = ('0' * 40).freeze TAG_REF_PREFIX = "refs/tags/".freeze BRANCH_REF_PREFIX = "refs/heads/".freeze diff --git a/spec/lib/gitlab/git_spec.rb b/spec/lib/gitlab/git_spec.rb deleted file mode 100644 index 3b4052fa549..00000000000 --- a/spec/lib/gitlab/git_spec.rb +++ /dev/null @@ -1,9 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Git do - describe "BLANK_SHA" do - it "is a string of 40 zero's" do - expect(Gitlab::Git::BLANK_SHA).to eq('0' * 40) - end - end -end From 94af78ac4a7a0b76eb370a22320ee6410b6a4695 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 5 Feb 2016 18:20:29 +0200 Subject: [PATCH 021/134] Faster snippet search --- CHANGELOG | 1 + app/helpers/snippets_helper.rb | 6 +-- .../search/results/_snippet_blob.html.haml | 44 ++++++++++--------- lib/gitlab/snippet_search_results.rb | 8 +--- 4 files changed, 30 insertions(+), 29 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index d43b8f69063..b510ade27e8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -34,6 +34,7 @@ v 8.5.0 (unreleased) - Add sort dropdown to dashboard projects page - Hide remove source branch button when the MR is merged but new commits are pushed (Zeger-Jan van de Weg) - In seach autocomplete show only groups and projects you are member of + - Faster snippet search v 8.4.3 - Increase lfs_objects size column to 8-byte integer to allow files larger diff --git a/app/helpers/snippets_helper.rb b/app/helpers/snippets_helper.rb index bc36434f549..41ae4048992 100644 --- a/app/helpers/snippets_helper.rb +++ b/app/helpers/snippets_helper.rb @@ -33,7 +33,7 @@ module SnippetsHelper # surrounding code. # # @returns Array, unique and sorted. - def matching_lines(lined_content, surrounding_lines) + def matching_lines(lined_content, surrounding_lines, query) used_lines = [] lined_content.each_with_index do |line, line_number| used_lines.concat bounded_line_numbers( @@ -51,9 +51,9 @@ module SnippetsHelper # surrounding_lines() worth of unmatching lines. # # @returns a hash with {snippet_object, snippet_chunks:{data,start_line}} - def chunk_snippet(snippet, surrounding_lines = 3) + def chunk_snippet(snippet, query, surrounding_lines = 3) lined_content = snippet.content.split("\n") - used_lines = matching_lines(lined_content, surrounding_lines) + used_lines = matching_lines(lined_content, surrounding_lines, query) snippet_chunk = [] snippet_chunks = [] diff --git a/app/views/search/results/_snippet_blob.html.haml b/app/views/search/results/_snippet_blob.html.haml index dcd61199717..6b77d24f50c 100644 --- a/app/views/search/results/_snippet_blob.html.haml +++ b/app/views/search/results/_snippet_blob.html.haml @@ -1,46 +1,50 @@ +- snippet_blob = chunk_snippet(snippet_blob, @search_term) +- snippet = snippet_blob[:snippet_object] +- snippet_chunks = snippet_blob[:snippet_chunks] + .search-result-row %span - = snippet_blob[:snippet_object].title + = snippet.title by - = link_to user_snippets_path(snippet_blob[:snippet_object].author) do - = image_tag avatar_icon(snippet_blob[:snippet_object].author_email), class: "avatar avatar-inline s16", alt: '' - = snippet_blob[:snippet_object].author_name - %span.light #{time_ago_with_tooltip(snippet_blob[:snippet_object].created_at)} + = link_to user_snippets_path(snippet.author) do + = image_tag avatar_icon(snippet.author_email), class: "avatar avatar-inline s16", alt: '' + = snippet.author_name + %span.light #{time_ago_with_tooltip(snippet.created_at)} %h4.snippet-title - - snippet_path = reliable_snippet_path(snippet_blob[:snippet_object]) + - snippet_path = reliable_snippet_path(snippet) = link_to snippet_path do .file-holder .file-title %i.fa.fa-file - %strong= snippet_blob[:snippet_object].file_name - - if markup?(snippet_blob[:snippet_object].file_name) + %strong= snippet.file_name + - if markup?(snippet.file_name) .file-content.wiki - - snippet_blob[:snippet_chunks].each do |snippet| - - unless snippet[:data].empty? - = render_markup(snippet_blob[:snippet_object].file_name, snippet[:data]) + - snippet_chunks.each do |chunk| + - unless chunk[:data].empty? + = render_markup(snippet.file_name, chunk[:data]) - else .file-content.code .nothing-here-block Empty file - else .file-content.code.js-syntax-highlight .line-numbers - - snippet_blob[:snippet_chunks].each do |snippet| - - unless snippet[:data].empty? - - snippet[:data].lines.to_a.size.times do |index| - - offset = defined?(snippet[:start_line]) ? snippet[:start_line] : 1 + - snippet_chunks.each do |chunk| + - unless chunk[:data].empty? + - chunk[:data].lines.to_a.size.times do |index| + - offset = defined?(chunk[:start_line]) ? chunk[:start_line] : 1 - i = index + offset = link_to snippet_path+"#L#{i}", id: "L#{i}", rel: "#L#{i}", class: "diff-line-num" do %i.fa.fa-link = i - - unless snippet == snippet_blob[:snippet_chunks].last + - unless snippet == snippet_chunks.last %a.diff-line-num = "." %pre.code %code - - snippet_blob[:snippet_chunks].each do |snippet| - - unless snippet[:data].empty? - = snippet[:data] - - unless snippet == snippet_blob[:snippet_chunks].last + - snippet_chunks.each do |chunk| + - unless chunk[:data].empty? + = chunk[:data] + - unless chunk == snippet_chunks.last %a = "..." - else diff --git a/lib/gitlab/snippet_search_results.rb b/lib/gitlab/snippet_search_results.rb index 38364a0b151..cadb010ef03 100644 --- a/lib/gitlab/snippet_search_results.rb +++ b/lib/gitlab/snippet_search_results.rb @@ -14,7 +14,7 @@ module Gitlab when 'snippet_titles' Kaminari.paginate_array(snippet_titles).page(page).per(per_page) when 'snippet_blobs' - Kaminari.paginate_array(snippet_blobs).page(page).per(per_page) + snippet_blobs.page(page).per(per_page) else super end @@ -39,11 +39,7 @@ module Gitlab end def snippet_blobs - search = Snippet.where(id: limit_snippet_ids).search_code(query) - search = search.order('updated_at DESC').to_a - snippets = [] - search.each { |e| snippets << chunk_snippet(e) } - snippets + Snippet.where(id: limit_snippet_ids).search_code(query).order('updated_at DESC') end def default_scope From 8f929b8747daf446bb6d31197e69f8948b951189 Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Fri, 5 Feb 2016 18:27:12 +0200 Subject: [PATCH 022/134] one more improvement to snippet search --- lib/gitlab/snippet_search_results.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/snippet_search_results.rb b/lib/gitlab/snippet_search_results.rb index cadb010ef03..addda95be2b 100644 --- a/lib/gitlab/snippet_search_results.rb +++ b/lib/gitlab/snippet_search_results.rb @@ -12,7 +12,7 @@ module Gitlab def objects(scope, page = nil) case scope when 'snippet_titles' - Kaminari.paginate_array(snippet_titles).page(page).per(per_page) + snippet_titles.page(page).per(per_page) when 'snippet_blobs' snippet_blobs.page(page).per(per_page) else From a64d881bac1e7b846a718ce1f105b7e39f4dd1b3 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 5 Feb 2016 17:43:05 +0100 Subject: [PATCH 024/134] fixed spec - at last! --- spec/services/system_note_service_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index a3d3147a79b..655efa0670b 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -428,15 +428,14 @@ describe SystemNoteService, services: true do context 'commit from fork' do let(:author2) { create(:user) } let(:forked_project) { Projects::ForkService.new(project, author2).execute } - let(:service) { CreateCommitBuildsService.new } let(:commit2) { forked_project.commit } before do - described_class.cross_reference(commit0, commit2, author2) + described_class.cross_reference(noteable, commit2, author2) end it 'is falsey when is a fork mentioning an external issue' do - expect(described_class.cross_reference_exists?(commit0, commit2)). + expect(described_class.cross_reference_exists?(noteable, commit2)). to be_falsey end end From 08064767615ad7b0e2599aed94c53117516e8377 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 5 Feb 2016 20:17:58 +0100 Subject: [PATCH 025/134] Remove current_user && when can? is used --- app/views/admin/builds/_build.html.haml | 2 +- app/views/projects/builds/show.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index bd7fb0b36f4..34d955568f2 100644 --- a/app/views/admin/builds/_build.html.haml +++ b/app/views/admin/builds/_build.html.haml @@ -63,7 +63,7 @@ - if can?(current_user, :read_build, project) && build.artifacts? = link_to build.artifacts_download_url, title: 'Download artifacts' do %i.fa.fa-download - - if current_user && can?(current_user, :update_build, build.project) + - if can?(current_user, :update_build, build.project) - if build.active? - if build.cancel_url = link_to build.cancel_url, method: :post, title: 'Cancel' do diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml index c43d6c3b427..ca1441a20d8 100644 --- a/app/views/projects/builds/show.html.haml +++ b/app/views/projects/builds/show.html.haml @@ -101,7 +101,7 @@ .build-widget %h4.title Build ##{@build.id} - - if current_user && can?(current_user, :update_build, @project) + - if can?(current_user, :update_build, @project) .pull-right - if @build.cancel_url = link_to "Cancel", @build.cancel_url, class: 'btn btn-sm btn-danger', method: :post From ebeb35a01cc41eb1b55271644a633397ca4ce7a1 Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Fri, 5 Feb 2016 18:18:34 -0500 Subject: [PATCH 026/134] Fix ugly state of dropdown when editing a dropdown, but not changing anything you end up in an ugly state Fixes #13215 --- app/assets/stylesheets/pages/issuable.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 9d5dc42b6cc..c0b5557bcbd 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -75,7 +75,7 @@ border: none; } - span { + span:not(.select2-container span) { margin-top: 7px; display: inline-block; } From d164f1982a865ea5ba7aa33308804cff755c6858 Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Fri, 5 Feb 2016 18:21:50 -0500 Subject: [PATCH 027/134] Change hover color of sidebar buttons --- app/assets/stylesheets/pages/issuable.scss | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index c0b5557bcbd..95e3758b0f1 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -194,6 +194,11 @@ border: 1px solid $border-gray-normal; } + .btn:hover { + background: $gray-dark; + border: 1px solid $border-gray-dark; + } + &.right-sidebar-collapsed { .issuable-count, .issuable-nav, From 9860bd72abe3cfd81cc4fdf367a65a310674180b Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Fri, 5 Feb 2016 18:25:26 -0500 Subject: [PATCH 028/134] Add hover for gutter toggle button --- app/assets/stylesheets/pages/issuable.scss | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 95e3758b0f1..93b88a11388 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -88,6 +88,10 @@ margin-left: 20px; border-left: 1px solid $border-gray-light; padding-left: 10px; + + &:hover { + color: $gray-darkest; + } } } @@ -192,11 +196,10 @@ .btn { background: $gray-normal; border: 1px solid $border-gray-normal; - } - - .btn:hover { - background: $gray-dark; - border: 1px solid $border-gray-dark; + &:hover { + background: $gray-dark; + border: 1px solid $border-gray-dark; + } } &.right-sidebar-collapsed { From dcfdef689a250ae4db6a92a1ad4413f2a86c9113 Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Fri, 5 Feb 2016 18:29:17 -0500 Subject: [PATCH 029/134] Fix span margin issues --- app/assets/stylesheets/pages/issuable.scss | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 93b88a11388..c440388090e 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -75,11 +75,15 @@ border: none; } - span:not(.select2-container span) { + span { margin-top: 7px; display: inline-block; } + .select2-container span { + margin-top: 0; + } + .issuable-count { } From 306c50cecde50b0bbfc9d665a09ecd2fa1676005 Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Fri, 5 Feb 2016 19:43:18 -0500 Subject: [PATCH 030/134] Make copy button in collapsed sidebar work. --- app/assets/stylesheets/pages/issuable.scss | 13 +++++++++++++ app/views/shared/issuable/_sidebar.html.haml | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index c440388090e..ef62f069dc2 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -235,6 +235,19 @@ display: block; margin-top: 0; } + + .btn-clipboard { + border: none; + + &:hover { + background: transparent; + } + + i { + color: #999999; + } + } + } } diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index ae96a45453f..eb19b936e3e 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -118,7 +118,7 @@ - project_ref = cross_project_reference(@project, issuable) .block.project-reference .sidebar-collapsed-icon - = icon('clipboard') + = clipboard_button(clipboard_text: project_ref) .title .cross-project-reference %span From 311f407651e9ad1859bb0e9b6b9d6de79fde1a3d Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sat, 6 Feb 2016 15:01:37 +0100 Subject: [PATCH 031/134] Fix commit status tests --- spec/requests/api/commit_status_spec.rb | 48 +++++++++++++++---------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/spec/requests/api/commit_status_spec.rb b/spec/requests/api/commit_status_spec.rb index 21482fc1070..89b554622ef 100644 --- a/spec/requests/api/commit_status_spec.rb +++ b/spec/requests/api/commit_status_spec.rb @@ -2,18 +2,17 @@ require 'spec_helper' describe API::CommitStatus, api: true do include ApiHelpers - let(:user) { create(:user) } - let(:user2) { create(:user) } - let!(:project) { create(:project, creator_id: user.id) } - let!(:reporter) { create(:project_member, user: user, project: project, access_level: ProjectMember::REPORTER) } - let!(:guest) { create(:project_member, user: user2, project: project, access_level: ProjectMember::GUEST) } + let!(:project) { create(:project) } let(:commit) { project.repository.commit } let!(:ci_commit) { project.ensure_ci_commit(commit.id) } let(:commit_status) { create(:commit_status, commit: ci_commit) } + let(:guest) { create_user(ProjectMember::GUEST) } + let(:reporter) { create_user(ProjectMember::REPORTER) } + let(:developer) { create_user(ProjectMember::DEVELOPER) } describe "GET /projects/:id/repository/commits/:sha/statuses" do it_behaves_like 'a paginated resources' do - let(:request) { get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", user) } + let(:request) { get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter) } end context "reporter user" do @@ -29,7 +28,7 @@ describe API::CommitStatus, api: true do end it "should return latest commit statuses" do - get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", user) + get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter) expect(response.status).to eq(200) expect(json_response).to be_an Array @@ -39,7 +38,7 @@ describe API::CommitStatus, api: true do end it "should return all commit statuses" do - get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?all=1", user) + get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?all=1", reporter) expect(response.status).to eq(200) expect(json_response).to be_an Array @@ -47,7 +46,7 @@ describe API::CommitStatus, api: true do end it "should return latest commit statuses for specific ref" do - get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?ref=develop", user) + get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?ref=develop", reporter) expect(response.status).to eq(200) expect(json_response).to be_an Array @@ -55,7 +54,7 @@ describe API::CommitStatus, api: true do end it "should return latest commit statuses for specific name" do - get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?name=coverage", user) + get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?name=coverage", reporter) expect(response.status).to eq(200) expect(json_response).to be_an Array @@ -65,7 +64,7 @@ describe API::CommitStatus, api: true do context "guest user" do it "should not return project commits" do - get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", user2) + get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", guest) expect(response.status).to eq(403) end end @@ -81,10 +80,10 @@ describe API::CommitStatus, api: true do describe 'POST /projects/:id/statuses/:sha' do let(:post_url) { "/projects/#{project.id}/statuses/#{commit.id}" } - context 'reporter user' do + context 'developer user' do context 'should create commit status' do it 'with only required parameters' do - post api(post_url, user), state: 'success' + post api(post_url, developer), state: 'success' expect(response.status).to eq(201) expect(json_response['sha']).to eq(commit.id) expect(json_response['status']).to eq('success') @@ -95,7 +94,7 @@ describe API::CommitStatus, api: true do end it 'with all optional parameters' do - post api(post_url, user), state: 'success', context: 'coverage', ref: 'develop', target_url: 'url', description: 'test' + post api(post_url, developer), state: 'success', context: 'coverage', ref: 'develop', target_url: 'url', description: 'test' expect(response.status).to eq(201) expect(json_response['sha']).to eq(commit.id) expect(json_response['status']).to eq('success') @@ -108,25 +107,32 @@ describe API::CommitStatus, api: true do context 'should not create commit status' do it 'with invalid state' do - post api(post_url, user), state: 'invalid' + post api(post_url, developer), state: 'invalid' expect(response.status).to eq(400) end it 'without state' do - post api(post_url, user) + post api(post_url, developer) expect(response.status).to eq(400) end it 'invalid commit' do - post api("/projects/#{project.id}/statuses/invalid_sha", user), state: 'running' + post api("/projects/#{project.id}/statuses/invalid_sha", developer), state: 'running' expect(response.status).to eq(404) end end end + context 'reporter user' do + it 'should not create commit status' do + post api(post_url, reporter) + expect(response.status).to eq(403) + end + end + context 'guest user' do it 'should not create commit status' do - post api(post_url, user2) + post api(post_url, guest) expect(response.status).to eq(403) end end @@ -138,4 +144,10 @@ describe API::CommitStatus, api: true do end end end + + def create_user(access_level) + user = create(:user) + create(:project_member, user: user, project: project, access_level: access_level) + user + end end From 89b3ddd609063d93f39a02279f0f3e23b8b6b364 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 3 Feb 2016 23:01:04 -0500 Subject: [PATCH 032/134] Allow limited Markdown in Broadcast Messages Closes #11853 --- CHANGELOG | 1 + app/assets/javascripts/admin.js.coffee | 13 ----------- .../javascripts/broadcast_message.js.coffee | 22 +++++++++++++++++++ app/assets/stylesheets/pages/admin.scss | 10 +++++++++ .../admin/broadcast_messages_controller.rb | 4 ++++ app/helpers/broadcast_messages_helper.rb | 6 ++++- .../admin/broadcast_messages/_form.html.haml | 7 ++++-- .../admin/broadcast_messages/preview.js.haml | 1 + config/routes.rb | 5 ++++- features/admin/broadcast_messages.feature | 6 +++++ features/steps/admin/broadcast_messages.rb | 14 +++++++++++- .../pipeline/broadcast_message_pipeline.rb | 16 ++++++++++++++ 12 files changed, 87 insertions(+), 18 deletions(-) create mode 100644 app/assets/javascripts/broadcast_message.js.coffee create mode 100644 app/views/admin/broadcast_messages/preview.js.haml create mode 100644 lib/banzai/pipeline/broadcast_message_pipeline.rb diff --git a/CHANGELOG b/CHANGELOG index 50b27e25492..fba6c3ccd1b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -19,6 +19,7 @@ v 8.5.0 (unreleased) - Track project import failure - Support Two-factor Authentication for LDAP users - Display database type and version in Administration dashboard + - Allow limited Markdown in Broadcast Messages - Fix visibility level text in admin area (Zeger-Jan van de Weg) - Warn admin during OAuth of granting admin rights (Zeger-Jan van de Weg) - Update the ExternalIssue regex pattern (Blake Hitchcock) diff --git a/app/assets/javascripts/admin.js.coffee b/app/assets/javascripts/admin.js.coffee index eb951f71711..b2b8e1b7ffb 100644 --- a/app/assets/javascripts/admin.js.coffee +++ b/app/assets/javascripts/admin.js.coffee @@ -12,19 +12,6 @@ class @Admin e.preventDefault() $('.js-toggle-colors-container').toggle() - $('input#broadcast_message_color').on 'input', -> - previewColor = $(@).val() - $('div.broadcast-message-preview').css('background-color', previewColor) - - $('input#broadcast_message_font').on 'input', -> - previewColor = $(@).val() - $('div.broadcast-message-preview').css('color', previewColor) - - $('textarea#broadcast_message_message').on 'input', -> - previewMessage = $(@).val() - previewMessage = "Your message here" if previewMessage.trim() == '' - $('div.broadcast-message-preview span').text(previewMessage) - $('.log-tabs a').click (e) -> e.preventDefault() $(this).tab('show') diff --git a/app/assets/javascripts/broadcast_message.js.coffee b/app/assets/javascripts/broadcast_message.js.coffee new file mode 100644 index 00000000000..a38a329c4c2 --- /dev/null +++ b/app/assets/javascripts/broadcast_message.js.coffee @@ -0,0 +1,22 @@ +$ -> + $('input#broadcast_message_color').on 'input', -> + previewColor = $(@).val() + $('div.broadcast-message-preview').css('background-color', previewColor) + + $('input#broadcast_message_font').on 'input', -> + previewColor = $(@).val() + $('div.broadcast-message-preview').css('color', previewColor) + + previewPath = $('textarea#broadcast_message_message').data('preview-path') + + $('textarea#broadcast_message_message').on 'input', -> + message = $(@).val() + + if message == '' + $('.js-broadcast-message-preview').text("Your message here") + else + $.ajax( + url: previewPath + type: "POST" + data: { broadcast_message: { message: message } } + ) diff --git a/app/assets/stylesheets/pages/admin.scss b/app/assets/stylesheets/pages/admin.scss index 144852e7874..a61161810a3 100644 --- a/app/assets/stylesheets/pages/admin.scss +++ b/app/assets/stylesheets/pages/admin.scss @@ -55,6 +55,16 @@ @extend .alert-warning; padding: 10px; text-align: center; + + > div, p { + display: inline; + margin: 0; + + a { + color: inherit; + text-decoration: underline; + } + } } .broadcast-message-preview { diff --git a/app/controllers/admin/broadcast_messages_controller.rb b/app/controllers/admin/broadcast_messages_controller.rb index a470d865408..fc342924987 100644 --- a/app/controllers/admin/broadcast_messages_controller.rb +++ b/app/controllers/admin/broadcast_messages_controller.rb @@ -36,6 +36,10 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController end end + def preview + @message = broadcast_message_params[:message] + end + protected def finder diff --git a/app/helpers/broadcast_messages_helper.rb b/app/helpers/broadcast_messages_helper.rb index 1ed8c710f77..43a29c96bca 100644 --- a/app/helpers/broadcast_messages_helper.rb +++ b/app/helpers/broadcast_messages_helper.rb @@ -3,7 +3,7 @@ module BroadcastMessagesHelper return unless message.present? content_tag :div, class: 'broadcast-message', style: broadcast_message_style(message) do - icon('bullhorn') << ' ' << message.message + icon('bullhorn') << ' ' << render_broadcast_message(message.message) end end @@ -31,4 +31,8 @@ module BroadcastMessagesHelper 'Pending' end end + + def render_broadcast_message(message) + Banzai.render(message, pipeline: :broadcast_message).html_safe + end end diff --git a/app/views/admin/broadcast_messages/_form.html.haml b/app/views/admin/broadcast_messages/_form.html.haml index 953b8b69368..5c9403fa0c2 100644 --- a/app/views/admin/broadcast_messages/_form.html.haml +++ b/app/views/admin/broadcast_messages/_form.html.haml @@ -1,6 +1,7 @@ .broadcast-message-preview{ style: broadcast_message_style(@broadcast_message) } = icon('bullhorn') - %span= @broadcast_message.message || "Your message here" + .js-broadcast-message-preview + = render_broadcast_message(@broadcast_message.message.presence || "Your message here") = form_for [:admin, @broadcast_message], html: { class: 'broadcast-message-form form-horizontal js-requires-input'} do |f| -if @broadcast_message.errors.any? @@ -10,7 +11,9 @@ .form-group = f.label :message, class: 'control-label' .col-sm-10 - = f.text_area :message, class: "form-control js-quick-submit", rows: 2, required: true + = f.text_area :message, class: "form-control js-quick-submit js-autosize", + required: true, + data: { preview_path: preview_admin_broadcast_messages_path } .form-group.js-toggle-colors-container .col-sm-10.col-sm-offset-2 = link_to 'Customize colors', '#', class: 'js-toggle-colors-link' diff --git a/app/views/admin/broadcast_messages/preview.js.haml b/app/views/admin/broadcast_messages/preview.js.haml new file mode 100644 index 00000000000..fbc9453c72e --- /dev/null +++ b/app/views/admin/broadcast_messages/preview.js.haml @@ -0,0 +1 @@ +$('.js-broadcast-message-preview').html("#{j(render_broadcast_message(@message))}"); diff --git a/config/routes.rb b/config/routes.rb index 034bfaf1bcd..f5951bf3e8d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -227,7 +227,10 @@ Rails.application.routes.draw do get :test end - resources :broadcast_messages, only: [:index, :edit, :create, :update, :destroy] + resources :broadcast_messages, only: [:index, :edit, :create, :update, :destroy] do + post :preview, on: :collection + end + resource :logs, only: [:show] resource :background_jobs, controller: 'background_jobs', only: [:show] diff --git a/features/admin/broadcast_messages.feature b/features/admin/broadcast_messages.feature index fd3bac77f86..4f9c651561e 100644 --- a/features/admin/broadcast_messages.feature +++ b/features/admin/broadcast_messages.feature @@ -25,3 +25,9 @@ Feature: Admin Broadcast Messages When I remove an existing broadcast message Then I should be redirected to admin messages page And I should not see the removed broadcast message + + @javascript + Scenario: Live preview a customized broadcast message + When I visit admin messages page + And I enter a broadcast message with Markdown + Then I should see a live preview of the rendered broadcast message diff --git a/features/steps/admin/broadcast_messages.rb b/features/steps/admin/broadcast_messages.rb index 6cacdf4764c..af2b4a29313 100644 --- a/features/steps/admin/broadcast_messages.rb +++ b/features/steps/admin/broadcast_messages.rb @@ -19,7 +19,7 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps end step 'submit form with new customized broadcast message' do - fill_in 'broadcast_message_message', with: 'Application update from 4:00 CST to 5:00 CST' + fill_in 'broadcast_message_message', with: 'Application update from **4:00 CST to 5:00 CST**' fill_in 'broadcast_message_color', with: '#f2dede' fill_in 'broadcast_message_font', with: '#b94a48' select Date.today.next_year.year, from: "broadcast_message_ends_at_1i" @@ -28,6 +28,7 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps step 'I should see a customized broadcast message' do expect(page).to have_content 'Application update from 4:00 CST to 5:00 CST' + expect(page).to have_selector 'strong', text: '4:00 CST to 5:00 CST' expect(page).to have_selector %(div[style="background-color: #f2dede; color: #b94a48"]) end @@ -51,4 +52,15 @@ class Spinach::Features::AdminBroadcastMessages < Spinach::FeatureSteps step 'I should not see the removed broadcast message' do expect(page).not_to have_content 'Migration to new server' end + + step 'I enter a broadcast message with Markdown' do + fill_in 'broadcast_message_message', with: "Live **Markdown** previews. :tada:" + end + + step 'I should see a live preview of the rendered broadcast message' do + page.within('.broadcast-message-preview') do + expect(page).to have_selector('strong', text: 'Markdown') + expect(page).to have_selector('img.emoji') + end + end end diff --git a/lib/banzai/pipeline/broadcast_message_pipeline.rb b/lib/banzai/pipeline/broadcast_message_pipeline.rb new file mode 100644 index 00000000000..4bb85e24c38 --- /dev/null +++ b/lib/banzai/pipeline/broadcast_message_pipeline.rb @@ -0,0 +1,16 @@ +module Banzai + module Pipeline + class BroadcastMessagePipeline < DescriptionPipeline + def self.filters + @filters ||= [ + Filter::MarkdownFilter, + Filter::SanitizationFilter, + + Filter::EmojiFilter, + Filter::AutolinkFilter, + Filter::ExternalLinkFilter + ] + end + end + end +end From 9eba0eaac3199ddf581c74b73fe69b94fa3e5498 Mon Sep 17 00:00:00 2001 From: Aaron Hamilton Date: Sat, 6 Feb 2016 23:18:34 +0000 Subject: [PATCH 033/134] Prevent rasterization artefacts in the logo, and simplify markup. - Remove sketch-namespaced attributes. - Remove unused groups, and cancel out unnecessary transforms. - Use overlap to avoid rasterization artefacts in the logo. - Extend comment for `#logo` click event handler, as it's a non-obvious hack for Safari. - Remove viewBox parameter and define paths in their final form. --- app/assets/javascripts/logo.js.coffee | 3 ++- app/views/shared/_logo.svg | 28 ++++++++------------------- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/logo.js.coffee b/app/assets/javascripts/logo.js.coffee index e7d884662ea..35b2fbbba07 100644 --- a/app/assets/javascripts/logo.js.coffee +++ b/app/assets/javascripts/logo.js.coffee @@ -44,6 +44,7 @@ $(document).on('page:fetch', start) $(document).on('page:change', stop) $ -> - # Make logo clickable + # Make logo clickable as part of a workaround for Safari visited + # link behaviour (See !2690). $('#logo').on 'click', -> $('#js-shortcuts-home').get(0).click() diff --git a/app/views/shared/_logo.svg b/app/views/shared/_logo.svg index 3d279ec228c..b07f1c5603e 100644 --- a/app/views/shared/_logo.svg +++ b/app/views/shared/_logo.svg @@ -1,21 +1,9 @@ -ASCII' - - allow(Asciidoctor).to receive(:convert).and_return(html) - expect(Banzai).to receive(:render) - .with(html, context.merge(pipeline: :asciidoc)) - .and_return(filtered_html) - - expect( render('foo', context) ).to eql filtered_html - end - end - def render(*args) described_class.render(*args) end From 4089be8fedbe0499108b32dc0c08b6378efc534c Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 12 Feb 2016 18:01:47 +0100 Subject: [PATCH 118/134] woo hoo - getting there with the cross-reference issue. Should fix the problem and updated spec --- app/services/system_note_service.rb | 15 +++++++++++---- spec/services/system_note_service_spec.rb | 10 ++++++---- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 34b6636c39f..843b44abd17 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -280,6 +280,7 @@ class SystemNoteService # mentioner - Mentionable object # # Returns Boolean + def self.cross_reference_exists?(noteable, mentioner) # Initial scope should be system notes of this noteable type notes = Note.system.where(noteable_type: noteable.class) @@ -291,14 +292,20 @@ class SystemNoteService notes = notes.where(noteable_id: noteable.id) end - gfm_reference = mentioner.gfm_reference(nil) - notes = notes.where('note LIKE ?', "#{cross_reference_note_prefix}%#{gfm_reference}") - - notes.count > 0 + notes_for_mentioner(mentioner, noteable, notes).count > 0 end private + def self.notes_for_mentioner(mentioner, noteable, notes) + if mentioner.is_a?(Commit) + notes.where('note LIKE ?', "#{cross_reference_note_prefix}%#{mentioner.to_reference(nil)}") + else + gfm_reference = mentioner.gfm_reference(noteable.project) + notes.where(note: cross_reference_note_content(gfm_reference)) + end + end + def self.create_note(args = {}) Note.create(args.merge(system: true)) end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 655efa0670b..0c063fcb959 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -425,18 +425,20 @@ describe SystemNoteService, services: true do end end - context 'commit from fork' do + context 'commit with cross-reference from fork' do let(:author2) { create(:user) } let(:forked_project) { Projects::ForkService.new(project, author2).execute } let(:commit2) { forked_project.commit } before do - described_class.cross_reference(noteable, commit2, author2) + allow(commit0).to receive(:to_reference) { noteable.project.to_reference + + commit0.class.reference_prefix + commit0.id} + described_class.cross_reference(noteable, commit0, author2) end - it 'is falsey when is a fork mentioning an external issue' do + it 'is true when a fork mentions an external issue' do expect(described_class.cross_reference_exists?(noteable, commit2)). - to be_falsey + to be true end end end From 99a50447a15abb49ae6a3225332f850af78744a9 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 12 Feb 2016 18:21:01 +0100 Subject: [PATCH 119/134] fix rubocop warning --- spec/services/system_note_service_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 0c063fcb959..9b9732ea01f 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -431,8 +431,10 @@ describe SystemNoteService, services: true do let(:commit2) { forked_project.commit } before do - allow(commit0).to receive(:to_reference) { noteable.project.to_reference + - commit0.class.reference_prefix + commit0.id} + allow(commit0).to receive(:to_reference) { + noteable.project.to_reference + + commit0.class.reference_prefix + commit0.id + } described_class.cross_reference(noteable, commit0, author2) end From 8a62fbdb42304b1c9012a718819aa0d363249c16 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Sat, 13 Feb 2016 00:37:43 +0200 Subject: [PATCH 120/134] Remove note on backing up GitLab CI as a separate service [ci skip] --- doc/raketasks/backup_restore.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/doc/raketasks/backup_restore.md b/doc/raketasks/backup_restore.md index cdd6652b7b0..f6d1234ac4a 100644 --- a/doc/raketasks/backup_restore.md +++ b/doc/raketasks/backup_restore.md @@ -18,8 +18,6 @@ for two-factor authentication. If you restore a GitLab backup without restoring the database encryption key, users who have two-factor authentication enabled will lose access to your GitLab server. -If you are interested in GitLab CI backup please follow to the [CI backup documentation](https://gitlab.com/gitlab-org/gitlab-ci/blob/master/doc/raketasks/backup_restore.md)* - ``` # use this command if you've installed GitLab with the Omnibus package sudo gitlab-rake gitlab:backup:create From 4646cbc4c65854f8f3d0fc42ec385fb9f31db82b Mon Sep 17 00:00:00 2001 From: Takuya Noguchi Date: Sat, 13 Feb 2016 12:59:55 +0900 Subject: [PATCH 121/134] Update omniauth to 1.3.1 for memory performance --- Gemfile | 2 +- Gemfile.lock | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 9114fdd33ac..a1c57af4bac 100644 --- a/Gemfile +++ b/Gemfile @@ -21,7 +21,7 @@ gem "pg", '~> 0.18.2', group: :postgres gem 'devise', '~> 3.5.4' gem 'devise-async', '~> 0.9.0' gem 'doorkeeper', '~> 2.2.0' -gem 'omniauth', '~> 1.2.2' +gem 'omniauth', '~> 1.3.1' gem 'omniauth-azure-oauth2', '~> 0.0.6' gem 'omniauth-bitbucket', '~> 0.0.2' gem 'omniauth-cas3', '~> 1.1.2' diff --git a/Gemfile.lock b/Gemfile.lock index 98a36724fe0..718285e1665 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -491,9 +491,9 @@ GEM rack (~> 1.2) octokit (3.8.0) sawyer (~> 0.6.0, >= 0.5.3) - omniauth (1.2.2) + omniauth (1.3.1) hashie (>= 1.2, < 4) - rack (~> 1.0) + rack (>= 1.0, < 3) omniauth-azure-oauth2 (0.0.6) jwt (~> 1.0) omniauth (~> 1.0) @@ -963,7 +963,7 @@ DEPENDENCIES nprogress-rails (~> 0.1.6.7) oauth2 (~> 1.0.0) octokit (~> 3.8.0) - omniauth (~> 1.2.2) + omniauth (~> 1.3.1) omniauth-azure-oauth2 (~> 0.0.6) omniauth-bitbucket (~> 0.0.2) omniauth-cas3 (~> 1.1.2) From 7ffcc5a17bc462b652e91c1cff36b8baaeb6a41a Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 12 Feb 2016 17:41:52 -0800 Subject: [PATCH 122/134] Fix duplicate "me" in tooltip of the "thumbsup" awards Emoji Steps to reproduce: 1. Go to a MR or issue 2. Click on "thumbsup" Emoji 3 times 3. Observe the tooltip becomes "me, me" Closes #13374, #12788 --- CHANGELOG | 1 + app/assets/javascripts/awards_handler.coffee | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 9b2d6d58a5c..a3ceca34b07 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.5.0 (unreleased) + - Fix duplicate "me" in tooltip of the "thumbsup" awards Emoji (Stan Hu) - Cache various Repository methods to improve performance (Yorick Peterse) - Fix duplicated branch creation/deletion Web hooks/service notifications when using Web UI (Stan Hu) - Ensure rake tasks that don't need a DB connection can be run without one diff --git a/app/assets/javascripts/awards_handler.coffee b/app/assets/javascripts/awards_handler.coffee index 047df4786a9..360acb864f6 100644 --- a/app/assets/javascripts/awards_handler.coffee +++ b/app/assets/javascripts/awards_handler.coffee @@ -49,10 +49,11 @@ class @AwardsHandler counter.text(parseInt(counter.text()) - 1) emojiIcon.removeClass("active") @removeMeFromAuthorList(emoji) - else if emoji =="thumbsup" || emoji == "thumbsdown" + else if emoji == "thumbsup" || emoji == "thumbsdown" emojiIcon.tooltip("destroy") counter.text(0) emojiIcon.removeClass("active") + @removeMeFromAuthorList(emoji) else emojiIcon.tooltip("destroy") emojiIcon.remove() From 46d35cdbe572e7cad434ed9c61fc264160184aba Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 13 Feb 2016 02:47:15 -0800 Subject: [PATCH 123/134] Add spinach tests for award emoji --- features/project/issues/award_emoji.feature | 11 ++++++++- features/steps/project/issues/award_emoji.rb | 26 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/features/project/issues/award_emoji.feature b/features/project/issues/award_emoji.feature index bfde89fd896..2945bb3753a 100644 --- a/features/project/issues/award_emoji.feature +++ b/features/project/issues/award_emoji.feature @@ -7,7 +7,16 @@ Feature: Award Emoji And I visit "Bugfix" issue page @javascript - Scenario: I add and remove award in the issue + Scenario: I repeatedly add and remove thumbsup award in the issue + Given I click the thumbsup award Emoji + Then I have award added + Given I click the thumbsup award Emoji + Then I have no awards added + Given I click the thumbsup award Emoji + Then I have award added + + @javascript + Scenario: I add and remove custom award in the issue Given I click to emoji-picker Then The search field is focused And I click to emoji in the picker diff --git a/features/steps/project/issues/award_emoji.rb b/features/steps/project/issues/award_emoji.rb index 69695d493f3..8b9aa6aabfa 100644 --- a/features/steps/project/issues/award_emoji.rb +++ b/features/steps/project/issues/award_emoji.rb @@ -8,6 +8,15 @@ class Spinach::Features::AwardEmoji < Spinach::FeatureSteps visit namespace_project_issue_path(@project.namespace, @project, @issue) end + step 'I click the thumbsup award Emoji' do + page.within '.awards' do + thumbsup = page.find('.award .emoji-1F44D') + thumbsup.click + thumbsup.hover + sleep 0.3 + end + end + step 'I click to emoji-picker' do page.within '.awards-controls' do page.find('.add-award').click @@ -40,6 +49,23 @@ class Spinach::Features::AwardEmoji < Spinach::FeatureSteps page.within '.awards' do expect(page).to have_selector '.award' expect(page.find('.award.active .counter')).to have_content '1' + expect(page.find('.award.active')['data-original-title']).to eq('me') + end + end + + step 'I have no awards added' do + page.within '.awards' do + expect(page).to have_selector '.award' + expect(page.all('.award').size).to eq(2) + + # Check tooltip data + page.all('.award').each do |element| + expect(element['title']).to eq("") + end + + page.all('.award .counter').each do |element| + expect(element).to have_content '0' + end end end From 54613b6af56008588a3cc8a9e9f2ee642ca59a36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 15 Feb 2016 15:19:23 +0100 Subject: [PATCH 124/134] Fix the "x of y" displayed at the top of Issuables' sidebar 1. We now display the index of the current issuable among all its project's issuables, of the same type and with the same state. 2. Also, refactored a bit the Issuable helpers into a new IssuablesHelper module. 3. Added acceptance specs for the sidebar counter. --- app/helpers/application_helper.rb | 70 -------------------- app/helpers/issuables_helper.rb | 41 ++++++++++++ app/helpers/nav_helper.rb | 18 +---- app/models/concerns/issuable.rb | 10 +++ app/views/shared/issuable/_sidebar.html.haml | 17 ++--- features/project/issues/issues.feature | 9 ++- features/project/merge_requests.feature | 1 + features/steps/project/issues/issues.rb | 5 ++ features/steps/shared/issuable.rb | 24 +++++++ 9 files changed, 99 insertions(+), 96 deletions(-) create mode 100644 app/helpers/issuables_helper.rb diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 02357e2f23e..ecefa9b006d 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -280,76 +280,6 @@ module ApplicationHelper end end - def issuable_link_next(project,issuable) - if project.nil? - nil - elsif current_controller?(:issues) - namespace_project_issue_path(project.namespace, project, next_issuable_for(project, issuable.id).try(:iid)) - elsif current_controller?(:merge_requests) - namespace_project_merge_request_path(project.namespace, project, next_issuable_for(project, issuable.id).try(:iid)) - end - end - - def issuable_link_prev(project,issuable) - if project.nil? - nil - elsif current_controller?(:issues) - namespace_project_issue_path(project.namespace, project, prev_issuable_for(project, issuable.id).try(:iid)) - elsif current_controller?(:merge_requests) - namespace_project_merge_request_path(project.namespace, project, prev_issuable_for(project, issuable.id).try(:iid)) - end - end - - def issuable_count(entity, project) - if project.nil? - 0 - elsif current_controller?(:issues) - project.issues.send(entity).count - elsif current_controller?(:merge_requests) - project.merge_requests.send(entity).count - end - end - - def next_issuable_for(project, id) - if project.nil? - nil - elsif current_controller?(:issues) - project.issues.where("id > ?", id).last - elsif current_controller?(:merge_requests) - project.merge_requests.where("id > ?", id).last - end - end - - def has_next_issuable?(project, id) - if project.nil? - nil - elsif current_controller?(:issues) - project.issues.where("id > ?", id).last - elsif current_controller?(:merge_requests) - project.merge_requests.where("id > ?", id).last - end - end - - def prev_issuable_for(project, id) - if project.nil? - nil - elsif current_controller?(:issues) - project.issues.where("id < ?", id).first - elsif current_controller?(:merge_requests) - project.merge_requests.where("id < ?", id).first - end - end - - def has_prev_issuable?(project, id) - if project.nil? - nil - elsif current_controller?(:issues) - project.issues.where("id < ?", id).first - elsif current_controller?(:merge_requests) - project.merge_requests.where("id < ?", id).first - end - end - def state_filters_text_for(entity, project) titles = { opened: "Open" diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb new file mode 100644 index 00000000000..ccbd93967a2 --- /dev/null +++ b/app/helpers/issuables_helper.rb @@ -0,0 +1,41 @@ +module IssuablesHelper + + def sidebar_gutter_toggle_icon + sidebar_gutter_collapsed? ? icon('angle-double-left') : icon('angle-double-right') + end + + def sidebar_gutter_collapsed_class + "right-sidebar-#{sidebar_gutter_collapsed? ? 'collapsed' : 'expanded'}" + end + + def issuable_index(issuable) + base_issuable_scope(issuable).where('id < ?', issuable.id).size + 1 + end + + def issuables_count(issuable) + base_issuable_scope(issuable).size + end + + def next_issuable_for(issuable) + base_issuable_scope(issuable).where('id > ?', issuable.id).last + end + + def prev_issuable_for(issuable) + base_issuable_scope(issuable).where('id < ?', issuable.id).first + end + + private + + def sidebar_gutter_collapsed? + cookies[:collapsed_gutter] == 'true' + end + + def base_issuable_scope(issuable) + issuable.project.send(issuable.to_scope_name).send(issuable_state_scope(issuable)) + end + + def issuable_state_scope(issuable) + issuable.open? ? :opened : :closed + end + +end diff --git a/app/helpers/nav_helper.rb b/app/helpers/nav_helper.rb index 75f2ed5e054..29cb753e62c 100644 --- a/app/helpers/nav_helper.rb +++ b/app/helpers/nav_helper.rb @@ -3,18 +3,6 @@ module NavHelper cookies[:collapsed_nav] == 'true' end - def sidebar_gutter_collapsed_class - if cookies[:collapsed_gutter] == 'true' - "right-sidebar-collapsed" - else - "right-sidebar-expanded" - end - end - - def sidebar_gutter_collapsed? - cookies[:collapsed_gutter] == 'true' - end - def nav_sidebar_class if nav_menu_collapsed? "sidebar-collapsed" @@ -32,9 +20,9 @@ module NavHelper end def page_gutter_class - if current_path?('merge_requests#show') || - current_path?('merge_requests#diffs') || - current_path?('merge_requests#commits') || + if current_path?('merge_requests#show') || + current_path?('merge_requests#diffs') || + current_path?('merge_requests#commits') || current_path?('issues#show') if cookies[:collapsed_gutter] == 'true' "page-gutter right-sidebar-collapsed" diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index cf6aa592e2a..e08687135d6 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -168,6 +168,16 @@ module Issuable self.class.to_s.underscore end + # Convert this Issuable class name to a format usable for scoping + # + # Examples: + # + # issuable.class # => MergeRequest + # issuable.to_scope_name # => "merge_requests" + def to_scope_name + self.class.to_s.tableize + end + # Returns a Hash of attributes to be used for Twitter card metadata def card_attributes { diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index ae96a45453f..cb5378964bc 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -2,23 +2,20 @@ .issuable-sidebar .block %span.issuable-count.pull-left - = issuable.iid + = issuable_index(issuable) of - = issuable_count(:all, @project) + = issuables_count(issuable) %span.pull-right %a.gutter-toggle{href: '#'} - - if sidebar_gutter_collapsed? - = icon('angle-double-left') - - else - = icon('angle-double-right') + = sidebar_gutter_toggle_icon .issuable-nav.pull-right.btn-group{role: 'group', "aria-label" => '...'} - - if has_prev_issuable?(@project, issuable.id) - = link_to 'Prev', issuable_link_prev(@project, issuable), class: 'btn btn-default prev-btn' + - if prev_issuable = prev_issuable_for(issuable) + = link_to 'Prev', [@project.namespace.becomes(Namespace), @project, prev_issuable], class: 'btn btn-default prev-btn' - else %a.btn.btn-default.disabled{href: '#'} Prev - - if has_next_issuable?(@project, issuable.id) - = link_to 'Next', issuable_link_next(@project, issuable), class: 'btn btn-default next-btn' + - if next_issuable = next_issuable_for(issuable) + = link_to 'Next', [@project.namespace.becomes(Namespace), @project, next_issuable], class: 'btn btn-default next-btn' - else %a.btn.btn-default.disabled{href: '#'} Next diff --git a/features/project/issues/issues.feature b/features/project/issues/issues.feature index 0b3d03aa2a5..ca2399d85a9 100644 --- a/features/project/issues/issues.feature +++ b/features/project/issues/issues.feature @@ -25,9 +25,16 @@ Feature: Project Issues Scenario: I visit issue page Given I click link "Release 0.4" Then I should see issue "Release 0.4" + And I should see "1 of 2" in the sidebar + + Scenario: I navigate between issues + Given I click link "Release 0.4" + Then I click link "Next" in the sidebar + Then I should see issue "Tweet control" + And I should see "2 of 2" in the sidebar @javascript - Scenario: I visit issue page + Scenario: I filter by author Given I add a user to project "Shop" And I click "author" dropdown Then I see current user as the first user diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index ca1ee6b3c2b..5995e787961 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -39,6 +39,7 @@ Feature: Project Merge Requests Scenario: I visit merge request page Given I click link "Bug NS-04" Then I should see merge request "Bug NS-04" + And I should see "1 of 1" in the sidebar Scenario: I close merge request page Given I click link "Bug NS-04" diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb index d556b73f9fd..09a89e99831 100644 --- a/features/steps/project/issues/issues.rb +++ b/features/steps/project/issues/issues.rb @@ -54,6 +54,10 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps expect(page).to have_content "Release 0.4" end + step 'I should see issue "Tweet control"' do + expect(page).to have_content "Tweet control" + end + step 'I click link "New Issue"' do click_link "New Issue" end @@ -301,4 +305,5 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps def filter_issue(text) fill_in 'issue_search', with: text end + end diff --git a/features/steps/shared/issuable.rb b/features/steps/shared/issuable.rb index 25c2b476f43..2117feaedb8 100644 --- a/features/steps/shared/issuable.rb +++ b/features/steps/shared/issuable.rb @@ -119,6 +119,24 @@ module SharedIssuable end end + step 'I should see "1 of 1" in the sidebar' do + expect_sidebar_content('1 of 1') + end + + step 'I should see "1 of 2" in the sidebar' do + expect_sidebar_content('1 of 2') + end + + step 'I should see "2 of 2" in the sidebar' do + expect_sidebar_content('2 of 2') + end + + step 'I click link "Next" in the sidebar' do + page.within '.issuable-sidebar' do + click_link 'Next' + end + end + def create_issuable_for_project(project_name:, title:, type: :issue) project = Project.find_by(name: project_name) @@ -159,4 +177,10 @@ module SharedIssuable expect(page).to have_content("mentioned in #{issuable.class.to_s.titleize.downcase} #{issuable.to_reference(project)}") end + def expect_sidebar_content(content) + page.within '.issuable-sidebar' do + expect(page).to have_content content + end + end + end From 447568d15f9ec2d47a15fc04aeb2cb507cd6a55c Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 15 Feb 2016 14:13:47 -0500 Subject: [PATCH 125/134] Fix undefined method `postgresql?` during migration --- lib/gitlab/database.rb | 8 ++------ spec/lib/gitlab/database_spec.rb | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 6ebb8027553..6f9da69983a 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -17,7 +17,7 @@ module Gitlab end def true_value - if self.class.postgresql? + if Gitlab::Database.postgresql? "'t'" else 1 @@ -25,7 +25,7 @@ module Gitlab end def false_value - if self.class.postgresql? + if Gitlab::Database.postgresql? "'f'" else 0 @@ -47,9 +47,5 @@ module Gitlab row.first end end - - def connection - self.class.connection - end end end diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index bd8688fefa1..d0a447753b7 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -1,5 +1,9 @@ require 'spec_helper' +class MigrationTest + include Gitlab::Database +end + describe Gitlab::Database, lib: true do # These are just simple smoke tests to check if the methods work (regardless # of what they may return). @@ -34,4 +38,32 @@ describe Gitlab::Database, lib: true do end end end + + describe '#true_value' do + it 'returns correct value for PostgreSQL' do + expect(described_class).to receive(:postgresql?).and_return(true) + + expect(MigrationTest.new.true_value).to eq "'t'" + end + + it 'returns correct value for MySQL' do + expect(described_class).to receive(:postgresql?).and_return(false) + + expect(MigrationTest.new.true_value).to eq 1 + end + end + + describe '#false_value' do + it 'returns correct value for PostgreSQL' do + expect(described_class).to receive(:postgresql?).and_return(true) + + expect(MigrationTest.new.false_value).to eq "'f'" + end + + it 'returns correct value for MySQL' do + expect(described_class).to receive(:postgresql?).and_return(false) + + expect(MigrationTest.new.false_value).to eq 0 + end + end end From f5ab126fd0e607811638ca36b6752d5c74535adf Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 15 Feb 2016 15:48:16 -0500 Subject: [PATCH 126/134] Ensure Commit#show responds 404 instead of 500 when given an invalid ID Closes #13467 --- app/controllers/projects/commit_controller.rb | 4 +- .../projects/commit_controller_spec.rb | 37 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 spec/controllers/projects/commit_controller_spec.rb diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 21f4d9f44ec..36951b91372 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -11,8 +11,6 @@ class Projects::CommitController < Projects::ApplicationController before_action :define_show_vars, only: [:show, :builds] def show - return git_not_found! unless @commit - apply_diff_view_cookie! @line_notes = commit.notes.inline @@ -68,6 +66,8 @@ class Projects::CommitController < Projects::ApplicationController end def define_show_vars + return git_not_found! unless commit + if params[:w].to_i == 1 @diffs = commit.diffs({ ignore_whitespace_change: true }) else diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb new file mode 100644 index 00000000000..438e776ec4b --- /dev/null +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -0,0 +1,37 @@ +require 'rails_helper' + +describe Projects::CommitController do + describe 'GET show' do + let(:project) { create(:project) } + + before do + user = create(:user) + project.team << [user, :master] + + sign_in(user) + end + + context 'with valid id' do + it 'responds with 200' do + go id: project.commit.id + + expect(response).to be_ok + end + end + + context 'with invalid id' do + it 'responds with 404' do + go id: project.commit.id.reverse + + expect(response).to be_not_found + end + end + + def go(id:) + get :show, + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: id + end + end +end From 012c75d1b0a24aff3c31ca5b5d475e06d5f77428 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 15 Feb 2016 15:52:39 -0500 Subject: [PATCH 127/134] Properly render the `errors/git_not_found` page --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 48b1f95acb9..7afe3c0c471 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -164,7 +164,7 @@ class ApplicationController < ActionController::Base end def git_not_found! - render html: "errors/git_not_found", layout: "errors", status: 404 + render "errors/git_not_found", layout: "errors", status: 404 end def method_missing(method_sym, *arguments, &block) From ae13389b0b8654abcffead659788580e8c1f1a15 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 15 Feb 2016 16:38:27 -0500 Subject: [PATCH 128/134] Provide explicit html format when rendering git_not_found page Prior, if the request format was, for example, .zip, we'd get an `ActionView::MissingTemplate` error. --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7afe3c0c471..2c329b60a19 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -164,7 +164,7 @@ class ApplicationController < ActionController::Base end def git_not_found! - render "errors/git_not_found", layout: "errors", status: 404 + render "errors/git_not_found.html", layout: "errors", status: 404 end def method_missing(method_sym, *arguments, &block) From 3de6edd6041a725aaffba95603d4eb2912627d42 Mon Sep 17 00:00:00 2001 From: Sytse Sijbrandij Date: Mon, 15 Feb 2016 19:38:36 -0800 Subject: [PATCH 129/134] Updates to git flow documentation. --- doc/workflow/gitlab_flow.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/doc/workflow/gitlab_flow.md b/doc/workflow/gitlab_flow.md index be32f0c720b..0b205ea6de7 100644 --- a/doc/workflow/gitlab_flow.md +++ b/doc/workflow/gitlab_flow.md @@ -187,12 +187,15 @@ If you have an issue that spans across multiple repositories, the best thing is ![Vim screen showing the rebase view](rebase.png) With git you can use an interactive rebase (`rebase -i`) to squash multiple commits into one and reorder them. +In GitLab EE and .com you can also [rebase before merge](http://doc.gitlab.com/ee/workflow/rebase_before_merge.html) from the web interface. This functionality is useful if you made a couple of commits for small changes during development and want to replace them with a single commit or if you want to make the order more logical. However you should never rebase commits you have pushed to a remote server. Somebody can have referred to the commits or cherry-picked them. When you rebase you change the identifier (SHA-1) of the commit and this is confusing. If you do that the same change will be known under multiple identifiers and this can cause much confusion. If people already reviewed your code it will be hard for them to review only the improvements you made since then if you have rebased everything into one commit. +Another reasons not to rebase is that you lose authorship information, maybe someone created a merge request, another person pushed a commit on there to improve it and a third one merged it. +In this case rebasing all the commits into one prevent the other authors from being properly attributed and sharing part of the [git blame](https://git-scm.com/docs/git-blame). People are encouraged to commit often and to frequently push to the remote repository so other people are aware what everyone is working on. This will lead to many commits per change which makes the history harder to understand. @@ -221,13 +224,11 @@ You can reuse recorded resolutions (rerere) sometimes, but without rebasing you There has to be a better way to avoid many merge commits. The way to prevent creating many merge commits is to not frequently merge master into the feature branch. -We'll discuss the three reasons to merge in master: leveraging code, solving merge conflicts and long running branches. +We'll discuss the three reasons to merge in master: leveraging code, merge conflicts, and long running branches. If you need to leverage some code that was introduced in master after you created the feature branch you can sometimes solve this by just cherry-picking a commit. If your feature branch has a merge conflict, creating a merge commit is a normal way of solving this. -You should aim to prevent merge conflicts where they are likely to occur. -One example is the CHANGELOG file where each significant change in the codebase is documented under a version header. -Instead of everyone adding their change at the bottom of the list for the current version it is better to randomly insert it in the current list for that version. -This it is likely that multiple feature branches that add to the CHANGELOG can be merged before a conflict occurs. +You can prevent some merge conflicts by using [gitattributes](http://git-scm.com/docs/gitattributes) for files that can be in a random order. +For example in GitLab our changelog file is specified in .gitattributes as `CHANGELOG merge=union` so that there are fewer merge conflicts in it. The last reason for creating merge commits is having long lived branches that you want to keep up to date with the latest state of the project. Martin Fowler, in [his article about feature branches](http://martinfowler.com/bliki/FeatureBranch.html) talks about this Continuous Integration (CI). At GitLab we are guilty of confusing CI with branch testing. Quoting Martin Fowler: "I've heard people say they are doing CI because they are running builds, perhaps using a CI server, on every branch with every commit. From 11913a762a3ed6514594e6ac6ffe1717dd362ae1 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 16 Feb 2016 09:01:56 +0100 Subject: [PATCH 130/134] updated system note service and spec based on feedback --- app/services/system_note_service.rb | 4 +++- spec/services/system_note_service_spec.rb | 4 ---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 843b44abd17..edced010811 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -274,7 +274,9 @@ class SystemNoteService # Check if a cross reference to a noteable from a mentioner already exists # # This method is used to prevent multiple notes being created for a mention - # when a issue is updated, for example. + # when a issue is updated, for example. The method also calls notes_for_mentioner + # to check if the mentioner is a commit, and return matches only on commit hash + # instead of project + commit, to avoid repeated mentions from forks. # # noteable - Noteable object being referenced # mentioner - Mentionable object diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 9b9732ea01f..1bdc03af12d 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -431,10 +431,6 @@ describe SystemNoteService, services: true do let(:commit2) { forked_project.commit } before do - allow(commit0).to receive(:to_reference) { - noteable.project.to_reference + - commit0.class.reference_prefix + commit0.id - } described_class.cross_reference(noteable, commit0, author2) end From 5cd20de7499840463eb1ee91793359ea52f29af8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 16 Feb 2016 10:48:37 +0100 Subject: [PATCH 131/134] Eager-load image blob data in diffs Since gitlab_git 8.0, blob data are lazy-loaded so we have to call blob.load_all_data!(repo) to eager-load them. Fixes #13458. --- app/views/projects/diffs/_image.html.haml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/views/projects/diffs/_image.html.haml b/app/views/projects/diffs/_image.html.haml index 058b71b21f5..4fcf7ea0b26 100644 --- a/app/views/projects/diffs/_image.html.haml +++ b/app/views/projects/diffs/_image.html.haml @@ -1,4 +1,5 @@ - diff = diff_file.diff +- file.load_all_data!(@project.repository) - if diff.renamed_file || diff.new_file || diff.deleted_file .image %span.wrap @@ -6,6 +7,7 @@ %img{src: "data:#{file.mime_type};base64,#{Base64.encode64(file.data)}"} %p.image-info= "#{number_to_human_size file.size}" - else + - old_file.load_all_data!(@project.repository) .image %div.two-up.view %span.wrap From f253f72529824198a591e06537d321917eca1d5e Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 16 Feb 2016 12:13:13 +0100 Subject: [PATCH 132/134] Cleaned up Repository#initialize The "default_branch" argument is never used and the "project" argument isn't optional. --- app/models/project.rb | 2 +- app/models/project_wiki.rb | 2 +- app/models/repository.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index f11c6d7c6be..d18131a6762 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -342,7 +342,7 @@ class Project < ActiveRecord::Base end def repository - @repository ||= Repository.new(path_with_namespace, nil, self) + @repository ||= Repository.new(path_with_namespace, self) end def commit(id = 'HEAD') diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index c847eba8d1c..c96e6f0b8ea 100644 --- a/app/models/project_wiki.rb +++ b/app/models/project_wiki.rb @@ -123,7 +123,7 @@ class ProjectWiki end def repository - Repository.new(path_with_namespace, default_branch, @project) + Repository.new(path_with_namespace, @project) end def default_branch diff --git a/app/models/repository.rb b/app/models/repository.rb index 7f0047a002e..ba275fd9803 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -15,7 +15,7 @@ class Repository Gitlab::Popen.popen(%W(find #{repository_downloads_path} -not -path #{repository_downloads_path} -mmin +120 -delete)) end - def initialize(path_with_namespace, default_branch = nil, project = nil) + def initialize(path_with_namespace, project) @path_with_namespace = path_with_namespace @project = project end From d28fa2ce233e057dc47de17bb6c1f2dff141059a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 16 Feb 2016 12:58:12 +0100 Subject: [PATCH 133/134] Display "iid of max_iid" in Issuables' sidebar --- app/helpers/issuables_helper.rb | 12 ++++-------- app/models/concerns/issuable.rb | 10 ---------- app/views/shared/issuable/_sidebar.html.haml | 2 +- 3 files changed, 5 insertions(+), 19 deletions(-) diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index ccbd93967a2..91a3aa371ef 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -8,20 +8,16 @@ module IssuablesHelper "right-sidebar-#{sidebar_gutter_collapsed? ? 'collapsed' : 'expanded'}" end - def issuable_index(issuable) - base_issuable_scope(issuable).where('id < ?', issuable.id).size + 1 - end - def issuables_count(issuable) - base_issuable_scope(issuable).size + base_issuable_scope(issuable).maximum(:iid) end def next_issuable_for(issuable) - base_issuable_scope(issuable).where('id > ?', issuable.id).last + base_issuable_scope(issuable).where('iid > ?', issuable.iid).last end def prev_issuable_for(issuable) - base_issuable_scope(issuable).where('id < ?', issuable.id).first + base_issuable_scope(issuable).where('iid < ?', issuable.iid).first end private @@ -31,7 +27,7 @@ module IssuablesHelper end def base_issuable_scope(issuable) - issuable.project.send(issuable.to_scope_name).send(issuable_state_scope(issuable)) + issuable.project.send(issuable.class.table_name).send(issuable_state_scope(issuable)) end def issuable_state_scope(issuable) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index e08687135d6..cf6aa592e2a 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -168,16 +168,6 @@ module Issuable self.class.to_s.underscore end - # Convert this Issuable class name to a format usable for scoping - # - # Examples: - # - # issuable.class # => MergeRequest - # issuable.to_scope_name # => "merge_requests" - def to_scope_name - self.class.to_s.tableize - end - # Returns a Hash of attributes to be used for Twitter card metadata def card_attributes { diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index cb5378964bc..e1aecdd9436 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -2,7 +2,7 @@ .issuable-sidebar .block %span.issuable-count.pull-left - = issuable_index(issuable) + = issuable.iid of = issuables_count(issuable) %span.pull-right From 1de95137906e1b262eeaf2d627469c9a1e47a536 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 16 Feb 2016 13:03:00 +0100 Subject: [PATCH 134/134] Use project.web_url instead deprecated repository.homepage in PushoverService --- app/models/project_services/pushover_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/project_services/pushover_service.rb b/app/models/project_services/pushover_service.rb index 3d7e8bbee61..e76d9eca2ab 100644 --- a/app/models/project_services/pushover_service.rb +++ b/app/models/project_services/pushover_service.rb @@ -112,7 +112,7 @@ class PushoverService < Service priority: priority, title: "#{project.name_with_namespace}", message: message, - url: data[:repository][:homepage], + url: data[:project][:web_url], url_title: "See project #{project.name_with_namespace}" }