Commit 938e63fed9562f65f8c3233d87b085e88c7b3a2b
Committed by
Rafael Manzo
1 parent
2976a8f6
Exists in
colab
and in
4 other branches
Preventing users to directly call unauthorized actions
Regarding to ownership
Showing
3 changed files
with
25 additions
and
13 deletions
Show diff stats
app/controllers/projects_controller.rb
| 1 | 1 | class ProjectsController < ApplicationController |
| 2 | 2 | before_action :authenticate_user!, |
| 3 | 3 | except: [:index, :show] |
| 4 | + before_action :check_ownership, only: [:edit, :update, :destroy] | |
| 4 | 5 | |
| 5 | 6 | # GET /projects/new |
| 6 | 7 | def new |
| ... | ... | @@ -39,14 +40,7 @@ class ProjectsController < ApplicationController |
| 39 | 40 | # GET /projects/1/edit |
| 40 | 41 | # GET /projects/1/edit.json |
| 41 | 42 | def edit |
| 42 | - if current_user.project_ownerships.find_by_project_id(params[:id]).nil? | |
| 43 | - respond_to do |format| | |
| 44 | - format.html { redirect_to projects_url, notice: "You shall not edit projects that aren't yours." } | |
| 45 | - format.json { head :no_content } | |
| 46 | - end | |
| 47 | - else | |
| 48 | - set_project | |
| 49 | - end | |
| 43 | + set_project | |
| 50 | 44 | end |
| 51 | 45 | |
| 52 | 46 | def update |
| ... | ... | @@ -81,4 +75,13 @@ class ProjectsController < ApplicationController |
| 81 | 75 | params[:project] |
| 82 | 76 | end |
| 83 | 77 | |
| 78 | + def check_ownership | |
| 79 | + if current_user.project_ownerships.find_by_project_id(params[:id]).nil? | |
| 80 | + respond_to do |format| | |
| 81 | + format.html { redirect_to projects_url, notice: "You're not allowed to do this operation" } | |
| 82 | + format.json { head :no_content } | |
| 83 | + end | |
| 84 | + end | |
| 85 | + end | |
| 86 | + | |
| 84 | 87 | end | ... | ... |
features/project/edition.feature
| ... | ... | @@ -27,7 +27,7 @@ Feature: Project |
| 27 | 27 | And I have a sample project |
| 28 | 28 | And I am at the All Projects page |
| 29 | 29 | When I visit the sample project edit page |
| 30 | - Then I should see You shall not edit | |
| 30 | + Then I should see You're not allowed to do this operation | |
| 31 | 31 | |
| 32 | 32 | @kalibro_restart |
| 33 | 33 | Scenario: Filling up the form | ... | ... |
spec/controllers/projects_controller_spec.rb
| ... | ... | @@ -72,7 +72,7 @@ describe ProjectsController do |
| 72 | 72 | it { should render_template(:show) } |
| 73 | 73 | end |
| 74 | 74 | |
| 75 | - describe 'delete' do | |
| 75 | + describe 'destroy' do | |
| 76 | 76 | before :each do |
| 77 | 77 | sign_in FactoryGirl.create(:user) |
| 78 | 78 | |
| ... | ... | @@ -82,8 +82,12 @@ describe ProjectsController do |
| 82 | 82 | @ownership = FactoryGirl.build(:project_ownership) |
| 83 | 83 | @ownership.expects(:destroy) |
| 84 | 84 | @ownerships = [] |
| 85 | + | |
| 86 | + #Those two mocks looks the same but they are necessary since params[:id] is a String and @project.id is an Integer :( | |
| 87 | + @ownerships.expects(:find_by_project_id).with("#{@subject.id}").returns(@ownership) | |
| 85 | 88 | @ownerships.expects(:find_by_project_id).with(@subject.id).returns(@ownership) |
| 86 | - User.any_instance.expects(:project_ownerships).returns(@ownerships) | |
| 89 | + | |
| 90 | + User.any_instance.expects(:project_ownerships).at_least_once.returns(@ownerships) | |
| 87 | 91 | |
| 88 | 92 | Project.expects(:find).with(@subject.id.to_s).returns(@subject) |
| 89 | 93 | delete :destroy, :id => @subject.id |
| ... | ... | @@ -136,7 +140,7 @@ describe ProjectsController do |
| 136 | 140 | end |
| 137 | 141 | |
| 138 | 142 | it { should redirect_to(projects_path) } |
| 139 | - | |
| 143 | + | |
| 140 | 144 | it 'should set the flash' do |
| 141 | 145 | pending("This ShouldaMatcher test is not compatible yet with Rails 4") do |
| 142 | 146 | should set_the_flash[:notice].to("You shall not edit projects that aren't yours.") |
| ... | ... | @@ -147,7 +151,8 @@ describe ProjectsController do |
| 147 | 151 | |
| 148 | 152 | describe 'update' do |
| 149 | 153 | before do |
| 150 | - sign_in FactoryGirl.create(:user) | |
| 154 | + @user = FactoryGirl.create(:user) | |
| 155 | + sign_in @user | |
| 151 | 156 | end |
| 152 | 157 | |
| 153 | 158 | context 'with valid fields' do |
| ... | ... | @@ -155,6 +160,8 @@ describe ProjectsController do |
| 155 | 160 | @subject = FactoryGirl.build(:project) |
| 156 | 161 | @subject_params = Hash[FactoryGirl.attributes_for(:project).map { |k,v| [k.to_s, v.to_s] }] #FIXME: Mocha is creating the expectations with strings, but FactoryGirl returns everything with sybols and integers |
| 157 | 162 | |
| 163 | + FactoryGirl.create(:project_ownership, {user_id: @user.id, project_id: @subject.id}) | |
| 164 | + | |
| 158 | 165 | Project.expects(:find).with(@subject.id.to_s).returns(@subject) |
| 159 | 166 | Project.any_instance.expects(:update).with(@subject_params).returns(true) |
| 160 | 167 | end |
| ... | ... | @@ -185,6 +192,8 @@ describe ProjectsController do |
| 185 | 192 | @subject = FactoryGirl.build(:project) |
| 186 | 193 | @subject_params = Hash[FactoryGirl.attributes_for(:project).map { |k,v| [k.to_s, v.to_s] }] #FIXME: Mocha is creating the expectations with strings, but FactoryGirl returns everything with sybols and integers |
| 187 | 194 | |
| 195 | + FactoryGirl.create(:project_ownership, {user_id: @user.id, project_id: @subject.id}) | |
| 196 | + | |
| 188 | 197 | Project.expects(:find).with(@subject.id.to_s).returns(@subject) |
| 189 | 198 | Project.any_instance.expects(:update).with(@subject_params).returns(false) |
| 190 | 199 | ... | ... |