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

fix: Handle users and groups with spaces correctly in SSH #448

Merged
merged 1 commit into from
Jul 21, 2024

Conversation

annagrram
Copy link
Contributor

This is a fix for a problem I had when using this plugin to SSH our servers at work.
We have group names containing spaces, which makes the line parsing break and raise an exception.

My solution was to work directly with the IDs of the users and groups.

Please let me know if there's a better or more appropriate solution, and I'll be happy to amend this PR.

@github-actions github-actions bot requested a review from stevearc July 17, 2024 07:12
@stevearc
Copy link
Owner

I'm fine with this change. Can you confirm that you've tested:

  1. permissions work on allowed directories (i.e. a directory that you have user or group write permissions on is modifiable)
  2. permissions work on disallowed directories (a directory you have no write permissions for is not modifiable)

@annagrram
Copy link
Contributor Author

Yeah.
I tested it again today, and it seems to work.

@annagrram
Copy link
Contributor Author

annagrram commented Jul 18, 2024

As a side note, I was about to test it on another server but got some exceptions.
The exceptions are related to locale settings that result in an unexpected format when trying to parse the ls line (all of my problems with the SSH adapter seem to originate from this ls line parsing).
I have a minor fix for it.
Would you prefer a new PR, or should I add it to this PR?

@stevearc
Copy link
Owner

Great, thanks for checking!

I'd say put that fix in a new PR, just so I can go ahead and merge this one.

@stevearc stevearc merged commit a6cea1a into stevearc:master Jul 21, 2024
9 checks passed
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 this pull request may close these issues.

2 participants