From 02a6a6aa1bb66e7553388068cd4d40d6e0dc6cd9 Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Fri, 23 Oct 2026 10:23:01 +0800 Subject: [PATCH 1/2] fix(security): never link a profile to another organization's login account Creating a driver, customer or contact looked up the login account by email or phone across the whole instance and linked any managed account of the same type to the new profile, adding it to the caller's organization. Any API key holder could reveal another tenant's driver (name, email, phone came back in the response) by guessing a phone number, attach that login to their own organization, and then change its email, phone or password through the profile update and credential endpoints. - ProfileAccountManager only reuses an account that already belongs to the requesting organization (or belongs to none). Another organization's account is refused with the generic "already in use by another account" error, which reveals neither its details nor its type. - Accounts already shared with another organization only take the name from profile updates, and driver/customer credential and login actions refuse them. - POST /v1/drivers no longer creates drivers in whatever company the `company` parameter names: anything but the credential's own organization is refused like an unknown company. --- .../Controllers/Api/v1/DriverController.php | 18 ++-- .../Internal/v1/CustomerController.php | 19 ++-- .../Internal/v1/DriverController.php | 7 +- server/src/Models/Contact.php | 2 +- server/src/Support/ProfileAccountManager.php | 58 ++++++++-- .../ApiDriverControllerContractsTest.php | 31 ++++-- .../DriverControllerSessionHelpersTest.php | 8 +- .../CustomerControllerCredentialsTest.php | 21 ++++ ...iverControllerExistingUserAdoptionTest.php | 33 +++++- .../DriverControllerLoginManagementTest.php | 26 +++++ server/tests/ModelAccessorContractsTest.php | 2 +- .../Unit/Models/SmallModelSurfacesTest.php | 8 ++ .../Support/ProfileAccountManagerTest.php | 101 ++++++++++++++++-- 13 files changed, 277 insertions(+), 57 deletions(-) diff --git a/server/src/Http/Controllers/Api/v1/DriverController.php b/server/src/Http/Controllers/Api/v1/DriverController.php index e1b331c50..cdb187245 100644 --- a/server/src/Http/Controllers/Api/v1/DriverController.php +++ b/server/src/Http/Controllers/Api/v1/DriverController.php @@ -91,11 +91,12 @@ public function create(CreateDriverRequest $request) // get user details for driver $userDetails = $request->only(['name', 'password', 'email', 'phone', 'timezone']); - // Get current company session - $company = $request->has('company') ? $this->companyFromRequest($request) : $this->currentCompany(); - - // Debugging: Ensure company is retrieved correctly - if (!$company) { + // Drivers are created in the credential's own organization. A request + // may still name it, but naming any other organization is refused the + // same way as one that does not exist. + $company = $this->currentCompany(); + $requestedCompany = $request->input('company'); + if (!$company || ($request->has('company') && !in_array($requestedCompany, [$company->uuid, $company->public_id], true))) { return $this->apiError('Company not found.'); } @@ -206,7 +207,7 @@ public function update($id, UpdateDriverRequest $request) // update the driver's login account through its proxy fields try { - ProfileAccountManager::syncProxyFields($driver->getUser(), $userDetails); + ProfileAccountManager::syncProxyFields($driver->getUser(), $userDetails, $driver->company_uuid); } catch (ProfileIdentityConflictException $exception) { return $this->jsonResponse(['error' => $exception->getMessage()], 422); } @@ -996,11 +997,6 @@ protected function driverInputFromRequest(Request $request, ?string $companyUuid return $input; } - protected function companyFromRequest(Request $request): ?Company - { - return Auth::getCompanyFromRequest($request); - } - protected function currentCompany(): ?Company { return Auth::getCompany(); diff --git a/server/src/Http/Controllers/Internal/v1/CustomerController.php b/server/src/Http/Controllers/Internal/v1/CustomerController.php index b20ed00b4..03851bb39 100644 --- a/server/src/Http/Controllers/Internal/v1/CustomerController.php +++ b/server/src/Http/Controllers/Internal/v1/CustomerController.php @@ -36,7 +36,7 @@ public function createPortalLogin(Request $request) return response()->error('Unable to create customer portal login.'); } - if ($error = $this->staffLinkedError($user)) { + if ($error = $this->staffLinkedError($user, $customer)) { return $error; } @@ -58,7 +58,7 @@ public function sendCredentials(Request $request) return response()->error('Unable to send customer portal credentials.'); } - if ($error = $this->staffLinkedError($user)) { + if ($error = $this->staffLinkedError($user, $customer)) { return $error; } @@ -80,7 +80,7 @@ public function deactivatePortalLogin(Request $request) return response()->error('Customer portal login not found.'); } - if ($error = $this->staffLinkedError($user)) { + if ($error = $this->staffLinkedError($user, $customer)) { return $error; } @@ -100,7 +100,7 @@ public function reactivatePortalLogin(Request $request) return response()->error('Customer portal login not found.'); } - if ($error = $this->staffLinkedError($user)) { + if ($error = $this->staffLinkedError($user, $customer)) { return $error; } @@ -180,7 +180,7 @@ public function resetCredentials(Request $request) return response()->error('Unable to reset customer credentials'); } - if ($error = $this->staffLinkedError($user)) { + if ($error = $this->staffLinkedError($user, $customer)) { return $error; } @@ -200,14 +200,19 @@ public function resetCredentials(Request $request) /** * A customer linked to a team member's account signs in with that account, - * which is managed in IAM. + * which is managed in IAM. A login another organization also uses is not + * this organization's to change either. */ - protected function staffLinkedError(User $user): ?\Illuminate\Http\JsonResponse + protected function staffLinkedError(User $user, Contact $customer): ?\Illuminate\Http\JsonResponse { if (ProfileAccountManager::isStaffAccount($user)) { return response()->error('This customer signs in with a team member account. Manage this login in IAM.', 422); } + if ($customer->company_uuid && ProfileAccountManager::isSharedWithAnotherCompany($user, $customer->company_uuid)) { + return response()->error(ProfileAccountManager::SHARED_LOGIN_MESSAGE, 422); + } + return null; } diff --git a/server/src/Http/Controllers/Internal/v1/DriverController.php b/server/src/Http/Controllers/Internal/v1/DriverController.php index 6e4dcc212..f121aeb83 100644 --- a/server/src/Http/Controllers/Internal/v1/DriverController.php +++ b/server/src/Http/Controllers/Internal/v1/DriverController.php @@ -179,7 +179,7 @@ function (&$request, &$driver, &$input) { $input = $input->except(['name', 'password', 'email', 'phone', 'meta', 'avatar_uuid', 'photo_uuid', 'user_uuid', 'user'])->toArray(); // Update the driver's login account through its proxy fields - ProfileAccountManager::syncProxyFields($driver->getUser(), $userInput); + ProfileAccountManager::syncProxyFields($driver->getUser(), $userInput, $driver->company_uuid); // Flush cache $driver->flushAttributesCache(); @@ -333,6 +333,11 @@ protected function resolveDriverLogin(string $id): array return [$driver, $user, response()->error('This driver signs in with a team member account. Manage this login in IAM.', 422)]; } + // Its password and status are not this organization's to change + if (ProfileAccountManager::isSharedWithAnotherCompany($user, $driver->company_uuid)) { + return [$driver, $user, response()->error(ProfileAccountManager::SHARED_LOGIN_MESSAGE, 422)]; + } + return [$driver, $user, null]; } diff --git a/server/src/Models/Contact.php b/server/src/Models/Contact.php index 9f0af9374..60c75c6d5 100644 --- a/server/src/Models/Contact.php +++ b/server/src/Models/Contact.php @@ -580,7 +580,7 @@ public function syncWithUser(): bool return false; } - return ProfileAccountManager::syncProxyFields($this->getUser(), $updates); + return ProfileAccountManager::syncProxyFields($this->getUser(), $updates, $this->company_uuid); } /** diff --git a/server/src/Support/ProfileAccountManager.php b/server/src/Support/ProfileAccountManager.php index 8619a957b..c73626fea 100644 --- a/server/src/Support/ProfileAccountManager.php +++ b/server/src/Support/ProfileAccountManager.php @@ -24,6 +24,11 @@ * staff account (type `user` or `admin`) of the same organization. A staff * account is managed in IAM, so a profile never changes its email or phone, * resets its password, deactivates it or deletes it. + * + * Emails and phone numbers identify one account across the whole instance, + * but an organization only ever reuses an account that already belongs to it. + * An account of another organization is never linked, and the refusal does + * not say whose it is. */ class ProfileAccountManager { @@ -32,6 +37,12 @@ class ProfileAccountManager */ public const MANAGED_TYPES = ['driver', 'contact', 'customer']; + /** + * Why an organization can't change the password or status of a login that + * another organization's profile also holds. + */ + public const SHARED_LOGIN_MESSAGE = 'This login is also used with another organization, so it can only be managed by its holder.'; + /** * The organization role given to each managed account type. */ @@ -126,8 +137,8 @@ public static function lookup(string $companyUuid, string $type, ?string $email, /** * Resolve the account for a new profile. A staff member of the organization - * or a managed account of the same type with the email/phone is linked; - * otherwise a managed account is created. + * or a managed account of the same type in the organization with the + * email/phone is linked; otherwise a managed account is created. * * @param array $attributes extra user attributes: password, status, timezone, avatar_uuid, country, ip_address, meta * @@ -191,18 +202,21 @@ public static function createManagedAccount(string $companyUuid, string $type, ? /** * Push the profile's proxy fields to its account. A staff account only - * takes the name: its email and phone are its console identity. + * takes the name: its email and phone are its console identity. So does a + * managed account that another organization's profile also holds, since + * its sign-in details are not this organization's to change. * * @throws ProfileIdentityConflictException */ - public static function syncProxyFields(?User $user, array $attributes): bool + public static function syncProxyFields(?User $user, array $attributes, ?string $companyUuid = null): bool { if (!$user) { return false; } - $fields = static::isStaffAccount($user) ? ['name'] : ['name', 'email', 'phone', 'timezone', 'avatar_uuid', 'password']; - $input = array_intersect_key($attributes, array_flip($fields)); + $nameOnly = static::isStaffAccount($user) || ($companyUuid && static::isSharedWithAnotherCompany($user, $companyUuid)); + $fields = $nameOnly ? ['name'] : ['name', 'email', 'phone', 'timezone', 'avatar_uuid', 'password']; + $input = array_intersect_key($attributes, array_flip($fields)); if (array_key_exists('email', $input)) { $input['email'] = static::normalizeEmail($input['email']); @@ -312,11 +326,13 @@ public static function deliverCredentials(Driver|Contact $profile, User $user, s */ public static function assertAccountCanHoldProfile(User $account, string $companyUuid, string $type, string $field = 'email'): void { - if (static::isStaffAccount($account)) { - if (!static::isCompanyMember($account, $companyUuid)) { - throw new ProfileIdentityConflictException(static::takenMessage($field), $field); - } - } elseif ($account->type !== $type) { + // An account of another organization is never linked, and nothing + // about it (its name, type or which organization holds it) is told + if (!static::isCompanyMember($account, $companyUuid) && !(static::isManagedAccount($account) && static::isUnowned($account))) { + throw new ProfileIdentityConflictException(static::takenMessage($field), $field); + } + + if (static::isManagedAccount($account) && $account->type !== $type) { throw new ProfileIdentityConflictException('This ' . static::fieldLabel($field) . ' is already used by a ' . $account->type . '.', $field); } @@ -345,6 +361,26 @@ public static function profileCompanies(User $user): \Illuminate\Support\Collect return $drivers->merge($contacts)->filter()->unique()->values(); } + /** + * Whether another organization holds the account through a membership or a + * profile. The account's own `company_uuid` is left out: it is only where + * the account was first created, and stays after that organization lets go. + */ + public static function isSharedWithAnotherCompany(User $user, string $companyUuid): bool + { + $memberships = CompanyUser::where('user_uuid', $user->uuid)->pluck('company_uuid'); + + return $memberships->merge(static::profileCompanies($user))->filter()->contains(fn ($uuid) => $uuid !== $companyUuid); + } + + /** + * Whether the account belongs to no organization at all. + */ + public static function isUnowned(User $user): bool + { + return !$user->company_uuid && !CompanyUser::where('user_uuid', $user->uuid)->exists(); + } + public static function isCompanyMember(User $user, ?string $companyUuid): bool { if (!$companyUuid) { diff --git a/server/tests/ApiDriverControllerContractsTest.php b/server/tests/ApiDriverControllerContractsTest.php index 27f52381e..39ead8ea5 100644 --- a/server/tests/ApiDriverControllerContractsTest.php +++ b/server/tests/ApiDriverControllerContractsTest.php @@ -41,13 +41,6 @@ class FleetOpsApiDriverControllerProbe extends DriverController public array $resolvedFiles = []; public string $sessionCompanyUuid = 'company-uuid'; - protected function companyFromRequest(Request $request): ?Company - { - $this->companyCalls[] = ['request', $request->input('company')]; - - return $this->missingCompany ? null : $this->company(); - } - protected function currentCompany(): ?Company { $this->companyCalls[] = ['current']; @@ -401,7 +394,7 @@ public function or(array $keys, mixed $default = null): mixed ])); expect($response)->toBe(['resource' => 'driver', 'driver' => $controller->driver]) - ->and($controller->companyCalls)->toBe([['request', 'company_public']]) + ->and($controller->companyCalls)->toBe([['current']]) // The login account is resolved by ProfileAccountManager in the driver's company ->and($controller->resolvedAccounts)->toBe(['company-uuid']) ->and($controller->createdUsers[0])->toMatchArray([ @@ -444,6 +437,28 @@ public function or(array $keys, mixed $default = null): mixed ->and($controller->createdUsers)->toBe([]); }); +test('api driver controller only creates drivers in the credential organization', function (string $company) { + // Naming the credential's own organization, by uuid or public id, is fine + $controller = new FleetOpsApiDriverControllerProbe(); + $controller->create(new CreateDriverRequest(['company' => $company, 'name' => 'Driver One'])); + + expect($controller->resolvedAccounts)->toBe(['company-uuid']) + ->and($controller->createdDrivers[0]['company_uuid'])->toBe('company-uuid'); +})->with(['company-uuid', 'company_public']); + +test('api driver controller refuses to create drivers in another organization', function (mixed $company) { + // Any other organization is refused exactly like one that does not exist, + // so the endpoint can't be used to probe or write into another tenant + $controller = new FleetOpsApiDriverControllerProbe(); + + expect($controller->create(new CreateDriverRequest(['company' => $company, 'name' => 'Driver One'])))->toBe([ + 'apiError' => 'Company not found.', + 'status' => 400, + ]) + ->and($controller->resolvedAccounts)->toBe([]) + ->and($controller->createdDrivers)->toBe([]); +})->with(['other-company-uuid', 'company_other', '', null]); + test('api driver controller updates drivers user details assignments location and photo', function () { fleetopsApiDriverContractsUsersTable(); diff --git a/server/tests/Feature/Http/Api/DriverControllerSessionHelpersTest.php b/server/tests/Feature/Http/Api/DriverControllerSessionHelpersTest.php index 5a7fdcae9..257c8918d 100644 --- a/server/tests/Feature/Http/Api/DriverControllerSessionHelpersTest.php +++ b/server/tests/Feature/Http/Api/DriverControllerSessionHelpersTest.php @@ -174,12 +174,8 @@ public function parameters() expect($probe->callHelper('sessionCompany'))->toBe('22222222-2222-4222-8222-222222222222') ->and($probe->callHelper('currentCompany')?->public_id)->toBe('company_one') - ->and(true)->toBeTrue(); - - // With no identifying inputs the core auth resolver finds nothing and - // violates its own non-nullable declaration — the helper body still - // executes, which is the covered contract here. - expect(fn () => $probe->callHelper('companyFromRequest', Request::create('/x', 'GET')))->toThrow(TypeError::class); + // A create request can no longer pick its organization from the input + ->and(method_exists($probe, 'companyFromRequest'))->toBeFalse(); }); test('user and driver persistence helpers write records', function () { diff --git a/server/tests/Feature/Http/Internal/CustomerControllerCredentialsTest.php b/server/tests/Feature/Http/Internal/CustomerControllerCredentialsTest.php index 82dc5f916..ef2979ccb 100644 --- a/server/tests/Feature/Http/Internal/CustomerControllerCredentialsTest.php +++ b/server/tests/Feature/Http/Internal/CustomerControllerCredentialsTest.php @@ -127,6 +127,8 @@ public function __call($method, $arguments) 'companies' => ['uuid', 'public_id', 'name', 'country'], // session_status reads the user's membership of the session company 'company_users' => ['uuid', 'company_uuid', 'user_uuid', 'status'], + // a login held by another organization's driver profile is shared + 'drivers' => ['uuid', 'company_uuid', 'user_uuid'], 'files' => ['uuid', 'public_id', 'company_uuid', 'subject_uuid', 'subject_type', 'name', 'path', 'disk', 'type', '_key'], ]; foreach ($tables as $table => $columns) { @@ -180,6 +182,25 @@ public function __call($method, $arguments) ->and($GLOBALS['fleetopsCustomerMailFake']->sent)->not->toBeEmpty(); }); +test('credential actions refuse a login another organization also holds', function () { + $connection = fleetopsCustomerCredentialsBoot(); + $connection->table('company_users')->insert(['uuid' => 'cu-elsewhere', 'company_uuid' => 'company-2', 'user_uuid' => 'user-1', 'status' => 'active']); + $connection->table('users')->where('uuid', 'user-1')->update(['password' => 'hashed:original']); + $controller = new CustomerController(); + $request = Request::create('/x', 'POST', ['customer' => 'contact_custcred1', 'password' => 'takeover', 'password_confirmation' => 'takeover']); + + foreach (['resetCredentials', 'sendCredentials', 'createPortalLogin', 'deactivatePortalLogin', 'reactivatePortalLogin'] as $action) { + $response = $controller->{$action}($request); + + expect($response->getStatusCode())->toBe(422) + ->and($response->getData(true))->toBe(['error' => 'This login is also used with another organization, so it can only be managed by its holder.']); + } + + expect($connection->table('users')->value('password'))->toBe('hashed:original') + ->and($connection->table('users')->value('status'))->toBe('active') + ->and($GLOBALS['fleetopsCustomerMailFake']->sent)->toBeEmpty(); +}); + test('customer resolution and payload helpers expose user details', function () { fleetopsCustomerCredentialsBoot(); $probe = new FleetOpsInternalCustomerProbe(); diff --git a/server/tests/Feature/Http/Internal/DriverControllerExistingUserAdoptionTest.php b/server/tests/Feature/Http/Internal/DriverControllerExistingUserAdoptionTest.php index 38aa20dd3..3cff3d1bf 100644 --- a/server/tests/Feature/Http/Internal/DriverControllerExistingUserAdoptionTest.php +++ b/server/tests/Feature/Http/Internal/DriverControllerExistingUserAdoptionTest.php @@ -408,13 +408,14 @@ function fleetopsDriverAdoptionRoles(SQLiteConnection $connection, string $userU ->and($connection->table('drivers')->value('company_uuid'))->toBe('company-1'); }); -test('a managed driver account of another organization is linked and joins the company', function () { +test('a managed driver account of the organization is linked', function () { + // e.g. a driver whose profile was deleted while the account stayed a member $connection = fleetopsDriverAdoptionBoot([]); - $connection->table('users')->insert(['uuid' => '11111111-1111-4111-8111-111111111112', 'company_uuid' => 'company-2', 'name' => 'Shared Driver', 'phone' => '+6591234568', 'slug' => 'shared', 'type' => 'driver']); - $connection->table('drivers')->insert(['uuid' => 'driver-elsewhere', 'company_uuid' => 'company-2', 'user_uuid' => '11111111-1111-4111-8111-111111111112']); + $connection->table('users')->insert(['uuid' => '11111111-1111-4111-8111-111111111112', 'company_uuid' => 'company-2', 'name' => 'Returning Driver', 'phone' => '+6591234568', 'slug' => 'returning', 'type' => 'driver']); + $connection->table('company_users')->insert(['uuid' => 'cu-returning', 'company_uuid' => 'company-1', 'user_uuid' => '11111111-1111-4111-8111-111111111112', 'status' => 'active']); $result = (new DriverController())->createRecord(fleetopsDriverAdoptionRequest([ - 'name' => 'Shared Driver', + 'name' => 'Returning Driver', 'phone' => '+6591234568', ])); @@ -422,7 +423,7 @@ function fleetopsDriverAdoptionRoles(SQLiteConnection $connection, string $userU ->and($result['driver']->resource->user_uuid)->toBe('11111111-1111-4111-8111-111111111112') ->and($connection->table('drivers')->where('company_uuid', 'company-1')->count())->toBe(1) ->and($connection->table('company_users')->where(['company_uuid' => 'company-1', 'user_uuid' => '11111111-1111-4111-8111-111111111112'])->count())->toBe(1) - ->and(fleetopsDriverAdoptionRoles($connection, '11111111-1111-4111-8111-111111111112'))->toBe(['Driver']) + ->and($connection->table('users')->count())->toBe(1) ->and($connection->table('invites')->count())->toBe(0); }); @@ -438,7 +439,11 @@ function fleetopsDriverAdoptionRoles(SQLiteConnection $connection, string $userU expect($result)->toBeInstanceOf(Illuminate\Http\JsonResponse::class) ->and($result->getStatusCode())->toBe(422) ->and($result->getData(true)['error'])->toBe($message) + // Nothing about the existing account is echoed back + ->and($result->getContent())->not->toContain('Existing') + ->and($result->getContent())->not->toContain('11111111-1111-4111-8111-111111111111') ->and($connection->table('drivers')->where('company_uuid', 'company-1')->where('user_uuid', '!=', '11111111-1111-4111-8111-111111111111')->count())->toBe(0) + ->and($connection->table('company_users')->where('company_uuid', 'company-1')->count())->toBe(0) ->and($connection->table('users')->count())->toBe(1); })->with([ 'staff of another organization' => [ @@ -447,6 +452,24 @@ function fleetopsDriverAdoptionRoles(SQLiteConnection $connection, string $userU ['email' => 'outsider@example.com'], 'This email is already in use by another account.', ], + 'a driver of another organization, by email' => [ + ['company_uuid' => 'company-2', 'email' => 'erin@example.com', 'type' => 'driver'], + ['uuid' => 'driver-elsewhere', 'company_uuid' => 'company-2', 'user_uuid' => '11111111-1111-4111-8111-111111111111'], + ['email' => 'Erin@Example.com', 'phone' => '+6599990000'], + 'This email is already in use by another account.', + ], + 'a driver of another organization, by phone' => [ + ['company_uuid' => 'company-2', 'email' => 'erin@example.com', 'phone' => '+6591234568', 'type' => 'driver'], + ['uuid' => 'driver-elsewhere', 'company_uuid' => 'company-2', 'user_uuid' => '11111111-1111-4111-8111-111111111111'], + ['email' => 'olive@example.com', 'phone' => '+65 9123 4568'], + 'This phone number is already in use by another account.', + ], + 'a customer of another organization does not reveal its type' => [ + ['company_uuid' => 'company-2', 'phone' => '+6591112222', 'type' => 'customer'], + null, + ['phone' => '+6591112222'], + 'This phone number is already in use by another account.', + ], 'a customer account' => [ ['company_uuid' => 'company-1', 'phone' => '+6591112222', 'type' => 'customer'], null, diff --git a/server/tests/Feature/Http/Internal/DriverControllerLoginManagementTest.php b/server/tests/Feature/Http/Internal/DriverControllerLoginManagementTest.php index 16bb6cd74..df8e93e0d 100644 --- a/server/tests/Feature/Http/Internal/DriverControllerLoginManagementTest.php +++ b/server/tests/Feature/Http/Internal/DriverControllerLoginManagementTest.php @@ -474,6 +474,32 @@ function fleetopsDriverLoginUser(SQLiteConnection $connection, string $uuid = '1 ->and(Illuminate\Support\Facades\Mail::getFacadeRoot()->sent)->toBe([]); }); +test('login management endpoints refuse a login another organization also holds', function () { + $connection = fleetopsDriverLoginFixture(); + // The managed driver account also drives for another organization + $connection->table('drivers')->insert(['uuid' => 'aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaa9', 'public_id' => 'driver_elsewhere', 'company_uuid' => 'company-2', 'user_uuid' => '11111111-1111-4111-8111-111111111111']); + $before = fleetopsDriverLoginUser($connection); + $controller = new DriverController(); + $reset = fn (string $id) => $controller->resetCredentials(Request::create('/x', 'POST', ['password' => 'taken-over', 'password_confirmation' => 'taken-over']), $id); + + foreach ([ + fn (string $id) => $controller->sendCredentials($id), + $reset, + fn (string $id) => $controller->deactivateLogin($id), + fn (string $id) => $controller->reactivateLogin($id), + ] as $endpoint) { + $shared = $endpoint('driver_managed'); + + expect($shared->getStatusCode())->toBe(422) + ->and($shared->getData(true))->toBe(['error' => 'This login is also used with another organization, so it can only be managed by its holder.']); + } + + $after = fleetopsDriverLoginUser($connection); + expect($after->password)->toBe($before->password) + ->and($after->status)->toBe($before->status) + ->and(Illuminate\Support\Facades\Mail::getFacadeRoot()->sent)->toBe([]); +}); + test('reset credentials validates the password and optionally sends it', function () { $connection = fleetopsDriverLoginFixture(); $controller = new DriverController(); diff --git a/server/tests/ModelAccessorContractsTest.php b/server/tests/ModelAccessorContractsTest.php index ca9fd6052..2e034c2ff 100644 --- a/server/tests/ModelAccessorContractsTest.php +++ b/server/tests/ModelAccessorContractsTest.php @@ -159,7 +159,7 @@ function fleetopsModelAccessorsUseProfileAccountTables(): void $table->string('phone')->nullable(); $table->timestamp('deleted_at')->nullable(); }); - foreach (['drivers', 'contacts'] as $profileTable) { + foreach (['drivers', 'contacts', 'company_users'] as $profileTable) { $schema->create($profileTable, function ($table) { $table->string('uuid')->nullable(); $table->string('user_uuid')->nullable(); diff --git a/server/tests/Unit/Models/SmallModelSurfacesTest.php b/server/tests/Unit/Models/SmallModelSurfacesTest.php index 6aecd0683..a782c58a6 100644 --- a/server/tests/Unit/Models/SmallModelSurfacesTest.php +++ b/server/tests/Unit/Models/SmallModelSurfacesTest.php @@ -212,6 +212,14 @@ public function __call($method, $arguments) test('contact observer rejects saves whose email or phone belongs to another account', function () { $connection = fleetopsSmallModelBoot(); + // The sync checks whether another organization holds the account + $connection->getSchemaBuilder()->create('drivers', function ($blueprint) { + $blueprint->increments('id'); + $blueprint->string('uuid')->nullable(); + $blueprint->string('company_uuid')->nullable(); + $blueprint->string('user_uuid')->nullable(); + $blueprint->timestamp('deleted_at')->nullable(); + }); $connection->table('users')->insert([ ['uuid' => '11111111-1111-4111-8111-111111111111', 'company_uuid' => 'company-1', 'type' => 'contact', 'email' => 'free@example.test', 'phone' => '+6599999999'], ['uuid' => '22222222-2222-4222-8222-222222222222', 'company_uuid' => 'company-1', 'type' => 'user', 'email' => 'taken@example.test', 'phone' => '+6512345678'], diff --git a/server/tests/Unit/Support/ProfileAccountManagerTest.php b/server/tests/Unit/Support/ProfileAccountManagerTest.php index 079beea04..1d15d2bee 100644 --- a/server/tests/Unit/Support/ProfileAccountManagerTest.php +++ b/server/tests/Unit/Support/ProfileAccountManagerTest.php @@ -312,11 +312,10 @@ function fleetopsProfileAccountUser(array $attributes): User ->and($staff['conflict'])->toBeNull() ->and($outsider['staff'])->toBeNull() ->and($outsider['conflict'])->toBe('This email is already in use by another account.') - ->and($customer['conflict'])->toBe('This phone number is already used by a driver.') - // A driver account of another organization is linked, not reported as staff - ->and($sameType['account']?->uuid)->toBe('driver-1') + ->and($customer['conflict'])->toBe('This phone number is already in use by another account.') + // A driver account of another organization is never linked, whatever its type ->and($sameType['staff'])->toBeNull() - ->and($sameType['conflict'])->toBeNull() + ->and($sameType['conflict'])->toBe('This phone number is already in use by another account.') // An existing profile can't swap its account for another one ->and($taken['conflict'])->toBe('This email is already in use by another account.') ->and($taken['staff'])->toBeNull() @@ -369,15 +368,16 @@ function fleetopsProfileAccountUser(array $attributes): User ->and($connection->table('company_users')->where('user_uuid', $contact->uuid)->count())->toBe(0); }); -test('resolve for profile links staff members and managed accounts of the same type', function () { +test('resolve for profile links staff members and managed accounts of the organization', function () { $connection = fleetopsProfileAccountBoot(); fleetopsProfileAccountRows($connection, 'users', [ ['uuid' => 'staff-1', 'company_uuid' => 'company-2', 'name' => 'Staff', 'email' => 'staff@example.com', 'type' => 'user'], ['uuid' => 'driver-1', 'company_uuid' => 'company-2', 'phone' => '+6590001111', 'type' => 'driver'], ['uuid' => 'driver-2', 'company_uuid' => null, 'phone' => '+6590003333', 'type' => 'driver'], ]); - // The staff member belongs to company-1 through a membership row + // The staff member and the driver account belong to company-1 through membership rows $connection->table('company_users')->insert(['uuid' => 'cu-staff', 'company_uuid' => 'company-1', 'user_uuid' => 'staff-1', 'status' => 'active']); + $connection->table('company_users')->insert(['uuid' => 'cu-driver', 'company_uuid' => 'company-1', 'user_uuid' => 'driver-1', 'status' => 'active']); $staff = ProfileAccountManager::resolveForProfile('company-1', 'driver', 'Other Name', 'STAFF@example.com', null); $driver = ProfileAccountManager::resolveForProfile('company-1', 'driver', 'Driver', null, '+6590001111'); @@ -434,6 +434,95 @@ function fleetopsProfileAccountUser(array $attributes): User ->and(new ProfileIdentityConflictException('Taken'))->getField()->toBe('email'); }); +test('resolve for profile never links an account of another organization nor tells whose it is', function () { + $connection = fleetopsProfileAccountBoot(); + fleetopsProfileAccountRows($connection, 'users', [ + ['uuid' => 'erin-1', 'company_uuid' => 'company-2', 'name' => 'Erin Driver', 'email' => 'erin@example.com', 'phone' => '+6590001111', 'type' => 'driver'], + ['uuid' => 'cust-1', 'company_uuid' => 'company-2', 'name' => 'Carla Customer', 'email' => 'carla@example.com', 'type' => 'customer'], + ['uuid' => 'member-1', 'company_uuid' => null, 'name' => 'Member Elsewhere', 'phone' => '+6590002222', 'type' => 'driver'], + ]); + $connection->table('drivers')->insert(['uuid' => 'd-erin', 'company_uuid' => 'company-2', 'user_uuid' => 'erin-1']); + // An account without its own organization but a membership elsewhere is held too + $connection->table('company_users')->insert(['uuid' => 'cu-member', 'company_uuid' => 'company-2', 'user_uuid' => 'member-1', 'status' => 'active']); + + $conflict = function (callable $callback): ?ProfileIdentityConflictException { + try { + $callback(); + } catch (ProfileIdentityConflictException $exception) { + return $exception; + } + + return null; + }; + + $byPhone = $conflict(fn () => ProfileAccountManager::resolveForProfile('company-1', 'driver', 'Olive Odoo', 'olive@example.com', '+65 9000 1111')); + $byEmail = $conflict(fn () => ProfileAccountManager::resolveForProfile('company-1', 'driver', 'Olive Odoo', 'ERIN@example.com', '+6599999999')); + $customer = $conflict(fn () => ProfileAccountManager::resolveForProfile('company-1', 'customer', 'Carla', 'carla@example.com', null)); + $wrongType = $conflict(fn () => ProfileAccountManager::resolveForProfile('company-1', 'contact', 'Carla', 'carla@example.com', null)); + $membership = $conflict(fn () => ProfileAccountManager::resolveForProfile('company-1', 'driver', 'M', null, '+6590002222')); + + expect($byPhone?->getMessage())->toBe('This phone number is already in use by another account.') + ->and($byPhone?->getField())->toBe('phone') + ->and($byEmail?->getMessage())->toBe('This email is already in use by another account.') + ->and($byEmail?->getField())->toBe('email') + ->and($customer?->getMessage())->toBe('This email is already in use by another account.') + // Not "already used by a customer": the type of another organization's account is not told + ->and($wrongType?->getMessage())->toBe('This email is already in use by another account.') + ->and($membership?->getMessage())->toBe('This phone number is already in use by another account.'); + + foreach ([$byPhone, $byEmail, $customer, $wrongType, $membership] as $exception) { + expect(json_encode([$exception->getMessage(), $exception->getErrors()])) + ->not->toContain('Erin') + ->not->toContain('erin@') + ->not->toContain('Carla') + ->not->toContain('company-2'); + } + + // Nothing was linked, created or added to company-1 + expect($connection->table('users')->count())->toBe(3) + ->and($connection->table('company_users')->where('company_uuid', 'company-1')->count())->toBe(0) + ->and($connection->table('users')->where('uuid', 'member-1')->value('company_uuid'))->toBeNull(); +}); + +test('accounts held by another organization are told apart from unowned ones', function () { + $connection = fleetopsProfileAccountBoot(); + $own = fleetopsProfileAccountUser(['uuid' => 'own-1', 'company_uuid' => 'company-1', 'type' => 'driver']); + $created = fleetopsProfileAccountUser(['uuid' => 'created-1', 'company_uuid' => 'company-2', 'type' => 'driver']); + $member = fleetopsProfileAccountUser(['uuid' => 'member-1', 'company_uuid' => 'company-1', 'type' => 'driver']); + $profile = fleetopsProfileAccountUser(['uuid' => 'profile-1', 'company_uuid' => 'company-1', 'type' => 'contact']); + $orphan = fleetopsProfileAccountUser(['uuid' => 'orphan-1', 'company_uuid' => null, 'type' => 'driver']); + $connection->table('company_users')->insert(['uuid' => 'cu-1', 'company_uuid' => 'company-2', 'user_uuid' => 'member-1', 'status' => 'active']); + $connection->table('contacts')->insert(['uuid' => 'c-1', 'company_uuid' => 'company-2', 'user_uuid' => 'profile-1', 'type' => 'contact']); + + expect(ProfileAccountManager::isSharedWithAnotherCompany($own, 'company-1'))->toBeFalse() + // Being created by another organization that no longer holds it is not sharing + ->and(ProfileAccountManager::isSharedWithAnotherCompany($created, 'company-1'))->toBeFalse() + ->and(ProfileAccountManager::isSharedWithAnotherCompany($member, 'company-1'))->toBeTrue() + ->and(ProfileAccountManager::isSharedWithAnotherCompany($profile, 'company-1'))->toBeTrue() + ->and(ProfileAccountManager::isSharedWithAnotherCompany($orphan, 'company-1'))->toBeFalse() + ->and(ProfileAccountManager::isUnowned($orphan))->toBeTrue() + ->and(ProfileAccountManager::isUnowned($own))->toBeFalse() + ->and(ProfileAccountManager::isUnowned($member))->toBeFalse(); +}); + +test('sync proxy fields only pushes the name to a managed account another organization also holds', function () { + $connection = fleetopsProfileAccountBoot(); + $shared = fleetopsProfileAccountUser(['uuid' => 'shared-1', 'company_uuid' => 'company-2', 'name' => 'Shared', 'email' => 'shared@example.com', 'phone' => '+6590000000', 'type' => 'driver']); + $connection->table('company_users')->insert([ + ['uuid' => 'cu-1', 'company_uuid' => 'company-1', 'user_uuid' => 'shared-1', 'status' => 'active'], + ['uuid' => 'cu-2', 'company_uuid' => 'company-2', 'user_uuid' => 'shared-1', 'status' => 'active'], + ]); + + expect(ProfileAccountManager::syncProxyFields($shared, ['name' => 'Renamed', 'email' => 'mine@example.com', 'phone' => '+6591111111', 'password' => 'taken-over'], 'company-1'))->toBeTrue() + ->and($connection->table('users')->where('uuid', 'shared-1')->first()) + ->name->toBe('Renamed') + ->email->toBe('shared@example.com') + ->phone->toBe('+6590000000') + ->password->not->toBe('hashed:taken-over') + // Nor can its own organization while another one also holds it + ->and(ProfileAccountManager::syncProxyFields($shared, ['email' => 'new@example.com'], 'company-2'))->toBeFalse(); +}); + test('sync proxy fields pushes normalized changes to managed accounts and only the name to staff', function () { $connection = fleetopsProfileAccountBoot(); $connection->table('users')->insert(['uuid' => 'other-1', 'email' => 'taken@example.com', 'phone' => '+6599990000', 'type' => 'user']); From 416137554f0f218b01dbceca448f6fa5a1304b53 Mon Sep 17 00:00:00 2001 From: "Ronald A. Richardson" Date: Fri, 23 Oct 2026 11:19:50 +0800 Subject: [PATCH 2/2] fix(security): an account with a live profile elsewhere is not unowned isUnowned() only looked at users.company_uuid and company_users, so an account with neither but a live driver or contact profile in another organization (a customer who signed up through a storefront, for example) could still be adopted by any organization that knew its email or phone. It now also requires that the account holds no profile anywhere. --- server/src/Support/ProfileAccountManager.php | 7 +++++-- .../Unit/Support/ProfileAccountManagerTest.php | 16 +++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/server/src/Support/ProfileAccountManager.php b/server/src/Support/ProfileAccountManager.php index c73626fea..c54db2be9 100644 --- a/server/src/Support/ProfileAccountManager.php +++ b/server/src/Support/ProfileAccountManager.php @@ -374,11 +374,14 @@ public static function isSharedWithAnotherCompany(User $user, string $companyUui } /** - * Whether the account belongs to no organization at all. + * Whether the account belongs to no organization at all: no organization of + * its own, no membership and no live profile anywhere. */ public static function isUnowned(User $user): bool { - return !$user->company_uuid && !CompanyUser::where('user_uuid', $user->uuid)->exists(); + return !$user->company_uuid + && !CompanyUser::where('user_uuid', $user->uuid)->exists() + && static::profileCompanies($user)->isEmpty(); } public static function isCompanyMember(User $user, ?string $companyUuid): bool diff --git a/server/tests/Unit/Support/ProfileAccountManagerTest.php b/server/tests/Unit/Support/ProfileAccountManagerTest.php index 1d15d2bee..fa1eda3c9 100644 --- a/server/tests/Unit/Support/ProfileAccountManagerTest.php +++ b/server/tests/Unit/Support/ProfileAccountManagerTest.php @@ -491,8 +491,11 @@ function fleetopsProfileAccountUser(array $attributes): User $member = fleetopsProfileAccountUser(['uuid' => 'member-1', 'company_uuid' => 'company-1', 'type' => 'driver']); $profile = fleetopsProfileAccountUser(['uuid' => 'profile-1', 'company_uuid' => 'company-1', 'type' => 'contact']); $orphan = fleetopsProfileAccountUser(['uuid' => 'orphan-1', 'company_uuid' => null, 'type' => 'driver']); + // No organization of its own and no membership, but a live profile elsewhere (a self-signup customer) + $signup = fleetopsProfileAccountUser(['uuid' => 'signup-1', 'company_uuid' => null, 'email' => 'signup@example.com', 'type' => 'customer']); $connection->table('company_users')->insert(['uuid' => 'cu-1', 'company_uuid' => 'company-2', 'user_uuid' => 'member-1', 'status' => 'active']); $connection->table('contacts')->insert(['uuid' => 'c-1', 'company_uuid' => 'company-2', 'user_uuid' => 'profile-1', 'type' => 'contact']); + $connection->table('contacts')->insert(['uuid' => 'c-2', 'company_uuid' => 'company-2', 'user_uuid' => 'signup-1', 'type' => 'customer']); expect(ProfileAccountManager::isSharedWithAnotherCompany($own, 'company-1'))->toBeFalse() // Being created by another organization that no longer holds it is not sharing @@ -502,7 +505,18 @@ function fleetopsProfileAccountUser(array $attributes): User ->and(ProfileAccountManager::isSharedWithAnotherCompany($orphan, 'company-1'))->toBeFalse() ->and(ProfileAccountManager::isUnowned($orphan))->toBeTrue() ->and(ProfileAccountManager::isUnowned($own))->toBeFalse() - ->and(ProfileAccountManager::isUnowned($member))->toBeFalse(); + ->and(ProfileAccountManager::isUnowned($member))->toBeFalse() + ->and(ProfileAccountManager::isUnowned($signup))->toBeFalse(); + + // So another organization can't adopt it by its email + $conflict = null; + try { + ProfileAccountManager::resolveForProfile('company-1', 'customer', 'X', $signup->email, null); + } catch (ProfileIdentityConflictException $exception) { + $conflict = $exception; + } + expect($conflict?->getMessage())->toBe('This email is already in use by another account.') + ->and($connection->table('company_users')->where('user_uuid', 'signup-1')->count())->toBe(0); }); test('sync proxy fields only pushes the name to a managed account another organization also holds', function () {