-
Notifications
You must be signed in to change notification settings - Fork 175
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
Allow custom params for keyword "Connect To Database" #220
Conversation
…the password, if it was the last parameter
@markus-leben could you please have a look? |
@staticmethod | ||
def _hide_password_values(string_with_pass, params_separator=","): | ||
string_with_hidden_pass = string_with_pass | ||
for pass_param_name in ["pass", "passwd", "password", "pwd", "PWD"]: |
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.
Could there ever be non-password parameters a person may want to hide?
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.
Emm, like what?
msg += f'"{connection_string}"' | ||
params_separator = ";" | ||
for param_name, param_value in connection_params.items(): | ||
msg += f", {param_name}=" |
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 currently doesn't use params_separator. I think it may break password hiding as-is for connection string based params.
Also, I think using string replacement may not be ideal for dbPassword, as a short password string might get replaced in the string. Like:
Connect *** DB using : pymysql.connect(dbPort=3306, dbCharset='utf8mb4', dbUsername='InsecurePWWriter', dbPassword='***')
(I'm not sure that's the exact result, just a proof of concept)
I think you could do param name spreading as:
params_separator = ", "
if connection_string:
msg += f'"{connection_string}"'
params_separator = "; "
sanitized_params = copy.copy(connection_params)
if dbPassword:
sanitized_params['dbPassword'] = "***"
msg += params_separator.join([f"{param_name}='{param_value}'" if isinstance(param_value, str)
else f"{param_name}={param_value}"
for param_name, param_value in sanitized_params.items()])
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 params_separator
is not used directly here, it's passed to the function _hide_password_values()
. The separator is just different when using connection params as arguments or a connection string.
The replacement could indeed go wrong, if the password value is identical with some other parameter value - like username or host. I don't think it's really critical - but it might be a fix for future releases.
port=dbPort, | ||
charset="utf8mb4" or dbCharset, | ||
_log_all_connection_params(**con_params) | ||
db_connection = db_api_2.connect(**con_params) |
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 do still think that having some sort of connection function that passes extra info into the exception on failure may be a good idea.
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.
Could you please elaborate a bit on this? Currently, if the connect()
call failed, the exception would not be caught, but fully printed in the RF log.
The entire connection logic and implementation was refactored
Other changes
The PR contains also new and updated tests