Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Remove existence & length checks on passwords & phrases #3854

Merged
merged 5 commits into from
Dec 19, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 15, 2016

... do display a security reminder when passwords are selected.

Closes https://github.com/ethcore/parity/issues/3838

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 15, 2016
export default {
invalidPassword:
<FormattedMessage
id='createAccount.error.invalidPassword'
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency & code readability, this should be noPassword, similar to the errors below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping it as it was.

@@ -136,7 +136,7 @@ export default class RecoveryPhrase extends Component {
.filter((part) => part.length);
let recoveryPhraseError = null;

if (!recoveryPhrase || recoveryPhrase.length < 25 || phraseParts.length < 8) {
if (!recoveryPhrase || !recoveryPhrase.length) {
Copy link
Contributor

@derhuerst derhuerst Dec 15, 2016

Choose a reason for hiding this comment

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

[tiny grumble] I'd prefer the more explicit recoveryPhrase.length === 0 here. Same below and above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't. Since I read it as "no recoveryPhrase or no recoveryPhrase length" as opposed to "no recoveryPhrase or recoveryPhrase has length 0".

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd like the user to be able to create the key with the empty recovery phrase, if they so choose. remember informative, not dictatorial.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.565% when pulling ec14b86 on jg-password-lengths into 6d41168 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.565% when pulling ec14b86 on jg-password-lengths into 6d41168 on master.

@gavofyork gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 15, 2016
@jacogr jacogr changed the title Only check existence (not length) on phrases & passwords Remove existence & length checks on passwords & phrases Dec 15, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 15, 2016
@@ -43,9 +43,9 @@ export default class CreateAccount extends Component {
isValidPass: false,
passwordHint: '',
password1: '',
password1Error: ERRORS.invalidPassword,
password1Error: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

afaict password1Error can be removed safely.

Copy link
Contributor Author

@jacogr jacogr Dec 18, 2016

Choose a reason for hiding this comment

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

100% - I have kept all these in for now for circling back. The following to be logged after doing this -

DRY-up the error checks & validation across the different parts and move to a store - validation is duplicated in the different sections, I guess by organic growth and lack of a proper store (Not a critical item, but it is messy atm and can be done much better - in short, all these are to be cleaned, removed & moved)

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is #3887.

@derhuerst derhuerst added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 19, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 85.848% when pulling 6884085 on jg-password-lengths into 817a58c on master.

@jacogr jacogr merged commit 1b59ceb into master Dec 19, 2016
@jacogr jacogr deleted the jg-password-lengths branch December 19, 2016 12:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants