From a8b1ad250e1ebc1c1e835399ccd010b223108a1d Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 8 Aug 2016 19:03:41 -0300 Subject: [PATCH] Add authorization to issues board related controllers --- .../projects/board_issues_controller.rb | 11 +++ .../projects/board_lists_controller.rb | 6 ++ app/controllers/projects/boards_controller.rb | 13 +++ app/models/ability.rb | 2 + app/services/boards/issues/move_service.rb | 1 - .../projects/board_issues_controller_spec.rb | 46 ++++++++-- .../projects/board_lists_controller_spec.rb | 87 ++++++++++++++----- .../projects/boards_controller_spec.rb | 36 +++++++- 8 files changed, 166 insertions(+), 36 deletions(-) diff --git a/app/controllers/projects/board_issues_controller.rb b/app/controllers/projects/board_issues_controller.rb index fdc3a8795ef..89d0be54855 100644 --- a/app/controllers/projects/board_issues_controller.rb +++ b/app/controllers/projects/board_issues_controller.rb @@ -1,6 +1,9 @@ class Projects::BoardIssuesController < Projects::ApplicationController respond_to :json + before_action :authorize_read_issue!, only: [:index] + before_action :authorize_update_issue!, only: [:update] + rescue_from ActiveRecord::RecordNotFound, with: :record_not_found def index @@ -27,6 +30,14 @@ class Projects::BoardIssuesController < Projects::ApplicationController private + def authorize_read_issue! + return render_403 unless can?(current_user, :read_issue, project) + end + + def authorize_update_issue! + return render_403 unless can?(current_user, :update_issue, project) + end + def filter_params params.merge(id: params[:list_id]) end diff --git a/app/controllers/projects/board_lists_controller.rb b/app/controllers/projects/board_lists_controller.rb index 63daba09e6a..75b80c55b21 100644 --- a/app/controllers/projects/board_lists_controller.rb +++ b/app/controllers/projects/board_lists_controller.rb @@ -1,6 +1,8 @@ class Projects::BoardListsController < Projects::ApplicationController respond_to :json + before_action :authorize_admin_list! + rescue_from ActiveRecord::RecordNotFound, with: :record_not_found def create @@ -45,6 +47,10 @@ class Projects::BoardListsController < Projects::ApplicationController private + def authorize_admin_list! + return render_403 unless can?(current_user, :admin_list, project) + end + def list_params params.require(:list).permit(:label_id) end diff --git a/app/controllers/projects/boards_controller.rb b/app/controllers/projects/boards_controller.rb index 50311acec32..301c718ad57 100644 --- a/app/controllers/projects/boards_controller.rb +++ b/app/controllers/projects/boards_controller.rb @@ -1,4 +1,6 @@ class Projects::BoardsController < Projects::ApplicationController + before_action :authorize_read_board!, only: [:show] + def show board = Boards::CreateService.new(project, current_user).execute @@ -7,4 +9,15 @@ class Projects::BoardsController < Projects::ApplicationController format.json { render json: board.lists.as_json(only: [:id, :list_type, :position], methods: [:title], include: { label: { only: [:id, :title, :color] } }) } end end + + private + + def authorize_read_board! + unless can?(current_user, :read_board, project) + respond_to do |format| + format.html { return access_denied! } + format.json { return render_403 } + end + end + end end diff --git a/app/models/ability.rb b/app/models/ability.rb index d9113ffd99a..b70451db12f 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -228,6 +228,7 @@ class Ability :read_project, :read_wiki, :read_issue, + :read_board, :read_label, :read_milestone, :read_project_snippet, @@ -249,6 +250,7 @@ class Ability :update_issue, :admin_issue, :admin_label, + :admin_list, :read_commit_status, :read_build, :read_container_image, diff --git a/app/services/boards/issues/move_service.rb b/app/services/boards/issues/move_service.rb index 398a02ebbfd..381bd95c837 100644 --- a/app/services/boards/issues/move_service.rb +++ b/app/services/boards/issues/move_service.rb @@ -4,7 +4,6 @@ module Boards def execute return false unless issue.present? return false unless valid_move? - return false unless user.can?(:update_issue, issue) update_service.execute(issue) reopen_service.execute(issue) if moving_from.done? diff --git a/spec/controllers/projects/board_issues_controller_spec.rb b/spec/controllers/projects/board_issues_controller_spec.rb index 44a20411abc..6ec3a1b8cfc 100644 --- a/spec/controllers/projects/board_issues_controller_spec.rb +++ b/spec/controllers/projects/board_issues_controller_spec.rb @@ -12,7 +12,6 @@ describe Projects::BoardIssuesController do before do project.team << [user, :master] - sign_in(user) end describe 'GET #index' do @@ -23,7 +22,7 @@ describe Projects::BoardIssuesController do create(:labeled_issue, project: project, labels: [development]) create(:labeled_issue, project: project, labels: [development], assignee: johndoe) - list_issues list_id: list2 + list_issues user: user, list_id: list2 parsed_response = JSON.parse(response.body) @@ -34,13 +33,26 @@ describe Projects::BoardIssuesController do context 'with invalid list id' do it 'returns a not found 404 response' do - list_issues list_id: 999 + list_issues user: user, list_id: 999 expect(response).to have_http_status(404) end end - def list_issues(list_id:) + context 'with unauthorized user' do + it 'returns a successful 403 response' do + allow(Ability.abilities).to receive(:allowed?).with(user, :read_project, project).and_return(true) + allow(Ability.abilities).to receive(:allowed?).with(user, :read_issue, project).and_return(false) + + list_issues user: user, list_id: list2 + + expect(response).to have_http_status(403) + end + end + + def list_issues(user:, list_id:) + sign_in(user) + get :index, namespace_id: project.namespace.to_param, project_id: project.to_param, list_id: list_id.to_param @@ -52,13 +64,13 @@ describe Projects::BoardIssuesController do context 'with valid params' do it 'returns a successful 200 response' do - move issue: issue, from: list1.id, to: list2.id + move user: user, issue: issue, from: list1.id, to: list2.id expect(response).to have_http_status(200) end it 'moves issue to the desired list' do - move issue: issue, from: list1.id, to: list2.id + move user: user, issue: issue, from: list1.id, to: list2.id expect(issue.reload.labels).to contain_exactly(development) end @@ -66,19 +78,35 @@ describe Projects::BoardIssuesController do context 'with invalid params' do it 'returns a unprocessable entity 422 response for invalid lists' do - move issue: issue, from: nil, to: nil + move user: user, issue: issue, from: nil, to: nil expect(response).to have_http_status(422) end it 'returns a not found 404 response for invalid issue id' do - move issue: 999, from: list1.id, to: list2.id + move user: user, issue: 999, from: list1.id, to: list2.id expect(response).to have_http_status(404) end end - def move(issue:, from:, to:) + context 'with unauthorized user' do + let(:guest) { create(:user) } + + before do + project.team << [guest, :guest] + end + + it 'returns a successful 403 response' do + move user: guest, issue: issue, from: list1.id, to: list2.id + + expect(response).to have_http_status(403) + end + end + + def move(user:, issue:, from:, to:) + sign_in(user) + patch :update, namespace_id: project.namespace.to_param, project_id: project.to_param, id: issue.to_param, diff --git a/spec/controllers/projects/board_lists_controller_spec.rb b/spec/controllers/projects/board_lists_controller_spec.rb index c37ced574a9..4b824164f69 100644 --- a/spec/controllers/projects/board_lists_controller_spec.rb +++ b/spec/controllers/projects/board_lists_controller_spec.rb @@ -4,24 +4,25 @@ describe Projects::BoardListsController do let(:project) { create(:project_with_board) } let(:board) { project.board } let(:user) { create(:user) } + let(:guest) { create(:user) } before do project.team << [user, :master] - sign_in(user) + project.team << [guest, :guest] end describe 'POST #create' do - context 'with valid params' do - let(:label) { create(:label, project: project, name: 'Development') } + let(:label) { create(:label, project: project, name: 'Development') } + context 'with valid params' do it 'returns a successful 200 response' do - create_board_list label_id: label.id + create_board_list user: user, label_id: label.id expect(response).to have_http_status(200) end it 'returns the created list' do - create_board_list label_id: label.id + create_board_list user: user, label_id: label.id expect(response).to match_response_schema('list') end @@ -29,7 +30,7 @@ describe Projects::BoardListsController do context 'with invalid params' do it 'returns an error' do - create_board_list label_id: nil + create_board_list user: user, label_id: nil parsed_response = JSON.parse(response.body) @@ -38,7 +39,19 @@ describe Projects::BoardListsController do end end - def create_board_list(label_id:) + context 'with unauthorized user' do + let(:label) { create(:label, project: project, name: 'Development') } + + it 'returns a successful 403 response' do + create_board_list user: guest, label_id: label.id + + expect(response).to have_http_status(403) + end + end + + def create_board_list(user:, label_id:) + sign_in(user) + post :create, namespace_id: project.namespace.to_param, project_id: project.to_param, list: { label_id: label_id }, @@ -52,13 +65,13 @@ describe Projects::BoardListsController do context 'with valid position' do it 'returns a successful 200 response' do - move list: planning, position: 1 + move user: user, list: planning, position: 1 expect(response).to have_http_status(200) end it 'moves the list to the desired position' do - move list: planning, position: 1 + move user: user, list: planning, position: 1 expect(planning.reload.position).to eq 1 end @@ -66,7 +79,7 @@ describe Projects::BoardListsController do context 'with invalid position' do it 'returns a unprocessable entity 422 response' do - move list: planning, position: 6 + move user: user, list: planning, position: 6 expect(response).to have_http_status(422) end @@ -74,13 +87,23 @@ describe Projects::BoardListsController do context 'with invalid list id' do it 'returns a not found 404 response' do - move list: 999, position: 1 + move user: user, list: 999, position: 1 expect(response).to have_http_status(404) end end - def move(list:, position:) + context 'with unauthorized user' do + it 'returns a successful 403 response' do + move user: guest, list: planning, position: 6 + + expect(response).to have_http_status(403) + end + end + + def move(user:, list:, position:) + sign_in(user) + patch :update, namespace_id: project.namespace.to_param, project_id: project.to_param, id: list.to_param, @@ -90,29 +113,39 @@ describe Projects::BoardListsController do end describe 'DELETE #destroy' do - context 'with valid list id' do - let!(:planning) { create(:list, board: board, position: 0) } + let!(:planning) { create(:list, board: board, position: 0) } + context 'with valid list id' do it 'returns a successful 200 response' do - remove_board_list list: planning + remove_board_list user: user, list: planning expect(response).to have_http_status(200) end it 'removes list from board' do - expect { remove_board_list list: planning }.to change(board.lists, :size).by(-1) + expect { remove_board_list user: user, list: planning }.to change(board.lists, :size).by(-1) end end context 'with invalid list id' do it 'returns a not found 404 response' do - remove_board_list list: 999 + remove_board_list user: user, list: 999 expect(response).to have_http_status(404) end end - def remove_board_list(list:) + context 'with unauthorized user' do + it 'returns a successful 403 response' do + remove_board_list user: guest, list: planning + + expect(response).to have_http_status(403) + end + end + + def remove_board_list(user:, list:) + sign_in(user) + delete :destroy, namespace_id: project.namespace.to_param, project_id: project.to_param, id: list.to_param, @@ -123,13 +156,13 @@ describe Projects::BoardListsController do describe 'POST #generate' do context 'when board lists is empty' do it 'returns a successful 200 response' do - generate_default_board_lists + generate_default_board_lists user: user expect(response).to have_http_status(200) end it 'returns the defaults lists' do - generate_default_board_lists + generate_default_board_lists user: user expect(response).to match_response_schema('list', array: true) end @@ -139,13 +172,23 @@ describe Projects::BoardListsController do it 'returns a unprocessable entity 422 response' do create(:list, board: board) - generate_default_board_lists + generate_default_board_lists user: user expect(response).to have_http_status(422) end end - def generate_default_board_lists + context 'with unauthorized user' do + it 'returns a successful 403 response' do + generate_default_board_lists user: guest + + expect(response).to have_http_status(403) + end + end + + def generate_default_board_lists(user:) + sign_in(user) + post :generate, namespace_id: project.namespace.to_param, project_id: project.to_param, format: :json diff --git a/spec/controllers/projects/boards_controller_spec.rb b/spec/controllers/projects/boards_controller_spec.rb index 2392ee18602..7ef4b786b42 100644 --- a/spec/controllers/projects/boards_controller_spec.rb +++ b/spec/controllers/projects/boards_controller_spec.rb @@ -12,22 +12,33 @@ describe Projects::BoardsController do describe 'GET #show' do context 'when project does not have a board' do it 'creates a new board' do - expect { get :show, namespace_id: project.namespace.to_param, project_id: project.to_param }.to change(Board, :count).by(1) + expect { read_board }.to change(Board, :count).by(1) end end context 'when format is HTML' do it 'renders HTML template' do - get :show, namespace_id: project.namespace.to_param, project_id: project.to_param + read_board expect(response).to render_template :show expect(response.content_type).to eq 'text/html' end + + context 'with unauthorized user' do + it 'returns a successful 404 response' do + allow(Ability.abilities).to receive(:allowed?).with(user, :read_project, project).and_return(true) + allow(Ability.abilities).to receive(:allowed?).with(user, :read_board, project).and_return(false) + + read_board + + expect(response).to have_http_status(404) + end + end end context 'when format is JSON' do it 'returns a successful 200 response' do - get :show, namespace_id: project.namespace.to_param, project_id: project.to_param, format: :json + read_board format: :json expect(response).to have_http_status(200) expect(response.content_type).to eq 'application/json' @@ -39,13 +50,30 @@ describe Projects::BoardsController do create(:list, board: board) create(:done_list, board: board) - get :show, namespace_id: project.namespace.to_param, project_id: project.to_param, format: :json + read_board format: :json parsed_response = JSON.parse(response.body) expect(response).to match_response_schema('list', array: true) expect(parsed_response.length).to eq 3 end + + context 'with unauthorized user' do + it 'returns a successful 403 response' do + allow(Ability.abilities).to receive(:allowed?).with(user, :read_project, project).and_return(true) + allow(Ability.abilities).to receive(:allowed?).with(user, :read_board, project).and_return(false) + + read_board format: :json + + expect(response).to have_http_status(403) + end + end + end + + def read_board(format: :html) + get :show, namespace_id: project.namespace.to_param, + project_id: project.to_param, + format: format end end end