Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add use two otp options on create wallet #65

Merged
merged 30 commits into from
Aug 5, 2021

Conversation

haolinj
Copy link
Collaborator

@haolinj haolinj commented Aug 4, 2021

This is working in progress as part of client-security branch.

// otpauth://TYPE/LABEL?PARAMETERS
return `otpauth://totp/${name}?secret=${b32.encode(seed)}&issuer=Harmony`
return `otpauth://totp/${name}?secret=${b32.encode(otpSeed)}&issuer=Harmony`
Copy link
Owner

Choose a reason for hiding this comment

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

Need to use a slightly different name here. e.g. instead of ${name}, use ${name} (2nd)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the name is the same, but append (2nd)?

Copy link
Owner

Choose a reason for hiding this comment

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

yes

} else if (doubleOtp && !settingUpSecondOtp) {
setSection(sectionViews.setupSecondOtp)

setName(generateNewOtpName())
Copy link
Owner

Choose a reason for hiding this comment

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

should not generate a new name. The name is the (local) name of the wallet to help the user identify the wallet. It should be the same for both OTP entries on the authenticator, and the same name should be stored in redux state

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean the name is the same, but the second one has (2nd) appended?

Copy link
Owner

Choose a reason for hiding this comment

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

yes

@@ -232,6 +272,34 @@ const Create = () => {
</Row>
</Space>
</Row>
<Row justify='center'>
<Checkbox onChange={() => setDoubleOtp(!doubleOtp)}>
<Hint>Use two One Time Passwords for enhanced security</Hint>
Copy link
Owner

Choose a reason for hiding this comment

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

Should let user know what to expect - i.e. they will need to scan again on the next page. Otherwise they tick the box and nothing happens

} else if (doubleOtp && !settingUpSecondOtp) {
setSection(sectionViews.setupSecondOtp)

setName(`${name} (2nd)`)
Copy link
Owner

Choose a reason for hiding this comment

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

It would be incorrect to change the name state variable because the wallet would be saved as ${name} (2nd) in redux

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. So we still want to save the name as original name in the state, but the authenticator would have a different name for the second OTP (that is only append (2nd) text).

const resetOtp = () => {
setOtpInput('')
setOtp2Input('')
Copy link
Owner

Choose a reason for hiding this comment

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

otp2Ref is missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we are focusing on OTP 1 input after reset, we don't focus on the OTP 2 input I suppose.

Copy link
Owner

Choose a reason for hiding this comment

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

you are right

Copy link
Owner

Choose a reason for hiding this comment

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

we should auto focus to second box, after first box has 6 digits

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Will add that.

@polymorpher polymorpher changed the title WIP: Add use two otp options on create wallet Add use two otp options on create wallet Aug 5, 2021
@polymorpher
Copy link
Owner

Everything works - very good job

@polymorpher polymorpher merged commit 6cb198c into client-security Aug 5, 2021
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.

None yet

2 participants