-
Notifications
You must be signed in to change notification settings - Fork 26
Conversation
There was a problem hiding this 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...
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! ✨ |
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an additive API change rather than a true breaking change, so this isn't a strong opinion of mine. |
There was a problem hiding this 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).
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 fs-admin/src/fs-admin-linux.cc Lines 1 to 19 in 86dab87
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 |
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! 🙇 |
This pull request relies on Polkit to stream bytes into a file with administrator privileges. In particular it will use the
pkexec
command to invokedd
as an administrator, which in turn will display an authentication message similar to the following: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 anatom.policy
file that will display a custom message as well as retaining admin access todd
for a short period of time. See atom/atom#19412 for more information.