Fix double personal key encryption when using personal key as second factor

[mitchellhenke]

🛠 Summary of changes

In looking at some of the personal key 2FA usage, I found that we seem to be doubling the encryption and associated User database update after successful usage.

In the controller, we call:

      current_user.personal_key = PersonalKeyGenerator.new(current_user).create
      current_user.save!

The personal key assignment in the PersonalKeyGenerator has the same method calls. The issue is calling personal_key= encrypts the personal key, and we're encrypting the same value twice (for both KMS keys).

Looking at a trace of a successful verification in NewRelic shows create_digest_pair being called twice which confirms this.

The included changes remove the personal key assignment and save operations. We do have tests covering this behavior which should fail if we are not regenerating the code properly and I've added a couple extra assertions to it.

[zachmargolis]

would it be clearer if we flipped this? like removed the user.personal_key = assignment from from inside the PersonalKeyGenerator?

Or maybe we rename the method from #create to like #assign! so it's clear there's that side effect?

[mitchellhenke]

The hurdle is there are a handful of tests that rely on the behavior (example).

#create definitely doesn't tell the whole story, so a rename makes sense to me. Something like encrypt_and_save!?

[zachmargolis]

It's just tests? They already have the user handy so they could do:

user.personal_key = PersonalKeyGenerator.new(...).create
user.save!

[mitchellhenke]

It was closer to 20 and I think I lean towards keeping that behavior in the class. If we did remove the assignment/save, we'd probably want to refactor a bit more since the user being passed in would only be used for personal key verification (which maybe shouldn't be in this class based on the name?).

I've renamed the method #generate! to try to make the existence of side effects clearer.