Conversation
**Why**: Personal key has been changed to behave like password, where it is nil until a key is assigned to the user during a request. This commit checks the digest instead of the personal key method to determine if a user can sign in with a personal key.
jmhooper
requested review from
davemcorwin,
jgsmith-usds,
monfresh,
mryenq and
stevegsa
|
|
||
| def personal_key | ||
| @personal_key | ||
| end |
There was a problem hiding this comment.
What do you think about making this point to encrypted_recovery_code_digest? That way, we can still keep the old calls to user.personal_key, which matches what we call it on the site, and is clearer than encrypted_recovery_code_digest.
There was a problem hiding this comment.
This is in the revert commit which is code that was already merged.
But, I didn't do that because we did a touch of refactoring here to make the personal_key and personal_key= methods work like the password and password= methods since that is how we actually use it.
| context 'when email is valid and user exists' do | ||
| it 'returns hash with properties about the event and the user' do | ||
| user = build(:user, :signed_up, email: 'test1@test.com') | ||
| user = create(:user, :signed_up, email: 'test1@test.com') |
There was a problem hiding this comment.
Why does this require creating vs building?
There was a problem hiding this comment.
The the test is testing the behavior when a user exists. The app does not know the user exists without creating them. This is a bug that was uncovered when changing the personal key behavior. Previously, setting the personal key (which is done in the factory here) was saving the record.
There was a problem hiding this comment.
Are you sure? This test was working fine before. It knows that the user exists because we are building a user with the same email address as the one that gets passed into the form in the line below this one. If you change the user's email address on line 12, the test will fail.
There was a problem hiding this comment.
Ah. I see what you mean. The factory was saving the user, so it was acting as create instead of build. I think the fix is to stub User.find_with_email so that it returns a user. We want to minimize accessing the DB in specs. The user doesn't need a personal key in this spec.
There was a problem hiding this comment.
Anyways, we can do this later since it's not specific to this PR.
2 participants
This branch has 2 commits:
Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:
For DB changes, check for missing indexes, check to see if the changes
affect other apps (such as the dashboard), make sure the DB columns in the
various environments are properly populated, coordinate with devops, plan
migrations in separate steps.
For route changes, make sure GET requests don't change state or result in
destructive behavior. GET requests should only result in information being
read, not written.
For encryption changes, make sure it is compatible with data that was
encrypted with the old code.
For secrets changes, make sure to update the S3 secrets bucket with the
new configs in all environments.
Do not disable Rubocop or Reek offenses unless you are absolutely sure
they are false positives. If you're not sure how to fix the offense, please
ask a teammate.
When reading data, write tests for nil values, empty strings,
and invalid formats.
When calling
redirect_toin a controller, use_url, not_path.When adding user data to the session, use the
user_sessionhelperinstead of the
sessionhelper so the data does not persist beyond the user'ssession.
When adding a new controller that requires the user to be fully
authenticated, make sure to add
before_action :confirm_two_factor_authenticated.