Fix server owner permissions

[danielkurek]

I have stumbled upon a possible bug Server owner cannot start/restart/stop their own server if they have assigned some admin role without server update permission. I would expect that being a server owner would allow you to do anything with the server.

I am not sure if this is just a bug or if it is intentional. I have not read most of the codebase so I do not know if this change has some unwanted behaviour.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds an early owner check in GetUserPermissionsService::handle() to immediately return ['*'] for server owners and removes a duplicate later owner check. Admin and subuser permission-resolution paths remain unchanged. No public signatures were modified.

Changes

Cohort / File(s)	Summary
Permission retrieval logic app/Services/Servers/GetUserPermissionsService.php	Added an early owner check in handle() that returns ['*'] for owners and removed a redundant later owner check; admin and subuser branches unchanged; no signature changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as Caller
  participant S as GetUserPermissionsService

  U->>S: handle(user, server)
  rect rgba(200,235,255,0.28)
    note over S: New: Early owner check
    alt User is server owner
      S-->>U: return ["*"]
    else Not owner
      alt User is admin
        S-->>U: return admin permissions
      else
        S-->>U: return aggregated subuser permissions
      end
    end
  end

Loading

sequenceDiagram
  autonumber
  participant U as Caller
  participant S as GetUserPermissionsService

  U->>S: handle(user, server)
  rect rgba(255,245,200,0.28)
    note over S: Previous: Owner check occurred after admin/subuser logic
    alt User is admin
      S-->>U: return admin permissions
    else
      alt Later owner check true
        S-->>U: return ["*"]
      else
        S-->>U: return aggregated subuser permissions
      end
    end
  end

Loading

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "Fix server owner permissions" succinctly and accurately describes the primary change in this patch, which adds an early owner check to grant owners full permissions. It is concise, focused on the main intent, and avoids irrelevant detail. A reviewer scanning PR history will understand the primary purpose from this title.
Description Check	✅ Passed	The description directly describes the observed bug (server owners could not start/restart/stop when assigned an admin role lacking update permission) and aligns with the changeset that grants owners all permissions, so it is on-topic. It also states the submitter's expectation and uncertainty about unintended consequences, which is relevant context for reviewers. This satisfies the lenient requirement that the description be related to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

📜 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 99b343d and 993cca9.

📒 Files selected for processing (1)

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

🚧 Files skipped from review as they are similar to previous changes (1)

• app/Services/Servers/GetUserPermissionsService.php

---

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

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[danielkurek]

I have read the CLA Document and I hereby sign the CLA

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

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

20-22: Harden the owner check against ID type mismatches.

Strict comparison can fail if one side is string-cast (e.g., UUIDs) and the other int. Cast both to the same type or use Eloquent’s key getter.

Apply:

-        if ($user->id === $server->owner_id) {
+        if ((string) $user->getKey() === (string) $server->owner_id) {

---

20-22: Add an explicit test for the owner+admin edge case.

Cover: (1) pure owner → ['']; (2) owner with admin lacking 'update' → still ['']; (3) non-owner admin with only 'view' → limited set.

I can draft PHPUnit tests (feature or service-level) that set up a server, owner user, and an admin role without 'update', then assert the returned permissions.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f1ec20 and 99b343d.

📒 Files selected for processing (1)

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

🔇 Additional comments (1)

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

20-22: Early owner short-circuit is correct and resolves the reported issue.

Returning ['*'] for the server owner before admin/subuser resolution matches the expected behavior and prevents an admin role with restricted perms from masking ownership.

[Boy132]

Seems fine to me, this service is only used for checking subuser permissions.