Fix user permissions service

[Boy132]

Closes #1796
Supersedes #1814

[coderabbitai]

📝 Walkthrough

Walkthrough

Updated GetUserPermissionsService to use explicit owner/admin flags, centralize admin websocket permissions, change owner-only behavior to exclude admins, and simplify subuser permission retrieval with null coalescing.

Changes

Cohort / File(s)

Summary

Permission Service Refactoring app/Services/Servers/GetUserPermissionsService.php

Imported Permission model; introduced $isOwner and $isAdmin flags; owner-only return of ['*'] now applies only when owner and not admin; extracted admin websocket permissions into $adminPermissions; adjusted admin branch to return either ['*'] + $adminPermissions or Permission::ACTION_WEBSOCKET_CONNECT + $adminPermissions depending on update/owner checks; renamed $subuserPermissions to $subuser and return $subuser->permissions ?? [].

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Service as GetUserPermissionsService
  note right of Service #f3f4f6: Inputs: user, server, subuser (optional)

  Client->>Service: requestPermissions(user, server, subuser)
  Service->>Service: determine $isOwner, $isAdmin
  alt owner AND NOT admin
    Service-->>Client: return ['*']
  else admin
    Service->>Service: $adminPermissions = [websocket-related]
    alt canUpdate OR isOwner
      Service-->>Client: return ['*'] + $adminPermissions
    else
      Service-->>Client: return [Permission::ACTION_WEBSOCKET_CONNECT] + $adminPermissions
    end
  else subuser present
    Service->>Service: $subuser = ... (rename)
    Service-->>Client: return $subuser->permissions ?? []
  else
    Service-->>Client: return []
  end

Loading

Pre-merge checks

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "Fix user permissions service" is directly related to the changeset, which modifies the GetUserPermissionsService.php file. The title accurately reflects the primary change—fixing permission handling logic in the user permissions service. It is clear, specific, and concise without unnecessary verbosity.
Linked Issues Check	✅ Passed	Issue #1796 requires fixing console output visibility during server installation/reinstallation. The code changes to GetUserPermissionsService.php address permission handling and introduce websocket-related permissions through the new $adminPermissions array and Permission::ACTION_WEBSOCKET_CONNECT usage. These permission modifications are relevant to enabling proper console access and communication, which aligns with the issue's requirements for console output to be displayed during installation processes.
Out of Scope Changes Check	✅ Passed	All code changes are confined to GetUserPermissionsService.php and involve refactoring permission logic for owners, admins, and subusers. The modifications, including updates to websocket permission handling and permission array management, are directly related to fixing user permissions in scope with issue #1796. No unrelated or out-of-scope changes are evident in the provided changeset summary.
Description Check	✅ Passed	The PR description references issue #1796 by linking it ("Closes #1796"), which connects the changeset to the console output problem during installation/reinstallation. While minimal in detail, the description is related to the changeset through issue linkage and establishes the rationale for the permissions service modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 12aa957 and add5f40.

📒 Files selected for processing (1)

• app/Services/Servers/GetUserPermissionsService.php (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

app/Services/Servers/GetUserPermissionsService.php (2)

app/Models/Permission.php (2)

• Permission (11-221)
• permissions (194-213)

app/Models/User.php (2)

• isAdmin (384-387)
• can (357-370)

🔇 Additional comments (2)

app/Services/Servers/GetUserPermissionsService.php (2)

5-5: LGTM: importing Permission model

Needed to reference Permission::ACTION_WEBSOCKET_CONNECT. No concerns.

---

22-22: Verify design intent: does view-only admin require admin.websocket. permissions?*

No recursion risk detected—ServerPolicy::before() intercepts calls to $user->can('view', $server) and $user->can('update', $server) without re-entering GetUserPermissionsService.

However, the logic grants admin.websocket.* permissions to any admin who can pass the can('view') OR can('update') check (line 22), including view-only admins. Confirm this is intentional, as it may be overly permissive if only update-admins should access transfer logs.

Separately, apply the optional refactoring for readability:

• Extract $canView and $canUpdate variables to avoid repeated policy checks
• Replace the magic string array with a class constant ADMIN_WEBSOCKET_PERMISSIONS

         $isOwner = $user->id === $server->owner_id;
-        $isAdmin = $user->isAdmin() && ($user->can('view', $server) || $user->can('update', $server));
+        $canView = $user->can('view', $server);
+        $canUpdate = $user->can('update', $server);
+        $isAdmin = $user->isAdmin() && ($canView || $canUpdate);

         if ($isOwner && !$isAdmin) {
             return ['*'];
         }

-        $adminPermissions = [
-            'admin.websocket.errors',
-            'admin.websocket.install',
-            'admin.websocket.transfer',
-        ];

         if ($isAdmin) {
-            return $isOwner || $user->can('update', $server) ? array_merge(['*'], $adminPermissions) : array_merge([Permission::ACTION_WEBSOCKET_CONNECT], $adminPermissions);
+            return ($isOwner || $canUpdate)
+                ? array_merge(['*'], self::ADMIN_WEBSOCKET_PERMISSIONS)
+                : array_merge([Permission::ACTION_WEBSOCKET_CONNECT], self::ADMIN_WEBSOCKET_PERMISSIONS);
         }

Add class constant:

private const ADMIN_WEBSOCKET_PERMISSIONS = [
    'admin.websocket.errors',
    'admin.websocket.install',
    'admin.websocket.transfer',
];

Also applies to: 28–32, 34–36

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a4fa5e and 12aa957.

📒 Files selected for processing (1)

• app/Services/Servers/GetUserPermissionsService.php (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

app/Services/Servers/GetUserPermissionsService.php (2)

app/Models/Permission.php (2)

• Permission (11-221)
• permissions (194-213)

app/Models/User.php (3)

• isAdmin (384-387)
• can (357-370)
• subusers (309-312)

🔇 Additional comments (2)

app/Services/Servers/GetUserPermissionsService.php (2)

21-26: LGTM! Clean separation of owner and admin logic.

The explicit flags improve readability, and the logic correctly ensures that users who are both owner and admin are routed through the admin block to receive admin-specific permissions.

---

28-36: Admin permissions refactor correctly addresses empty console during installation.

The refactoring centralizes admin websocket permissions (admin.websocket.errors, admin.websocket.install, admin.websocket.transfer) into a dedicated array. The admin.websocket.install permission is essential—Wings validates this permission before sending installation event logs to the connected websocket, enabling console output during server installation and reinstallation. This fix resolves issue #1796.

[notAreYouScared]

Appears to work as intended.