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

Implement --owner and --perms #689

Merged
merged 11 commits into from
Jul 13, 2023
Merged

Implement --owner and --perms #689

merged 11 commits into from
Jul 13, 2023

Conversation

pvdrz
Copy link
Collaborator

@pvdrz pvdrz commented Jul 12, 2023

Describe the changes done on this pull request
This PR adds the required logic to implement the --owner/-O and --perms/-P flags for visudo.

Pull Request Checklist

  • I have read and accepted the code of conduct for this project.
  • I have tested, formatted and ran clippy over my changes.
  • I have commented and documented my changes.
  • This pull request will partially issue implement visudo #657 where a proper discussion about a solution has taken place.

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.19 ⚠️

Comparison is base (e1dff57) 57.66% compared to head (7efb68d) 57.48%.

❗ Current head 7efb68d differs from pull request most recent head 5b89978. Consider uploading reports for the commit 5b89978 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #689      +/-   ##
==========================================
- Coverage   57.66%   57.48%   -0.19%     
==========================================
  Files          68       69       +1     
  Lines        8960     8985      +25     
==========================================
- Hits         5167     5165       -2     
- Misses       3793     3820      +27     
Impacted Files Coverage Δ
src/system/file/chown.rs 0.00% <0.00%> (ø)
src/system/file/lock.rs 77.04% <ø> (ø)
src/system/mod.rs 82.05% <0.00%> (-0.83%) ⬇️
src/visudo/cli.rs 0.00% <0.00%> (ø)
src/visudo/mod.rs 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Jul 12, 2023

Number of dependencies and binary size impact report

Metric main PR #689 Delta
Direct dependencies 3 3 -
Total dependencies 4 4 -
Binary size 996.5 KiB 996.6 KiB -
Text size 580.6 KiB 580.3 KiB -
Dependencies diff
 └─ sudo-rs [v0.2.0-dev.20230711]
    ├─ glob [v0.3.1]
    ├─ libc [v0.2.147]
    └─ log [v0.4.19]

@pvdrz pvdrz mentioned this pull request Jul 12, 2023
4 tasks
@japaric japaric self-assigned this Jul 13, 2023
Copy link
Collaborator

@japaric japaric left a comment

Choose a reason for hiding this comment

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

left some comments but otherwise LGTM

src/system/file/chown.rs Outdated Show resolved Hide resolved
if perms || file_arg.is_none() {
sudoers_file.set_permissions(Permissions::from_mode(0o440))?;
}
let result: io::Result<()> = (|| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as an alternative to this closure that gets called immediately ("pseudo try-block"), I think you could create a newtype over a locked file that releases the lock when it's dropped. so long we don't call exit from here that should release the lock even when an error occurs.

that being said, according to the glibc manual, the locks associated to a file are released when the file is closed and I think that should happen even if the process call exit because the OS should release all the resources associated to the process.

@pvdrz pvdrz added this pull request to the merge queue Jul 13, 2023
Merged via the queue into main with commit fad6b90 Jul 13, 2023
12 checks passed
@japaric japaric deleted the pvdrz-owner-and-perms branch July 14, 2023 10:32
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