From db0b2416c38c43a43dc006383ef18870066b1b4c Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Sat, 24 Oct 2026 13:57:04 -0700
Subject: [PATCH 1/3] Lock the reviewer's email address, so a code holder can't
 move its membership out of revocation's reach

An email change moves a user to another identity. A holder of the reviewer code could move the reviewer's membership to an address they control, where rotating the code and saas:reviewer_sign_in:sign_out can't reach it. The reviewer's users now refuse an email change, both the request and a confirmation link minted earlier. Sign-out also deletes pending session transfers and resets the join codes of the reviewer's accounts, the other ways a code holder could keep a way back in.
---
 .../users/reviewer_email_address_lock.rb      | 21 ++++++++
 saas/app/models/identity/reviewer_sign_in.rb  | 29 ++++++++---
 .../user/reviewer_email_address_lock.rb       | 22 ++++++++
 saas/lib/fizzy/saas/engine.rb                 |  2 +
 .../users/reviewer_email_address_lock_test.rb | 40 ++++++++++++++
 .../models/identity/reviewer_sign_in_test.rb  | 13 +++++
 .../user/reviewer_email_address_lock_test.rb  | 52 +++++++++++++++++++
 7 files changed, 173 insertions(+), 6 deletions(-)
 create mode 100644 saas/app/controllers/concerns/users/reviewer_email_address_lock.rb
 create mode 100644 saas/app/models/user/reviewer_email_address_lock.rb
 create mode 100644 saas/test/controllers/users/reviewer_email_address_lock_test.rb
 create mode 100644 saas/test/models/user/reviewer_email_address_lock_test.rb

diff --git a/saas/app/controllers/concerns/users/reviewer_email_address_lock.rb b/saas/app/controllers/concerns/users/reviewer_email_address_lock.rb
new file mode 100644
index 000000000..d9f8deb41
--- /dev/null
+++ b/saas/app/controllers/concerns/users/reviewer_email_address_lock.rb
@@ -0,0 +1,21 @@
+# Refuses an email change for the directory reviewer's users, before any
+# confirmation is sent (see User::ReviewerEmailAddressLock).
+module Users::ReviewerEmailAddressLock
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :refuse_reviewer_email_address_change, only: %i[ new create ]
+  end
+
+  private
+    def refuse_reviewer_email_address_change
+      if @user.email_address_locked?
+        error = "This email address can't be changed"
+
+        respond_to do |format|
+          format.html { redirect_to edit_user_path(@user), alert: error }
+          format.json { render json: { error: error }, status: :forbidden }
+        end
+      end
+    end
+end
diff --git a/saas/app/models/identity/reviewer_sign_in.rb b/saas/app/models/identity/reviewer_sign_in.rb
index 6f8b3e430..fbb7b0e70 100644
--- a/saas/app/models/identity/reviewer_sign_in.rb
+++ b/saas/app/models/identity/reviewer_sign_in.rb
@@ -23,27 +23,40 @@ module Identity::ReviewerSignIn
   class << self
     def authenticate(email_address:, code:)
       if matches?(email_address, code)
-        Identity.find_by(email_address: configured_email_address).tap do |identity|
+        configured_identity.tap do |identity|
           record_use(identity) if identity
         end
       end
     end
 
     # Ends every session the configured identity has, however it signed in,
-    # closes its open Action Cable connections, and removes the access tokens
-    # and passkeys a code holder could have added to get back in. Run
-    # it when rotating the code, so sessions started with the old one end too.
-    # It needs only the email, so it still works after the digest is cleared.
+    # closes its open Action Cable connections, and removes the access tokens,
+    # passkeys and pending session transfers a code holder could have added to
+    # get back in. It also resets the join codes of the reviewer's accounts, so
+    # a code holder who read one can't join under an identity of their own.
+    # Run it when rotating the code, so sessions started with the old one end
+    # too. It needs only the email, so it still works after the digest is
+    # cleared.
+    #
+    # The reviewer can't move its users to another email address (see
+    # User::ReviewerEmailAddressLock), so the configured email always finds
+    # the identity and the accounts it signs in to.
     def sign_out_everywhere
