Add an option to configure permission on dump #138
Conversation
8c02a6a to
fe5219b
Compare
|
Hi @orgrim, This MR try to implement #132 I still want to improve it before some feedback about the code itself, but I think we should discuss about what we should do whith the parent directory when using the 'd' format and also we should also handle the situation where pg_back users simply want pg_back to use/respect the umask. What do you think we should do about those cases ? Best regards, |
d6e4304 to
e774640
Compare
|
Hi @l00ptr, thanks a lot for taking care of this. After a quick look: some lead on the default mode and how to "handle umask" would be to keep the current 0o600 mode for compatibility and use a negative value use to let the umask apply, since it ends up being store as a integer. What do you think? On the directory format, I need to confirm if we are currently consistent on the permissions of all extra files, as this question applies to checksum files and others as well. I would expect the mode to be set once and applied everywhere. While I need to test the patch, it looks promising. Regards |
|
After a quick look: some lead on the default mode and how to "handle umask" would be to keep the current 0o600 mode for compatibility and use a negative value use to let the umask apply, since it ends up being store as a integer. What do you think?
good idea, done within the fixup.
I'll also look into how checksums (files) are generated and written (and how file permissions are set for them).
|
afc72d0 to
274c33d
Compare
|
i have done some modification to also handle changing permission on hash and encrypted files. directory permission still probably need some improvement. @orgrim could you make a first overall review on this ? Best regards, |
|
Hi @orgrim How can i help to test and integrate that MR ? What can I provide to help you on this ? Best regards, |
main.go
Outdated
| // directory, this won't make the contents executable | ||
| // we keep this for compatibility reason, but since file mode is under | ||
| // user control, we should probably remove this | ||
| mode = 0700 |
There was a problem hiding this comment.
The directory format should be handled as best as possible. I'd imagine a user would expect that if mode is set to 0640, the directory would be chmod to 0750 and its contents to 0640.
On the file, it's ease, just walk the contents of the directory and os.Chmod to the given mode.
Deducing the mode of the directory is trickier. What do you think of "grant x when r or w is present in the mode for each category of users"?
I mean:
if (mode & 0o400 > 0) || (mode & 0o200 > 0) {
mode = mode | 0o100
}
/// and os on to add 0o010 and 0o001
There was a problem hiding this comment.
done this way, i think it's a first good approach... but may be we should add a specific settings to configure the permission on the root directory when using the directory format.
There was a problem hiding this comment.
The update of the mode should be done also for the "group" and "other" bits, in a similar way. Apart from that, LGTM.
There was a problem hiding this comment.
IMO, just updating the permission for the directory like this (for the group and other as well) is enough to have something that work for the vast majority of use cases. So no need to add an extra configuration parameter for the directory format.
|
This fixes #132 Also, do not hesitate to rebase, the CI is fixed in the master branch |
261c57d to
1020c86
Compare
Add an option / configuration item to define the permissions for the dumps and hash files created by pg_back. The 0600 permissions mode is defined by default to ensure compatibility with the old (non configurable) approach. pg_back adds appropriate (+x) permission for parent directory when using the 'd' (AKA directory) dump format, files under the parent directory use the permission configured by the user (though backup_file_mode).
1020c86 to
a7e92a7
Compare
We add an option / configuration item to define the permissions for the dumps created with pg_back. We reuse / configure the 0600 permissions by default to ensure some compatibility with the old (non configurable) approach.
We probably need to discuss and improve what we should do for the parent directory when using 'd' for the dump format. Currently we still apply the 0700 mode in such situation, but maybe we should define a specific option or reuse the one we add right now.