-
Notifications
You must be signed in to change notification settings - Fork 12
Separate auth command into more useful subparsers #84
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| from ethpm_cli._utils.logger import cli_logger | ||
|
|
||
|
|
||
| def parse_bool_flag(question: str) -> bool: | ||
| while True: | ||
| response = input(f"{question} (y/n) ") | ||
| if response.lower() == "y": | ||
| return True | ||
| elif response.lower() == "n": | ||
| return False | ||
| else: | ||
| cli_logger.info(f"Invalid response: {response}.") | ||
| continue |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,104 @@ | ||
| import json | ||
| from pathlib import Path | ||
| import tempfile | ||
| from typing import Any, Dict | ||
|
|
||
| import eth_keyfile | ||
| from eth_typing import Address | ||
| from eth_utils import to_bytes | ||
| from eth_utils import add_0x_prefix, to_bytes | ||
|
|
||
| from ethpm_cli._utils.filesystem import atomic_replace | ||
| from ethpm_cli._utils.input import parse_bool_flag | ||
| from ethpm_cli._utils.logger import cli_logger | ||
| from ethpm_cli._utils.xdg import get_xdg_ethpmcli_root | ||
| from ethpm_cli.constants import KEYFILE_PATH | ||
| from ethpm_cli.exceptions import AuthorizationError | ||
|
|
||
| PRIVATE_KEY_WARNING = ( | ||
| "Please be careful when using your private key, it is a sensitive piece of information.\n", | ||
| "~ ~ ~ Not your keys, not your crypto. ~ ~ ~\n", | ||
| "ethPM doesn't save your private key, but it is best practice to re-use an already encrypted ", | ||
| "keyfile and link it with `ethpm auth link` rather than regenerating a new encrypted keyfile.", | ||
| ) | ||
|
|
||
|
|
||
| def link_keyfile(keyfile_path: Path) -> None: | ||
| if valid_keyfile_exists(): | ||
| xdg_keyfile = get_keyfile_path() | ||
| cli_logger.info( | ||
| f"Keyfile detected at {xdg_keyfile}. Please use `ethpm auth unlink` to delete this " | ||
| "keyfile before linking a new one." | ||
| ) | ||
| else: | ||
| import_keyfile(keyfile_path) | ||
| address = get_authorized_address() | ||
| cli_logger.info( | ||
| f"Keyfile stored for address: {address}\n" | ||
| "It's now available for use when its password is passed in with the " | ||
| "`--keyfile-password` flag." | ||
| ) | ||
|
|
||
|
|
||
| def unlink_keyfile() -> None: | ||
| if not valid_keyfile_exists(): | ||
| cli_logger.info("Unable to unlink keyfile: empty keyfile found.") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The phrase: "empty keyfile found" might be misleading. Should this say something like "no keyfile found" and maybe include the search path so that they know where it was supposed to be? |
||
| else: | ||
| keyfile_path = get_keyfile_path() | ||
| address = get_authorized_address() | ||
| keyfile_path.write_text("") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this behavior surprising. I would expect it to remove the file... |
||
| cli_logger.info(f"Keyfile removed for address: {address}") | ||
|
|
||
|
|
||
| def init_keyfile() -> None: | ||
| if valid_keyfile_exists(): | ||
| cli_logger.info( | ||
| f"Keyfile detected. Please use `ethpm auth unlink` to delete this " | ||
| "keyfile before initializing a new one." | ||
| ) | ||
| return | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think expected behavior would be for the process to return a non-zero return code so that if this is run in some sort of script that this logic branch produces a detectable failure. |
||
|
|
||
| cli_logger.info(PRIVATE_KEY_WARNING) | ||
| agreement = parse_bool_flag( | ||
| "Are you sure you want to proceed with initializing a keyfile? " | ||
| ) | ||
| if not agreement: | ||
| cli_logger.info("Aborting keyfile initialization.") | ||
| return | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, this should probably result in the CLI exiting with a non-zero return code.. |
||
|
|
||
| private_key = to_bytes(text=input("Please enter your 32-length private key: ")) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is ok. I know you are probably expecting people to copy/paste which might be fine if it could be enforced, but this will also result in people typing the key in, or pasting it with an accidental newline at the end or typing it in its hexidecimal representation, or any number of insane things we can't think of.... I think that if someone wants to bring in a raw private key we should require them to do the heavy lifting of creating the keyfile themselves so that we're not responsible for mistakes by encouraging this pattern... thoughts? cc @holiman |
||
| validate_private_key_length(private_key) | ||
| password = to_bytes( | ||
| text=input( | ||
| "Please enter a password to encrypt your keyfile with (Don't forget this password!): " | ||
| ) | ||
| ) | ||
| keyfile_json = eth_keyfile.create_keyfile_json(private_key, password) | ||
| ethpm_xdg_root = get_xdg_ethpmcli_root() | ||
| ethpm_cli_keyfile_path = ethpm_xdg_root / KEYFILE_PATH | ||
| with atomic_replace(ethpm_cli_keyfile_path) as file: | ||
| file.write(json.dumps(keyfile_json)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick you can change this to |
||
| address = get_authorized_address() | ||
| cli_logger.info(f"Encrypted keyfile saved for address: {address}") | ||
|
|
||
|
|
||
| def valid_keyfile_exists() -> bool: | ||
| try: | ||
| get_authorized_address() | ||
| except AuthorizationError: | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def validate_private_key_length(private_key: str) -> None: | ||
| if len(private_key) != 32: | ||
| raise AuthorizationError(f"{private_key} is not 32 long") | ||
|
|
||
|
|
||
| def import_keyfile(keyfile_path: Path) -> None: | ||
| validate_keyfile(keyfile_path) | ||
| ethpm_xdg_root = get_xdg_ethpmcli_root() | ||
| ethpm_cli_keyfile_path = ethpm_xdg_root / KEYFILE_PATH | ||
| tmp_keyfile = Path(tempfile.NamedTemporaryFile().name) | ||
| tmp_keyfile.write_text(keyfile_path.read_text()) | ||
| tmp_keyfile.replace(ethpm_cli_keyfile_path) | ||
| with atomic_replace(ethpm_cli_keyfile_path) as file: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. orthogonal I'm thinking we should move this utility into |
||
| file.write(keyfile_path.read_text()) | ||
|
|
||
|
|
||
| def get_keyfile_path() -> Path: | ||
|
|
@@ -49,7 +130,7 @@ def get_authorized_address() -> Address: | |
| Returns the address associated with stored keyfile. No password required. | ||
| """ | ||
| keyfile = get_keyfile_data() | ||
| return keyfile["address"] | ||
| return add_0x_prefix(keyfile["address"]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thoughts on making this a checksum address? It's getting logged in a bunch of places which means it will end up in people's clipboards. Since people will need to fund this address, they're likely to copy/pasta it to send funds... |
||
|
|
||
|
|
||
| def get_authorized_private_key(password: str) -> str: | ||
|
|
||
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 find it odd that this branch of the logic completely ignores the function argument
keyfile_path. Haven't gotten to the place where this function is used yet so maybe this is benign...