-      if configured_email_address.present? && identity = Identity.find_by(email_address: configured_email_address)
+      if identity = configured_identity
         identity.access_tokens.destroy_all
         identity.passkeys.destroy_all
+        identity.transfers.delete_all
+        identity.accounts.each { |account| account.join_code&.reset }
         identity.sessions.destroy_all.size.tap { identity.close_remote_connections }
       else
         0
       end
     end
 
+    def reviewer?(identity)
+      configured_email_address.present? && identity.present? && identity.email_address == configured_email_address.downcase
+    end
+
     def configured?
       configured_email_address.present? && configured_code_digest.present?
     end
@@ -68,6 +81,10 @@ def record_use(identity)
         Sentry.capture_message "Reviewer sign-in code used", level: :info if defined?(Sentry)
       end
 
+      def configured_identity
+        Identity.find_by(email_address: configured_email_address) if configured_email_address.present?
+      end
+
       def configured_email_address
         config.email_address.presence
       end
diff --git a/saas/app/models/user/reviewer_email_address_lock.rb b/saas/app/models/user/reviewer_email_address_lock.rb
new file mode 100644
index 000000000..509c5c56f
--- /dev/null
+++ b/saas/app/models/user/reviewer_email_address_lock.rb
@@ -0,0 +1,22 @@
+# The directory reviewer's users keep the reviewer's email address. An email
+# change moves a user to another identity, so a holder of the reviewer code
+# could otherwise move the reviewer's membership to an address they control,
+# where rotating the code and `saas:reviewer_sign_in:sign_out` can't reach it.
+# The reviewer is a comped internal identity, so nobody needs to change it.
+module User::ReviewerEmailAddressLock
+  def send_email_address_change_confirmation(new_email_address)
+    super unless email_address_locked?
+  end
+
+  def change_email_address(new_email_address)
+    if email_address_locked?
+      false
+    else
+      super
+    end
+  end
+
+  def email_address_locked?
+    Identity::ReviewerSignIn.reviewer?(identity)
+  end
+end
diff --git a/saas/lib/fizzy/saas/engine.rb b/saas/lib/fizzy/saas/engine.rb
index 6ca82cb63..093511e72 100644
--- a/saas/lib/fizzy/saas/engine.rb
+++ b/saas/lib/fizzy/saas/engine.rb
@@ -220,6 +220,8 @@ class Engine < ::Rails::Engine
         ::Session.include Session::Devices
         ::Signup.prepend Signup
         Sessions::MagicLinksController.prepend Sessions::ReviewerSignIn
+        ::User.prepend User::ReviewerEmailAddressLock
+        Users::EmailAddressesController.include Users::ReviewerEmailAddressLock
         ApplicationController.include Authorization::Controller
         CardsController.include(Card::StorageLimited::Creation)
         Cards::CommentsController.include(Card::StorageLimited::Commenting)
