Skip to content

Catch error when recovery code cannot decrypt PII#1000

Merged
pkarman merged 1 commit intomasterfrom
pek-catch-recovery-code-error
Jan 27, 2017
Merged

Catch error when recovery code cannot decrypt PII#1000
pkarman merged 1 commit intomasterfrom
pek-catch-recovery-code-error

Conversation

@pkarman
Copy link
Copy Markdown
Contributor

@pkarman pkarman commented Jan 26, 2017

Why: Generating a new recovery code will render the de-activated
profile useless.

How: Catch the exception thrown when a valid recovery
code cannot be used to decrypt the PII, and prevent users from
unintentionally creating a new recovery code when they have a
de-activated profile.

TODO: Some messaging needed to allow the user to definitively say "No, I don't have my recovery code and I'm sorry but I'll need to re-verify my account."

screen shot 2017-01-26 at 4 23 54 pm

@pkarman pkarman self-assigned this Jan 26, 2017
@pkarman pkarman force-pushed the pek-catch-recovery-code-error branch 2 times, most recently from 99b395b to 6222d28 Compare January 27, 2017 03:32
@zachmargolis
Copy link
Copy Markdown
Contributor

@pkarman I think the plan is to put the "no I don't have my recovery code" link on the reactivation page?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this class isn't used for styling, maybe we could prefix it with something like spec- so it's clear what it's being used for? We currently do something similar for js- when using classes as javascript selectors. This class could probably also just be added to the p element instead of adding a new span.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit reluctant to add to the p because the selection logic seemed to insist on matching all the class attributes, and I was only interested in the one that had semantic meaning.

Happy to prefix with spec- -- though I wonder how @el-mapache is accessing these words in his modal work? Maybe there's a need to be able to pull out words live as well as in tests?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkarman In the modal, I'm passing the code into a js.erb file. If I was using selectors I would probably make a data attribute, data-key-word or something along those lines.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can/should we make a helper method for this?

Also would you prefer .all(:css, '.recovery-code-word')? (I find CSS selectors much more readable than xpath)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I like :css better -- I'll try that. thanks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just add the rescue to valid_recovery_code??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep the rescue as close to where the exception would be thrown as possible, so it was clear to whomever read it 6 mos from now why there's a rescue at all.

The code can be valid and still not be able to decrypt the PII. In fact, that's exactly the use case here. The code supplied is valid as a recovery code, but not as a decryption key.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, for some reason I thought that RecoveryCodeGenerator#verify actually tried decrypting, but it just compares the recovery code to its hash on the profile. This makes more sense now

@pkarman pkarman force-pushed the pek-catch-recovery-code-error branch from 6222d28 to ee6830e Compare January 27, 2017 18:44
@pkarman
Copy link
Copy Markdown
Contributor Author

pkarman commented Jan 27, 2017

now using CSS and data-recovery attribute. PTAL

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

**Why**: Generating a new recovery code will render the de-activated
profile useless.

**How**: Catch the exception thrown when a valid recovery
code cannot be used to decrypt the PII, and prevent users from
unintentionally creating a new recovery code when they have a
de-activated profile.
@pkarman pkarman force-pushed the pek-catch-recovery-code-error branch from ee6830e to ebd3ebf Compare January 27, 2017 19:00
@pkarman pkarman merged commit deb8b6b into master Jan 27, 2017
@pkarman pkarman deleted the pek-catch-recovery-code-error branch January 27, 2017 19:33
amoose pushed a commit that referenced this pull request Mar 7, 2017
**Why**: Generating a new recovery code will render the de-activated
profile useless.

**How**: Catch the exception thrown when a valid recovery
code cannot be used to decrypt the PII, and prevent users from
unintentionally creating a new recovery code when they have a
de-activated profile.
amoose pushed a commit that referenced this pull request Mar 8, 2017
**Why**: Generating a new recovery code will render the de-activated
profile useless.

**How**: Catch the exception thrown when a valid recovery
code cannot be used to decrypt the PII, and prevent users from
unintentionally creating a new recovery code when they have a
de-activated profile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants