advplyr / audiobookshelf

Self-hosted audiobook and podcast server
https://audiobookshelf.org
GNU General Public License v3.0
6.11k stars 432 forks source link

[Bug]: Book file inaccessible outside ABS after using "embed metadata" due to permissions change #3146

Closed bilbags closed 3 weeks ago

bilbags commented 1 month ago

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

  1. Access a book folder via Windows Explorer, e.g. Tim Schwab\2023 - The Bill Gates Problem {Tim Schwab}\The Bill Gates Problem.m4b
  2. Open M4B file with VLC & it plays without issue. Open M4B with MP3tag and it opens the file without any issue. I can copy, move, and delete the M4B file without issue.
  3. Go to ABS Web and edit book from step 1
  4. Go to Tools tab in editor and choose "Quick Embed"
  5. Verify log file does not report any errors
  6. Go back to Windows Explorer & attempt to open the M4B file with VLC. VLC throws an error "cannot open"
  7. Attempt to open the M4B file with MP3tag, does not work.
  8. Attempt to delete the M4B file from the directory in Windows Explorer. Receive error 0x0007003b image
  9. Login to Synology NAS and delete corrupted M4B file, and copy backup of original file (prior to using "quick embed") to the book folder.
  10. Open M4B file with VLC in Explorer without issue, and no issue with MP3tag either.
  11. Attempt to copy, delete, move the original M4B (no issues).
  12. ***See notes below - I checked permissions & found rights are being stripped after using "embed metadata"

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

2024-07-09 22:34:47.158 INFO 
[AudioMetadataManager] Starting metadata embed task Embedding metadata in audiobook "The Bill Gates Problem".

2024-07-09 22:35:03.347 INFO
[AudioMetadataManager] Successfully tagged audio file "/NonFiction/Tim Schwab/2023 - The Bill Gates Problem {Tim Schwab}/The Bill Gates Problem.m4b"

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) image

Modified file permissions (after using embed metadata option) image

mikiher commented 1 month ago

Thanks for reporting.

So just to be clear, a few questions:

  1. You said you're running Audiobookshelf on Docker. Is Docker running on your Windows system or on your Synology NAS?
  2. I understand your audiobooks library is stored on the NAS, correct?
  3. Can I see your Docker Compose settings?
  4. Where are the permission screenshots taken from? Windows? Synology NAS?
  5. How do you access your Synology NAS files from Windows (Network Drive? Some other option?)
bilbags commented 1 month ago
  1. I am running Docker on my Synology NAS.

  2. Yes, the libraries are stored on the NAS. My Windows 10 account has full rights to all shares.

  3. Docker: docker run -d --name=audiobookshelf \ -p 13378:80 \ -e PUID=xxxx \ -e PGID=xxx \ -v /volume1/docker/audiobookshelf:/config \ -v /volume1/media/audiobooks/GreatCourses:/GreatCourses \ -v /volume1/media/audiobooks/Fiction:/Fiction \ -v /volume1/media/audiobooks/NonFiction:/NonFiction \ -v /volume1/media/Podcasts:/Podcasts \ -v /volume1/Books/zmetadata:/metadata \ --restart always \ advplyr/audiobookshelf

  4. The screenshots were taken from the NAS. On Windows 10, the file has the "read only" flag and I cannot change it because my user no longer has rights to do so. If I reapply the rights explicitly on the NAS, everything goes back to normal.

  5. I access the NAS files from Windows through mapped network drives to the root NAS share "Media"

As noted previously, I noticed this issue several months ago when I first used ABS to convert an MP3 book to M4B. I tried converting a book earlier using ABS to see if the issue persists there as well, and it does.

Here are screenshots from the Windows side. The first one is from Explorer, where you can see the backup "copy"' original file and its attributes showing, and the "embed metadata" file without "copy" in the name and its attributes are not showing. image

This screenshot is the Windows properties of the original backup "copy" file image

This one is the Windows properties of the other file after I did "embed metadata" where the read-only flag is checked image

I used the "embed metadata" at least a couple hundred times in the past week as I have been making updates to my library, and never had this issue until the ABS upgrade except for when I use the "Make M4B Audiobook File" tool I noted above.

bilbags commented 1 month ago

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:

  1. It assigns full individual user rights for the active NAS admin account
  2. It assigns full individual user rights for the disabled admin account (which had deny all in the inherited rights)
  3. It assigns read permissions, take ownership, and change permissions to my user account

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.

advplyr commented 1 month ago

Passing PUID and PGID environment variables into the container isn't going to do anything. The UID and GID for the container should be set using the --user option.

Like -u UID:GID or --user UID:GID

See how that goes for you

bilbags commented 1 month ago

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.

nichwall commented 1 month ago

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 -user flag tells the entire container to run as a specific user, otherwise the container is run as the user that docker is running under (which is probably the deactivated admin account or somethibg, depending on what Synology is doing).

https://docs.docker.com/reference/cli/docker/container/run/

ABS did use the AUDIOBOOKSHELF_UID and AUDIOBOOKSHELF_GID environment variables in the past, but they were removed in 2.4.0 https://github.com/advplyr/audiobookshelf/releases/tag/v2.4.0

bilbags commented 1 month ago

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.

mikiher commented 1 month ago

So, just to make sure we all understand each other, what does your docker command line look like now?

bilbags commented 1 month ago

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?

nichwall commented 1 month ago

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.

mikiher commented 1 month ago

You said earlier:

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.

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:

  1. spawns an ffmpeg process to encode the input A into a single temp m4b output file B
  2. spawns an ffmpeg process (used to be Tone before 2.11.0) on B to encode ABS metadata and cover into another temp m4b output file C
  3. backs up A to the library item's cache path
  4. moves C to directory A was in.

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?

bilbags commented 1 month ago

@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?

bilbags commented 1 month ago

Things I've looked at and/or tried today:

  1. Checked rights in metadata volume - same as audiobooks folders
  2. Added PUID and PGID (sorry, I think I sometimes referred to this as GUID cause Windows) back and used my username instead of an admin, restarted ABS & tested with the same resultant issue.

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: https://github.com/ipsingh06/seedsync/issues/91

I also found the below at this link: https://wiki.servarr.com/docker-guide UMASK Many Docker images accept -e UMASK=002 as an environment variable and some software can be configured with a user, group and umask (NZBGet) or folder/file permission (Sonarr/Radarr), inside the container. This will ensure that files and folders created by one can be read and written by the others. If you are using existing folders and files, you will need to fix their current ownership and permissions too, but going forward they will be correct because you set each software up right.

rendyhd commented 1 month ago

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. All Permissions are gone (Properties -> Permissions), and clicking "Include inherited permissions" lets other see the files again.

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?

mikiher commented 4 weeks ago

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.

bilbags commented 4 weeks ago

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). https://github.com/sabnzbd/sabnzbd/issues/1989

rendyhd commented 4 weeks ago

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 :)

mikiher commented 4 weeks ago

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 https://github.com/advplyr/audiobookshelf/issues/3146#issuecomment-2254427715, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFMDFVXGSAP3XGCQMLCSHRTZOSZ6NAVCNFSM6AAAAABKTY6J76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJUGQZDONZRGU . You are receiving this because you were mentioned.Message ID: @.***>

mikiher commented 4 weeks ago

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:

github-actions[bot] commented 3 weeks ago

Fixed in v2.12.0.