diff --git a/saas/test/controllers/users/reviewer_email_address_lock_test.rb b/saas/test/controllers/users/reviewer_email_address_lock_test.rb
new file mode 100644
index 000000000..d3ae92b42
--- /dev/null
+++ b/saas/test/controllers/users/reviewer_email_address_lock_test.rb
@@ -0,0 +1,40 @@
+require "test_helper"
+
+class Users::ReviewerEmailAddressLockTest < ActionDispatch::IntegrationTest
+  include ActionMailer::TestHelper
+
+  setup do
+    @reviewer = identities(:kevin)
+    @user = users(:kevin)
+    configure email_address: @reviewer.email_address, code_digest: Identity::ReviewerSignIn.digest(Identity::ReviewerSignIn.generate_code)
+    sign_in_as :kevin
+  end
+
+  teardown { configure email_address: nil, code_digest: nil }
+
+  test "the reviewer can't request an email change" do
+    assert_no_enqueued_emails do
+      post user_email_addresses_path(@user, script_name: @user.account.slug), params: { email_address: "attacker@example.com" }
+    end
+
+    assert_redirected_to edit_user_path(@user, script_name: @user.account.slug)
+
+    post user_email_addresses_path(@user, script_name: @user.account.slug), params: { email_address: "attacker@example.com" }, as: :json
+    assert_response :forbidden
+  end
+
+  test "a confirmation link minted for the reviewer doesn't move it" do
+    token = @user.send(:generate_email_address_change_token, to: "attacker@example.com")
+
+    post user_email_address_confirmation_path(user_id: @user.id, email_address_token: token, script_name: @user.account.slug), as: :json
+
+    assert_response :unprocessable_entity
+    assert_equal @reviewer, @user.reload.identity
+  end
+
+  private
+    def configure(email_address:, code_digest:)
+      Rails.application.config.x.reviewer_sign_in.email_address = email_address
+      Rails.application.config.x.reviewer_sign_in.code_digest = code_digest
+    end
+end
diff --git a/saas/test/models/identity/reviewer_sign_in_test.rb b/saas/test/models/identity/reviewer_sign_in_test.rb
index d0abaa7eb..326fd5653 100644
--- a/saas/test/models/identity/reviewer_sign_in_test.rb
+++ b/saas/test/models/identity/reviewer_sign_in_test.rb
@@ -56,6 +56,7 @@ class Identity::ReviewerSignInTest < ActiveSupport::TestCase
 
   test "signs the configured identity out everywhere, and no one else" do
     @identity.sessions.create!
+    @identity.transfer_id
     @identity.access_tokens.create!(description: "Reviewer", permission: :read)
     other_tokens = identities(:jason).access_tokens.count
     other_sessions = identities(:jason).sessions.count
@@ -65,10 +66,22 @@ class Identity::ReviewerSignInTest < ActiveSupport::TestCase
     assert_empty @identity.sessions.reload
     assert_empty @identity.access_tokens.reload
     assert_empty @identity.passkeys.reload
+    assert_empty @identity.transfers.reload
     assert_equal other_tokens, identities(:jason).access_tokens.count
     assert_equal other_sessions, identities(:jason).sessions.count
   end
 
+  test "signing out resets the join codes of the reviewer's accounts, and no others" do
+    reviewer_codes = @identity.accounts.map { |account| account.join_code.code }
+    other = Account::JoinCode.where.not(account: @identity.accounts).first
+    other_code = other&.code
+
+    Identity::ReviewerSignIn.sign_out_everywhere
+
+    assert_empty @identity.accounts.map { |account| account.join_code.reload.code } & reviewer_codes
+    assert_equal other_code, other&.reload&.code
+  end
+
   test "signing out still works after the digest is cleared" do
     @identity.sessions.create!
     configure email_address: @identity.email_address, code_digest: nil
diff --git a/saas/test/models/user/reviewer_email_address_lock_test.rb b/saas/test/models/user/reviewer_email_address_lock_test.rb
new file mode 100644
index 000000000..1911a6072
--- /dev/null
+++ b/saas/test/models/user/reviewer_email_address_lock_test.rb
@@ -0,0 +1,52 @@
+require "test_helper"
+
+class User::ReviewerEmailAddressLockTest < ActiveSupport::TestCase
+  include ActionMailer::TestHelper
+
+  setup do
+    @code = Identity::ReviewerSignIn.generate_code
+    @reviewer = identities(:kevin)
+    @user = users(:kevin)
+    configure email_address: @reviewer.email_address, code_digest: Identity::ReviewerSignIn.digest(@code)
+  end
+
+  teardown { configure email_address: nil, code_digest: nil }
+
+  test "the reviewer can't move its user to an address a code holder controls, so sign-out still reaches it" do
+    token = @user.send(:generate_email_address_change_token, to: "attacker@example.com")
+    @reviewer.sessions.create!
+
+    assert_not @user.change_email_address_using_token(token)
+    assert_equal @reviewer, @user.reload.identity
+    assert_nil Identity.find_by(email_address: "attacker@example.com")
+
+    assert_operator Identity::ReviewerSignIn.sign_out_everywhere, :>, 0
+    assert_empty @reviewer.sessions.reload
+    assert_nil Identity::ReviewerSignIn.authenticate(email_address: "attacker@example.com", code: @code)
+  end
+
+  test "the reviewer gets no email change confirmation" do
+    assert_no_enqueued_emails do
+      @user.send_email_address_change_confirmation("attacker@example.com")
+    end
+  end
+
+  test "everyone else can still change their email address" do
+    user = users(:david)
+
+    assert user.change_email_address("david.new@example.com")
+    assert_equal "david.new@example.com", user.reload.identity.email_address
+  end
+
+  test "nobody is locked when the reviewer isn't configured" do
+    configure email_address: nil, code_digest: nil
+
+    assert_not @user.email_address_locked?
+  end
+
+  private
+    def configure(email_address:, code_digest:)
+      Rails.application.config.x.reviewer_sign_in.email_address = email_address
+      Rails.application.config.x.reviewer_sign_in.code_digest = code_digest
+    end
+end

