Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Migrate passphrase command - Closes #561 #573

Merged
merged 7 commits into from
Aug 17, 2018

Conversation

shuse2
Copy link
Contributor

@shuse2 shuse2 commented Jun 30, 2018

Description

Migration from

lisk encrypt passphrase
lisk decrypt passphrase

npm t
./bin/run passphrase:encrypt
./bin/run passphrase:decrypt

Review checklist

@shuse2 shuse2 self-assigned this Jun 30, 2018
@shuse2 shuse2 requested a review from willclarktech June 30, 2018 17:53
@shuse2 shuse2 changed the title 561 migrate passphrase command Migrate passphrase command Jun 30, 2018
@shuse2 shuse2 changed the title Migrate passphrase command Migrate passphrase command - Closes #561 Jul 4, 2018
@shuse2 shuse2 force-pushed the 556_migrate_config_command branch 2 times, most recently from 4669cd4 to 865d40c Compare July 16, 2018 13:22
@shuse2 shuse2 changed the base branch from 556_migrate_config_command to 2.0.0 August 1, 2018 15:51
@shuse2 shuse2 force-pushed the 561-migrate_passphrase_command branch from f7128d8 to c04cbed Compare August 2, 2018 15:11
@shuse2 shuse2 force-pushed the 561-migrate_passphrase_command branch from 1e415ea to 2cab2ec Compare August 15, 2018 15:34
`;

DecryptCommand.examples = [
'passphrase:decrypt "salt=xxx&cipherText=xxx&iv=xxx&tag=xxx&version=1"',
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why you'd shorten this, but in general realistic examples are preferable. Do you have any specific reasoning for changing this?

};

DecryptCommand.description = `
Decrypts your secret passphrase using a password using the initialisation vector (IV) which was provided at the time of encryption.
Copy link
Contributor

Choose a reason for hiding this comment

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

Decrypts your secret passphrase using the password which was provided at the time of encryption.

const encryptedPassphrase = cryptography.stringifyEncryptedPassphrase(
encryptedPassphraseObject,
);
const cipherAndIv = { encryptedPassphrase };
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable name could use an update.

describe('passphrase:decrypt', () => {
const defaultEncryptedPassphrase =
'salt=d3887df959ed2bfe5961a6831da6e177&cipherText=1c08a1&iv=096ede534df9092fd4523ec7&tag=2a055e1c860b3ef76084a6c9aca68ce9&version=1';
const passphrase = '123';
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a recommended example.

setupTest()
.stdout()
.command(['passphrase:decrypt'])
.catch(error =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we put brackets around expectations even when there's only one for consistency.

});
},
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

What about when both come from stdin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stdin should be covered by two flags case because it's stubbed.

version: 1,
};
const defaultInputs = {
passphrase: '123',
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a passphrase in a form we recommend.

};
const defaultInputs = {
passphrase: '123',
password: '456',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for password.

version: 1,
};
const defaultInputs = {
password: '456',
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a stronger password.

setupTest()
.stdout()
.command(['passphrase:encrypt', '--passphrase=pass:123'])
.it('should call print with the user config', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong description.

setupTest()
.command([
'passphrase:encrypt',
'--passphrase=pass:enemy pill squeeze gold spoil aisle awake thumb congress false box wagon',
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't exactly how a user would provide this. It would be --passphrase="pass:enemy pill squeeze gold spoil aisle awake thumb congress false box wagon"

setupTest()
.command([
'passphrase:encrypt',
'--passphrase=pass:enemy pill squeeze gold spoil aisle awake thumb congress false box wagon',
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

@shuse2 shuse2 merged commit 11b8841 into 2.0.0 Aug 17, 2018
@shuse2 shuse2 deleted the 561-migrate_passphrase_command branch August 17, 2018 10:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants