Add migration+change tests for blind_index update

The update to the blind_index gem requires a number
of changes no matter what.

This commit switches the application to the newer
blind_index naming conventions and algorithm (argon2id),
requiring a migration to do it and changes to the
test fixture for user data.

This commit was more work than expected.

Calculating the bidx values doesn't work correctly
from the fixtures within the ERB (the calls *run* but don't
produce the right values, presumably because it's not fully set up
at that point). This was resolved by inserting the calculated values
into the fixtures, which also makes the tests more independent
of the system being tested.
One of the older test values was subtly wrong (.com vs. .org
email address), which created a mysterious hard-to-track failure
(we prevent future versions of this problem
via a new test that sanity-checks all test values).

The migration had to be tweaked several times to work with
real production data. We need to not change the user updated_at
by setting touch to false (the user-visible data isn't changing),
we need not validate (some entries may not be acceptable any more
but we still need to migrate them as they are), and we need to
skip cases where encrypted_email is nil (in those cases
we have no data to decrypt).

Signed-off-by: David A. Wheeler <[email protected]>

Author: david-a-wheeler

app/models/user.rb

@@ -41,13 +41,14 @@ class User < ApplicationRecord
   attr_encrypted :email, algorithm: 'aes-256-gcm', key: [(
     ENV['EMAIL_ENCRYPTION_KEY'] || '1' * DIGITS_OF_EMAIL_ENCRYPTION_KEY
   )].pack('H*')
+
   # Email addresses are indexed as blind indexes of downcased email addresses,
   # so we can efficiently search for them while keeping them encrypted.
-  # Usage: User.where(email: "[email protected]")
-  # or:    User.where(email: "[email protected]", provider: "local")
+  # Usage: User.where(email: '[email protected]')
+  # or:    User.where(email: '[email protected]', provider: 'local')
   blind_index :email, key: [(
     ENV['EMAIL_BLIND_INDEX_KEY'] || '2' * DIGITS_OF_EMAIL_BLIND_INDEX_KEY
-  )].pack('H*'), expression: ->(v) { v.try(:downcase) }, legacy: true
+  )].pack('H*'), expression: ->(v) { v.try(:downcase) }
 
   scope :created_since, (
     lambda do |time|

db/migrate/20210216184638_blind_index_2_2.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+# Migrate to the use of gem blind_index version 2.2, which
+# has somewhat different conventions than its old version.
+# In particular, its default column names have changed.
+# We're changing the cryptographic hash algorithm
+# (to its new default argon2id),
+# so there's no need to keep the old hash values.
+# Instead, we destroy the old columns & blind indexes,
+# create the new ones, and backfill the data (a forced recalculation).
+# The backfill takes some time, but it's a one-time operation.
+#
+# To see how we used to do this, see:
+# db/migrate/20180525145445_add_encrypted_email_to_users.rb
+# db/migrate/20161102170815_change_email_column_type.rb
+
+class BlindIndex22 < ActiveRecord::Migration[6.1]
+  def change
+    rename_column :users, :encrypted_email_bidx, :email_bidx
+
+    # This rename is done automatically by rename_column:
+    # rename_index :users, 'index_users_on_encrypted_email_bidx',
+    #              'index_users_on_email_bidx'
+
+    rename_index :users, 'encrypted_email_local_unique_bidx',
+                 'email_local_unique_bidx'
+
+    # Backfill (recompute) the blind index. This is relatively quick;
+    # one test took 1m13s for 8999 users.
+    reversible do |dir|
+      # Update the blind index value "by hand", as
+      # this doesn't update it properly: BlindIndex.backfill(User)
+      User.find_each(batch_size: 400) do |user|
+        # puts(user.id)
+        if user.email.present?
+          user.email_bidx = User.generate_email_bidx(user.email)
+          # Do NOT change the changed timestamp, as this encryption change
+          # is an internal issue. Also, disable validations (e.g., blank names)
+          # since while that's not desirable we need to do the transition
+          # for all records, even less-than-perfect ones.
+          user.save!(touch: false, validate: false)
+        end
+      end
+    end
+  end
+end

db/schema.rb

@@ -2,15 +2,15 @@
 # of editing this file, please use the migrations feature of Active Record to
 # incrementally modify your database, and then regenerate this schema definition.
 #
-# This file is the source Rails uses to define your schema when running `rails
-# db:schema:load`. When creating a new database, `rails db:schema:load` tends to
+# This file is the source Rails uses to define your schema when running `bin/rails
+# db:schema:load`. When creating a new database, `bin/rails db:schema:load` tends to
 # be faster and is potentially less error prone than running all of your
 # migrations from scratch. Old migrations may fail to apply correctly if those
 # migrations use external dependencies or application code.
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_12_22_014804) do
+ActiveRecord::Schema.define(version: 2021_02_16_184638) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
@@ -417,11 +417,11 @@
     t.datetime "last_login_at"
     t.string "encrypted_email"
     t.string "encrypted_email_iv"