From 65bb7f0a4f418bf08838dbe849a5954ae87c13b7 Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Sat, 24 Oct 2026 16:48:33 -0700
Subject: [PATCH 2/3] List the reviewer accounts' members when signing the
 reviewer out

A code holder who redeemed a join code during a review has a user of their own in the demo account, and rotation doesn't remove it. Jeremy chose to check the members after each review rather than restrict the reviewer's role or deactivate users automatically, so saas:reviewer_sign_in:sign_out now prints every active member of the reviewer's accounts for that check.
---
 saas/app/models/identity/reviewer_sign_in.rb    | 15 +++++++++++++++
 saas/lib/tasks/reviewer_sign_in.rake            |  9 ++++++++-
 .../models/identity/reviewer_sign_in_test.rb    | 17 +++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/saas/app/models/identity/reviewer_sign_in.rb b/saas/app/models/identity/reviewer_sign_in.rb
index fbb7b0e70..c97748f96 100644
--- a/saas/app/models/identity/reviewer_sign_in.rb
+++ b/saas/app/models/identity/reviewer_sign_in.rb
@@ -17,6 +17,10 @@
 # the code's. Keep the identity in its own comped, internal account. Rotate by
 # generating a new code, replacing the digest, then running
 # `bin/rails saas:reviewer_sign_in:sign_out` to end the old code's sessions.
+# That task also lists every active member of the reviewer's accounts: check it
+# after each review and deactivate anyone who isn't the reviewer or the
+# account's own staff, since a code holder could have joined under an identity
+# of their own while the review ran.
 module Identity::ReviewerSignIn
   CODE_LENGTH = 16
 
@@ -53,6 +57,17 @@ def sign_out_everywhere
       end
     end
 
+    # Every active member of the reviewer's accounts, by account, for the
+    # after-review check: anyone here who isn't the reviewer or the account's
+    # own staff joined during a review and keeps access until deactivated.
+    def members_of_reviewer_accounts
+      if identity = configured_identity
+        identity.accounts.to_h { |account| [ account, account.users.active.includes(:identity).order(:created_at).to_a ] }
+      else
+        {}
+      end
+    end
+
     def reviewer?(identity)
       configured_email_address.present? && identity.present? && identity.email_address == configured_email_address.downcase
     end
diff --git a/saas/lib/tasks/reviewer_sign_in.rake b/saas/lib/tasks/reviewer_sign_in.rake
index 4d9738c4f..3d4854fc3 100644
--- a/saas/lib/tasks/reviewer_sign_in.rake
+++ b/saas/lib/tasks/reviewer_sign_in.rake
@@ -8,9 +8,16 @@ namespace :saas do
       puts "FIZZY_REVIEWER_SIGN_IN_CODE_DIGEST=#{Identity::ReviewerSignIn.digest(code)}"
     end
 
-    desc "End every session of the configured reviewer identity (run after rotating the code)"
+    desc "End every session of the configured reviewer identity and list its accounts' members (run after rotating the code)"
     task sign_out: :environment do
       puts "Ended #{Identity::ReviewerSignIn.sign_out_everywhere} reviewer session(s)"
