diff options
26 files changed, 279 insertions, 37 deletions
diff --git a/CHANGELOG b/CHANGELOG index 137257add3b..8fa927c4fb3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -15,14 +15,22 @@ v 8.13.0 (unreleased) - Only update issuable labels if they have been changed - Revoke button in Applications Settings underlines on hover. - Fix Long commit messages overflow viewport in file tree + - Revert avoid touching file system on Build#artifacts? - Update ruby-prof to 0.16.2. !6026 (Elan Ruusamäe) + - Fix unnecessary escaping of reserved HTML characters in milestone title. !6533 - Add organization field to user profile - Fix resolved discussion display in side-by-side diff view !6575 - Optimize GitHub importing for speed and memory - API: expose pipeline data in builds API (!6502, Guilherme Salazar) + - Fix broken repository 500 errors in project list + - Close todos when accepting merge requests via the API !6486 (tonygambone) -v 8.12.2 (unreleased) - - Added University content to doc/university +v 8.12.4 (unreleased) + +v 8.12.3 + - Update Gitlab Shell to support low IO priority for storage moves + +v 8.12.2 - Fix Import/Export not recognising correctly the imported services. - Fix snippets pagination - Fix List-Unsubscribe header in emails @@ -31,11 +39,14 @@ v 8.12.2 (unreleased) - Fix errors importing project feature and milestone models using GitLab project import - Make JWT messages Docker-compatible - Fix duplicate branch entry in the merge request version compare dropdown + - Respect the fork_project permission when forking projects + - Only update issuable labels if they have been changed + - Fix bug where 'Search results' repeated many times when a search in the emoji search form is cleared (Xavier Bick) (@zeiv) + - Fix resolve discussion buttons endpoint path v 8.12.1 - Fix a memory leak in HTML::Pipeline::SanitizationFilter::WHITELIST - Fix issue with search filter labels not displaying - - Fix bug where 'Search results' repeated many times when a search in the emoji search form is cleared (Xavier Bick) (@zeiv) v 8.12.0 - Update the rouge gem to 2.0.6, which adds highlighting support for JSX, Prometheus, and others. !6251 @@ -52,7 +63,6 @@ v 8.12.0 - Filter tags by name !6121 - Update gitlab shell secret file also when it is empty. !3774 (glensc) - Give project selection dropdowns responsive width, make non-wrapping. - - Fix resolve discussion buttons endpoint path - Fix note form hint showing slash commands supported for commits. - Make push events have equal vertical spacing. - API: Ensure invitees are not returned in Members API. @@ -223,6 +233,12 @@ v 8.12.0 - Fix non-master branch readme display in tree view - Add UX improvements for merge request version diffs +v 8.11.8 + - Respect the fork_project permission when forking projects + - Set a restrictive CORS policy on the API for credentialed requests + - API: disable rails session auth for non-GET/HEAD requests + - Escape HTML nodes in builds commands in CI linter + v 8.11.7 - Avoid conflict with admin labels when importing GitHub labels. !6158 - Restores `fieldName` to allow only string values in `gl_dropdown.js`. !6234 @@ -442,6 +458,12 @@ v 8.11.0 - Update gitlab_git gem to 10.4.7 - Simplify SQL queries of marking a todo as done +v 8.10.11 + - Respect the fork_project permission when forking projects + - Set a restrictive CORS policy on the API for credentialed requests + - API: disable rails session auth for non-GET/HEAD requests + - Escape HTML nodes in builds commands in CI linter + v 8.10.10 - Allow the Rails cookie to be used for API authentication. @@ -678,6 +700,12 @@ v 8.10.0 - Show tooltip on GitLab export link in new project page - Fix import_data wrongly saved as a result of an invalid import_url !5206 +v 8.9.11 + - Respect the fork_project permission when forking projects + - Set a restrictive CORS policy on the API for credentialed requests + - API: disable rails session auth for non-GET/HEAD requests + - Escape HTML nodes in builds commands in CI linter + v 8.9.10 - Allow the Rails cookie to be used for API authentication. @@ -51,7 +51,7 @@ gem 'browser', '~> 2.2' # Extracting information from a git repository # Provide access to Gitlab::Git library -gem 'gitlab_git', '~> 10.6.6' +gem 'gitlab_git', '~> 10.6.7' # LDAP Auth # GitLab fork with several improvements to original library. For full list of changes diff --git a/Gemfile.lock b/Gemfile.lock index 6f6667b51e9..f15715a20ff 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -276,7 +276,7 @@ GEM diff-lcs (~> 1.1) mime-types (>= 1.16, < 3) posix-spawn (~> 0.3) - gitlab_git (10.6.6) + gitlab_git (10.6.7) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) @@ -857,7 +857,7 @@ DEPENDENCIES github-linguist (~> 4.7.0) github-markup (~> 1.4) gitlab-flowdock-git-hook (~> 1.0.1) - gitlab_git (~> 10.6.6) + gitlab_git (~> 10.6.7) gitlab_omniauth-ldap (~> 1.2.1) gollum-lib (~> 4.2) gollum-rugged_adapter (~> 0.4.2) diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index aed77d0358a..aa7570cd896 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -10,7 +10,7 @@ class Admin::GroupsController < Admin::ApplicationController def show @members = @group.members.order("access_level DESC").page(params[:members_page]) - @requesters = @group.requesters + @requesters = AccessRequestsFinder.new(@group).execute(current_user) @projects = @group.projects.page(params[:projects_page]) end diff --git a/app/controllers/admin/projects_controller.rb b/app/controllers/admin/projects_controller.rb index 0d2f4f6eb38..1d963bdf7d5 100644 --- a/app/controllers/admin/projects_controller.rb +++ b/app/controllers/admin/projects_controller.rb @@ -22,7 +22,7 @@ class Admin::ProjectsController < Admin::ApplicationController end @project_members = @project.members.page(params[:project_members_page]) - @requesters = @project.requesters + @requesters = AccessRequestsFinder.new(@project).execute(current_user) end def transfer diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index 272164cd0cc..9c323d7705a 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -15,7 +15,7 @@ class Groups::GroupMembersController < Groups::ApplicationController end @members = @members.order('access_level DESC').page(params[:page]).per(50) - @requesters = @group.requesters if can?(current_user, :admin_group, @group) + @requesters = AccessRequestsFinder.new(@group).execute(current_user) @group_member = @group.group_members.new end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 935417d4ae8..020a21ddf93 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -308,8 +308,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController return end - TodoService.new.merge_merge_request(merge_request, current_user) - @merge_request.update(merge_error: nil) if params[:merge_when_build_succeeds].present? diff --git a/app/controllers/projects/project_members_controller.rb b/app/controllers/projects/project_members_controller.rb index 42a7e5a2c30..2343c7d20ec 100644 --- a/app/controllers/projects/project_members_controller.rb +++ b/app/controllers/projects/project_members_controller.rb @@ -29,7 +29,7 @@ class Projects::ProjectMembersController < Projects::ApplicationController @group_members = @group_members.order('access_level DESC') end - @requesters = @project.requesters if can?(current_user, :admin_project, @project) + @requesters = AccessRequestsFinder.new(@project).execute(current_user) @project_member = @project.project_members.new @project_group_links = @project.project_group_links diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index eaa38fa6c98..c99aeadb5e4 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -324,7 +324,12 @@ class ProjectsController < Projects::ApplicationController end def repo_exists? - project.repository_exists? && !project.empty_repo? + project.repository_exists? && !project.empty_repo? && project.repo + + rescue Gitlab::Git::Repository::NoRepository + project.repository.expire_exists_cache + + false end def project_view_files? diff --git a/app/finders/access_requests_finder.rb b/app/finders/access_requests_finder.rb new file mode 100644 index 00000000000..b6ee49df99b --- /dev/null +++ b/app/finders/access_requests_finder.rb @@ -0,0 +1,27 @@ +class AccessRequestsFinder + attr_accessor :source + + # Arguments: + # source - a Group or Project + def initialize(source) + @source = source + end + + def execute(*args) + execute!(*args) + rescue Gitlab::Access::AccessDeniedError + [] + end + + def execute!(current_user) + raise Gitlab::Access::AccessDeniedError unless can_see_access_requests?(current_user) + + source.requesters + end + + private + + def can_see_access_requests?(current_user) + source && Ability.allowed?(current_user, :"admin_#{source.class.to_s.underscore}", source) + end +end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 522e2264bb8..5dbf66173de 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -373,7 +373,7 @@ module Ci end def artifacts? - !artifacts_expired? && self[:artifacts_file].present? + !artifacts_expired? && artifacts_file.exists? end def artifacts_metadata? diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 2bd7f198030..44c3cbb2c73 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -158,7 +158,7 @@ class Milestone < ActiveRecord::Base end def title=(value) - write_attribute(:title, Sanitize.clean(value.to_s)) if value.present? + write_attribute(:title, sanitize_title(value)) if value.present? end # Sorts the issues for the given IDs. @@ -204,4 +204,8 @@ class Milestone < ActiveRecord::Base iid end end + + def sanitize_title(value) + CGI.unescape_html(Sanitize.clean(value.to_s)) + end end diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb index 8437d9b8b43..e8fb1b59752 100644 --- a/app/services/merge_requests/post_merge_service.rb +++ b/app/services/merge_requests/post_merge_service.rb @@ -7,6 +7,7 @@ module MergeRequests class PostMergeService < MergeRequests::BaseService def execute(merge_request) close_issues(merge_request) + todo_service.merge_merge_request(merge_request, current_user) merge_request.mark_as_merged create_merge_event(merge_request, current_user) create_note(merge_request) diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index cdad0426b02..e466ffa60eb 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -44,6 +44,11 @@ module Projects begin gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url) rescue => e + # Expire cache to prevent scenarios such as: + # 1. First import failed, but the repo was imported successfully, so +exists?+ returns true + # 2. Retried import, repo is broken or not imported but +exists?+ still returns true + project.repository.before_import if project.repository_exists? + raise Error, "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}" end end diff --git a/app/views/explore/projects/_filter.html.haml b/app/views/explore/projects/_filter.html.haml index cd485da5104..132bbe26fe0 100644 --- a/app/views/explore/projects/_filter.html.haml +++ b/app/views/explore/projects/_filter.html.haml @@ -8,7 +8,7 @@ - else Any %b.caret - %ul.dropdown-menu + %ul.dropdown-menu.dropdown-menu-align-right %li = link_to filter_projects_path(visibility_level: nil) do Any @@ -28,7 +28,7 @@ - else Any %b.caret - %ul.dropdown-menu + %ul.dropdown-menu.dropdown-menu-align-right %li = link_to filter_projects_path(tag: nil) do Any diff --git a/config/application.rb b/config/application.rb index 8166b6003f6..5dbe5a8120b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -99,13 +99,24 @@ module Gitlab config.action_view.sanitized_allowed_protocols = %w(smb) - config.middleware.use Rack::Attack + config.middleware.insert_before Warden::Manager, Rack::Attack # Allow access to GitLab API from other domains - config.middleware.use Rack::Cors do + config.middleware.insert_before Warden::Manager, Rack::Cors do + allow do + origins Gitlab.config.gitlab.url + resource '/api/*', + credentials: true, + headers: :any, + methods: :any, + expose: ['Link'] + end + + # Cross-origin requests must not have the session cookie available allow do origins '*' resource '/api/*', + credentials: false, headers: :any, methods: :any, expose: ['Link'] diff --git a/lib/api/access_requests.rb b/lib/api/access_requests.rb index 9d1d9058996..7b9de7c9598 100644 --- a/lib/api/access_requests.rb +++ b/lib/api/access_requests.rb @@ -16,9 +16,9 @@ module API # GET /projects/:id/access_requests get ":id/access_requests" do source = find_source(source_type, params[:id]) - authorize_admin_source!(source_type, source) - access_requesters = paginate(source.requesters.includes(:user)) + access_requesters = AccessRequestsFinder.new(source).execute!(current_user) + access_requesters = paginate(access_requesters.includes(:user)) present access_requesters.map(&:user), with: Entities::AccessRequester, source: source end diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb index 7b3bbcf6a32..5f67e97fa2a 100644 --- a/lib/gitlab/lfs_token.rb +++ b/lib/gitlab/lfs_token.rb @@ -20,13 +20,8 @@ module Gitlab def token Gitlab::Redis.with do |redis| token = redis.get(redis_key) - - if token - redis.expire(redis_key, EXPIRY_TIME) - else - token = Devise.friendly_token(TOKEN_LENGTH) - redis.set(redis_key, token, ex: EXPIRY_TIME) - end + token ||= Devise.friendly_token(TOKEN_LENGTH) + redis.set(redis_key, token, ex: EXPIRY_TIME) token end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index b0f740f48f7..da0fdce39db 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -63,6 +63,28 @@ describe ProjectsController do end end + context "project with broken repo" do + let(:empty_project) { create(:project_broken_repo, :public) } + + before { sign_in(user) } + + User.project_views.keys.each do |project_view| + context "with #{project_view} view set" do + before do + user.update_attributes(project_view: project_view) + + get :show, namespace_id: empty_project.namespace.path, id: empty_project.path + end + + it "renders the empty project view" do + allow(Project).to receive(:repo).and_raise(Gitlab::Git::Repository::NoRepository) + + expect(response).to render_template('projects/no_repo') + end + end + end + end + context "rendering default project view" do render_views diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index e61b1fd9647..873d3fcb5af 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -27,6 +27,14 @@ FactoryGirl.define do end end + trait :broken_repo do + after(:create) do |project| + project.create_repository + + FileUtils.rm_r(File.join(project.repository_storage_path, "#{project.path_with_namespace}.git", 'refs')) + end + end + # Nest Project Feature attributes transient do wiki_access_level ProjectFeature::ENABLED @@ -56,6 +64,13 @@ FactoryGirl.define do empty_repo end + # Project with broken repository + # + # Project with an invalid repository state + factory :project_broken_repo, parent: :empty_project do + broken_repo + end + # Project with test repository # # Test repository source can be found at diff --git a/spec/finders/access_requests_finder_spec.rb b/spec/finders/access_requests_finder_spec.rb new file mode 100644 index 00000000000..6cc90299417 --- /dev/null +++ b/spec/finders/access_requests_finder_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +describe AccessRequestsFinder, services: true do + let(:user) { create(:user) } + let(:access_requester) { create(:user) } + let(:project) { create(:project) } + let(:group) { create(:group) } + + before do + project.request_access(access_requester) + group.request_access(access_requester) + end + + shared_examples 'a finder returning access requesters' do |method_name| + it 'returns access requesters' do + access_requesters = described_class.new(source).public_send(method_name, user) + + expect(access_requesters.size).to eq(1) + expect(access_requesters.first).to be_a "#{source.class.to_s}Member".constantize + expect(access_requesters.first.user).to eq(access_requester) + end + end + + shared_examples 'a finder returning no results' do |method_name| + it 'raises Gitlab::Access::AccessDeniedError' do + expect(described_class.new(source).public_send(method_name, user)).to be_empty + end + end + + shared_examples 'a finder raising Gitlab::Access::AccessDeniedError' do |method_name| + it 'raises Gitlab::Access::AccessDeniedError' do + expect { described_class.new(source).public_send(method_name, user) }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + + describe '#execute' do + context 'when current user cannot see project access requests' do + it_behaves_like 'a finder returning no results', :execute do + let(:source) { project } + end + + it_behaves_like 'a finder returning no results', :execute do + let(:source) { group } + end + end + + context 'when current user can see access requests' do + before do + project.team << [user, :master] + group.add_owner(user) + end + + it_behaves_like 'a finder returning access requesters', :execute do + let(:source) { project } + end + + it_behaves_like 'a finder returning access requesters', :execute do + let(:source) { group } + end + end + end + + describe '#execute!' do + context 'when current user cannot see access requests' do + it_behaves_like 'a finder raising Gitlab::Access::AccessDeniedError', :execute! do + let(:source) { project } + end + + it_behaves_like 'a finder raising Gitlab::Access::AccessDeniedError', :execute! do + let(:source) { group } + end + end + + context 'when current user can see access requests' do + before do + project.team << [user, :master] + group.add_owner(user) + end + + it_behaves_like 'a finder returning access requesters', :execute! do + let(:source) { project } + end + + it_behaves_like 'a finder returning access requesters', :execute! do + let(:source) { group } + end + end + end +end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index d64d6cde2b5..33fe22dd98c 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -20,10 +20,10 @@ describe Milestone, models: true do let(:user) { create(:user) } describe "#title" do - let(:milestone) { create(:milestone, title: "<b>test</b>") } + let(:milestone) { create(:milestone, title: "<b>foo & bar -> 2.2</b>") } it "sanitizes title" do - expect(milestone.title).to eq("test") + expect(milestone.title).to eq("foo & bar -> 2.2") end end diff --git a/spec/requests/api/milestones_spec.rb b/spec/requests/api/milestones_spec.rb index b89dac01040..dd192bea432 100644 --- a/spec/requests/api/milestones_spec.rb +++ b/spec/requests/api/milestones_spec.rb @@ -104,6 +104,14 @@ describe API::API, api: true do expect(response).to have_http_status(400) end + + it 'creates a new project with reserved html characters' do + post api("/projects/#{project.id}/milestones", user), title: 'foo & bar 1.1 -> 2.2' + + expect(response).to have_http_status(201) + expect(json_response['title']).to eq('foo & bar 1.1 -> 2.2') + expect(json_response['description']).to be_nil + end end describe 'PUT /projects/:id/milestones/:milestone_id' do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 159f6817e8d..31167675d07 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -38,6 +38,30 @@ describe MergeRequests::MergeService, services: true do end end + context 'closes related todos' do + let(:merge_request) { create(:merge_request, assignee: user, author: user) } + let(:project) { merge_request.project } + let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') } + let!(:todo) do + create(:todo, :assigned, + project: project, + author: user, + user: user, + target: merge_request) + end + + before do + allow(service).to receive(:execute_hooks) + + perform_enqueued_jobs do + service.execute(merge_request) + todo.reload + end + end + + it { expect(todo).to be_done } + end + context 'remove source branch by author' do let(:service) do merge_request.merge_params['force_remove_source_branch'] = '1' diff --git a/spec/services/merge_requests/refresh_service_spec.rb b/spec/services/merge_requests/refresh_service_spec.rb index a162df5fc34..59d3912018a 100644 --- a/spec/services/merge_requests/refresh_service_spec.rb +++ b/spec/services/merge_requests/refresh_service_spec.rb @@ -79,8 +79,8 @@ describe MergeRequests::RefreshService, services: true do it { expect(@merge_request).to be_merged } it { expect(@fork_merge_request).to be_merged } it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') } - it { expect(@build_failed_todo).to be_pending } - it { expect(@fork_build_failed_todo).to be_pending } + it { expect(@build_failed_todo).to be_done } + it { expect(@fork_build_failed_todo).to be_done } end context 'manual merge of source branch' do @@ -99,8 +99,8 @@ describe MergeRequests::RefreshService, services: true do it { expect(@merge_request.diffs.size).to be > 0 } it { expect(@fork_merge_request).to be_merged } it { expect(@fork_merge_request.notes.last.note).to include('changed to merged') } - it { expect(@build_failed_todo).to be_pending } - it { expect(@fork_build_failed_todo).to be_pending } + it { expect(@build_failed_todo).to be_done } + it { expect(@fork_build_failed_todo).to be_done } end context 'push to fork repo source branch' do @@ -149,8 +149,8 @@ describe MergeRequests::RefreshService, services: true do it { expect(@merge_request).to be_merged } it { expect(@fork_merge_request).to be_open } it { expect(@fork_merge_request.notes).to be_empty } - it { expect(@build_failed_todo).to be_pending } - it { expect(@fork_build_failed_todo).to be_pending } + it { expect(@build_failed_todo).to be_done } + it { expect(@fork_build_failed_todo).to be_done } end context 'push new branch that exists in a merge request' do diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index d5d4d7c56ef..ed1384798ab 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -108,6 +108,16 @@ describe Projects::ImportService, services: true do expect(result[:status]).to eq :error expect(result[:message]).to eq 'Github: failed to connect API' end + + it 'expires existence cache after error' do + allow_any_instance_of(Project).to receive(:repository_exists?).and_return(true) + + expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).and_call_original + expect_any_instance_of(Repository).to receive(:expire_exists_cache).and_call_original + + subject.execute + end end def stub_github_omniauth_provider |