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] 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