-    t.string "encrypted_email_bidx"
+    t.string "email_bidx"
     t.boolean "use_gravatar", default: false, null: false
     t.datetime "can_login_starting_at"
-    t.index ["encrypted_email_bidx"], name: "encrypted_email_local_unique_bidx", unique: true, where: "((provider)::text = 'local'::text)"
-    t.index ["encrypted_email_bidx"], name: "index_users_on_encrypted_email_bidx"
+    t.index ["email_bidx"], name: "email_local_unique_bidx", unique: true, where: "((provider)::text = 'local'::text)"
+    t.index ["email_bidx"], name: "index_users_on_email_bidx"
     t.index ["last_login_at"], name: "index_users_on_last_login_at"
     t.index ["uid"], name: "index_users_on_uid"
   end

test/fixtures/users.yml

@@ -5,24 +5,38 @@
 # on the general principle that we should vary test values;
 # they don't have any particular meaning.  Make sure they match the tests!
 #
-# WARNING: The encrypted_email_bidx *MUST* end in newline, because that
-# is the format created (and required) by the gem blind_index.
-# The encrypted_email also ends in newline, but that's just because the
+# Note: The encrypted_email ends in newline, but that's just because the
 # gem attr_encrypted does that (it's not actually required).
+# At one time the encrypted_email_bidx *MUST* end in newline, because that
+# is the format created (and required) by the gem blind_index, but that is
+# no longer true.
 #
-# We encrypt user email addresses. For test purposes, we used a fixed
+# User email addresses are encrypted in the database, so for proper
+# testing we must store them as encrypted data.
+# For test purposes, we used a fixed
 # encryption IV (don't do this with real data), and provide precomputed
-# values for "encrypted_email" and "encrypted_email_bidx".  There's
-# lot of infrastructure that has to work to compute these values, and they
-# take time to compute, so providing precomputed values makes sense.
-# To find values for new email addresses, run "rails console" and then:
+# values for "encrypted_email" and "email_bidx".
+# We *must* do this for email_bidx, at least, because the fixture code is
+# loaded and run as an ERB before its keys are fully set up.
+# That's okay; there's lot of infrastructure
+# that has to work correctly to compute these values,
+# and they take time to compute, so providing their fixtures as
+# known-correct precomputed values makes sense.
+# (It means that tests are more independent of the system under test, and
+# thus failures are more likely to help point out the actual problems.)
+#
+# To find values for new email addresses, run "rails console"
+# (make sure to *not* set 'EMAIL_ENCRYPTION_KEY' nor 'EMAIL_BLIND_INDEX_KEY')
+# and then do something like this:
+# email='[email protected]'
 # encrypted_email_iv: "G4fbeClWL3aFMY9I"
-# encrypted_email:
-#   User.encrypt_email('[email protected]',
-#                       iv: Base64.decode64('G4fbeClWL3aFMY9I'))
-# encrypted_email_bidx:
+# encrypted_email: # Find its value with:
+#   User.encrypt_email(email, iv: Base64.decode64('G4fbeClWL3aFMY9I'))
+# email_bidx: # Find its value with:
+#   User.generate_email_bidx(email)
+# Another (longer) way to compute email_bidx is to run:
 #   BlindIndex.generate_bidx("[email protected]", User.blind_indexes[:email])
-# An alternative approach is to create a stub user:
+# You can create a stub user and find its email_bidx this way:
 # x = User.new; x.email = '[email protected]'; x.compute_email_bidx
 # This assumes that you're using the usual (default) test keys.
 #
@@ -32,41 +46,32 @@
 #      iv: Base64.decode64('G4fbeClWL3aFMY9I')).inspect)
 # puts('  encrypted_email_bidx: ' + BlindIndex.generate_bidx(
 #       current_email, User.blind_indexes[:email]).inspect)
