Fix migration failure due to config name change

[pkarman]

Why: Two Figaro environment variables changed names,
and a related class name also changed.

How: Add fallback config names and a class definition
for the old class name.

[monfresh]

This alone will not replace what I was attempting to do with the ConfigValidator checks. If the attribute_encryption_key does not match the email_encryption_key on the server, existing users will not be able to sign in. We need to make sure that when application.yml gets updated on the server, whether manually, or by Chef, the following conditions hold true:

1. email_encryption_key must not be modified from its previous value
2. attribute_encryption_key must match email_encryption_key

Also, I think that if the ReencryptEmailWithSalt fails, the migration should fail and prevent subsequent migrations from running, to indicate that there was a problem, i.e. we should not be rescuing Pii::EncryptionError. Thoughts?

[zachmargolis]

After this is merged + deployed to demo, should we squash all the DB migrations? The longer we leave files that reference old things around, the more painful it will get IMO

[pkarman]

@monfresh wrote:

We need to make sure that when application.yml gets updated on the server, whether manually, or by Chef, the following conditions hold true:

• email_encryption_key must not be modified from its previous value
• attribute_encryption_key must match email_encryption_key

I disagree with the first point. email_encryption_key is no longer supported or used, except by this migration, and now it is not needed by this migration at all.

As to the second point, if whomever does the migration forgets to remove email_encryption_key from the relevant application.yml file, then yes, it must be the same as the new attribute_encryption_key.

I can remove the raise/rescue. The reason it is there will not apply here (it's for problems with decryption, which typically happen because of point two above).

[pkarman]

@zachmargolis by "squash all the DB migrations" do you mean remove the files from the repo? I'm fine with that, after we've deployed to all existing environments.

[pkarman]

I have removed references to email_encryption_key completely since it does not exist in master. This is a case where incremental db changes in separate migrations was probably more trouble than it was worth.

[zachmargolis]

@pkarman yeah basically that -- by squash I mean we take the content of schema.rb and replace the most recent migration with it, and remove all the other migrations, that way we can easily get to our current structure and not have to worry about all the intermediate steps

[monfresh]

Should old_uak be new_uak? When this method is called below, new_uak is being passed as an argument.

[monfresh]

Also, do we even need to define this here? Can we not call EncryptedAttribute#new_from_decrypted directly?

[pkarman]

By stubbing out EncryptedEmail I was trying to follow the same pattern with ActiveRecord with stubbing models in case table names ever change.

But you're right; we don't need it. I have changed in 75445b0

[monfresh]

Agreed that we shouldn't rely on the presence of email_encryption_key. However, we must ensure that attribute_encryption_key must be set to the value of email_encryption_key before email_encryption_key is removed. I suppose I could ssh into the servers now and add attribute_encryption_key now and make it match email_encryption_key.

Can you think of another way to ensure that this step is done before deployment?

[monfresh]

In other words, if we were live in production, and we had to make sure we didn't screw this up and prevent all users from ever signing back in again, what would we do?

[pkarman]

In other words, if we were live in production, and we had to make sure we didn't screw this up and prevent all users from ever signing back in again, what would we do?

If the encryption key is not set correctly, the email can't be decrypted during the migration, so the migration will fail. Previously it was skipped. Now the migration won't proceed.

[pkarman]

Can you think of another way to ensure that this step is done before deployment?

As long as we are manually managing application.yml files, I assume there will need to be manual steps taken before deployments. So I consider this a step to take before deploying to a particular env. Perhaps we need some kind of release wiki page with TODO steps?

[monfresh]

lgtm