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

add EnableSubtitleManagement permission #10410

Merged

Conversation

Ch1nkara
Copy link
Contributor

@Ch1nkara Ch1nkara commented Oct 15, 2023

Add new feature: ability for a user to "Edit subtitles" without administrator rights

Changes
Create a new Jellyfin.Data/Enums/PermissionKind: EnableSubtitleManagement
Add that permission to the UserPolicy

Related PR

TODO

  1. fix policy jellyfin-plugin-opensubtitles#138
  2. fix regression in subtitle provider instantiation #10499

Issues
https://features.jellyfin.org/posts/185/non-admin-users-download-subtitles

@github-actions
Copy link

Changes in OpenAPI specification found. Expand to see details.

What's Changed


POST /Users/{userId}/Policy
Request:

Changed content type : application/json

Updated UserPolicy :

  • Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

Changed content type : text/json

Updated UserPolicy :

  • Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

Changed content type : application/*+json

Updated UserPolicy :

  • Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

GET /Users
Return Type:

Changed response : 200 OK

Users returned.

  • Changed content type : application/json

Changed items (object):

Class UserDto.

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="CamelCase"

Changed items (object):

Class UserDto.

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="PascalCase"

Changed items (object):

Class UserDto.

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

GET /Users/{userId}
Return Type:

Changed response : 200 OK

User returned.

  • Changed content type : application/json

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="CamelCase"

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="PascalCase"

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

POST /Users/{userId}
Request:

Changed content type : application/json

Updated UserDto :

  • Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

Changed content type : text/json

Updated UserDto :

  • Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

Changed content type : application/*+json

Updated UserDto :

  • Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

GET /Users/Me
Return Type:

Changed response : 200 OK

User returned.

  • Changed content type : application/json

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="CamelCase"

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="PascalCase"

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

POST /Users/New
Return Type:

Changed response : 200 OK

User created.

  • Changed content type : application/json

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="CamelCase"

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="PascalCase"

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

GET /Users/Public
Return Type:

Changed response : 200 OK

Public users returned.

  • Changed content type : application/json

Changed items (object):

Class UserDto.

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="CamelCase"

Changed items (object):

Class UserDto.

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="PascalCase"

Changed items (object):

Class UserDto.

openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

POST /Users/{userId}/Authenticate
Return Type:

Changed response : 200 OK

User authenticated.

  • Changed content type : application/json

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="CamelCase"

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="PascalCase"

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

POST /Users/AuthenticateByName
Return Type:

Changed response : 200 OK

User authenticated.

  • Changed content type : application/json

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="CamelCase"

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="PascalCase"

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

POST /Users/AuthenticateWithQuickConnect
Return Type:

Changed response : 200 OK

User authenticated.

  • Changed content type : application/json

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="CamelCase"

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

  • Changed content type : application/json; profile="PascalCase"

openapi-base openapi-changes.md openapi-head Changed property User (object)

Class UserDto.

Updated UserDto :
openapi-base openapi-changes.md openapi-head Changed property Policy (object)

Gets or sets the policy.

Updated UserPolicy :
openapi-base openapi-changes.md openapi-head Added property EnableSubtitleManagement (boolean)

Gets or sets a value indicating whether this instance can manage subtitles.

@cvium
Copy link
Member

cvium commented Oct 16, 2023

The endpoint used to download subtitles does not require admin: https://github.com/jellyfin/jellyfin/blob/master/Jellyfin.Api/Controllers/SubtitleController.cs#L138

I think it makes sense to restrict it. Maybe. But right now your PR is incomplete. The user policy should be set as a req on that endpoint.

@Ch1nkara
Copy link
Contributor Author

I think you either misunderstood my PR or I misunderstand your message.

The fact that the subtitle controller does not check for admin rights is fine in this case.
Currently, a user with no admin rights can't see the "edit subtitle" menu entry (in a movie view). The goal of that PR is to remedy to that.
An admin can choose to let a user add new subtitles to a movie (by using the opensubtile plugin for example).

Ideally, the subtitle controller should also restrict the rights to download, but the goal of that PR is to give more rights to non-admin users, not restrict them

@cvium
Copy link
Member

cvium commented Oct 16, 2023

You've misunderstood. The server is the authority on permissions. It doesn't make sense to make arbitrary restrictions in web if the server doesn't have the same restrictions. Anyone can edit the source in browser dev tools.

The subtitlecontroller must enforce the new user policy.

@Ch1nkara
Copy link
Contributor Author

Understood, thanks, I'll see to it and update the PR

@Ch1nkara
Copy link
Contributor Author

I think now the PR is complete.
Just to make sure my intent is clear: currently only an administrator had the option to "edit subtitles"
admin view
admin view

user view
user view

I want to give normal user (without admin rights) the ability to "edit subtitles". I now see two different ways to do that:

  1. This PR way, by adding a permission EnableSubtitleManagement (thus letting the admin choose who can "edit subtitles").
  2. By only contributing to jellyfin-web and let every user have the "edit subtitle" menu. Since there is currently no restriction server-side, there is no need to make changes to jellyfin in order to let normal users "edit subtitles".

I honestly don't really mind which way you guys prefer, as long as there is a way for normal users to download subtitles.

I'm open to discussion if you see other ways to achieve this requested feature.

@Ch1nkara Ch1nkara force-pushed the add-users-permission-to-edit-subtitles branch from e60d5e0 to c3b1997 Compare October 18, 2023 16:31
@Ch1nkara Ch1nkara force-pushed the add-users-permission-to-edit-subtitles branch from c3b1997 to 8ada8db Compare October 18, 2023 16:32
@nielsvanvelzen
Copy link
Member

I believe the server not checking for admin permissions right now is a bug. To fix it in a way that allows users to upload subtitles means adding the new permission for both the backend and frontend.

gaurovgiri

This comment was marked as spam.

Copy link
Member

@cvium cvium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good now

@cvium cvium merged commit db46056 into jellyfin:master Oct 27, 2023
18 checks passed
@Ch1nkara Ch1nkara deleted the add-users-permission-to-edit-subtitles branch October 27, 2023 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to sign into the opensubtitles plugin
5 participants