fix(api): use requestor's permissions for request approval

[kinggeorges12]

… than the logged in user account

In the POST /request API call, the permissions make the request and quotas are all inherited from the "requesting user". The only difference comes from the auto-approve permissions, which for some reason use the admin or API key holder permissions.

To make this code more consistent with the expected behavior, I updated the code to look at the requestUser permissions for auto-requests.

Description

The users reported a bug on my plugin that uses Jellyseerr: kinggeorges12/JellyBridge#17

I noticed that the behavior in this MediaRequest class was inconsistent, where the user requesting an item would be checked for quotas and requests (like do they have the ability to add requests AT ALL), but when checking for user overrides or auto-approve, the API keyholder or authorized user permissions were checked.

Bug: when requesting media through the API for another user, the approval process is skipped for the requesting user (userId in the API call for POST /requests). When creating new requests as the root account, the request is auto-approved since this permission cannot be changed for admin accounts.

What this means is that there is no way to make the approval process work when sending requests as another user from an admin account. Although this is noted in the API "If the user has the ADMIN or AUTO_APPROVE permissions, their request will be auomatically approved.", the comment is completely misleading, because the "user" it is talking about is NOT the userId provided by the function. I think this change brings it more in-line with expected behavior of this API endpoint.

I already tested this change here for v2.7.3: https://hub.docker.com/repository/docker/kinggeorges12/jellyseerr/tags/2.7.3-jb/

Screenshot (if UI-related)

To-Dos

• Disclosed any use of AI (see our policy)
• Successful build pnpm build
• Translation keys pnpm i18n:extract
• Database migration (if required)

Issues Fixed or Closed

• Fixes #XXXX

[gauthier-th]

Did you test this? I'm pretty sure using user is the correct way of doing this, especially for the override rules. We want to check that the user that made the request has the ADVANCED_REQUEST permission, not the user to whom the request was assigned. And I believe it's the same for the other changes.

There are 2 different things here:

• user: the person who made the request
• requestUser: the user to whom the request was assigned. By default it's the same as user, but can be changed by the user if he has MANAGE_REQUEST permission

[kinggeorges12]

Hi @gauthier-th , thanks for checking this PR. To answer your question: yes, I did test this using the docker image that I mentioned earlier, and tests pass for ensuring permissions of the logged in (or API caller) user to manage requests. Given two test accounts "usertest" and "root":

• I try to request a movie with root logged in providing the userId=2 ("usertest"), and the movie is sent to the queue for requests to accept/decline.
• If I try to request a movie with "usertest" logged in without specifying the userId, the request is also sent to the queue.
• If I try to request a movie with "usertest" logged in and specifying any userId, the error message appears { "message": "You do not have permission to modify the request user." }

I could not find any references to the "ADVANCED_REQUEST" permission in the code, so it may be an artifact of when this code was originally written. The comment on line 204 might now be attributed to the "MANAGE_REQUESTS" permission, which is required to actually submit the request.

The code flow is a bit confusing, because requestUser is initially set to user. However, on line 60 you can see this code flow that basically checks if the user is trying to create this request on behalf of another user:

    let requestUser = user;

    if (
      requestBody.userId &&
      !requestUser.hasPermission([
        Permission.MANAGE_USERS,
        Permission.MANAGE_REQUESTS,
      ])
    ) {
      throw new RequestPermissionError(
        'You do not have permission to modify the request user.'
      );
    } else if (requestBody.userId) {
      requestUser = await userRepository.findOneOrFail({
        where: { id: requestBody.userId },
      });
    }

From what I am reading, if the user is trying to request on behalf of another user, the userId would be set in the API call. If the current logged in user (which is referred to as requestUser because of the line let requestUser = user) is missing the MANAGE_REQUESTS or MANAGE_USERS permissions, then an error is thrown. Then at line 68, having passed the permission check for the API caller, the userId provided in the API call is set to the requestUser.

The only change I would make to my submission is to make the logic flow more clear on these lines I quoted above. The current state makes it ambiguous as to whose permissions are actually getting checked, since the requestUser changes from the API caller to the userId in the API call. In fact, I would say this if statement is TOO stringent, and should only check the MANAGE_USERS permission if the userId provided is different than the current logged in user, since the REQUEST_TV or REQUEST_MOVIE permissions for the request user is already checked in the code following this user permission check (lines 78-110).

Also, in debugging this i noticed an error in the code for existing requests. When checking for is4k in the request body on line 165, the where clause does not specify a default is4k value. This allows users to send multiple requests for non-4k content without hitting the error { "message": "Request for this media already exists." }.

      .where('request.is4k = :is4k', { is4k: requestBody.is4k })

For this other bug, I can create an additional pull request if needed.

[kinggeorges12]

Did you test this? I'm pretty sure using user is the correct way of doing this, especially for the override rules.

I had a question about this part of your comment, as I have not used override rules. The override rules seem to be for certain rules of that request user to change these request attributes: rootFolder, profileId, tags. When requesting content on behalf of another user, wouldn't you want to use the override rules for the target user of the request, not the override rules for the API caller?

[fallenbagel]

I'm kinda confused what you're trying to accomplish here. What was the issue? (Apologies it's still not very clear).

If your plugin connects to seerr using api-keys, it is expected that the request is handled as a superuser request being requested on behalf of someone. That is the expected behaviour. For the request to be handled as a certain user, it needs to be cookie-auth based. This is how our web client works. This is how other third-party streaming apps that has seerr support also works.