From 54b6e1ea24b03e5690eba6045956fe3151c6217a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 24 Nov 2023 14:36:51 +0100 Subject: [PATCH 1/4] Use explicit paths in direct messages URLs This way it's easier to search the code for places using them. --- app/controllers/direct_messages_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/direct_messages_controller.rb b/app/controllers/direct_messages_controller.rb index b105550ab..79fe5a400 100644 --- a/app/controllers/direct_messages_controller.rb +++ b/app/controllers/direct_messages_controller.rb @@ -14,7 +14,9 @@ class DirectMessagesController < ApplicationController if @direct_message.save Mailer.direct_message_for_receiver(@direct_message).deliver_later Mailer.direct_message_for_sender(@direct_message).deliver_later - redirect_to [@receiver, @direct_message], notice: I18n.t("flash.actions.create.direct_message") + + redirect_to user_direct_message_path(@receiver, @direct_message), + notice: I18n.t("flash.actions.create.direct_message") else render :new end From 80f0d710fd487648faf316c9dac2edaea231d4ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 24 Nov 2023 14:41:03 +0100 Subject: [PATCH 2/4] Simplify direct messages resource loading --- app/controllers/direct_messages_controller.rb | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/app/controllers/direct_messages_controller.rb b/app/controllers/direct_messages_controller.rb index 79fe5a400..33996565c 100644 --- a/app/controllers/direct_messages_controller.rb +++ b/app/controllers/direct_messages_controller.rb @@ -1,16 +1,13 @@ class DirectMessagesController < ApplicationController - load_and_authorize_resource + load_and_authorize_resource :user, instance_name: :receiver + load_and_authorize_resource through: :receiver, through_association: :direct_messages_received def new - @receiver = User.find(params[:user_id]) - @direct_message = DirectMessage.new(receiver: @receiver) end def create - @sender = current_user - @receiver = User.find(params[:user_id]) + @direct_message.sender = current_user - @direct_message = DirectMessage.new(parsed_params) if @direct_message.save Mailer.direct_message_for_receiver(@direct_message).deliver_later Mailer.direct_message_for_sender(@direct_message).deliver_later @@ -23,7 +20,6 @@ class DirectMessagesController < ApplicationController end def show - @direct_message = DirectMessage.find(params[:id]) end private @@ -35,8 +31,4 @@ class DirectMessagesController < ApplicationController def allowed_params [:title, :body] end - - def parsed_params - direct_message_params.merge(sender: @sender, receiver: @receiver) - end end From 2db807baa76575932d42764d5dd9263757edd09b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 24 Nov 2023 16:23:33 +0100 Subject: [PATCH 3/4] Restrict access to the "new" direct message action This way only verified users will be able to access this page, which shows the username of the receiver of the direct message. With this, it's no longer possible for unverified users to browse direct message URLs in order to collect usernames from every user. --- app/controllers/direct_messages_controller.rb | 6 +++++- app/models/abilities/everyone.rb | 1 - app/views/direct_messages/new.html.erb | 21 +++---------------- spec/models/abilities/common_spec.rb | 2 +- spec/system/direct_messages_spec.rb | 6 +++--- 5 files changed, 12 insertions(+), 24 deletions(-) diff --git a/app/controllers/direct_messages_controller.rb b/app/controllers/direct_messages_controller.rb index 33996565c..d680559c3 100644 --- a/app/controllers/direct_messages_controller.rb +++ b/app/controllers/direct_messages_controller.rb @@ -1,8 +1,12 @@ class DirectMessagesController < ApplicationController + before_action :authenticate_user! load_and_authorize_resource :user, instance_name: :receiver - load_and_authorize_resource through: :receiver, through_association: :direct_messages_received + load_resource through: :receiver, through_association: :direct_messages_received + authorize_resource except: :new def new + authorize! :new, @direct_message, message: t("users.direct_messages.new.verified_only", + verify_account: helpers.link_to_verify_account) end def create diff --git a/app/models/abilities/everyone.rb b/app/models/abilities/everyone.rb index 252ce3896..fc77948bc 100644 --- a/app/models/abilities/everyone.rb +++ b/app/models/abilities/everyone.rb @@ -18,7 +18,6 @@ module Abilities can :read_results, Budget, id: Budget.finished.results_enabled.ids can :read_stats, Budget, id: Budget.valuating_or_later.stats_enabled.ids can :read_executions, Budget, phase: "finished" - can :new, DirectMessage can [:read, :debate, :draft_publication, :allegations, :result_publication, :proposals, :milestones], Legislation::Process, published: true can :summary, Legislation::Process, diff --git a/app/views/direct_messages/new.html.erb b/app/views/direct_messages/new.html.erb index 1343f6363..a945a03a5 100644 --- a/app/views/direct_messages/new.html.erb +++ b/app/views/direct_messages/new.html.erb @@ -6,21 +6,7 @@ <%= t("users.direct_messages.new.title", receiver: @receiver.name) %> - <% if not current_user %> -
-

