Skip to content

LG-6160: Validate personal key value as case-insensitive, Crockford base32#6240

Merged
aduth merged 6 commits intomainfrom
aduth-personal-key-input-case-insensitive
Apr 22, 2022
Merged

LG-6160: Validate personal key value as case-insensitive, Crockford base32#6240
aduth merged 6 commits intomainfrom
aduth-personal-key-input-case-insensitive

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 22, 2022

Extracted from #6229

Why: So that a user is not prevented from submitting the personal key confirmation step due to case sensitivity, for feature parity with the existing screen.

changelog: Upcoming Features, Identity Verification, Add personal key step screen
Copy link
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

const input = getByRole('textbox') as HTMLInputElement;

await userEvent.type(input, '0000-0000-0000-000');
await userEvent.type(input, 'ABCD-0000-defg-000');
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have a second test where the values.toLowercase() match and we expect not to see an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you share a snippet of the test case you have in mind?

The idea with mixed case here and in the expectedValue above is to try to cover both upper- and lower- variations in either the expected value or the given value.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh one more thought! the keys are base32-crockford encoded, so that they fold 0 and o together, 1 and i, when we compare on the server we normalize them. We should probably do the same thing here while we're fixing

Copy link
Contributor

Choose a reason for hiding this comment

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

link to source:

def verify(plaintext_code)
user.valid_personal_key?(normalize(plaintext_code))
end
def normalize(plaintext_code)
normed = plaintext_code.gsub(/\W/, '')
split_length = RandomPhrase::WORD_LENGTH
normed_length = normed.length
return INVALID_CODE unless normed_length == personal_key_length * split_length
encode_code(code: normed, length: normed_length, split: split_length)
rescue ArgumentError, RegexpError
INVALID_CODE
end
private
attr_reader :user
def encode_code(code:, length:, split:)
decoded = Base32::Crockford.decode(code)
Base32::Crockford.encode(decoded, length: length, split: split).tr('-', ' ')
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call @zachmargolis , I'll add that. I guess we don't have any feature specs covering that? (That's what originally prompted me to create this pull request)

I also forgot we had created a package for that purpose. Ideally we could fold that in here, but since we still have to maintain the old page, we might have to keep it around for a while in its current form.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we don't have any feature specs covering that?

I'm not seeing any feature specs... but we do have unit test coverage of it:

it 'forgives user mistaking O for 0' do
expect(generator.verify(personal_key.tr('0', 'o'))).to eq true
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enhanced feature spec coverage in d4599a5. Will implement it here shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented in a6404d6. I could have pulled in the library we used in @18f/identity-personal-key-input, but it kinda seemed a bit overkill?

@aduth aduth changed the title LG-6160: Validate personal key value as case-insensitive LG-6160: Validate personal key value as case-insensitive, Crockford base32 Apr 22, 2022
*
* @return Normalized value.
*/
const normalize = (string: string) => string.toLowerCase().replace(/o/g, '0').replace(/[il]/g, '1');
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it's silly to have a whole library that we can replicate with two regexes, this seems easy enough to maintain 👍

@aduth aduth merged commit 6c7f3be into main Apr 22, 2022
@aduth aduth deleted the aduth-personal-key-input-case-insensitive branch April 22, 2022 18:48
@jmdembe jmdembe mentioned this pull request Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants