diff --git a/CHANGELOG b/CHANGELOG index 605b707..864222d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,10 @@ += 5.0.2 + * read :resource_path aliases only from request.path_parameters, preventing query and body parameters + from removing enclosing resources and bypassing the current user's scoped association. Route-defined + aliases, request.path fallback and memoization are preserved + * add regression coverage for query/body overrides, scoped resource lookup and deletion, route-defined + aliases and request path memoization + = 5.0.1 * renamed the internal RequestPathIntrospection#request_path to #rc_request_path: a route like `resources :requests` generates a named route helper called request_path, which is included into diff --git a/lib/resources_controller/request_path_introspection.rb b/lib/resources_controller/request_path_introspection.rb index 359bb7c..cc4842b 100644 --- a/lib/resources_controller/request_path_introspection.rb +++ b/lib/resources_controller/request_path_introspection.rb @@ -13,7 +13,7 @@ module RequestPathIntrospection # name lookup - resources_controller would then read the helper's value instead of # the path of the current request. See spec/lib/request_path_introspection_spec.rb. def rc_request_path - @rc_request_path ||= params[:resource_path] || request.path + @rc_request_path ||= request.path_parameters[:resource_path] || request.path end def nesting_request_path diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index 684c43c..e40da5c 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/spec/controllers/accounts_controller_spec.rb @@ -27,9 +27,15 @@ def setup_mocks end end - describe AccountsController, " requesting garbage url" do - it "should raise ResourcesController::Specification::NoClassFoundError" do - expect { get :show, params: { :resource_path => "/crayzeee" }}.to raise_error(ResourcesController::Specification::NoClassFoundError) + describe AccountsController, " requesting a garbage resource_path parameter" do + include AccountsControllerSpecHelper + + it "should ignore the parameter and find the current user" do + setup_mocks + get :show, params: { :resource_path => "/crayzeee" } + + expect(response).to have_http_status(:ok) + expect(assigns[:account]).to eq(@current_user) end end diff --git a/spec/controllers/forums_controller_spec.rb b/spec/controllers/forums_controller_spec.rb index 8fc4138..dbd9b43 100644 --- a/spec/controllers/forums_controller_spec.rb +++ b/spec/controllers/forums_controller_spec.rb @@ -150,9 +150,12 @@ #end end - describe ForumsController, " requesting garbage url" do - it "should raise ResourcesController::Specification::NoClassFoundError" do - expect { get :index, params: { :resource_path => "/forums/\ncrayzeee" } }.to raise_error(ResourcesController::Specification::NoClassFoundError) + describe ForumsController, " requesting a garbage resource_path parameter" do + it "should ignore the parameter and find forums" do + get :index, params: { :resource_path => "/forums/\ncrayzeee" } + + expect(response).to have_http_status(:ok) + expect(assigns[:forums]).to eq(Forum.all) end end diff --git a/spec/lib/request_path_introspection_spec.rb b/spec/lib/request_path_introspection_spec.rb index 5222e70..5cabb1c 100644 --- a/spec/lib/request_path_introspection_spec.rb +++ b/spec/lib/request_path_introspection_spec.rb @@ -11,8 +11,9 @@ class User < ActiveRecord::Base; end @controller = @klass.new allow(@controller).to receive(:controller_name).and_return('forums') allow(@controller).to receive(:controller_path).and_return('forums') - allow(@controller).to receive(:params).and_return({}) - allow(@controller).to receive(:request).and_return(double('request', :path => '/forums')) + @request = ActionDispatch::TestRequest.create + @request.path = '/forums' + allow(@controller).to receive(:request).and_return(@request) end describe "#rc_request_path" do @@ -20,9 +21,58 @@ class User < ActiveRecord::Base; end expect(@controller.send(:rc_request_path)).to eq('/forums') end - it " should be params[:resource_path], when set" do - @controller.params[:resource_path] = '/foo' - expect(@controller.send(:rc_request_path)).to eq('/foo') + it "should honor a route-defined resource_path alias" do + @request.path = '/' + @request.path_parameters = Rails.application.routes.recognize_path('/', :method => :get) + + expect(@controller.send(:rc_request_path)).to eq('/forums') + end + + it "should honor a route-defined alias over query parameters" do + @request = ActionDispatch::Request.new(Rack::MockRequest.env_for( + '/create_forum?resource_path=%2Ftags' + )) + @request.path_parameters = Rails.application.routes.recognize_path('/create_forum', :method => :get) + allow(@controller).to receive(:request).and_return(@request) + + expect(@controller.send(:rc_request_path)).to eq('/forums') + end + + it "should ignore query parameters that remove the enclosing resource" do + @request = ActionDispatch::Request.new(Rack::MockRequest.env_for( + '/account/forums/123.json?resource_path=%2Fforums' + )) + allow(@controller).to receive(:request).and_return(@request) + + expect(@controller.params[:resource_path]).to eq('/forums') + expect(@controller.send(:rc_request_path)).to eq('/account/forums/123.json') + expect(@controller.send(:nesting_request_path)).to eq('/account') + end + + it "should ignore body parameters that remove the enclosing resource" do + @request = ActionDispatch::Request.new(Rack::MockRequest.env_for( + '/account/forums/123.json', :method => 'POST', :input => 'resource_path=%2Fforums', + 'CONTENT_TYPE' => 'application/x-www-form-urlencoded' + )) + allow(@controller).to receive(:request).and_return(@request) + + expect(@controller.params[:resource_path]).to eq('/forums') + expect(@controller.send(:rc_request_path)).to eq('/account/forums/123.json') + expect(@controller.send(:nesting_request_path)).to eq('/account') + end + + it "should memoize the request path fallback" do + expect(@controller.send(:rc_request_path)).to eq('/forums') + @request.path = '/tags' + expect(@controller.send(:rc_request_path)).to eq('/forums') + end + + it "should memoize the route-defined alias" do + @request.path_parameters[:resource_path] = '/forums' + expect(@controller.send(:rc_request_path)).to eq('/forums') + @request.path_parameters[:resource_path] = '/tags' + @controller.params[:resource_path] = '/tags' + expect(@controller.send(:rc_request_path)).to eq('/forums') end end diff --git a/spec/requests/request_path_introspection_spec.rb b/spec/requests/request_path_introspection_spec.rb new file mode 100644 index 0000000..ea24008 --- /dev/null +++ b/spec/requests/request_path_introspection_spec.rb @@ -0,0 +1,42 @@ +require 'spec_helper' + +describe "Resource path scoping", :type => :request do + before do + @info = Info.create! + @account = User.create!(:info => @info) + @tag = @info.tags.create! + other_info = Info.create! + User.create!(:info => other_info) + @other_tag = other_info.tags.create! + allow_any_instance_of(ApplicationController).to receive(:current_user).and_return(@account) + end + + it "should reject a query override that looks up another user's resource" do + get "/account/info/tags/#{@other_tag.id}", :params => { :resource_path => '/tags' } + + expect(response).to have_http_status(:not_found) + end + + it "should find the current user's resource despite a query override" do + get "/account/info/tags/#{@tag.id}", :params => { :resource_path => '/tags' } + + expect(response).to have_http_status(:ok) + expect(assigns[:tag]).to eq(@tag) + expect(assigns[:account]).to eq(@account) + end + + it "should reject a body override that deletes another user's resource" do + delete "/account/info/tags/#{@other_tag.id}", :params => { :resource_path => '/tags' } + + expect(response).to have_http_status(:not_found) + expect(Tag.exists?(@other_tag.id)).to be true + end + + it "should delete the current user's resource despite a body override" do + delete "/account/info/tags/#{@tag.id}", :params => { :resource_path => '/tags' } + + expect(response).to redirect_to('/account/info/tags') + expect(Tag.exists?(@tag.id)).to be false + expect(Tag.exists?(@other_tag.id)).to be true + end +end