-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add providers for opening files and getting server config #228
Conversation
Hi @pfitzseb, thank you for your support. The first part can be merged without problems. With the second part I agree with you that it is a security risk without asking the user. So if you integrate the prompt, the second part can also be merged. |
when switching to local file
Hi @cschindl, Let me know if you think that's enough. The last commit also fixes a random bug I noticed. |
91698d0
to
d91624e
Compare
I suggest that the confirmation could be stored somewhere so that the user need only be asked once. Also, I think the confirmation makes sense when revealing a password, but could perhaps be disabled when only a private key file is revealed (since we have access to that anyway). |
Hi @pfitzseb, Did you look at the suggestion from @MikeInnes? The one-time request for confirmation might be helpful. If you don't think so, I can merge your PR. |
Looks good and is hereby released. Thanks for your support. |
Thanks for the merge! :) |
I'm in the process of making Juno play nicer with remote servers (see here for the relevant PR) and it would be nice if there were some small hooks into this package to facilitate integration on our side.
This PR adds
ftp-remote-edit
. This is probably relatively uncontroversial, but the implementation could probably be nicer.ftp-remote-edit
. This definitely shouldn't be merged as-is since every package can get e.g. the password or private key file that way. The best way to handle this would probably be to ask the user to confirm explicitly when another package tries to get the server config, I guess. If that's not an option for you I'll just remove this part of the API.