+      puts "Check these members; deactivate anyone who isn't the reviewer or the account's own staff:"
+      Identity::ReviewerSignIn.members_of_reviewer_accounts.each do |account, users|
+        puts "  #{account.name} (#{account.id}): #{users.size} active"
+        users.each do |user|
+          puts "    #{user.role} #{user.name} <#{user.identity&.email_address}> joined #{user.created_at.utc.iso8601}"
+        end
+      end
     end
   end
 end
diff --git a/saas/test/models/identity/reviewer_sign_in_test.rb b/saas/test/models/identity/reviewer_sign_in_test.rb
index 326fd5653..daee87a55 100644
--- a/saas/test/models/identity/reviewer_sign_in_test.rb
+++ b/saas/test/models/identity/reviewer_sign_in_test.rb
@@ -82,6 +82,23 @@ class Identity::ReviewerSignInTest < ActiveSupport::TestCase
     assert_equal other_code, other&.reload&.code
   end
 
+  test "lists the active members of the reviewer's accounts, including one who joined during a review" do
+    account = @identity.accounts.first
+    joiner = account.users.create!(name: "Code holder", identity: Identity.create!(email_address: "code-holder@example.com"), role: :member)
+
+    members = Identity::ReviewerSignIn.members_of_reviewer_accounts
+
+    assert_equal @identity.accounts.sort_by(&:id), members.keys.sort_by(&:id)
+    assert_includes members[account], joiner
+    assert members.values.flatten.all?(&:active?)
+  end
+
+  test "lists no members when unconfigured" do
+    configure email_address: nil, code_digest: nil
+
+    assert_equal({}, Identity::ReviewerSignIn.members_of_reviewer_accounts)
+  end
+
   test "signing out still works after the digest is cleared" do
     @identity.sessions.create!
     configure email_address: @identity.email_address, code_digest: nil

From 357bd4632aec4efa162f463bfafa16eefd285318 Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Sat, 24 Oct 2026 17:02:12 -0700
Subject: [PATCH 3/3] Keep the reviewer's memberships in reach, and print the
 member audit escaped

Deactivating a user clears its identity, so a reviewer deactivated from the app would drop its account out of sign_out's join-code reset and member list. The app now refuses to deactivate the reviewer's users, whoever asks; operators use a console. The audit prints names and emails with inspect, so a member's display name can't inject newlines or terminal control sequences into the check.
---
 .../users/reviewer_membership_lock.rb         | 25 +++++++++
 saas/lib/fizzy/saas/engine.rb                 |  1 +
 saas/lib/tasks/reviewer_sign_in.rake          |  4 +-
 .../users/reviewer_membership_lock_test.rb    | 51 +++++++++++++++++++
 4 files changed, 79 insertions(+), 2 deletions(-)
 create mode 100644 saas/app/controllers/concerns/users/reviewer_membership_lock.rb
 create mode 100644 saas/test/controllers/users/reviewer_membership_lock_test.rb

diff --git a/saas/app/controllers/concerns/users/reviewer_membership_lock.rb b/saas/app/controllers/concerns/users/reviewer_membership_lock.rb
new file mode 100644
index 000000000..123e74b7d
--- /dev/null
+++ b/saas/app/controllers/concerns/users/reviewer_membership_lock.rb
@@ -0,0 +1,25 @@
+# Refuses to deactivate the directory reviewer's users from the app, whoever
+# asks. Deactivating a user clears its identity, so the account would drop out
+# of the reviewer's accounts, and `saas:reviewer_sign_in:sign_out` would then
+# neither reset that account's join code nor list its members for the
+# after-review check. Operators who mean to remove the reviewer do it from a
+# console.
+module Users::ReviewerMembershipLock
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :refuse_reviewer_deactivation, only: %i[ destroy ]
+  end
+
+  private
+    def refuse_reviewer_deactivation
+      if Identity::ReviewerSignIn.reviewer?(@user.identity)
+        error = "This person can't be removed"
+
+        respond_to do |format|
+          format.html { redirect_to account_settings_path, alert: error }
+          format.json { render json: { error: error }, status: :forbidden }
+        end
+      end
+    end
+end
diff --git a/saas/lib/fizzy/saas/engine.rb b/saas/lib/fizzy/saas/engine.rb
index 093511e72..e4e637078 100644
--- a/saas/lib/fizzy/saas/engine.rb
+++ b/saas/lib/fizzy/saas/engine.rb
@@ -222,6 +222,7 @@ class Engine < ::Rails::Engine
         Sessions::MagicLinksController.prepend Sessions::ReviewerSignIn
         ::User.prepend User::ReviewerEmailAddressLock
         Users::EmailAddressesController.include Users::ReviewerEmailAddressLock
