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/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/app/models/identity/reviewer_sign_in.rb b/saas/app/models/identity/reviewer_sign_in.rb index 6f8b3e430..c97748f96 100644 --- a/saas/app/models/identity/reviewer_sign_in.rb +++ b/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 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..e4e637078 100644 --- a/saas/lib/fizzy/saas/engine.rb +++ b/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) diff --git a/saas/lib/tasks/reviewer_sign_in.rake b/saas/lib/tasks/reviewer_sign_in.rake index 4d9738c4f..61b8b67b2 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.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 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/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 diff --git a/saas/test/models/identity/reviewer_sign_in_test.rb b/saas/test/models/identity/reviewer_sign_in_test.rb index d0abaa7eb..daee87a55 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,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 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