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

Run service as particular user for systemd and launchd #11

Merged
merged 1 commit into from Oct 18, 2023

Conversation

jacderida
Copy link
Contributor

Nice crate you have here! In our project, we were looking for a cross platform solution for managing services and this crate seemed a great fit, but it was just lacking the ability to specify which user the service runs as, so I decided to submit a PR.

It's important to be able to specify which user the service runs as, particularly in a Linux-based environment. Therefore, the ServiceInstallCtx is extended with an optional username field.

It's also possible to run services on Windows with a non-Administrator user account, and I tried hard to get this to work. I could get the service created with another account, but it would not start. I ended up just submitting my PR with support for macOS and Linux.

New system tests were provided, with the base test being extended to check if the service process is running as the user specified in the ServiceInstallCtx definition. It just does this by checking the service definition. I tried to check the running service process to see if it was running as the correct user, but this proved to be difficult to do in a cross platform way.

Running with a different user only applies to system-wide services.

I made a small change to the way the plist file was constructed for launchd, preferring to use the plist crate rather than a hard coded string.

@chipsenkbeil
Copy link
Owner

Thanks for the contribution! I'll give your PR a review over the next couple of days, but in the meantime going to kick off some tests. Conceptually, what you described sounds fine to me, so let's see if we can get it integrated and a new release pushed shortly.

@chipsenkbeil
Copy link
Owner

@jacderida it looks like the FreeBSD vm action I was using is now broken, so I tried switching to a different one on the primary branch, but that one is hitting cross-platform-actions/action#61 (comment).

So I'll most likely disable the FreeBSD test action. Once I do so, pull in those changes and rebase on top. We can work from there!

@jacderida
Copy link
Contributor Author

@jacderida it looks like the FreeBSD vm action I was using is now broken, so I tried switching to a different one on the primary branch, but that one is hitting cross-platform-actions/action#61 (comment).

So I'll most likely disable the FreeBSD test action. Once I do so, pull in those changes and rebase on top. We can work from there!

OK cool. I ran the tests on my fork before I submitted the PR and couldn't get that one working, but I didn't think it was caused by any of my changes.

Copy link
Owner

@chipsenkbeil chipsenkbeil left a comment

Choose a reason for hiding this comment

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

One nitpick to resolve given you're export two private functions as public. Once that's taken care of and you rebase on the latest commit, we can get this landed 😄

src/systemd.rs Outdated
@@ -228,11 +229,11 @@ fn systemctl(cmd: &str, label: &str, user: bool) -> io::Result<()> {
}

#[inline]
fn global_dir_path() -> PathBuf {
pub fn global_dir_path() -> PathBuf {
Copy link
Owner

Choose a reason for hiding this comment

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

If you want to make public global_dir_path and user_dir_path from the systemd code, can they either be exposed with a prefix like systemd_ or within a module like service_manager::systemd::global_dir_path?

Otherwise, it isn't obvious for someone accessing those methods that they're specific to systemd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I amended my commit to add the systemd prefixes to the functions.

The amend also contains another little change in the tests. The existence of the user being specified was being asserted before the user account was deleted. I changed the order of that so that the assertion is made after the account is deleted. I'm sure I had it that way originally. This ensures the user account for the test will be tidied up even if that assertion fails.

It's important to be able to specify which user the service runs as, particularly in a Linux-based
environment. Therefore, the `ServiceInstallCtx` is extended with an optional `username` field.

It's also possible to run services on Windows with a non-Administrator user account. I tried hard to
get this to work. I could get the service created with another account, but it would not start. I
ended up just submitting my PR with support for macOS and Linux.

New system tests were provided, with the base test being extended to check if the service process is
running as the user specified in the `ServiceInstallCtx` definition. It just does this by checking
the service definition. I tried to check the running service process to see if it was running as the
correct user, but this proved to be difficult to do in a cross platform way.

Running with a different user only applies to system-wide services.

I made a small change to the way the plist file was constructed for launchd, preferring to use the
`plist` crate rather than a hard coded string.
@chipsenkbeil chipsenkbeil merged commit 74037f4 into chipsenkbeil:main Oct 18, 2023
9 checks passed
@chipsenkbeil
Copy link
Owner

@jacderida something I forgot to do was ask you to amend the changelog with a note about this PR. Would you like to submit a second PR with the change documented? If not, I'll add it later this week and then publish a new version under 0.4.0.

@jacderida
Copy link
Contributor Author

@jacderida something I forgot to do was ask you to amend the changelog with a note about this PR. Would you like to submit a second PR with the change documented? If not, I'll add it later this week and then publish a new version under 0.4.0.

Ah sure, no problem! I'll do it later today.

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.

None yet

2 participants