- <%= sanitize(t("users.login_to_continue", - signin: link_to_signin, - signup: link_to_signup)) %> -

-
- <% elsif not @receiver.email_on_direct_message? %> -
-

- <%= t("users.direct_messages.new.direct_messages_bloqued") %> -

-
- <% elsif can? :create, @direct_message %> + <% if @receiver.email_on_direct_message? %> <%= form_for [@receiver, @direct_message] do |f| %> <%= render "shared/errors", resource: @direct_message %> @@ -32,10 +18,9 @@ <% end %> <% else %> -
+

- <%= sanitize(t("users.direct_messages.new.verified_only", - verify_account: link_to_verify_account)) %> + <%= t("users.direct_messages.new.direct_messages_bloqued") %>

<% end %> diff --git a/spec/models/abilities/common_spec.rb b/spec/models/abilities/common_spec.rb index 392d4367b..9f80ee67c 100644 --- a/spec/models/abilities/common_spec.rb +++ b/spec/models/abilities/common_spec.rb @@ -92,7 +92,7 @@ describe Abilities::Common do it { should_not be_able_to(:comment_as_administrator, proposal) } it { should_not be_able_to(:comment_as_moderator, proposal) } - it { should be_able_to(:new, DirectMessage) } + it { should_not be_able_to(:new, DirectMessage) } it { should_not be_able_to(:create, DirectMessage) } it { should_not be_able_to(:show, DirectMessage) } diff --git a/spec/system/direct_messages_spec.rb b/spec/system/direct_messages_spec.rb index dd008174e..74193ea6b 100644 --- a/spec/system/direct_messages_spec.rb +++ b/spec/system/direct_messages_spec.rb @@ -54,7 +54,7 @@ describe "Direct messages" do visit new_user_direct_message_path(receiver) expect(page).to have_content "To send a private message verify your account" - expect(page).not_to have_link "Send private message" + expect(page).to have_current_path root_path end scenario "User not logged in" do @@ -62,8 +62,8 @@ describe "Direct messages" do visit new_user_direct_message_path(receiver) - expect(page).to have_content "You must sign in or sign up to continue." - expect(page).not_to have_link "Send private message" + expect(page).to have_content "You must sign in or register to continue." + expect(page).to have_current_path new_user_session_path end scenario "Accessing form directly" do From 77c043b68a99f8571a5dbda9b45b43782e3d02b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 28 Nov 2023 17:49:02 +0100 Subject: [PATCH 4/4] Add a username slug to the user URL This way it won't be possible to browse all user URLs by just going to /users/1, /users/2, /users/3, ... and collect usernames, which might not be desirable in some cases. Note we could use the username as a URL parameter and just find the user with `@user = User.find_by!(id: id, username: username)`, but since usernames might contain strange characters, this might lead to strange/ugly URLs. Finally, note we're using `username.to_s` in order to cover the case where the username is `nil` (as is the case with erased users). --- app/controllers/direct_messages_controller.rb | 7 +++ app/controllers/users_controller.rb | 7 +++ app/models/user.rb | 8 +++ .../direct_messages_controller_spec.rb | 62 +++++++++++++++++++ spec/controllers/users_controller_spec.rb | 39 ++++++++++++ 5 files changed, 123 insertions(+) create mode 100644 spec/controllers/direct_messages_controller_spec.rb create mode 100644 spec/controllers/users_controller_spec.rb diff --git a/app/controllers/direct_messages_controller.rb b/app/controllers/direct_messages_controller.rb index d680559c3..706d6fea2 100644 --- a/app/controllers/direct_messages_controller.rb +++ b/app/controllers/direct_messages_controller.rb @@ -1,6 +1,7 @@ class DirectMessagesController < ApplicationController before_action :authenticate_user! load_and_authorize_resource :user, instance_name: :receiver + before_action :check_slug load_resource through: :receiver, through_association: :direct_messages_received authorize_resource except: :new @@ -28,6 +29,12 @@ class DirectMessagesController < ApplicationController private + def check_slug + slug = params[:user_id].split("-", 2)[1] + + raise ActiveRecord::RecordNotFound unless @receiver.slug == slug.to_s + end + def direct_message_params params.require(:direct_message).permit(allowed_params) end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 8ccee9692..f38464d75 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,5 +1,6 @@ class UsersController < ApplicationController load_and_authorize_resource + before_action :check_slug helper_method :valid_interests_access? def show @@ -8,6 +9,12 @@ class UsersController < ApplicationController private + def check_slug + slug = params[:id].split("-", 2)[1] + + raise ActiveRecord::RecordNotFound unless @user.slug == slug.to_s + end + def valid_interests_access?(user) user.public_interests || user == current_user end diff --git a/app/models/user.rb b/app/models/user.rb index b7aa66e3f..d9fc28938 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -433,6 +433,14 @@ class User < ApplicationRecord (Tenant.current_secrets.dig(:security, :lockable, :unlock_in) || 1).to_f.hours end + def to_param + "#{id}-#{slug}" + end + + def slug + username.to_s.parameterize + end + private def clean_document_number diff --git a/spec/controllers/direct_messages_controller_spec.rb b/spec/controllers/direct_messages_controller_spec.rb new file mode 100644 index 000000000..9cc8cc6d7 --- /dev/null +++ b/spec/controllers/direct_messages_controller_spec.rb @@ -0,0 +1,62 @@ +require "rails_helper" + +describe DirectMessagesController do + before { sign_in(create :user, :level_two) } + + describe "GET new" do + let!(:user) { create(:user, username: "James Jameson") } + + it "finds a user by ID and slug" do + get :new, params: { user_id: "#{user.id}-james-jameson" } + + expect(response).to be_successful + end + + it "does not find a user by just an ID" do + expect do + get :new, params: { user_id: user.id } + end.to raise_error ActiveRecord::RecordNotFound + end + + it "does not find a user by just a slug" do + expect do + get :new, params: { user_id: "james-jameson" } + end.to raise_error ActiveRecord::RecordNotFound + end + + it "does not find a user with the wrong slug" do + expect do + get :new, params: { user_id: "#{user.id}-James Jameson" } + end.to raise_error ActiveRecord::RecordNotFound + end + end + + describe "POST create" do + let!(:user) { create(:user, username: "James Jameson") } + let(:message_params) { { direct_message: { title: "Hello!", message: "How are you doing?" }} } + + it "finds a user by ID and slug" do + post :create, params: message_params.merge(user_id: "#{user.id}-james-jameson") + + expect(response).to be_successful + end + + it "does not find a user by just an ID" do + expect do + post :create, params: message_params.merge(user_id: user.id) + end.to raise_error ActiveRecord::RecordNotFound + end + + it "does not find a user by just a slug" do + expect do + post :create, params: message_params.merge(user_id: "james-jameson") + end.to raise_error ActiveRecord::RecordNotFound + end + + it "does not find a user with the wrong slug" do + expect do + post :create, params: message_params.merge(user_id: "#{user.id}-James Jameson") + end.to raise_error ActiveRecord::RecordNotFound + end + end +end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb new file mode 100644 index 000000000..4f7e28740 --- /dev/null +++ b/spec/controllers/users_controller_spec.rb @@ -0,0 +1,39 @@ +require "rails_helper" + +describe UsersController do + describe "GET show" do + let!(:user) { create(:user, username: "James Jameson") } + + it "finds a user by ID and slug" do + get :show, params: { id: "#{user.id}-james-jameson" } + + expect(response).to be_successful + end + + it "does not find a user by just an ID" do + expect do + get :show, params: { id: user.id } + end.to raise_error ActiveRecord::RecordNotFound + end + + it "does not find a user by just a slug" do + expect do + get :show, params: { id: "james-jameson" } + end.to raise_error ActiveRecord::RecordNotFound + end + + it "does not find a user with the wrong slug" do + expect do + get :show, params: { id: "#{user.id}-James Jameson" } + end.to raise_error ActiveRecord::RecordNotFound + end + + it "finds users without username by ID" do + user.erase + + get :show, params: { id: user.id } + + expect(response).to be_successful + end + end +end