Fix WHM activation failing when package already exists (#4565)

* Fix WHM activation failing when package already exists (#4560)

- Request listpkgs with want=all and parse both legacy package and modern data.pkg shapes
- On addpkg already-exists, sync via editpkg and continue; only block activation on other errors
- Include WHM reason in Server_Exception and fix placeholder keys

* Add WHM API terms to spellcheck dictionary

* Address review: use typed exception for WHM already-exists, keep reasons out of API errors

- Add Server_PackageExistsException; request() now throws generic messages and translates already-exists at the WHM boundary
- checkPackageExists() catches the typed exception instead of matching getMessage()

* Fix Rector findings: remove unused catch variable and setAccessible call

* Handle modern WHM metadata.result failures in Whm::request

---------

Co-authored-by: opencode <opencode@localhost>

cspell.json

@@ -622,9 +622,11 @@
         "unemailml",
         "listpkgs",
         "addpkg",
+        "editpkg",
         "createacct",
         "setupreseller",
         "setacls",
+        "statusmsg",
         "acllist",
         "creationtime",
         "domsecret",

src/library/Server/Manager/Whm.php

@@ -534,30 +534,37 @@ class Server_Manager_Whm extends Server_Manager
      */
     public function getPackages(): array
     {
-        // Send a request to the WHM server to list the packages
-        $pkgs = $this->request('listpkgs');
+        // Send a request to the WHM server to list the packages.
+        // `want=all` ensures reseller tokens also see packages they cannot
+        // create accounts with, which plain `listpkgs` may filter out.
+        $pkgs = $this->request('listpkgs', ['want' => 'all']);
+        $packages = $this->extractPackageList($pkgs);
         $return = [];
 
         // Iterate over the packages and add their details to the return array
-        foreach ($pkgs->package as $pkg) {
+        foreach ($packages as $pkg) {
+            $name = (string) $this->getPackageField($pkg, ['name'], '');
+            if ($name === '') {
+                continue;
+            }
             $return[] = [
-                'title' => $pkg->name,
-                'name' => $pkg->name,
-                'feature_list' => $pkg->FEATURELIST,
-                'theme' => $pkg->CPMOD,
-                'quota' => $pkg->QUOTA,
-                'bandwidth' => $pkg->BWLIMIT,
-                'max_ftp' => $pkg->MAXFTP,
-                'max_sql' => $pkg->MAXSQL,
-                'max_emails' => $pkg->MAXLST,
-                'max_sub' => $pkg->MAXSUB,
-                'max_pop' => $pkg->MAXPOP,
-                'max_park' => $pkg->MAXPARK,
-                'max_addon' => $pkg->MAXADDON,
-                'has_shell' => $pkg->HASSHELL == 'n' ? 0 : 1,
-                'has_ip' => $pkg->IP == 'n' ? 0 : 1,
-                'has_cgi' => $pkg->CGI == 'n' ? 0 : 1,
-                'has_frontpage' => $pkg->FRONTPAGE == 'n' ? 0 : 1,
+                'title' => $name,
+                'name' => $name,
+                'feature_list' => $this->getPackageField($pkg, ['FEATURELIST', 'featurelist']),
+                'theme' => $this->getPackageField($pkg, ['CPMOD', 'cpmod']),
+                'quota' => $this->getPackageField($pkg, ['QUOTA', 'quota']),
+                'bandwidth' => $this->getPackageField($pkg, ['BWLIMIT', 'bwlimit']),
+                'max_ftp' => $this->getPackageField($pkg, ['MAXFTP', 'maxftp']),
+                'max_sql' => $this->getPackageField($pkg, ['MAXSQL', 'maxsql']),
+                'max_emails' => $this->getPackageField($pkg, ['MAXLST', 'maxlst']),
+                'max_sub' => $this->getPackageField($pkg, ['MAXSUB', 'maxsub']),
+                'max_pop' => $this->getPackageField($pkg, ['MAXPOP', 'maxpop']),
+                'max_park' => $this->getPackageField($pkg, ['MAXPARK', 'maxpark']),
+                'max_addon' => $this->getPackageField($pkg, ['MAXADDON', 'maxaddon']),
+                'has_shell' => $this->getPackageField($pkg, ['HASSHELL', 'hasshell'], 'n') == 'n' ? 0 : 1,
+                'has_ip' => $this->getPackageField($pkg, ['IP', 'ip'], 'n') == 'n' ? 0 : 1,
+                'has_cgi' => $this->getPackageField($pkg, ['CGI', 'cgi'], 'n') == 'n' ? 0 : 1,
+                'has_frontpage' => $this->getPackageField($pkg, ['FRONTPAGE', 'frontpage'], 'n') == 'n' ? 0 : 1,
                 'free_registration' => 0,
                 'free_transfer' => 0,
                 'free_renewal' => 0,
@@ -633,37 +640,62 @@ class Server_Manager_Whm extends Server_Manager
         }
 
         if (isset($json->cpanelresult->error)) {
-            $this->getLog()->critical('WHM server response error calling action {action}: "{error}"', ['action' => $action, 'error' => $json->cpanelresult->error]);
-            $placeholders = ['action' => $action, 'type' => 'cPanel'];
+            $reason = (string) $json->cpanelresult->error;
 
-            throw new Server_Exception('Failed to :action: on the :type: server, check the error logs for further details', $placeholders);
+            $this->throwWhmError($action, $reason);
         }
 
         if (isset($json->data->result) && $json->data->result == '0') {
-            $this->getLog()->critical('WHM server response error calling action {action}: "{error}"', ['action' => $action, 'error' => $json->data->reason]);
-            $placeholders = [':action:' => $action, ':type:' => 'cPanel'];
+            $reason = (string) ($json->data->reason ?? 'Unknown error');
 
-            throw new Server_Exception('Failed to :action: on the :type: server, check the error logs for further details', $placeholders);
+            $this->throwWhmError($action, $reason);
         }
 
         if (isset($json->result) && is_array($json->result) && $json->result[0]->status == 0) {
-            $this->getLog()->critical('WHM server response error calling action {action}: "{error}"', ['action' => $action, 'error' => $json->result[0]->statusmsg]);
-            $placeholders = [':action:' => $action, ':type:' => 'cPanel'];
+            $reason = (string) ($json->result[0]->statusmsg ?? 'Unknown error');
 
-            throw new Server_Exception('Failed to :action: on the :type: server, check the error logs for further details', $placeholders);
+            $this->throwWhmError($action, $reason);
         }
 
         if (isset($json->status) && $json->status != '1') {
-            $this->getLog()->critical('WHM server response error calling action {action}: "{error}"', ['action' => $action, 'error' => $json->statusmsg]);
-            $placeholders = [':action:' => $action, ':type:' => 'cPanel'];
+            $reason = (string) ($json->statusmsg ?? 'Unknown error');
 
-            throw new Server_Exception('Failed to :action: on the :type: server, check the error logs for further details', $placeholders);
+            $this->throwWhmError($action, $reason);
+        }
+
+        if (isset($json->metadata->result) && $json->metadata->result == '0') {
+            $reason = (string) ($json->metadata->reason ?? 'Unknown error');
+
+            $this->throwWhmError($action, $reason);
         }
 
         // Return the response
         return $json;
     }
 
+    /**
+     * Logs the WHM failure reason and throws a generic exception.
+     *
+     * The full WHM reason stays in the logs and is never exposed to API
+     * callers or stored in order history. An "already exists" WHM response
+     * is translated to a dedicated exception so callers can handle it
+     * without matching on message text.
+     *
+     * @throws Server_Exception
+     */
+    private function throwWhmError(string $action, string $reason): never
+    {
+        $this->getLog()->critical('WHM server response error calling action {action}: "{error}"', ['action' => $action, 'error' => $reason]);
+        $placeholders = [':action:' => $action, ':type:' => 'cPanel'];
+        $message = 'Failed to :action: on the :type: server, check the error logs for further details';
+
+        if (str_contains(strtolower($reason), 'already exists')) {
+            throw new Server_PackageExistsException($message, $placeholders);
+        }
+
+        throw new Server_Exception($message, $placeholders);
+    }
+
     /**
      * Checks if a package exists on the WHM server.
      *
@@ -677,14 +709,16 @@ class Server_Manager_Whm extends Server_Manager
         // Get the name of the package
         $name = $this->getPackageName($package);
 
-        // Send a request to the WHM server to list the packages
-        $json = $this->request('listpkgs');
-        $packages = $json->package;
+        // Send a request to the WHM server to list the packages.
+        // `want=all` lists every package on the system instead of only the
+        // ones the API user may use during account creation (see #4560).
+        $json = $this->request('listpkgs', ['want' => 'all']);
+        $packages = $this->extractPackageList($json);
 
         // Check if the package exists
         $exists = false;
         foreach ($packages as $p) {
-            if ($p->name == $name) {
+            if ((string) $this->getPackageField($p, ['name'], '') === $name) {
                 $exists = true;
 
                 break;
@@ -693,27 +727,104 @@ class Server_Manager_Whm extends Server_Manager
 
         // If the package does not exist and the $create parameter is true, create the package
         if (!$exists && $create) {
-            $varHash = [
-                'name' => $name,
-                'quota' => $package->getQuota(),
-                'bwlimit' => $package->getBandwidth(),
-                'maxsub' => $package->getMaxSubdomains(),
-                'maxpark' => $package->getMaxParkedDomains(),
-                'maxaddon' => $package->getMaxDomains(),
-                'maxftp' => $package->getMaxFtp(),
-                'maxsql' => $package->getMaxSql(),
-                'maxpop' => $package->getMaxPop(),
-                'cgi' => $package->getCustomValue('cgi'),
-                'cpmod' => $package->getCustomValue('cpmod'),
-                'maxlst' => $package->getCustomValue('maxlst'),
-                'hasshell' => $package->getCustomValue('hasshell'),
-            ];
+            $varHash = $this->buildPackageParams($package, $name);
 
-            // Send a request to the WHM server to add the package
-            $this->request('addpkg', $varHash);
+            try {
+                $this->request('addpkg', $varHash);
+            } catch (Server_PackageExistsException) {
+                // listpkgs only returns packages visible to the API user, while
+                // addpkg fails globally when the name is taken. When the
+                // package already exists (e.g. created manually or by another
+                // user), sync its limits via editpkg instead of aborting the
+                // account creation. See #4560.
+                try {
+                    $this->request('editpkg', $varHash);
+                    $this->getLog()->info('Updated existing WHM package {package}', ['package' => $name]);
+                } catch (Server_Exception $editException) {
+                    // The package exists but cannot be edited (e.g. owned by
+                    // another user). Account creation can still use it, so do
+                    // not block activation - just log and continue.
+                    $this->getLog()->warning('WHM package {package} already exists and could not be updated: {error}', ['package' => $name, 'error' => $editException->getMessage()]);
+                }
+            }
         }
     }
 
+    /**
+     * Builds the WHM API parameters for creating or updating a package.
+     * addpkg and editpkg share the same shape, differing only in the verb.
+     *
+     * @return array<string, mixed>
+     */
+    private function buildPackageParams(Server_Package $package, string $name): array
+    {
+        return [
+            'name' => $name,
+            'quota' => $package->getQuota(),
+            'bwlimit' => $package->getBandwidth(),
+            'maxsub' => $package->getMaxSubdomains(),
+            'maxpark' => $package->getMaxParkedDomains(),
+            'maxaddon' => $package->getMaxDomains(),
+            'maxftp' => $package->getMaxFtp(),
+            'maxsql' => $package->getMaxSql(),
+            'maxpop' => $package->getMaxPop(),
+            'cgi' => $package->getCustomValue('cgi'),
+            'cpmod' => $package->getCustomValue('cpmod'),
+            'maxlst' => $package->getCustomValue('maxlst'),
+            'hasshell' => $package->getCustomValue('hasshell'),
+        ];
+    }
+
+    /**
+     * Extracts the raw package entries from a listpkgs response.
+     * Handles both the legacy `{"package": [...]}` shape and the modern
+     * WHM API `{"data": {"pkg": [...]}}` shape.
+     *
+     * @return array<int, object>
+     */
+    private function extractPackageList(mixed $json): array
+    {
+        $raw = null;
+        if (is_object($json)) {
+            if (isset($json->package)) {
+                $raw = $json->package;
+            } elseif (isset($json->data->pkg)) {
+                $raw = $json->data->pkg;
+            }
+        }
+
+        if ($raw === null) {
+            return [];
+        }
+
+        if (is_object($raw)) {
+            return [$raw];
+        }
+
+        return is_array($raw) ? array_values($raw) : [];
+    }
+
+    /**
+     * Case-insensitive field lookup for listpkgs entries, whose keys are
+     * upper-case in the legacy API and lower-case in the modern WHM API.
+     */
+    private function getPackageField(mixed $pkg, array $candidates, mixed $default = null): mixed
+    {
+        if (!is_object($pkg)) {
+            return $default;
+        }
+
+        $lowered = array_change_key_case(get_object_vars($pkg), CASE_LOWER);
+        foreach ($candidates as $candidate) {
+            $lookup = strtolower((string) $candidate);
+            if (array_key_exists($lookup, $lowered)) {
+                return $lowered[$lookup];
+            }
+        }
+
+        return $default;
+    }
+
     /**
      * Generates a package name based on the WHM server's username and the package's name.
      *

src/library/Server/PackageExistsException.php

@@ -0,0 +1,13 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * Copyright 2022-2025 FOSSBilling
+ * SPDX-License-Identifier: Apache-2.0.
+ *
+ * @copyright FOSSBilling (https://www.fossbilling.org)
+ * @license   http://www.apache.org/licenses/LICENSE-2.0 Apache-2.0
+ */
+class Server_PackageExistsException extends Server_Exception
+{
+}

tests/Unit/Server/Manager/WhmTest.php

@@ -114,3 +114,150 @@ test('createAccount never calls setupreseller or setacls for a non-reseller acco
     $actions = array_map(actionOf(...), $requests);
     expect($actions)->toBe(['listpkgs', 'addpkg', 'createacct']);
 });
+
+test('createAccount skips addpkg when the package is already listed (legacy format)', function (): void {
+    $requests = [];
+    $client = new MockHttpClient(function (string $method, string $url, array $options) use (&$requests): MockResponse {
+        $requests[] = ['method' => $method, 'url' => $url, 'options' => $options];
+
+        if (str_contains($url, 'listpkgs')) {
+            return new MockResponse(json_encode(['package' => [['name' => 'admin_FOSSBilling']]]));
+        }
+
+        if (str_contains($url, 'createacct')) {
+            return new MockResponse(json_encode(['result' => [['status' => 1]]]));
+        }
+
+        return new MockResponse(json_encode(['status' => 1]));
+    });
+    $manager = createWhmManager($client);
+    $account = createWhmAccount(new Server_Package(), false);
+
+    expect($manager->createAccount($account))->toBeTrue();
+
+    $actions = array_map(actionOf(...), $requests);
+    expect($actions)->toBe(['listpkgs', 'createacct']);
+});
+
+test('createAccount skips addpkg when listpkgs uses the modern data.pkg format', function (): void {
+    $requests = [];
+    $client = new MockHttpClient(function (string $method, string $url, array $options) use (&$requests): MockResponse {
+        $requests[] = ['method' => $method, 'url' => $url, 'options' => $options];
+
+        if (str_contains($url, 'listpkgs')) {
+            return new MockResponse(json_encode(['data' => ['pkg' => [['name' => 'admin_FOSSBilling']]]]));
+        }
+
+        if (str_contains($url, 'createacct')) {
+            return new MockResponse(json_encode(['result' => [['status' => 1]]]));
+        }
+
+        return new MockResponse(json_encode(['status' => 1]));
+    });
+    $manager = createWhmManager($client);
+    $account = createWhmAccount(new Server_Package(), false);
+
+    expect($manager->createAccount($account))->toBeTrue();
+
+    $actions = array_map(actionOf(...), $requests);
+    expect($actions)->toBe(['listpkgs', 'createacct']);
+});
+
+test('createAccount updates the existing package via editpkg when addpkg reports already exists (see #4560)', function (): void {
+    $requests = [];
+    $client = new MockHttpClient(function (string $method, string $url, array $options) use (&$requests): MockResponse {
+        $requests[] = ['method' => $method, 'url' => $url, 'options' => $options];
+
+        if (str_contains($url, 'listpkgs')) {
+            return new MockResponse(json_encode(['package' => []]));
+        }
+
+        if (str_contains($url, 'addpkg')) {
+            return new MockResponse(json_encode(['result' => [['status' => 0, 'statusmsg' => '(XID n25fcu) The package "admin_FOSSBilling" already exists. If you wish to make changes, please edit the package.']]]));
+        }
+
+        if (str_contains($url, 'editpkg')) {
+            return new MockResponse(json_encode(['status' => 1]));
+        }
+
+        if (str_contains($url, 'createacct')) {
+            return new MockResponse(json_encode(['result' => [['status' => 1]]]));
+        }
+
+        return new MockResponse(json_encode(['status' => 1]));
+    });
+    $manager = createWhmManager($client);
+    $account = createWhmAccount(new Server_Package(), false);
+
+    expect($manager->createAccount($account))->toBeTrue();
+
+    $actions = array_map(actionOf(...), $requests);
+    expect($actions)->toBe(['listpkgs', 'addpkg', 'editpkg', 'createacct']);
+});
+
+test('createAccount still creates the account when editpkg also fails after already-exists (see #4560)', function (): void {
+    $requests = [];
+    $client = new MockHttpClient(function (string $method, string $url, array $options) use (&$requests): MockResponse {
+        $requests[] = ['method' => $method, 'url' => $url, 'options' => $options];
+
+        if (str_contains($url, 'listpkgs')) {
+            return new MockResponse(json_encode(['package' => []]));
+        }
+
+        if (str_contains($url, 'addpkg')) {
+            return new MockResponse(json_encode(['result' => [['status' => 0, 'statusmsg' => '(XID n25fcu) The package "admin_FOSSBilling" already exists.']]]));
+        }
+
+        if (str_contains($url, 'editpkg')) {
+            return new MockResponse(json_encode(['result' => [['status' => 0, 'statusmsg' => 'Permission denied']]]));
+        }
+
+        if (str_contains($url, 'createacct')) {
+            return new MockResponse(json_encode(['result' => [['status' => 1]]]));
+        }
+
+        return new MockResponse(json_encode(['status' => 1]));
+    });
+    $manager = createWhmManager($client);
+    $account = createWhmAccount(new Server_Package(), false);
+
+    expect($manager->createAccount($account))->toBeTrue();
+
+    $actions = array_map(actionOf(...), $requests);
+    expect($actions)->toBe(['listpkgs', 'addpkg', 'editpkg', 'createacct']);
+});
+
+test('createAccount rethrows addpkg failures unrelated to already-exists', function (): void {
+    $client = new MockHttpClient(function (string $method, string $url): MockResponse {
+        if (str_contains($url, 'listpkgs')) {
+            return new MockResponse(json_encode(['package' => []]));
+        }
+
+        if (str_contains($url, 'addpkg')) {
+            return new MockResponse(json_encode(['result' => [['status' => 0, 'statusmsg' => 'Invalid quota value']]]));
+        }
+
+        return new MockResponse(json_encode(['status' => 1]));
+    });
+    $manager = createWhmManager($client);
+    $account = createWhmAccount(new Server_Package(), false);
+
+    // The WHM reason stays in the logs; API callers only see the generic message.
+    expect(fn () => $manager->createAccount($account))->toThrow(Server_Exception::class, 'Failed to addpkg on the cPanel server');
+});
+
+test('addpkg already-exists does not leak the WHM reason to callers', function (): void {
+    $client = new MockHttpClient(fn (string $method, string $url): MockResponse => new MockResponse(json_encode(['result' => [['status' => 0, 'statusmsg' => '(XID n25fcu) The package "admin_FOSSBilling" already exists.']]])));
+    $manager = createWhmManager($client);
+
+    // Directly verify the typed exception carries the generic message, not the WHM reason.
+    $ref = new ReflectionMethod($manager, 'request');
+
+    try {
+        $ref->invoke($manager, 'addpkg', ['name' => 'admin_FOSSBilling']);
+        expect(false)->toBeTrue('Expected Server_PackageExistsException was not thrown');
+    } catch (Server_PackageExistsException $e) {
+        expect($e->getMessage())->not->toContain('already exists')
+            ->and($e->getMessage())->toContain('Failed to addpkg');
+    }
+});