-#
-# Here are some additional encryptions if you need them:
-# current_email = '[email protected]'
-# encrypted_email_iv: "G4fbeClWL3aFMY9I"
-# encrypted_email: "0LWldZP1iahMHmhaQwjU9g27hpaNo56iCfMNh+ZsERg=\n"
-# encrypted_email_bidx: "C/0XJyW8Xitx19MPv1j8B65iNOaykMLZnMZ/TctNp3U=\n"
-#
-# current_email = '[email protected]'
-# encrypted_email_iv: "G4fbeClWL3aFMY9I"
-# encrypted_email: "27ulebbkl7xNLmFHDArW/ZzHIFjaYveE2hxJl2Kxdk2/rm4xXw==\n"
-# encrypted_email_bidx: "0SHo5Lc+XTrcY6HvouDwAIvEdy+8odILw9DP3MGXMfM=\n"
-#
-# current_email = '[email protected]'
-# encrypted_email: "3LC6d73QlLFAA3RTCEnJ455qaUF28kYI7ZOGoIDUhkzm\n"
-# encrypted_email_iv: "G4fbeClWL3aFMY9I"
-# encrypted_email_bidx: "yspd41bZJvT9bn0MQXU2l2dnhpZTIKpf0u46w6+qf1U=\n"
 
 # current_email = '[email protected]'
 test_user:
   name: Test
+  # This is our conventional IV value
   encrypted_email_iv: "G4fbeClWL3aFMY9I"
+  # encrypted_email: (%= User.encrypt_email('[email protected]',
+  #                      iv: Base64.decode64('G4fbeClWL3aFMY9I')).inspect %)
   encrypted_email: "ybGkapP1iahMHmhaQwjU9qTfGB7w/KCFTiUGkIVZZ08=\n"
-  encrypted_email_bidx: "hEghi+qNCA63qDAuTIq8jBWtOweoQ33EstwMnvM3/gk=\n"
+  # email_bidx: (%= User.generate_email_bidx('[email protected]').inspect %)
+  email_bidx: "vmUX8sb4/akruZRPDwQ7e2bZHiAcqxO2BS7u2vdg/Cs="
   password_digest: <%= User.digest('password') %>
   provider: local
   preferred_locale: en
   activated: true
   activated_at: <%= Time.zone.now %>
 
 # current_email = '[email protected]'
+# User.encrypt_email('[email protected]',
+#                     iv: Base64.decode64('G4fbeClWL3aFMY9I'))
 test_user_melissa:
   name: Melissa Lewis
   encrypted_email_iv: "G4fbeClWL3aFMY9I"
   encrypted_email: "0LG7d6DjkIlEFmVSHQvDv5qGImt+oNOl9LLibPvLAwHeWmo=\n"
-  encrypted_email_bidx: "ip6E/EM+EYkbzsRm6ZTprRg4PTsN/T5xY9E80U3Hlfo=\n"
+  # email_bidx: (%= User.generate_email_bidx('[email protected]').inspect %)
+  email_bidx: "gkyruOK5LMxyHFElzYO9YtdKJBrIJZnkDamwU6M1+Jc="
   password_digest: <%= User.digest('password1') %>
   provider: local
   preferred_locale: en
@@ -78,7 +83,8 @@ test_user_mark:
   name: Mark Watney
   encrypted_email_iv: "G4fbeClWL3aFMY9I"
   encrypted_email: "0LWldZP1iahMHmhaQwTJ/JxzMJME5stBTXCm7WcmYyg=\n"
-  encrypted_email_bidx: "aZpkg4oEr8KjacLrZINGN1JRGD1caIWoWUTM57KOePk=\n"
+  # email_bidx: (%= User.generate_email_bidx('[email protected]').inspect %)
+  email_bidx: "PM4RbQebE5i3FU4Zm/+cNT6P0POakuR8Fz9QzP7L01U="
   password_digest: <%= User.digest('password') %>
   provider: local
   preferred_locale: en
@@ -90,7 +96,8 @@ test_user_not_active:
   name: Forgetful
   encrypted_email_iv: "G4fbeClWL3aFMY9I"
   encrypted_email: "27ulebbkl7xNLmFHDArW/ZzHLEXQQHbOqCyJ9yfVGoZn2ok7ew==\n"
-  encrypted_email_bidx: "nRtJqO5uutJ7Z/HcjLHM/jt3cA++pN1t85URKscQfsQ=\n"
+  # email_bidx: (%= User.generate_email_bidx('[email protected]').inspect %)
+  email_bidx: "y5UiQko516f50w/YRAZAuW7FzzpWKmvg1Yzd44ry+JQ="
   password_digest: <%= User.digest('password') %>
   provider: local
   preferred_locale: en
@@ -101,7 +108,8 @@ test_user_cs:
   name: Case Sensitive
   encrypted_email_iv: "G4fbeClWL3aFMY9I"
   encrypted_email: "/rWke4D1n7pIGm1JCCfD6ZiEP0bY51bxymIp4AvOLZGyoX8ln1EALDc=\n"
