Lock the reviewer's email address so revocation can't be bypassed

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

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

saas/app/models/identity/reviewer_sign_in.rb

@@ -17,33 +17,61 @@
 # 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
 
   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
 
+    # 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
+
     def configured?
       configured_email_address.present? && configured_code_digest.present?
     end
@@ -68,6 +96,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

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

saas/lib/fizzy/saas/engine.rb

@@ -220,6 +220,9 @@ 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
+        UsersController.include Users::ReviewerMembershipLock
         ApplicationController.include Authorization::Controller
         CardsController.include(Card::StorageLimited::Creation)
         Cards::CommentsController.include(Card::StorageLimited::Commenting)

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.inspect} (#{account.id}): #{users.size} active"
+        users.each do |user|
+          puts "    #{user.role} #{user.name.inspect} #{user.identity&.email_address.inspect} joined #{user.created_at.utc.iso8601}"
+        end
+      end
     end
   end
 end

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

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

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,39 @@ 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 "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

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