-
Notifications
You must be signed in to change notification settings - Fork 680
Feat/341/rework password logic #1436
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: main
Are you sure you want to change the base?
Changes from all commits
8ff1879
c19bdb7
9bc7a60
44e3b03
1732cf3
67eb501
2fd8107
6945990
f8ad468
005d257
867f915
da6ed82
dd2346f
59f3870
ffe5178
ff4ad5e
02698b4
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 |
|---|---|---|
|
|
@@ -64,7 +64,7 @@ | |
| from mycli.packages.tabular_output import sql_format | ||
| from mycli.packages.toolkit.history import FileHistoryWithTimestamp | ||
| from mycli.sqlcompleter import SQLCompleter | ||
| from mycli.sqlexecute import ERROR_CODE_ACCESS_DENIED, FIELD_TYPES, SQLExecute | ||
| from mycli.sqlexecute import FIELD_TYPES, SQLExecute | ||
|
|
||
| try: | ||
| import paramiko | ||
|
|
@@ -460,7 +460,7 @@ def connect( | |
| self, | ||
| database: str | None = "", | ||
| user: str | None = "", | ||
| passwd: str | None = "", | ||
| passwd: str | None = None, | ||
| host: str | None = "", | ||
| port: str | int | None = "", | ||
| socket: str | None = "", | ||
|
|
@@ -528,10 +528,19 @@ def connect( | |
| # if the passwd is not specified try to set it using the password_file option | ||
| password_from_file = self.get_password_from_file(password_file) | ||
| passwd = passwd if isinstance(passwd, str) else password_from_file | ||
| passwd = '' if passwd is None else passwd | ||
|
|
||
| # Connect to the database. | ||
| # password hierarchy | ||
| # 1. -p / --pass/--password CLI options | ||
| # 2. envvar (MYSQL_PWD) | ||
| # 3. DSN (mysql://user:password) | ||
| # 4. cnf (.my.cnf / etc) | ||
| # 5. --password-file CLI option | ||
|
|
||
| # if no password was found from all of the above sources, ask for a password | ||
| if passwd is None: | ||
| passwd = click.prompt("Enter password", hide_input=True, show_default=False, default='', type=str, err=True) | ||
|
|
||
| # Connect to the database. | ||
| def _connect() -> None: | ||
| try: | ||
| self.sqlexecute = SQLExecute( | ||
|
|
@@ -552,31 +561,7 @@ def _connect() -> None: | |
| init_command, | ||
| ) | ||
| except pymysql.OperationalError as e1: | ||
| if e1.args[0] == ERROR_CODE_ACCESS_DENIED: | ||
| if password_from_file is not None: | ||
| new_passwd = password_from_file | ||
| else: | ||
| new_passwd = click.prompt( | ||
| f"Password for {user}", hide_input=True, show_default=False, default='', type=str, err=True | ||
| ) | ||
| self.sqlexecute = SQLExecute( | ||
| database, | ||
| user, | ||
| new_passwd, | ||
| host, | ||
| int_port, | ||
| socket, | ||
| charset, | ||
| use_local_infile, | ||
| ssl_config_or_none, | ||
| ssh_user, | ||
| ssh_host, | ||
| int(ssh_port) if ssh_port else None, | ||
| ssh_password, | ||
| ssh_key_filename, | ||
| init_command, | ||
| ) | ||
| elif e1.args[0] == HANDSHAKE_ERROR and ssl is not None and ssl.get("mode", None) == "auto": | ||
| if e1.args[0] == HANDSHAKE_ERROR and ssl is not None and ssl.get("mode", None) == "auto": | ||
| try: | ||
| self.sqlexecute = SQLExecute( | ||
| database, | ||
|
|
@@ -595,33 +580,8 @@ def _connect() -> None: | |
| ssh_key_filename, | ||
| init_command, | ||
| ) | ||
| except pymysql.OperationalError as e2: | ||
| if e2.args[0] == ERROR_CODE_ACCESS_DENIED: | ||
| if password_from_file is not None: | ||
| new_passwd = password_from_file | ||
| else: | ||
| new_passwd = click.prompt( | ||
| f"Password for {user}", hide_input=True, show_default=False, default='', type=str, err=True | ||
| ) | ||
| self.sqlexecute = SQLExecute( | ||
| database, | ||
| user, | ||
| new_passwd, | ||
| host, | ||
| int_port, | ||
| socket, | ||
| charset, | ||
| use_local_infile, | ||
| None, | ||
| ssh_user, | ||
| ssh_host, | ||
| int(ssh_port) if ssh_port else None, | ||
| ssh_password, | ||
| ssh_key_filename, | ||
| init_command, | ||
| ) | ||
| else: | ||
| raise e2 | ||
| except Exception as e2: | ||
| raise e2 | ||
| else: | ||
| raise e1 | ||
|
|
||
|
|
@@ -1487,8 +1447,16 @@ def get_last_query(self) -> str | None: | |
| @click.option("-P", "--port", envvar="MYSQL_TCP_PORT", type=int, help="Port number to use for connection. Honors $MYSQL_TCP_PORT.") | ||
| @click.option("-u", "--user", help="User name to connect to the database.") | ||
| @click.option("-S", "--socket", envvar="MYSQL_UNIX_PORT", help="The socket file to use for connection.") | ||
| @click.option("-p", "--password", "password", envvar="MYSQL_PWD", type=str, help="Password to connect to the database.") | ||
| @click.option("--pass", "password", envvar="MYSQL_PWD", type=str, help="Password to connect to the database.") | ||
| @click.option( | ||
| "-p", | ||
| "--pass", | ||
| "--password", | ||
| "password", | ||
| is_flag=False, | ||
| flag_value="MYCLI_ASK_PASSWORD", | ||
| type=str, | ||
| help="Prompt for (or enter in cleartext) password to connect to the database.", | ||
| ) | ||
| @click.option("--ssh-user", help="User name to connect to ssh server.") | ||
| @click.option("--ssh-host", help="Host name to connect to ssh server.") | ||
| @click.option("--ssh-port", default=22, help="Port to connect to ssh server.") | ||
|
|
@@ -1548,9 +1516,11 @@ def get_last_query(self) -> str | None: | |
| @click.option( | ||
| "--password-file", type=click.Path(), help="File or FIFO path containing the password to connect to the db if not specified otherwise." | ||
| ) | ||
| @click.argument("database", default="", nargs=1) | ||
| @click.argument("database", default=None, nargs=1) | ||
| @click.pass_context | ||
| def cli( | ||
| database: str, | ||
| ctx: click.Context, | ||
| database: str | None, | ||
| user: str | None, | ||
| host: str | None, | ||
| port: int | None, | ||
|
|
@@ -1603,6 +1573,20 @@ def cli( | |
| - mycli mysql://my_user@my_host.com:3306/my_database | ||
|
|
||
| """ | ||
| # if user passes the --p* flag, ask for the password right away | ||
| # to reduce lag as much as possible | ||
| if password == "MYCLI_ASK_PASSWORD": | ||
| password = click.prompt("Enter password", hide_input=True, show_default=False, default='', type=str, err=True) | ||
| # if the password value looks like a DSN, treat it as such and | ||
| # prompt for password | ||
| elif database is None and password is not None and password.startswith("mysql://"): | ||
|
Contributor
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 am still not sure about this, and predict we will get some issues filed. But we can try it.
Contributor
Author
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. Were you expecting a different behavior? Thought this is what we talked about last. I could add back in the option ordering logic and only check for the DSN URI if the password option/flag is the last one, maybe that's what you were thinking? |
||
| database = password | ||
| password = click.prompt("Enter password", hide_input=True, show_default=False, default='', type=str, err=True) | ||
| # getting the envvar ourselves because the envvar from a click | ||
| # option cannot be an empty string, but a password can be | ||
| elif password is None and os.environ.get("MYSQL_PWD") is not None: | ||
| password = os.environ.get("MYSQL_PWD") | ||
|
|
||
| mycli = MyCli( | ||
| prompt=prompt, | ||
| logfile=logfile, | ||
|
|
||
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.
Can we reduce the changelog entry to a single line, please? If documentation is needed there is a
docdirectory.Something like: "Improve password startup performance and clarify password option hierarchy."
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.
Will do; didn't realize we were keeping to one line. Makes sense for easier merging though!