From 0bea5ced8bf4c9306f8f8e912313731a43d16f4c Mon Sep 17 00:00:00 2001
From: Han Loong Liauw <hanloongliauw@gmail.com>
Date: Wed, 14 Oct 2015 09:04:22 +1100
Subject: [PATCH] Made suggested content changes based on MR Review
Changed the authentication method for removing fork through API
Reflected changes to new auth method in API specs
---
CHANGELOG | 2 +-
app/controllers/projects_controller.rb | 6 ++-
app/views/projects/edit.html.haml | 8 ++--
config/routes.rb | 2 +-
lib/api/projects.rb | 2 +-
spec/controllers/projects_controller_spec.rb | 26 ++++++----
spec/features/projects_spec.rb | 6 +--
spec/requests/api/projects_spec.rb | 50 +++++++++++++-------
8 files changed, 63 insertions(+), 39 deletions(-)
diff --git a/CHANGELOG b/CHANGELOG
index cf9e5b43c4d..e45fd2ef5a1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -46,7 +46,7 @@ v 8.1.0 (unreleased)
- Fix bug where Emojis in Markdown would truncate remaining text (Sakata Sinji)
- Persist filters when sorting on admin user page (Jerry Lukins)
- Adds ability to remove the forked relationship from project settings
- screen. #2578 (Han Loong Liauw)
+ screen. (Han Loong Liauw)
- Add spellcheck=false to certain input fields
- Invalidate stored service password if the endpoint URL is changed
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 1fb83d0eb6d..77b3af9a5d0 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -5,7 +5,7 @@ class ProjectsController < ApplicationController
before_action :repository, except: [:new, :create]
# Authorize
- before_action :authorize_admin_project!, only: [:edit, :update, :destroy, :transfer, :archive, :unarchive, :remove_fork]
+ before_action :authorize_admin_project!, only: [:edit, :update]
before_action :event_filter, only: [:show, :activity]
layout :determine_layout
@@ -56,6 +56,8 @@ class ProjectsController < ApplicationController
end
def transfer
+ return access_denied! unless can?(current_user, :change_namespace, @project)
+
namespace = Namespace.find_by(id: params[:new_namespace_id])
::Projects::TransferService.new(project, current_user).execute(namespace)
@@ -65,6 +67,8 @@ class ProjectsController < ApplicationController
end
def remove_fork
+ return access_denied! unless can?(current_user, :remove_fork_project, @project)
+
if @project.forked?
@project.forked_project_link.destroy
flash[:notice] = 'Fork relationship has been removed.'
diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml
index ce77a9242fc..ec58d0924b0 100644
--- a/app/views/projects/edit.html.haml
+++ b/app/views/projects/edit.html.haml
@@ -190,17 +190,17 @@
.nothing-here-block Only the project owner can transfer a project
- if @project.forked? && can?(current_user, :remove_fork_project, @project)
- = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :put, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f|
+ = form_for([@project.namespace.becomes(Namespace), @project], url: remove_fork_namespace_project_path(@project.namespace, @project), method: :delete, remote: true, html: { class: 'transfer-project form-horizontal' }) do |f|
.panel.panel-default.panel.panel-danger
- .panel-heading Remove forked relationship
+ .panel-heading Remove fork relationship
.panel-body
%p
This will remove the relationship to the source project from
= link_to project_path(@project.forked_from_project) do
= @project.forked_from_project.namespace.try(:name)
%br
- %strong Once removed it cannot be reversed through this interface
- = button_to 'Remove forked relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) }
+ %strong Once removed it cannot be reversed through this interface.
+ = button_to 'Remove fork relationship', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_fork_project_message(@project) }
- elsif @project.forked?
.nothing-here-block Only the project owner can remove the fork relationship
diff --git a/config/routes.rb b/config/routes.rb
index 3ac4342b23a..64bdd189f53 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -378,7 +378,7 @@ Gitlab::Application.routes.draw do
[:new, :create, :index], path: "/") do
member do
put :transfer
- put :remove_fork
+ delete :remove_fork
post :archive
post :unarchive
post :toggle_star
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index e8a0e7f3ec9..67ee66a2058 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -246,7 +246,7 @@ module API
# Example Request:
# DELETE /projects/:id/fork
delete ":id/fork" do
- authenticated_as_admin!
+ authorize! :remove_fork_project, user_project
if user_project.forked?
user_project.forked_project_link.destroy
end
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index 626b56cc789..e963e913512 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -72,9 +72,12 @@ describe ProjectsController do
context 'with forked project' do
let(:project_fork) { create(:project, namespace: user.namespace) }
- it 'should remove fork from project' do
+ before do
create(:forked_project_link, forked_to_project: project_fork)
- put(:remove_fork,
+ end
+
+ it 'should remove fork from project' do
+ delete(:remove_fork,
namespace_id: project_fork.namespace.to_param,
id: project_fork.to_param, format: :js)
@@ -84,19 +87,22 @@ describe ProjectsController do
end
end
- it 'should do nothing if project was not forked' do
- unforked_project = create(:project, namespace: user.namespace)
- put(:remove_fork,
- namespace_id: unforked_project.namespace.to_param,
- id: unforked_project.to_param, format: :js)
+ context 'when project not forked' do
+ let(:unforked_project) { create(:project, namespace: user.namespace) }
- expect(flash[:notice]).to be_nil
- expect(response).to render_template(:remove_fork)
+ it 'should do nothing if project was not forked' do
+ delete(:remove_fork,
+ namespace_id: unforked_project.namespace.to_param,
+ id: unforked_project.to_param, format: :js)
+
+ expect(flash[:notice]).to be_nil
+ expect(response).to render_template(:remove_fork)
+ end
end
end
it "does nothing if user is not signed in" do
- put(:remove_fork,
+ delete(:remove_fork,
namespace_id: project.namespace.to_param,
id: project.to_param, format: :js)
expect(response.status).to eq(401)
diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb
index df0dcb2bb21..f3d51641ece 100644
--- a/spec/features/projects_spec.rb
+++ b/spec/features/projects_spec.rb
@@ -45,13 +45,13 @@ feature 'Project', feature: true do
end
it 'should remove fork' do
- expect(page).to have_content 'Remove forked relationship'
+ expect(page).to have_content 'Remove fork relationship'
- remove_with_confirm('Remove forked relationship', project.path)
+ remove_with_confirm('Remove fork relationship', project.path)
expect(page).to have_content 'Fork relationship has been removed.'
expect(project.forked?).to be_falsey
- expect(page).not_to have_content 'Remove forked relationship'
+ expect(page).not_to have_content 'Remove fork relationship'
end
end
diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb
index 580bbec77d1..e9de9e0826d 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -606,28 +606,42 @@ describe API::API, api: true do
describe 'DELETE /projects/:id/fork' do
- it "shouldn't available for non admin users" do
+ it "shouldn't be visible to users outside group" do
delete api("/projects/#{project_fork_target.id}/fork", user)
- expect(response.status).to eq(403)
+ expect(response.status).to eq(404)
end
- it 'should make forked project unforked' do
- post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin)
- project_fork_target.reload
- expect(project_fork_target.forked_from_project).not_to be_nil
- expect(project_fork_target.forked?).to be_truthy
- delete api("/projects/#{project_fork_target.id}/fork", admin)
- expect(response.status).to eq(200)
- project_fork_target.reload
- expect(project_fork_target.forked_from_project).to be_nil
- expect(project_fork_target.forked?).not_to be_truthy
- end
+ context 'when users belong to project group' do
+ let(:project_fork_target) { create(:project, group: create(:group)) }
- it 'should be idempotent if not forked' do
- expect(project_fork_target.forked_from_project).to be_nil
- delete api("/projects/#{project_fork_target.id}/fork", admin)
- expect(response.status).to eq(200)
- expect(project_fork_target.reload.forked_from_project).to be_nil
+ before do
+ project_fork_target.group.add_owner user
+ project_fork_target.group.add_developer user2
+ end
+
+ it 'should be forbidden to non-owner users' do
+ delete api("/projects/#{project_fork_target.id}/fork", user2)
+ expect(response.status).to eq(403)
+ end
+
+ it 'should make forked project unforked' do
+ post api("/projects/#{project_fork_target.id}/fork/#{project_fork_source.id}", admin)
+ project_fork_target.reload
+ expect(project_fork_target.forked_from_project).not_to be_nil
+ expect(project_fork_target.forked?).to be_truthy
+ delete api("/projects/#{project_fork_target.id}/fork", admin)
+ expect(response.status).to eq(200)
+ project_fork_target.reload
+ expect(project_fork_target.forked_from_project).to be_nil
+ expect(project_fork_target.forked?).not_to be_truthy
+ end
+
+ it 'should be idempotent if not forked' do
+ expect(project_fork_target.forked_from_project).to be_nil
+ delete api("/projects/#{project_fork_target.id}/fork", admin)
+ expect(response.status).to eq(200)
+ expect(project_fork_target.reload.forked_from_project).to be_nil
+ end
end
end
end
--
GitLab