refactor: adopt EAFP and explicit error handling for password file #1203
Conversation
mycli/main.py
Outdated
|
|
||
| return password_from_file | ||
| password = fp.readline().strip() | ||
| if not password: |
There was a problem hiding this comment.
Doesn't this introduce a new constraint?
There was a problem hiding this comment.
Are you referring to if not? If so, I thought it makes sense to raise an error when the content is empty
There was a problem hiding this comment.
Yes, raising an error when the password is empty changes the functionality, which I suppose we should hold constant for this PR. Looking closer, I would even strip() the newline more precisely but again that is out of scope.
Because the empty string is a valid password, as are passwords containing whitespace. Whether they are advisable is a separate Q.
There was a problem hiding this comment.
Makes sense, I updated the code.
mycli/main.py
Outdated
| raise PasswordFileError(f"Permission denied reading password file '{password_file}'") from None | ||
| except IsADirectoryError: | ||
| raise PasswordFileError(f"Path '{password_file}' is a directory, not a file") from None | ||
| except OSError as e: |
There was a problem hiding this comment.
Also advisable to catch RuntimeError ? I'm unsure.
There was a problem hiding this comment.
I don't think it's useful, maybe I can leave this last exception generic to handle all other types of exceptions? what do you think?
|
@allrob23 I see this PR now has a merge conflict. Do you want me to fix that and merge, or take that on yourself? |
|
@rolandwalker can you please do it? thanks |
2 participants
Description
#1202
This PR enhances
get_password_from_fileby removing pre-checkifstatements and adding explicit error handling for cases like file not found or permission denied, e.g.,mycli.main.PasswordFileError: Password file '/tmp/a.txt' is empty or contains only whitespace, improving security and clarity.Checklist
changelog.md.AUTHORSfile (or it's already there).