From 9907501aaaecce7dde44e6bf0143719ed84dfe90 Mon Sep 17 00:00:00 2001 From: Diego-Thomaz Date: Wed, 6 Mar 2019 12:08:24 -0300 Subject: [PATCH 1/3] fix association between project and users --- app/controllers/projects_controller.rb | 3 ++- spec/requests/projects_request_spec.rb | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 041f151..432dde7 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -19,6 +19,7 @@ def new def create @project = Project.new(project_params) + @project.users << current_user if @project.save flash[:notice] = 'Project created' @@ -50,6 +51,6 @@ def load_project end def project_params - params.require(:project).permit(:name, :description) + params.require(:project).permit(:name, :description, :user_id) end end diff --git a/spec/requests/projects_request_spec.rb b/spec/requests/projects_request_spec.rb index 7add589..3443b1b 100644 --- a/spec/requests/projects_request_spec.rb +++ b/spec/requests/projects_request_spec.rb @@ -41,6 +41,7 @@ expect(response).to have_http_status :found expect(response).to redirect_to(project_path(Project.last)) + expect(Project.last.users.count).to eq 1 end end From a293bb894945cd66a22d99e93fe7ad0ef462a852 Mon Sep 17 00:00:00 2001 From: Diego-Thomaz Date: Wed, 6 Mar 2019 12:26:24 -0300 Subject: [PATCH 2/3] remove useless user_id param from white list --- app/controllers/projects_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 432dde7..f6b7efb 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -51,6 +51,6 @@ def load_project end def project_params - params.require(:project).permit(:name, :description, :user_id) + params.require(:project).permit(:name, :description) end end From f005fc9ffea302114e31707789cf418277bbe5ad Mon Sep 17 00:00:00 2001 From: Diego-Thomaz Date: Wed, 20 Mar 2019 17:13:21 -0300 Subject: [PATCH 3/3] index shows only user's projects and restrict access to other projects he is not associated with --- Gemfile | 1 + Gemfile.lock | 3 ++ app/controllers/application_controller.rb | 1 + app/controllers/projects_controller.rb | 10 +++- app/policies/application_policy.rb | 49 +++++++++++++++++ app/policies/project_policy.rb | 7 +++ spec/features/project_management_spec.rb | 7 +-- spec/features/target_management_spec.rb | 5 +- spec/features/task_management_spec.rb | 2 +- spec/requests/projects_request_spec.rb | 65 +++++++++++++++-------- 10 files changed, 121 insertions(+), 29 deletions(-) create mode 100644 app/policies/application_policy.rb create mode 100644 app/policies/project_policy.rb diff --git a/Gemfile b/Gemfile index 460a9de..025cc9f 100644 --- a/Gemfile +++ b/Gemfile @@ -14,6 +14,7 @@ gem 'font-awesome-rails' gem 'jquery-rails' gem 'pg', '>= 0.18', '< 2.0' gem 'puma', '~> 3.11' +gem 'pundit' gem 'rails', '~> 5.2.0' gem 'react-rails' gem 'sass-rails' diff --git a/Gemfile.lock b/Gemfile.lock index 300f162..79007af 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -186,6 +186,8 @@ GEM pry (>= 0.10.4) public_suffix (3.0.2) puma (3.11.4) + pundit (2.0.1) + activesupport (>= 3.0.0) rack (2.0.5) rack-proxy (0.6.4) rack @@ -337,6 +339,7 @@ DEPENDENCIES pg (>= 0.18, < 2.0) pry-rails puma (~> 3.11) + pundit rails (~> 5.2.0) rails-controller-testing react-rails diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 16045e9..abddad3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,4 +1,5 @@ class ApplicationController < ActionController::Base + include Pundit before_action :authenticate_user! before_action :configure_permitted_parameters, if: :devise_controller? diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index f6b7efb..e69a7a6 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -4,7 +4,7 @@ class ProjectsController < ApplicationController before_action :load_project, only: %i[show edit update] def index - @projects = Project.all + @projects = policy_scope(Project) end def show @@ -45,7 +45,13 @@ def update private def load_project - @project = Project.find(params[:id]) + begin + @project = policy_scope(Project).find(params[:id]) + rescue ActiveRecord::RecordNotFound + flash[:error] = 'You\'re not allowed to access this project' + return redirect_to projects_path + end + add_breadcrumb 'Projects', :projects_path add_breadcrumb @project.name, project_path(@project) end diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb new file mode 100644 index 0000000..eefe976 --- /dev/null +++ b/app/policies/application_policy.rb @@ -0,0 +1,49 @@ +class ApplicationPolicy + attr_reader :user, :record + + def initialize(user, record) + @user = user + @record = record + end + + def index? + false + end + + def show? + false + end + + def create? + false + end + + def new? + create? + end + + def update? + false + end + + def edit? + update? + end + + def destroy? + false + end + + class Scope + attr_reader :user, :scope + + def initialize(user, scope) + @user = user + @scope = scope + end + + def resolve + scope.all + end + end +end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb new file mode 100644 index 0000000..abc6d76 --- /dev/null +++ b/app/policies/project_policy.rb @@ -0,0 +1,7 @@ +class ProjectPolicy < ApplicationPolicy + class Scope < Scope + def resolve + scope.joins(:users).where('user_id = ?', user.id) + end + end +end diff --git a/spec/features/project_management_spec.rb b/spec/features/project_management_spec.rb index 9c0c239..dd02cc1 100644 --- a/spec/features/project_management_spec.rb +++ b/spec/features/project_management_spec.rb @@ -1,13 +1,14 @@ require 'rails_helper' feature 'Project management' do - let!(:project) { create(:project) } + let(:user) { create(:user) } + let!(:project) { create(:project, users: [user]) } before do - sign_in(create(:user)) + sign_in(user) end - it 'User can view all projects and project details' do + it 'User can view all projects and project details that he belongs to' do visit root_path expect(page).to have_content 'Projects' diff --git a/spec/features/target_management_spec.rb b/spec/features/target_management_spec.rb index 4a16124..a3fd5f1 100644 --- a/spec/features/target_management_spec.rb +++ b/spec/features/target_management_spec.rb @@ -2,13 +2,14 @@ require 'rails_helper' feature 'Target management', js: true do + let(:user) { create(:user) } let(:target_one_name) { 'TEST TARGET ONE' } let(:target_two_name) { 'TEST TARGET TWO' } - let(:project) { create(:project) } + let(:project) { create(:project, users: [user]) } let!(:terrestrial_target_type) { TargetType.create(name: 'Terrestrial Ecosystem') } before do - sign_in(create(:user)) + sign_in(user) end it 'User can view all targets for a Project' do diff --git a/spec/features/task_management_spec.rb b/spec/features/task_management_spec.rb index d082113..75cf929 100644 --- a/spec/features/task_management_spec.rb +++ b/spec/features/task_management_spec.rb @@ -6,8 +6,8 @@ let(:task_three_name) { 'TEST TASK 3' } let(:user_email) { 'blah@blah.com' } let(:project_name) { 'TEST PROJECT' } - let(:project) { build(:project, name: project_name) } let(:user) { create(:user, email: user_email) } + let(:project) { build(:project, name: project_name, users: [user]) } let!(:task_one) { create(:task, name: task_one_name, project: project) } let!(:task_two) { create(:task, name: task_two_name, project: project, user: user) } diff --git a/spec/requests/projects_request_spec.rb b/spec/requests/projects_request_spec.rb index 3443b1b..f02ac05 100644 --- a/spec/requests/projects_request_spec.rb +++ b/spec/requests/projects_request_spec.rb @@ -1,13 +1,15 @@ require 'rails_helper' describe 'Project Requests', type: :request do - let(:project) { create(:project) } - let(:new_project) { build(:project) } + let(:user) { create(:user) } + let(:new_user) { create(:user) } + let(:project) { create(:project, users: [user]) } + let(:new_project) { create(:project, users:[new_user]) } let!(:task) { create(:task, project: project) } let!(:archived_task) { create(:task, :archived, project: project) } before do - sign_in(create(:user)) + sign_in(user) end describe 'GET index' do @@ -17,20 +19,30 @@ expect(response).to have_http_status 200 expect(response.body).to include('Projects') expect(response.body).to include(project.name) + expect(response.body).to_not include(new_project.name) end end describe 'GET SHOW' do - before do - get "/projects/#{project.id}" - end + context 'when a project is associated to user' do + before do + get "/projects/#{project.id}" + end - it 'excludes archived tasks' do - expect(response.body).to_not include(archived_task.name) + it 'excludes archived tasks' do + expect(response.body).to_not include(archived_task.name) + end + + it 'includes non-archived tasks' do + expect(response.body).to include(task.name) + end end + context 'when a project is not associated to the user' do + subject { get project_path(new_project.id) } - it 'includes non-archived tasks' do - expect(response.body).to include(task.name) + it 'should be redirect_to index' do + expect(subject).to redirect_to(projects_path) + end end end @@ -56,21 +68,32 @@ end describe 'PUT update' do - describe 'success' do - it 'updates the project' do - put "/projects/#{project.id}", params: { project: { name: 'Bender saves the ocean' } } - follow_redirect! + context 'when editing a project that users is associated with' do + describe 'success' do + it 'updates the project' do + put "/projects/#{project.id}", params: { project: { name: 'Bender saves the ocean' } } + follow_redirect! - expect(response.body).to include('Bender saves the ocean') + expect(response.body).to include('Bender saves the ocean') + end end - end - describe 'failure' do - it 'shows errors' do - put "/projects/#{project.id}", params: { project: { name: '' } } + describe 'failure' do + it 'shows errors' do + put "/projects/#{project.id}", params: { project: { name: '' } } - expect(response).to have_http_status 422 - expect(response.body).to include('Name can't be blank') + expect(response).to have_http_status 422 + expect(response.body).to include('Name can't be blank') + end + end + end + context 'if somehow the user try to edit a project that not he\'s not associated with' do + it 'then it should be redirected to index page, and data must\'t be updated' do + put "/projects/#{new_project.id}", params: { project: { name: 'Bender saves the ocean' } } + expect(response).to redirect_to(projects_path) + + follow_redirect! + expect(response.body).to_not include('Bender saves the ocean') end end end