Commit 4868a1bc30a88e2d308b15c7f3df874527a08775
1 parent
88145f3a
Exists in
colab
and in
4 other branches
RepositoriesController ownership checking for new and create refactored
project_owner? is semantically more correct Signed off by: Guilherme Rojas V. de Lima <guilhermehrojas@gmail.com>
Showing
4 changed files
with
76 additions
and
4 deletions
Show diff stats
app/controllers/concerns/ownership_authentication.rb
| ... | ... | @@ -2,7 +2,15 @@ module OwnershipAuthentication |
| 2 | 2 | extend ActiveSupport::Concern |
| 3 | 3 | |
| 4 | 4 | def project_owner? |
| 5 | - check_project_ownership(params[:id]) | |
| 5 | + if self.kind_of?(ProjectsController) | |
| 6 | + id = params[:id] | |
| 7 | + elsif self.kind_of?(RepositoriesController) | |
| 8 | + id = params[:project_id] | |
| 9 | + else | |
| 10 | + raise "Not supported" | |
| 11 | + end | |
| 12 | + | |
| 13 | + check_project_ownership(id) | |
| 6 | 14 | end |
| 7 | 15 | |
| 8 | 16 | def repository_owner? |
| ... | ... | @@ -50,6 +58,8 @@ module OwnershipAuthentication |
| 50 | 58 | format.json { head :no_content } |
| 51 | 59 | end |
| 52 | 60 | end |
| 61 | + | |
| 62 | + return true | |
| 53 | 63 | end |
| 54 | 64 | |
| 55 | 65 | def check_reading_group_ownership(id) | ... | ... |
app/controllers/repositories_controller.rb
| ... | ... | @@ -2,7 +2,8 @@ include OwnershipAuthentication |
| 2 | 2 | |
| 3 | 3 | class RepositoriesController < ApplicationController |
| 4 | 4 | before_action :authenticate_user!, except: [:show, :state] |
| 5 | - before_action :repository_owner?, except: [:show, :state] | |
| 5 | + before_action :project_owner?, only: [:new, :create] | |
| 6 | + before_action :repository_owner?, only: [:edit, :update, :destroy, :process_repository] | |
| 6 | 7 | before_action :set_repository, only: [:show, :edit, :update, :destroy, :state, :process_repository] |
| 7 | 8 | |
| 8 | 9 | # GET /projects/1/repositories/1 | ... | ... |
spec/controllers/concerns/ownership_authentication_spec.rb
| ... | ... | @@ -79,4 +79,65 @@ describe OwnershipAuthentication, type: :controller do |
| 79 | 79 | end |
| 80 | 80 | end |
| 81 | 81 | end |
| 82 | + | |
| 83 | + describe 'project_owner?' do | |
| 84 | + let(:project) { FactoryGirl.build(:project) } | |
| 85 | + | |
| 86 | + context 'Not ProjectsController nor RepositoriesController' do | |
| 87 | + let!(:reading_group_controller) { ReadingGroupsController.new } | |
| 88 | + | |
| 89 | + before do | |
| 90 | + reading_group_controller.extend(OwnershipAuthentication) | |
| 91 | + end | |
| 92 | + | |
| 93 | + it 'should raise an exception' do | |
| 94 | + expect { reading_group_controller.project_owner? }.to raise_error("Not supported") | |
| 95 | + end | |
| 96 | + end | |
| 97 | + | |
| 98 | + context 'within RepositoriesController' do | |
| 99 | + let! (:repositories_controller) { RepositoriesController.new } | |
| 100 | + | |
| 101 | + before do | |
| 102 | + repositories_controller.params = {} | |
| 103 | + repositories_controller.params[:project_id] = project.id | |
| 104 | + end | |
| 105 | + | |
| 106 | + context 'with a user logged in' do | |
| 107 | + let! (:current_user) { FactoryGirl.create(:user) } | |
| 108 | + | |
| 109 | + before do | |
| 110 | + repositories_controller.expects(:current_user).returns(current_user) | |
| 111 | + end | |
| 112 | + | |
| 113 | + context 'when the user owns the Repository' do | |
| 114 | + let!(:project_ownership) { FactoryGirl.build(:project_ownership, {user_id: current_user.id, project_id: project.id}) } | |
| 115 | + | |
| 116 | + before do | |
| 117 | + project_ownerships = Object.new | |
| 118 | + project_ownerships.expects(:find_by_project_id).with(project.id).returns(project_ownership) | |
| 119 | + current_user.expects(:project_ownerships).returns(project_ownerships) | |
| 120 | + end | |
| 121 | + | |
| 122 | + it 'should return true' do | |
| 123 | + repositories_controller.project_owner?.should be_true | |
| 124 | + end | |
| 125 | + end | |
| 126 | + | |
| 127 | + context 'when the user does not own the Repository' do | |
| 128 | + before do | |
| 129 | + project_ownerships = Object.new | |
| 130 | + project_ownerships.expects(:find_by_project_id).with(project.id).returns(nil) | |
| 131 | + current_user.expects(:project_ownerships).returns(project_ownerships) | |
| 132 | + end | |
| 133 | + | |
| 134 | + it 'should respond' do # FIXME: this is not the best test, but it it's the closest we can do I think | |
| 135 | + repositories_controller.expects(:respond_to) | |
| 136 | + | |
| 137 | + repositories_controller.project_owner? | |
| 138 | + end | |
| 139 | + end | |
| 140 | + end | |
| 141 | + end | |
| 142 | + end | |
| 82 | 143 | end | ... | ... |
spec/controllers/repositories_controller_spec.rb
| ... | ... | @@ -11,7 +11,7 @@ describe RepositoriesController do |
| 11 | 11 | context 'when the current user owns the project' do |
| 12 | 12 | before :each do |
| 13 | 13 | Repository.expects(:repository_types).returns([]) |
| 14 | - subject.expects(:repository_owner?).returns true | |
| 14 | + subject.expects(:project_owner?).returns true | |
| 15 | 15 | |
| 16 | 16 | get :new, project_id: project.id.to_s |
| 17 | 17 | end |
| ... | ... | @@ -40,7 +40,7 @@ describe RepositoriesController do |
| 40 | 40 | |
| 41 | 41 | context 'when the current user owns the project' do |
| 42 | 42 | before :each do |
| 43 | - subject.expects(:repository_owner?).returns true | |
| 43 | + subject.expects(:project_owner?).returns true | |
| 44 | 44 | end |
| 45 | 45 | |
| 46 | 46 | context 'with valid fields' do | ... | ... |