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

Fix deleting subtitles with new SubtitleManagement permission #11507

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ch1nkara
Copy link
Contributor

@Ch1nkara Ch1nkara commented May 7, 2024

Changes
Fix the delete subtitle endpoint. This is a missing component in #10410

Issues
Fix #11318

@crobibero crobibero changed the title fix #11318 by editing the delete subtitle endpoint Fix deleting subtitles with new SubtitleManagement permission May 7, 2024
crobibero
crobibero previously approved these changes May 7, 2024
@crobibero crobibero added this to In progress in Release 10.9.z via automation May 7, 2024
@crobibero crobibero removed this from In progress in Release 10.9.z May 7, 2024
@crobibero crobibero added the blocked Blocked by another pull request label May 7, 2024
@Simsala91
Copy link

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.

@crobibero
Copy link
Member

At the very least I marked it as blocked as we are not making any unnecessary api changes.

Copy link
Member

@nielsvanvelzen nielsvanvelzen left a 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.

@Simsala91
Copy link

Simsala91 commented May 11, 2024

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:

Jellyfin Media Deletion Option

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?

@crobibero
Copy link
Member

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:

Jellyfin Media Deletion Option

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".

@nielsvanvelzen
Copy link
Member

I'd be fine to allow subtitle deletion from the library folder if that "allow media deletion from:" setting is used.

@Ch1nkara
Copy link
Contributor Author

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

@nielsvanvelzen
Copy link
Member

Yes, so that would make the behavior for subtitle files inside of the library be:

  • Admin can always delete
  • User with subtitle management permission can delete if they have delete access for that library

@jellyfin-bot
Copy link
Contributor

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@jellyfin-bot jellyfin-bot added merge conflict Merge conflicts should be resolved before a merge and removed merge conflict Merge conflicts should be resolved before a merge labels May 15, 2024
@Ch1nkara
Copy link
Contributor Author

Ch1nkara commented May 17, 2024

I made the requested changes. I will also make a PR to jellyfin-web so that:

  1. "Allow this user to edit subtitles" is changed to "Allow this user to add subtitles"
  2. the trash logo from the "edit subtitle" menu is hidden if the user do not have "deletion authorization"

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.

@Ch1nkara
Copy link
Contributor Author

Ch1nkara commented May 17, 2024

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:
Jellyfin Media Deletion Option
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 was re-reading the conversation, and before going for the submitted PR, is this alternative approach out of question ? to split the "allow user to edit subtitles" into "allow user to add subtitles" and allow user to delete subtitles" ?

@Simsala91
Copy link

I was re-reading the conversation, and before going for the submitted PR, is this alternative approach out of question ? to split the "allow user to edit subtitles" into "allow user to add subtitles" and allow user to delete subtitles" ?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by another pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue]: Can't manually add or delete subtitles
6 participants