-  encrypted_email_bidx: "+rRkCLpeIVcfwtPu+Jy6YyXtksI/w3frgdSu8J4kR2M=\n"
+  # email_bidx: (%= User.generate_email_bidx('[email protected]'.downcase).inspect %)
+  email_bidx: "I5hCRmgHRw0+XSK6TMfTRk/vhkbX+2cXUJzaiWTVqcs="
   password_digest: <%= User.digest('password') %>
   provider: local
   preferred_locale: en
@@ -113,7 +121,8 @@ admin_user:
   name: Admin
   encrypted_email_iv: "G4fbeClWL3aFMY9I"
   encrypted_email: "3LC6d73QlLFAA3RTCEnF/pQPuTWBDzfZWpDX1TJ+bsBO\n"
-  encrypted_email_bidx: "PH39U8dihphvLTlLEwJWXdy2OLz477WOn/DNsZaHqfE=\n"
+  # email_bidx: (%= User.generate_email_bidx('[email protected]').inspect %)
+  email_bidx: "pdszbZsBfJRl+Fg0ZZ7/Mwguy64JlXvJ7TflzSEIawM="
   password_digest: <%= User.digest('password') %>
   provider: local
   preferred_locale: en
@@ -128,7 +137,8 @@ github_user:
   nickname: github-user
   encrypted_email_iv: "G4fbeClWL3aFMY9I"
   encrypted_email: "2r2jdqby3LxSC3Z/CB/H/ImFKgTeplS8IDY+qfw8Fi73KhLn8MYC\n"
-  encrypted_email_bidx: "Vyiu7EdbDXioCgMAVIWqLHyBH148S9JcSspSWq6nPN8=\n"
+  # email_bidx: (%= User.generate_email_bidx('[email protected]').inspect %)
+  email_bidx: "ZjENXwF1sT4lGPhlXJMGGojv/NgxQvRgis647Pgotwg="
   password_digest: <%= User.digest('password') %>
   provider: github
   preferred_locale: en
@@ -140,7 +150,8 @@ fr_user:
   name: French Test
   encrypted_email: "26b6arbjhYlEFmVSHQvDv5abKOzHp6ZN4HPtuqnxJkbq84I=\n"
   encrypted_email_iv: "G4fbeClWL3aFMY9I"
-  encrypted_email_bidx: "7e3Bzzf0T1HnQKICsDEqJGosonRFiZZNtN445wgNDYU=\n"
+  # email_bidx: (%= User.generate_email_bidx('[email protected]').inspect %)
+  email_bidx: "iZlt4P3IjjuMUuajvkGqwatHglhDxoNCUsYMgebOEOQ="
   password_digest: <%= User.digest('password') %>
   provider: local
   preferred_locale: fr

test/models/user_test.rb

@@ -107,7 +107,7 @@ class UserTest < ActiveSupport::TestCase
     new_blind_index = BlindIndex.generate_bidx( # Recalc so test's less fragile
       email_address, User.blind_indexes[:email]
     )
-    assert new_blind_index, user1.encrypted_email_bidx
+    assert new_blind_index, user1.email_bidx
   end
 
   test 'associated projects should be destroyed' do
@@ -152,5 +152,30 @@ def email
     u = StubUserEmail.new
     assert_equal 'CANNOT_DECRYPT', u.email_if_decryptable
   end
+
+  test 'Data model encrypted email addresses and blind index keys work' do
+    # We precompute the user data fixtures, and it's possible we got it wrong.
+    # Walk through the data set to do sanity checks for each value.
+    User.find_each do |user|
+      # puts(user.name)
+      assert user.name.present?, "Empty name for #{user.id}"
+      assert user.encrypted_email.present?, "Email not present for #{user.name}"
+      assert(
+        user.encrypted_email_iv.present?,
+        "Email IV not present for #{user.name}"
+      )
+      # This will also fail if the email is not encrypted correctly:
+      assert user.email.present?, "Email not present for #{user.name}"
+      assert user.provider.present?, "Provider not present for #{user.name}"
+      assert(
+        (user.provider != 'local' || user.password_digest.present?),
+        "Local user has no password: #{user.name}, #{user.email}"
+      )
+      # An incorrect bidx could lead to confusing test results, so we
+      # *definitely* want the following check.
+      # Check that bidx was computed correctly:
+      assert_equal User.generate_email_bidx(user.email), user.email_bidx
+    end
+  end
 end
 # rubocop:enable Metrics/ClassLength