-
Notifications
You must be signed in to change notification settings - Fork 473
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
[Bug]: Book file inaccessible outside ABS after using "embed metadata" due to permissions change #3146
Comments
Thanks for reporting. So just to be clear, a few questions:
|
Quick postscript - I looked at this a bit more and discovered all inherited rights are being stripped from the file after embedding metadata and individual account rights are being assigned as follows:
Prior to the convert or the embed metadata, the disabled admin account had no rights, and my user account had full rights along with 4 other users. |
Passing Like See how that goes for you |
I am not running Docker Compose, and I pulled that from the original installation script I used to setup ABS. I don't know how to view the current Docker config for ABS, but these settings also show up in the ABS container settings I can see (but not modify) in Container Manager. |
The PUID and GUID are environment variables that depend on the application within the container using the environment variables, but ABS does not use those environment variables. The https://docs.docker.com/reference/cli/docker/container/run/ ABS did use the |
Thanks for the info, @nichwall. I removed the PUID & GUID environment variables and I confirmed Docker is running under the current, active admin account. Just tested everything, and the issue still persists. I have been running ABS on this NAS since late 2020 or early 2021. |
So, just to make sure we all understand each other, what does your docker command line look like now? |
I have checked all of my Docker containers and each one of them contains environment variables defining the PUID and GUID. As @nichwall said above, ABS doesn't use those environment variables. However, Docker on Synology does. If I didn't define those, then Docker would automatically run a container with the root account, which is not a best security practice. The PUID and GUID tell Docker on Synology what account to run the container under. The defined IDs have remained the same ever since I first installed ABS several years ago. The emphasis on the 2 env variables which ABS doesn't use but Docker on Synology does for what account to run under seems misplaced. Wouldn't the issue I have reported existed prior to the latest version? It did not appear until I installed the latest version of ABS. I have clearly explained the behavior I am seeing - when rewriting the M4B file to embed metadata, the inherited rights are stripped and my user is given explicit read/only access, and the account that the docker container is running under is given explicit full access. This only happens when using the embed metadata or conversion tools. I can do a match to Audible on a book & save it, which updates the metadata.json file, but the inherited rights to the metadata.json file are not removed. If this issue was related to the specified user the container is running under, wouldn't the issue occur with every file that it is saving or updating? |
I'm not finding anything in Synology documentation or forums saying the environment variables PUID and GUID need to be set to work as running as another user, can you provide a link? The reason we mention it is because permissions issues are reported periodically when people use PUID/GUID environment variables to try and control their permissions. I don't know specifically what systems everyone who has reported issues were running on, so it very well could be those specific environment variables are needed for Synology. The PUID and GUID environment variables were popularized by LinuxServer.IO containers https://docs.linuxserver.io/general/understanding-puid-and-pgid/ Either way, probably isn't these permissions then like you said. |
You said earlier:
The last version actually had a significant change in this area (#3111 - Replacement of Tone with ffmpeg). In a nuthshell, what the m4b conversion tool is doing is the following:
The M4b Embed tool is roughly similar to the above, but skipping step 1. Since this involves spawned processes which read and write files, I believe permissions are very relevant here. Perhaps spawned process permissions behave differently in Docker (or specifically in Docker on Synology). I have a Synology NAS, so I'll try to install Audiobookshelf on it and see if I can repro the problem. In the meantime, I wanted to make sure you have the correct PUID and GUID set - have you read through this article? |
@nichwall - Synology's Docker documentation sucks, and honestly, people get confused about using PUID and GUID because nothing is 100% clear on it. I used Marius Hosting to configure my Docker & containers (@mikiher) originally. I removed the environment variables for GUID & PUID from the ABS container, and still get the same results. I had read about the switch from Tone to ffmpeg, but I think somewhere in that process, the rights are getting changed in both instances. I'm trying to figure out what would cause the inherited rights to be removed and new explicit rights assigned (giving my account read-only access in that process). When using the embed metadata function, it writes the temporary file in the same directory the book is in... so losing the inherited rights baffles me. What makes it harder to understand is that my user login has RW access to the entire mounted volume, so having it get assigned explicit RO rights makes me wonder - where & how is that happening? |
Things I've looked at and/or tried today:
I really thought #2 might do something different, but I was wrong. I've been doing some reading about inherited rights being stripped with new files with other software running on Synology Docker. I'm at the point where I think it's likely a weird quirk with Synology. I really want ABS to stay in Docker on my NAS, and I may have to write & schedule a script to reapply inherited rights after I've done some updates to my library. Here's a reference (one of a couple) that I found talking about a similar issue: I also found the below at this link: https://wiki.servarr.com/docker-guide |
I have the same issue, when using embed data on v2.11.0 (Synology, Docker (via Portainer)), the files can't be seen anymore. Couldn't figure it out before you mentioning permissions. I can confirm the issue is that all Permissions are removed. When I revert back to v2.10.1 I don't have the issue. It also looks like v2.11.0 creates a .tmp. file first, while I don't observe that in the previous versions. Maybe it has something to do with creating a new file instead of updating the existing? |
As I said above, there was big change in this area in v2.11.0 (moving from Tone to ffmpeg for embedding), and I'm pretty sure it's related to that. ffmpeg has to write its output to file that's different from the input (that is the way ffmpeg works), and that is the likely temp file you're seeing, which is then copied back over the original file, presumably altering its permissions. I have not had a chance to put up an ABS Docker instance on Synology to invistigate this. I will do this in the coming week. Stay tuned. |
It is definitely tied to ffmpeg - for whatever reason the inherited ACL is not pplyied, but explicit permissions are being set on the created temp file which gets carried over when it overwrites the original file. I'm really curious if it's a DSM7 issue, a Synology Docker issue, an ffmpeg issue, or a combination of things. If my old Synology box hadn't died last month, I could have tested it out under DSM6. Can't wait to see what you find, @mikiher I did find a discussion with this problem using SABnzbd in the same environment (DSM7, Docker). |
Thanks! I've been trying to test something quick and crude by adding. |
I already tried (and you also need chown, since the tmp file is written
with root user and group). It might solve the access issue, but it still
does not preserve the extended acls of the original file, which is not good.
There are a couple of options I'm testing:
1. Use "cp --no-preserve=all", which just copies the contents and leaves
all the attributes of the destination file intact.
2. Open the destination file for write and pipe the contents of the tmp
file to it. This way the destination file attributes should remain
unchanged as well.
I'm leaning towards option 2, since it doesn't require running exec (the
no-preserve option is not acceible through node.js apis).
Will post progress when I make some.
…On Sun, Jul 28, 2024, 12:22 rendyhd ***@***.***> wrote:
As I said above, there was big change in this area in v2.11.0 (moving from
Tone to ffmpeg for embedding), and I'm pretty sure it's related to that.
ffmpeg has to write its output to file that's different from the input
(that is the way ffmpeg works), and that is the likely temp file you're
seeing, which is then copied back over the original file, presumably
altering its permissions.
I have not had a chance to put up an ABS Docker instance on Synology to
invistigate this. I will do this in the coming week. Stay tuned.
Thanks! I've been trying to test something quick and crude by adding.
const sourceStats = await fs.stat(audioFilePath);
await fs.chmod(tempFilePath, sourceStats.mode);
before the
await fs.move(tempFilePath, audioFilePath, { overwrite: true })
BUT I'm having all sorts of issues trying to test a docker image on
Synology, and I don't have a clue if this would actually work. Just a
thought :)
—
Reply to this email directly, view it on GitHub
<#3146 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFMDFVXGSAP3XGCQMLCSHRTZOSZ6NAVCNFSM6AAAAABKTY6J76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGQZDONZRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
OK, I just submitted a fix, which essentially impements approach 2 in my comment above. A few more details from my analysis of the problem:
I tried the following solutions:
|
Fixed in v2.12.0. |
What happened?
Prior to upgrading to v2.11.0, I could open book files from within ABS or the file share from Windows Explorer using VLC. I could also copy and/or delete the book file from Explorer. If I use the "embed metadata" option on a book, I can no longer open it in VLC and I lose rights to the file, meaning I cannot copy, move, or delete the file from Explorer any longer. I have tested this 3x and the issue continuously repeats itself. However, I can play the book in ABS Web.
What did you expect to happen?
To be able to open, copy, move, or delete book files from the file system like I could prior to upgrading, even after using "embed metadata"
Steps to reproduce the issue
Audiobookshelf version
v2.11.0
How are you running audiobookshelf?
Docker
What OS is your Audiobookshelf server hosted from?
Other (list in "Additional Notes" box)
If the issue is being seen in the UI, what browsers are you seeing the problem on?
None
Logs
Additional Notes
I am logged into ABS Web with the same admin account I've always used when doing updates to books.
This same issue occurred with the previous 2 ABS versions I had installed when I used the "Make M4B Audiobook File" tool to convert MP3s to M4B, so I stopped using that feature and manually did it using auto-m4b.
Well, I just decided to check file permissions to see if ABS was changing the rights on the file. Sure enough, it is changing the rights on the file to read only, write permissions removed. It was not doing that previously. I had files I used the "embed metatdata" option on yesterday and the permissions on those files did not get changed.
Original file permissions (before using embed metadata option)
Modified file permissions (after using embed metadata option)
The text was updated successfully, but these errors were encountered: