From 8a47fe350525fba65bc390d02a6d84a7769e3341 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Oct 2020 01:39:21 +0100 Subject: [PATCH 1/3] Avoid a brakeman security warning Although it wasn't a real security concern because we were only calling a `find` method based on the user input, it's a good practice to avoid using constants based on user parameters. Since we don't use the `find` method anymore but we still need to check the associated record exists, we're changing the `followable` validation in the `Follow` model to do exactly that. --- app/controllers/follows_controller.rb | 6 +++--- app/models/follow.rb | 3 +-- spec/models/follow_spec.rb | 12 ++++++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/app/controllers/follows_controller.rb b/app/controllers/follows_controller.rb index 325b38fb3..1d5d3d331 100644 --- a/app/controllers/follows_controller.rb +++ b/app/controllers/follows_controller.rb @@ -3,7 +3,7 @@ class FollowsController < ApplicationController load_and_authorize_resource def create - @follow = Follow.create!(user: current_user, followable: find_followable) + @follow = current_user.follows.create!(follow_params) flash.now[:notice] = t("shared.followable.#{followable_translation_key(@follow.followable)}.create.notice") render :refresh_follow_button end @@ -17,8 +17,8 @@ class FollowsController < ApplicationController private - def find_followable - params[:followable_type].constantize.find(params[:followable_id]) + def follow_params + params.permit(:followable_type, :followable_id) end def followable_translation_key(followable) diff --git a/app/models/follow.rb b/app/models/follow.rb index 8062ffecc..f67adcefd 100644 --- a/app/models/follow.rb +++ b/app/models/follow.rb @@ -3,6 +3,5 @@ class Follow < ApplicationRecord belongs_to :followable, polymorphic: true validates :user_id, presence: true - validates :followable_id, presence: true - validates :followable_type, presence: true + validates :followable, presence: true end diff --git a/spec/models/follow_spec.rb b/spec/models/follow_spec.rb index 60dbfe288..feaa2e8cd 100644 --- a/spec/models/follow_spec.rb +++ b/spec/models/follow_spec.rb @@ -21,4 +21,16 @@ describe Follow do follow.followable_type = nil expect(follow).not_to be_valid end + + it "is not valid with an invalid followable_type" do + follow.followable_type = "NotARealModel" + + expect { follow.valid? }.to raise_exception "uninitialized constant NotARealModel" + end + + it "is not valid with the ID of a non-existent record" do + follow.followable_id = Proposal.last.id + 1 + + expect(follow).not_to be_valid + end end From d7ad1a769f04db818795b3ae516120118780fa8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Oct 2020 01:39:36 +0100 Subject: [PATCH 2/3] Make sure users can only delete their own follows Since we're defining abilities with cancancan and using `load_and_authorize_resource`, we're also modifying the `create` action for consistency. --- app/controllers/follows_controller.rb | 3 +-- app/models/abilities/common.rb | 2 +- spec/models/abilities/common_spec.rb | 10 ++++++++++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/controllers/follows_controller.rb b/app/controllers/follows_controller.rb index 1d5d3d331..5eac2bbcc 100644 --- a/app/controllers/follows_controller.rb +++ b/app/controllers/follows_controller.rb @@ -3,13 +3,12 @@ class FollowsController < ApplicationController load_and_authorize_resource def create - @follow = current_user.follows.create!(follow_params) + @follow.save! flash.now[:notice] = t("shared.followable.#{followable_translation_key(@follow.followable)}.create.notice") render :refresh_follow_button end def destroy - @follow = Follow.find(params[:id]) @follow.destroy! flash.now[:notice] = t("shared.followable.#{followable_translation_key(@follow.followable)}.destroy.notice") render :refresh_follow_button diff --git a/app/models/abilities/common.rb b/app/models/abilities/common.rb index 4145d1eef..86cf67b6b 100644 --- a/app/models/abilities/common.rb +++ b/app/models/abilities/common.rb @@ -69,7 +69,7 @@ module Abilities can [:flag, :unflag], Budget::Investment cannot [:flag, :unflag], Budget::Investment, author_id: user.id - can [:create, :destroy], Follow + can [:create, :destroy], Follow, user_id: user.id can [:destroy], Document do |document| document.documentable&.author_id == user.id diff --git a/spec/models/abilities/common_spec.rb b/spec/models/abilities/common_spec.rb index b060f5944..b976a5ed6 100644 --- a/spec/models/abilities/common_spec.rb +++ b/spec/models/abilities/common_spec.rb @@ -119,6 +119,16 @@ describe Abilities::Common do end end + describe "follows" do + let(:other_user) { create(:user) } + + it { should be_able_to(:create, build(:follow, :followed_proposal, user: user)) } + it { should_not be_able_to(:create, build(:follow, :followed_proposal, user: other_user)) } + + it { should be_able_to(:destroy, create(:follow, :followed_proposal, user: user)) } + it { should_not be_able_to(:destroy, create(:follow, :followed_proposal, user: other_user)) } + end + describe "other users" do let(:other_user) { create(:user) } From 3645c333ab4aa97ecbe52d40b111fdab2e0f8312 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Oct 2020 01:39:43 +0100 Subject: [PATCH 3/3] Expire cache when users follow/unfollow When users followed/unfollowed a proposal or a budget investment, the cache did not expire and so the wrong button was displayed after reloading the page. --- app/views/budgets/investments/_investment_show.html.erb | 1 + app/views/proposals/show.html.erb | 1 + 2 files changed, 2 insertions(+) diff --git a/app/views/budgets/investments/_investment_show.html.erb b/app/views/budgets/investments/_investment_show.html.erb index c579f95d4..621efde66 100644 --- a/app/views/budgets/investments/_investment_show.html.erb +++ b/app/views/budgets/investments/_investment_show.html.erb @@ -12,6 +12,7 @@ investment.image, investment.author, Flag.flagged?(current_user, investment), + investment.followed_by?(current_user), @investment_votes] do %>
diff --git a/app/views/proposals/show.html.erb b/app/views/proposals/show.html.erb index ccd0e1a90..a5a05e050 100644 --- a/app/views/proposals/show.html.erb +++ b/app/views/proposals/show.html.erb @@ -17,6 +17,7 @@ @proposal, @proposal.author, Flag.flagged?(current_user, @proposal), + @proposal.followed_by?(current_user), @proposal_votes] do %>