-
Notifications
You must be signed in to change notification settings - Fork 14
Wallet local storage with spending password #110
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
base: master
Are you sure you want to change the base?
Wallet local storage with spending password #110
Conversation
52c1e29
to
3095c6c
Compare
3095c6c
to
a001d53
Compare
dfb5e1f
to
85eb7e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep in mind this is not a spending password , but a wallet lock instead. I'm behind this approach since it gives us pretty reasonable privacy/security, while being much easier to implement (it doesn't have to be plugged into every use of the keys), but it's important that we're all aware of the differences.
Please check that it's necessary to save the password's hash. If that were the case, I think proper use of password salt would make rainbow table attacks (which would be pretty difficult, since passwords would not be stored in a central location) a bit more difficult.
export function setAccount(privateKey: string): Thunk<SetKeysAction> { | ||
export function setAccount( | ||
privateKey: string, | ||
spendingPassword: ?string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's confusing to have a variable named the same as a type, and have it be of a different type 🙃
const accountKeys = readAccountKeysFromDEN(spendingPassword); | ||
if (accountKeys) { | ||
const spendingPasswordKeys = { | ||
walletId: 'wallet01', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary. I added it in case I could create multiple wallets as in other wallets, for example Daedalus.
.then(() => dispatch(push(routes.WALLET))) | ||
.catch(error => { | ||
console.log(error); | ||
dispatch(push(routes.INPUT_KEYS)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about adding (as documentation) a state diagram showing what triggers redirects and to where? do you think it would be overkill? should it be apparent in the code and we're making it hard to follow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea since community developers will understand the source code more easily. I will add it
dispatch(push(routes.INPUT_KEYS)); | ||
}); | ||
}) | ||
.catch(error => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from what I understand, if there's an error updating the transactions, getting node settings or updating the account state, we'd redirect to the walet page.
But if there's an error redirecting to the wallet, then we'll redirect to the input keys screen.
And if there's an error in ^, then we log the error.
I don't think this is intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected
return getAccountFromSeed(seed).then( | ||
curry(initializeKeysAndRedirect)(dispatch) | ||
return getAccountFromSeed(seed).then(keys => | ||
curry(initializeKeysAndRedirect)(dispatch, keys, spendingPassword) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's always used like this, consider swapping the last two parameters and doing:
.then(curry(initializeKeysAndRedirect)(dispatch, password))
If not, you could curry with a placeholder:
.then(curry(initializeKeysAndRedirect)(dispatch, _,password))
Please see the documentation to see what the _
should be, without importing the whole library.
Creating a function inline makes currying redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is more readable to use all the parameters. Will remain incurry (initializeKeysAndRedirect) (dispatch, keys, unlockWalletPassword)
// FIXME: this has no error handling, neither while parsing the address | ||
// nor when fetching the balance. | ||
export default ({ setAccount }: Props) => { | ||
const handleSubmitCreateSpending = function handleSubmitCreateSpending( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this name is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function was renamed to handleSubmit
); | ||
const [hiddenSpendingPassword, setHiddenSpendingPassword] = useState(false); | ||
|
||
const [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would'nt bother creating a new state variable for something which can be easily computed from existing ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this issue persists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hiddenSpendingPassword and setHiddenSpendingPassword does not exists
} | ||
} | ||
if (isValidMnemonic(newMnemonicPhrase)) { | ||
return Promise.all([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use Promise.all for a single promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise.all is deleted
} | ||
if (isValidMnemonic(newMnemonicPhrase)) { | ||
return Promise.all([ | ||
setAccountFromMnemonic(newMnemonicPhrase, newMnemonicPassword, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment about optional parameters applies here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
|
||
const [newMnemonicPassword, setNewMnemonicPassword] = useState(''); | ||
|
||
const checkValidSpendingPassword = function checkValidSpendingPassword( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is duplicated code from the other restoreWallet component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
4b2269c
to
d63b730
Compare
e741a49
to
c8ae5ed
Compare
onChange={event => setNewMnemonicPhrase(event.target.value)} | ||
onBlur={() => checkIsValidMnemonicPhrase()} | ||
/> | ||
<Form.Label className="text-danger" hidden={isMnemonicValid}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Form.Control.Feedback for this.
Also, research the props of that component to see if you can style it by passing a prop (some have a plaintext
prop)
If that's not possible, consider using <span>
, which is a more idiomatic way to apply styles to inline text, without implying a semantic distinction (<em>
is used for emphasis)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
span is used instead em
c8ae5ed
to
a7360a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a clear improvement, but I think we could have a component to input a password and its confirmation, because it's implemented differently in both screens where it's required.
Also, there are some informative labels which could be implemented better:
First of all, labels should be the, well, label, of an input. React-bootstrap accomplishes this with its Form.Group
component, which should have an input (Form.Control
) and corresponding label (Form.Label
).
React-bootstrap has an extra Form.Control.Feedback
which behaves automagically (you only have to tell it if it's for negative or positive feedback, and it will be rendered depending on the value of the isInvalid
/isValid
prop for the related input, respectively), which you should use for form feedback.
Regarding informative fields, they should not be red, since this implies they are errors, which they're not. And making them <em>
s implies they have some emphasis and displays them as italics.
isInvalid={!isValidPassword} | ||
onChange={event => setPassword(event.target.value)} | ||
/> | ||
<Form.Label º hidden={isValidPassword}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the º
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was an accident using keyboard. Corrected
onChange={event => setConfirmPassword(event.target.value)} | ||
className="mt-3" | ||
/> | ||
<Form.Label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment about Form.Control.Feedback applies here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use it on all input validation.
return function setKeysWithUnlockWalletPasswordThunk(dispatch) { | ||
const accountKeys = readEncryptedAccountInfo(unlockWalletPassword); | ||
if (accountKeys) { | ||
return getAccountFromPrivateKey(accountKeys.privateKey).then(keys => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary to generate the AccountKeys object again, since the AccountKeys object is stored already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wasn't addressed yet
dispatch(updateNodeSettings()), | ||
dispatch(updateAccountState()) | ||
]) | ||
.then(() => dispatch(push(routes.WALLET))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.then(() => dispatch(push(routes.WALLET)))
.catch(error => {
console.error(error);
dispatch(push(routes.WALLET));
});
could be written as
.catch(console.error);
.then(() => dispatch(push(routes.WALLET)))
which IMO is more idiomatic since you'd want to log the error and continue with the chain regardless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, this is repeated in the initializeKeysAndRedirect
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestion was applied
@@ -87,28 +104,37 @@ export function updateAccountTransactionsAndState(): Thunk<SetKeysAction> { | |||
const initializeKeysAndRedirect = ( | |||
dispatch: Dispatch, | |||
keys: AccountKeys, | |||
saveAccount?: boolean = true | |||
unlockWalletPassword: ?string = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the form, this should not be optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now mandatory
examples/wallet/app/utils/storage.js
Outdated
keys: AccountKeys | ||
): void { | ||
const plainTextAccountInfo = JSON.stringify(keys); | ||
const spedingPwd: string = unlockWalletPassword || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the password should not be optional. If we got here without a valid password, we should throw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now not optional
examples/wallet/app/utils/storage.js
Outdated
}; | ||
// eslint-disable-next-line flowtype/space-after-type-colon | ||
export function readEncryptedAccountInfo( | ||
unlockWalletPassword: ?string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't be optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not optional
@@ -37,6 +37,10 @@ export type AccountKeys = { | |||
identifier: Identifier | |||
}; | |||
|
|||
export type UnlockWalletPassword = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type is only used for a definition in the account actions, it could be defined inline there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted. Not a necessary type
export const SET_KEYS = 'SET_KEYS'; | ||
export const PRIVATE_KEY_ERROR = 'privateKeyError'; | ||
export const ACCOUNT_STATE_ERROR = 'accountStateError'; | ||
export const SET_UNLOCK_WALLET_PASSWORD = 'SET_UNLOCK_WALLET_PASSWORD'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reducer uses this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted
} | ||
setArePasswordAndConfirmationEqual(true); | ||
|
||
if (!/^[0-9a-zA-Z]{8,}$/.test(password)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just check the lenght, this makes all non-latin-script passwords invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation is deleted. Now there is a new component used to validate password only with lenght
35d376f
to
07131cd
Compare
…the functions to verify the keys to unlock the wallet with the encrypted record
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrections implemented
07131cd
to
86d49cc
Compare
return function setKeysWithUnlockWalletPasswordThunk(dispatch) { | ||
const accountKeys = readEncryptedAccountInfo(unlockWalletPassword); | ||
if (accountKeys) { | ||
return getAccountFromPrivateKey(accountKeys.privateKey).then(keys => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wasn't addressed yet
keys: AccountKeys, | ||
saveAccount?: boolean = true | ||
unlockWalletPassword: string = '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this hints the parameter is optional when it's not
return getAccountFromSeed(seed).then( | ||
curry(initializeKeysAndRedirect)(dispatch) | ||
return getAccountFromSeed(seed).then(keys => | ||
curry(initializeKeysAndRedirect)(dispatch, keys, unlockWalletPassword) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either go with:
.then(keys =>
initializeKeysAndRedirect(dispatch, keys, unlockWalletPassword)
);
or:
.then(
curry(initializeKeysAndRedirect)(dispatch, curry.placeholder, unlockWalletPassword)
);
or change the function signature to receive the keys last, and do:
.then(
curry(initializeKeysAndRedirect)(dispatch, unlockWalletPassword)
);
It's confusing to curry a function and execute it right away. It might lead a reader to infer the function receives even more parameters and'll be executed later on.
const [isValidPassword, setIsValidPassword] = useState(true); | ||
|
||
return ( | ||
<Form.Group> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why nest the form groups? there should be one group per input AFAIK: https://react-bootstrap.netlify.com/components/forms/
setValidCreateUnlockWalletPassword: PropTypes.func | ||
}; | ||
export default function CreateUnlockWalletPassword({ | ||
setValidCreateUnlockWalletPassword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the called component, the function to be called on a change or other event should be called on{event}
, to follow a react naming convention present in, for example, the <input>
element with its onChange
prop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here, it should be called onValidPassword
or something like that.
] = useState(true); | ||
|
||
const [password, setPassword] = useState(''); | ||
const [confirmPassword, setConfirmPassword] = useState(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all this variables and hooks could be typed via flow
@@ -0,0 +1,5 @@ | |||
// @flow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is now innecessary
setIsMnemonicValid(false); | ||
}; | ||
|
||
const setValidCreateUnlockWalletPassword = function setValidCreateUnlockWalletPassword( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
due to the aforementioned naming convention, this should probably be called validPasswordHandler
or somethin like that.
}; | ||
|
||
const setValidCreateUnlockWalletPassword = function setValidCreateUnlockWalletPassword( | ||
unlockPwd: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could export the type of the callback in the called component and import it wherever it's used, to avoid having to type the handlers manuallly every time
const handleSubmitMnemonic = function handleSubmitMnemonic(event) { | ||
event.preventDefault(); | ||
if (isValidMnemonic(newMnemonicPhrase)) { | ||
// TODO: ADD IF SENTENCE TO CHECK IF PASSWORD AND CONFIRM IS OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. Shouldn't this if be enough?
No description provided.