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
Fix deleting subtitles with new SubtitleManagement permission #11507
base: master
Are you sure you want to change the base?
Conversation
Is there any reason why this is taged as blocked? It fixes an issue with a feature that is being shipped with 10.9.0, and I don't see why admins shouldn't be able to give (non-admin) users the ability to delete subtitles if they can already give users the ability to delete any media. |
At the very least I marked it as blocked as we are not making any unnecessary api changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting from my comment in the issue:
We should only allow the deletion of remote subtitles with the EnableSubtitleManagement policy, subtitles already in the media directory should be left untouched.
Only admin users should be allowed to modify the library contents. Other users should never have write access to library folders.
There is no reason why admins shouldn't be allowed to give part of their rights to other users. If admins can give other users complete admin rights, then they should also be able to give them partial admin rights. Furthermore, there is already an option to give users the rights to completely delete library items: If you can allow non-admin users to delete your whole library, then why shouldn't you be able to allow users that are already allowed to create subtitle files to also delete them? |
By this logic the permission should be split to an explicit "allow user to delete subtitles from these libraries". |
I'd be fine to allow subtitle deletion from the library folder if that "allow media deletion from:" setting is used. |
if I understand correctly @nielsvanvelzen , I should modify the PR so that the endpoint checks for both "enable subtitle management" and "allow media deletion from" ? |
Yes, so that would make the behavior for subtitle files inside of the library be:
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
0611bcb
to
b8a3747
Compare
I made the requested changes. I will also make a PR to jellyfin-web so that:
Sounds good ? But to be honest, I'm a bit frustrated since what I consider the most useful usecase is not addressed. Inded, I would have appreciated a way to let a user add and delete subtitles, without giving him the right to delete the full movie. |
I would also prefer this approach, if I just want someone to have control to fix/change/add subtitles, I'd prefer to not allow them to delete my complete library. |
Changes
Fix the delete subtitle endpoint. This is a missing component in #10410
Issues
Fix #11318