Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Implement createWriteStream for Linux #15

Merged
merged 3 commits into from
May 29, 2019

Conversation

as-cii
Copy link
Contributor

@as-cii as-cii commented May 28, 2019

This pull request relies on Polkit to stream bytes into a file with administrator privileges. In particular it will use the pkexec command to invoke dd as an administrator, which in turn will display an authentication message similar to the following:

Screen Shot 2019-05-28 at 13 39 57

Users of fs-admin can customize this behavior by writing an appropriate .policy file in /usr/share/polkit-1/actions. Atom, for example, will include an atom.policy file that will display a custom message as well as retaining admin access to dd for a short period of time. See atom/atom#19412 for more information.

Copy link
Contributor

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance this all looks promising - thanks for the writeup!

Users of fs-admin can customize this behavior by writing an appropriate .policy file in /usr/share/polkit-1/actions.

We've not really needed a "packaging guide" for situations like this previously (because we relied on APIs included with the OS), but would you be interested in distilling this guidance into a document that we can add to the project? Linking off to the Atom PR in this document is also handy here, but I'd love something a bit more formal to explain what an app that distributes this library needs to be aware of...

index.js Outdated Show resolved Hide resolved
@as-cii
Copy link
Contributor Author

as-cii commented May 28, 2019

We've not really needed a "packaging guide" for situations like this previously (because we relied on APIs included with the OS), but would you be interested in distilling this guidance into a document that we can add to the project? Linking off to the Atom PR in this document is also handy here, but I'd love something a bit more formal to explain what an app that distributes this library needs to be aware of...

Sounds good! Technically, including a policy would not be mandatory but it seems like a good nice-to-have. I'll write something up real quick! ✨

index.js Show resolved Hide resolved
Copy link
Contributor Author

@as-cii as-cii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the promptly reviews, @shiftkey! 🙏

I left a couple of comments, let me know what you think!

index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
Copy link
Contributor

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @as-cii! I'm going to let this sit for the day to give @rafeca a chance to follow up too.

Planning to ship this as a 2.0.0 release (adding some proper Linux support feels like a good reason to bump the major version) later in the week if this is something you'd like to dogfood more in Atom.

@as-cii
Copy link
Contributor Author

as-cii commented May 28, 2019

Thanks for the quick turnaround, @shiftkey! I think we may even bump the minor version since people shouldn't be relying on createWriteStream anyway on Linux. Happy to bump to 2.0 though if you think that'd be better! 👍

Sounds good to wait for @rafeca to chime in before merging this 👍

@shiftkey
Copy link
Contributor

Happy to bump to 2.0 though if you think that'd be better! 👍

It's an additive API change rather than a true breaking change, so this isn't a strong opinion of mine.

Copy link

@rafeca rafeca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

question: do we only need the createWriteStream() method for linux? (noticed that the other platform implementations have more methods e.g to create symlinks and stuff).

@shiftkey
Copy link
Contributor

@rafeca

do we only need the createWriteStream() method for linux? (noticed that the other platform implementations have more methods e.g to create symlinks and stuff).

I'm not hugely fussed that this PR doesn't complete the full API surface. Given the underlying native module is an empty implementation on Linux I think there's still more work to be done here even if we reused the darwin JS code:

#include "fs-admin.h"
namespace fs_admin {
using std::vector;
using std::string;
void *StartChildProcess(const string &command, const vector<string> &args, bool test_mode) {
return nullptr;
}
int WaitForChildProcessToExit(void *child_process, bool test_mode) {
return -1;
}
string CreateAuthorizationForm() { return ""; }
void ClearAuthorizationCache() {}
} // namespace fs_admin

I think the easiest option here is to open tracking issue for the API gaps so we're aware of what isn't currently supported. Is there any other APIs in here that you would need to support Atom's usage of fs-admin on Linux?

@as-cii
Copy link
Contributor Author

as-cii commented May 29, 2019

Is there any other APIs in here that you would need to support Atom's usage of fs-admin on Linux?

Not really, as we're planning to use this just for escalating privileges when saving a file. We may create issues to track other APIs but I am not sure we will need those in the short/medium term.


@shiftkey: please, feel free to merge/release this at will. Thanks again for the quick reviews! 🙇

@shiftkey shiftkey merged commit 3076674 into master May 29, 2019
@shiftkey shiftkey deleted the as/escalate-privileges-on-linux branch May 29, 2019 11:35
@shiftkey
Copy link
Contributor

@as-cii @rafeca this has been published to NPM as v0.5.0

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

Successfully merging this pull request may close these issues.

None yet

3 participants