Skip to content
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

Client silently ignores PEM file if user expansion is needed #85

Closed
MrCreosote opened this issue Aug 12, 2024 · 3 comments · Fixed by #86
Closed

Client silently ignores PEM file if user expansion is needed #85

MrCreosote opened this issue Aug 12, 2024 · 3 comments · Fixed by #86
Assignees

Comments

@MrCreosote
Copy link
Contributor

def _read_client_secret_from_file(self, name):
if name is not None and Path(name).exists():
# If the user gives a full path, then use it
key_path = Path(name)

If name is something like ~/NERSC/key.pem then Path.exists() will return False, even though the file does exist at that location, and the client will remain unauthenticated, which is very confusing.

There's at least two options that could be reasonable:

  1. Add Path.expanduser() after each Path constructor call
  2. Throw an error if expanduser() results in a different path if you want to force the user to provide an absolute path or path relative to the working dir
@cjh1
Copy link
Collaborator

cjh1 commented Aug 13, 2024

@MrCreosote Thanks for report. I think your first suggest is probably the way to go. @tylern4 would you be able to take a look at fixed this up, as I am currently out of action.

@tylern4 tylern4 self-assigned this Aug 13, 2024
@MrCreosote
Copy link
Contributor Author

If it's just a matter of adding the expanduser calls and manually testing I can PR it if you want. I hesitated before because

  • I hadn't looked at the tests, but it seems like there are no tests for the key= case, so it's just a matter of testing manually
  • I'm not sure what platforms you're supporting and if expanduser would cause problems on Windows.

If adding the calls, testing on linux, and making a PR agrees with you LMK and I'll do it today

@MrCreosote
Copy link
Contributor Author

Looks like the code is duplicated in _sync/client.py as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants