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

archive: allow preserving ownerships when unpacking #276

Merged
merged 5 commits into from
Jan 12, 2022

Conversation

liushuyu
Copy link
Contributor

@liushuyu liushuyu commented Jan 6, 2022

Added support for unpacking files while preserving numeric ownership IDs on Unix, this is useful when you want to make a backup utility since ownerships are very important on Unix-like systems. (Also useful if you want to make a system installer, the underlying idea is the same.)

Currently, operations are only available on Unix.
On Windows, it's also possible to set user ownerships, however, it's a very different system and does not use numeric IDs AFAIK. Any other suggestions on the current implementations are welcome!

// it's not possible to unpack tar while preserving ownership
// without root permissions
assert!(ar.unpack(td.path()).is_err());
}
Copy link
Owner

Choose a reason for hiding this comment

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

AFAIK this is unlikely to ever get executed, even in CI, so could a test be written that runs in normal user mode as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK this is unlikely to ever get executed, even in CI, so could a test be written that runs in normal user mode as well?

I think it would be very difficult. chown itself is a privileged operation, even on Windows. So it would be not possible to test it properly using normal user permissions.

Although, there might be a way to change gid on a file that is currently owned by the current effective user (file uid == user euid). But this kind of test would not cover the uid change, so it would be very difficult if not impossible to thoroughly test this functionality under a normal user account.

Copy link
Owner

Choose a reason for hiding this comment

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

If that is the situation can you arrange for the test to happen with sudo on CI?

Also ideally tthis wouldn't check in a *.tar file but build it up here so the tar file can be easily changed as well.

src/entry.rs Outdated Show resolved Hide resolved
src/entry.rs Outdated Show resolved Hide resolved
src/entry.rs Outdated Show resolved Hide resolved
// it's not possible to unpack tar while preserving ownership
// without root permissions
assert!(ar.unpack(td.path()).is_err());
}
Copy link
Owner

Choose a reason for hiding this comment

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

If that is the situation can you arrange for the test to happen with sudo on CI?

Also ideally tthis wouldn't check in a *.tar file but build it up here so the tar file can be easily changed as well.

@liushuyu liushuyu force-pushed the master branch 3 times, most recently from e695234 to 2dabf8b Compare January 11, 2022 01:36
Copy link
Owner

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks! I think that the ownership.tar can be removed now as well?

src/entry.rs Outdated Show resolved Hide resolved
src/entry.rs Outdated Show resolved Hide resolved
... also deleted ownership.tar in the tests
@alexcrichton alexcrichton merged commit c3e2cb8 into alexcrichton:master Jan 12, 2022
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