From cfdd7ab1cb8c3ee8779c4c036b973570dbbbccda Mon Sep 17 00:00:00 2001 From: "Stanko K.R." Date: Wed, 5 Nov 2025 11:57:40 +0100 Subject: [PATCH] Prohibit duplicate memberships --- app/controllers/join_codes_controller.rb | 6 ++- .../confirmations_controller.rb | 2 +- .../memberships/email_addresses_controller.rb | 9 +++- config/routes.rb | 2 +- ...n_identity_id_and_tenant_to_memberships.rb | 15 +++++++ db/untenanted_schema.rb | 3 +- .../controllers/join_codes_controller_test.rb | 2 +- .../confirmations_controller_test.rb | 42 ++++++++++++++++++ .../email_addresses_controller_test.rb | 43 +++++++++++++++++++ .../memberships/unlink_controller_test.rb | 30 +++++++++++++ .../email_address_changeable_test.rb | 3 ++ 11 files changed, 150 insertions(+), 7 deletions(-) create mode 100644 db/untenanted_migrate/20251105082803_add_unique_constraint_on_identity_id_and_tenant_to_memberships.rb create mode 100644 test/controllers/memberships/email_addresses/confirmations_controller_test.rb create mode 100644 test/controllers/memberships/email_addresses_controller_test.rb create mode 100644 test/controllers/memberships/unlink_controller_test.rb diff --git a/app/controllers/join_codes_controller.rb b/app/controllers/join_codes_controller.rb index 0ddbcdd7b..f9980a8db 100644 --- a/app/controllers/join_codes_controller.rb +++ b/app/controllers/join_codes_controller.rb @@ -10,8 +10,10 @@ class JoinCodesController < ApplicationController def create Identity.transaction do - identity = Identity.find_or_create_by(email_address: params.expect(:email_address)) - identity.memberships.create!(tenant: tenant, join_code: code) + identity = Identity.find_or_create_by!(email_address: params.expect(:email_address)) + identity.memberships.find_or_create_by!(tenant: tenant) do |membership| + membership.join_code = code + end identity.send_magic_link end diff --git a/app/controllers/memberships/email_addresses/confirmations_controller.rb b/app/controllers/memberships/email_addresses/confirmations_controller.rb index 8ad43971f..b53fdc135 100644 --- a/app/controllers/memberships/email_addresses/confirmations_controller.rb +++ b/app/controllers/memberships/email_addresses/confirmations_controller.rb @@ -11,7 +11,7 @@ class Memberships::EmailAddresses::ConfirmationsController < ApplicationControll def create membership = Membership.change_email_address_using_token(token) - terminate_session + terminate_session if Current.session start_new_session_for membership.reload.identity redirect_to edit_user_url(script_name: "/#{@membership.tenant}", id: @membership.user) diff --git a/app/controllers/memberships/email_addresses_controller.rb b/app/controllers/memberships/email_addresses_controller.rb index e918005e1..c75783ac1 100644 --- a/app/controllers/memberships/email_addresses_controller.rb +++ b/app/controllers/memberships/email_addresses_controller.rb @@ -8,7 +8,14 @@ class Memberships::EmailAddressesController < ApplicationController end def create - @membership.send_email_address_change_confirmation(new_email_address) + identity = Identity.find_by_email_address(new_email_address) + + if identity&.memberships&.exists?(tenant: @membership.tenant) + flash[:alert] = "You already have a user in this account with that email address" + redirect_to new_email_address_path + else + @membership.send_email_address_change_confirmation(new_email_address) + end end private diff --git a/config/routes.rb b/config/routes.rb index ef99bad1b..af7b9c383 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -151,7 +151,7 @@ Rails.application.routes.draw do end scope module: :memberships, path: "memberships/:membership_id" do - resource :unlink, controller: :unlink, as: :unlink_membership + resource :unlink, only: %i[ show create ], controller: :unlink, as: :unlink_membership resources :email_addresses, param: :token do resource :confirmation, module: :email_addresses diff --git a/db/untenanted_migrate/20251105082803_add_unique_constraint_on_identity_id_and_tenant_to_memberships.rb b/db/untenanted_migrate/20251105082803_add_unique_constraint_on_identity_id_and_tenant_to_memberships.rb new file mode 100644 index 000000000..5d42ac6ed --- /dev/null +++ b/db/untenanted_migrate/20251105082803_add_unique_constraint_on_identity_id_and_tenant_to_memberships.rb @@ -0,0 +1,15 @@ +class AddUniqueConstraintOnIdentityIdAndTenantToMemberships < ActiveRecord::Migration[8.2] + def change + # Remove duplicates, keeping the youngest membership for each identity_id + tenant combination + execute <<-SQL + DELETE FROM memberships + WHERE id NOT IN ( + SELECT MAX(id) + FROM memberships + GROUP BY identity_id, tenant + ) + SQL + + add_index :memberships, %i[ tenant identity_id ], unique: true + end +end diff --git a/db/untenanted_schema.rb b/db/untenanted_schema.rb index b4582291a..efe4c961b 100644 --- a/db/untenanted_schema.rb +++ b/db/untenanted_schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2025_11_03_125952) do +ActiveRecord::Schema[8.2].define(version: 2025_11_05_082803) do create_table "identities", force: :cascade do |t| t.datetime "created_at", null: false t.string "email_address", null: false @@ -36,6 +36,7 @@ ActiveRecord::Schema[8.2].define(version: 2025_11_03_125952) do t.string "tenant", null: false t.datetime "updated_at", null: false t.index ["identity_id"], name: "index_memberships_on_identity_id" + t.index ["tenant", "identity_id"], name: "index_memberships_on_tenant_and_identity_id", unique: true t.index ["tenant"], name: "index_memberships_on_user_tenant_and_user_id" end diff --git a/test/controllers/join_codes_controller_test.rb b/test/controllers/join_codes_controller_test.rb index 2387e30ce..85a6fc15a 100644 --- a/test/controllers/join_codes_controller_test.rb +++ b/test/controllers/join_codes_controller_test.rb @@ -51,7 +51,7 @@ class JoinCodesControllerTest < ActionDispatch::IntegrationTest untenanted do assert_no_difference -> { Identity.count } do - assert_difference -> { Membership.count }, 1 do + assert_no_difference -> { Membership.count } do post join_path(tenant: @tenant, code: @join_code.code), params: { email_address: identity.email_address } end end diff --git a/test/controllers/memberships/email_addresses/confirmations_controller_test.rb b/test/controllers/memberships/email_addresses/confirmations_controller_test.rb new file mode 100644 index 000000000..86c5ff171 --- /dev/null +++ b/test/controllers/memberships/email_addresses/confirmations_controller_test.rb @@ -0,0 +1,42 @@ +require "test_helper" + +class Memberships::EmailAddresses::ConfirmationsControllerTest < ActionDispatch::IntegrationTest + test "show" do + untenanted do + membership = memberships(:kevin_in_37signals) + + get email_address_confirmation_path( + membership_id: membership.id, + email_address_token: "dummy_token" + ) + + assert_response :success + end + end + + test "create" do + untenanted do + membership = memberships(:kevin_in_37signals) + old_identity = membership.identity + new_email = "updated@example.com" + + # Generate a real token + token = membership.send(:generate_email_address_change_token, to: new_email) + + assert_difference -> { Identity.count }, 1 do + post email_address_confirmation_path( + membership_id: membership.id, + email_address_token: token + ), + params: { email_address_token: token } + end + + membership.reload + assert_equal new_email, membership.identity.email_address + assert_not_equal old_identity.id, membership.identity_id + + assert cookies[:session_token].present?, "Should have started new session" + assert_redirected_to edit_user_url(script_name: "/#{membership.tenant}", id: membership.user) + end + end +end diff --git a/test/controllers/memberships/email_addresses_controller_test.rb b/test/controllers/memberships/email_addresses_controller_test.rb new file mode 100644 index 000000000..ef88bfaa1 --- /dev/null +++ b/test/controllers/memberships/email_addresses_controller_test.rb @@ -0,0 +1,43 @@ +require "test_helper" + +class Memberships::EmailAddressesControllerTest < ActionDispatch::IntegrationTest + test "new" do + untenanted do + sign_in_as :kevin + + membership = memberships(:kevin_in_37signals) + + get new_email_address_path(membership_id: membership.id) + assert_response :success + end + end + + test "create" do + untenanted do + sign_in_as :kevin + + membership = memberships(:kevin_in_37signals) + + assert_enqueued_emails 1 do + post email_addresses_path(membership_id: membership.id), + params: { email_address: "newemail@example.com" } + end + + assert_response :success + end + end + + test "create with an email for someone already in the account" do + untenanted do + sign_in_as :kevin + + membership = memberships(:kevin_in_37signals) + + post email_addresses_path(membership_id: membership.id), + params: { email_address: identities(:david).email_address } + + assert_redirected_to new_email_address_path + assert_equal "You already have a user in this account with that email address", flash[:alert] + end + end +end diff --git a/test/controllers/memberships/unlink_controller_test.rb b/test/controllers/memberships/unlink_controller_test.rb new file mode 100644 index 000000000..2020a2eb2 --- /dev/null +++ b/test/controllers/memberships/unlink_controller_test.rb @@ -0,0 +1,30 @@ +require "test_helper" + +class Memberships::UnlinkControllerTest < ActionDispatch::IntegrationTest + test "show" do + untenanted do + sign_in_as :kevin + + membership = memberships(:kevin_in_37signals) + signed_id = membership.signed_id(purpose: :unlinking) + + get unlink_membership_path(membership_id: signed_id) + assert_response :success + end + end + + test "create" do + untenanted do + sign_in_as :kevin + + membership = memberships(:kevin_in_37signals) + signed_id = membership.signed_id(purpose: :unlinking) + + assert_difference -> { Membership.count }, -1 do + post unlink_membership_path(membership_id: signed_id) + end + + assert_redirected_to session_menu_path + end + end +end diff --git a/test/models/membership/email_address_changeable_test.rb b/test/models/membership/email_address_changeable_test.rb index bad777c81..fbc61bbb1 100644 --- a/test/models/membership/email_address_changeable_test.rb +++ b/test/models/membership/email_address_changeable_test.rb @@ -27,6 +27,9 @@ class Membership::EmailAddressChangeableTest < ActiveSupport::TestCase assert_not old_identity.reload.memberships.exists?(id: @membership.id) assert_equal @new_email, @membership.reload.identity.email_address + # Make sure that a prior membership doesn't exist + identities(:david).memberships.where(tenant: @tenant).delete_all + assert_no_difference -> { Identity.count } do @membership.change_email_address(identities(:david).email_address) end