+        UsersController.include Users::ReviewerMembershipLock
         ApplicationController.include Authorization::Controller
         CardsController.include(Card::StorageLimited::Creation)
         Cards::CommentsController.include(Card::StorageLimited::Commenting)
diff --git a/saas/lib/tasks/reviewer_sign_in.rake b/saas/lib/tasks/reviewer_sign_in.rake
index 3d4854fc3..61b8b67b2 100644
--- a/saas/lib/tasks/reviewer_sign_in.rake
+++ b/saas/lib/tasks/reviewer_sign_in.rake
@@ -13,9 +13,9 @@ namespace :saas do
       puts "Ended #{Identity::ReviewerSignIn.sign_out_everywhere} reviewer session(s)"
       puts "Check these members; deactivate anyone who isn't the reviewer or the account's own staff:"
       Identity::ReviewerSignIn.members_of_reviewer_accounts.each do |account, users|
-        puts "  #{account.name} (#{account.id}): #{users.size} active"
+        puts "  #{account.name.inspect} (#{account.id}): #{users.size} active"
         users.each do |user|
-          puts "    #{user.role} #{user.name} <#{user.identity&.email_address}> joined #{user.created_at.utc.iso8601}"
+          puts "    #{user.role} #{user.name.inspect} #{user.identity&.email_address.inspect} joined #{user.created_at.utc.iso8601}"
         end
       end
     end
diff --git a/saas/test/controllers/users/reviewer_membership_lock_test.rb b/saas/test/controllers/users/reviewer_membership_lock_test.rb
new file mode 100644
index 000000000..2e8a70147
--- /dev/null
+++ b/saas/test/controllers/users/reviewer_membership_lock_test.rb
@@ -0,0 +1,51 @@
+require "test_helper"
+
+class Users::ReviewerMembershipLockTest < ActionDispatch::IntegrationTest
+  setup do
+    @reviewer = identities(:kevin)
+    @user = users(:kevin)
+    configure email_address: @reviewer.email_address, code_digest: Identity::ReviewerSignIn.digest(Identity::ReviewerSignIn.generate_code)
+  end
+
+  teardown { configure email_address: nil, code_digest: nil }
+
+  test "the reviewer can't deactivate itself" do
+    sign_in_as :kevin
+
+    delete user_path(@user, script_name: @user.account.slug), as: :json
+
+    assert_response :forbidden
+    assert @user.reload.active?
+    assert_equal @reviewer, @user.identity
+    assert_includes @reviewer.accounts.reload, @user.account
+  end
+
+  test "another admin can't deactivate the reviewer either" do
+    admin = @user.account.users.where.not(id: @user.id).find { |user| user.identity && user.can_change?(@user) } || \
+      flunk("fixture needs another admin in the reviewer's account")
+    sign_in_as admin.identity
+
+    delete user_path(@user, script_name: @user.account.slug)
+
+    assert_redirected_to account_settings_path(script_name: @user.account.slug)
+    assert @user.reload.active?
+  end
+
+  test "other users can still be deactivated" do
+    configure email_address: identities(:jason).email_address, code_digest: nil
+    sign_in_as :kevin
+    other = @user.account.users.where.not(id: @user.id).find { |user| user.identity && @user.can_change?(user) && !user.owner? } || \
+      flunk("fixture needs a user kevin can change")
+
+    delete user_path(other, script_name: other.account.slug), as: :json
+
+    assert_response :no_content
+    assert_not other.reload.active?
+  end
+
+  private
+    def configure(email_address:, code_digest:)
+      Rails.application.config.x.reviewer_sign_in.email_address = email_address
+      Rails.application.config.x.reviewer_sign_in.code_digest = code_digest
+    end
+end