diff --git a/CHANGELOG.md b/CHANGELOG.md index db652f5e..ff2784b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,3 +19,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Removed - Legacy duck-typed transformers (`$target` property + static `output($model, $data)`), `ResourceTransformerRegistry::transform(Model, array)`, `resolveByTarget()`, `fixClassName()` and the static `$transformers` array. The `User` resource no longer calls the registry directly. - Adds first version + +### Security +- `NotificationRegistry` only resolves notifiables registered with `registerNotifiable()`. A company's saved notification settings could name any `dynamic:`, which reached any public no-argument method on the notification subject (for example fleetops `Order::dispatch()`), or any class name, which the container built. Unregistered definitions are now skipped; extensions must register every `dynamic:` name and notifiable class they expect to receive. diff --git a/src/Support/NotificationRegistry.php b/src/Support/NotificationRegistry.php index 2714dfe0..f8c96902 100644 --- a/src/Support/NotificationRegistry.php +++ b/src/Support/NotificationRegistry.php @@ -391,9 +391,35 @@ protected static function resolveNotificationSettings(...$params): mixed return Setting::lookupForCompany($companyUuid ?? session('company'), 'notification_settings', []); } + /** + * Determine whether a notifiable definition was registered with `registerNotifiable()`. + * + * @param mixed $definition a notifiable class name or a `dynamic:` name + */ + public static function isRegisteredNotifiable($definition): bool + { + if (!is_string($definition) || $definition === '') { + return false; + } + + $definition = ltrim($definition, '\\'); + foreach (static::$notifiables as $notifiable) { + if (is_string($notifiable) && ltrim($notifiable, '\\') === $definition) { + return true; + } + } + + return false; + } + /** * Resolve a notifiable object to an Eloquent model. * + * The notifiable object comes from the company's saved notification settings, so only registered + * notifiables are resolved. An unregistered `dynamic:` would reach any public method on the + * subject that takes no arguments, through `resolveDynamicNotifiable()` or Eloquent's relation getter, + * and an unregistered class would be built by the container. Anything else is skipped. + * * @param array $notifiableObject an associative array containing the definition and primary key to resolve the notifiable object * * @return Model|null the Eloquent model or null if it cannot be resolved @@ -404,6 +430,10 @@ protected static function resolveNotifiable(array $notifiableObject, $subject = $primaryKey = data_get($notifiableObject, 'primaryKey'); $key = data_get($notifiableObject, 'key'); + if (!static::isRegisteredNotifiable($definition)) { + return null; + } + // Handle dynamic subject resolution if (Str::startsWith($definition, 'dynamic:')) { $property = str_replace('dynamic:', '', $definition); diff --git a/tests/Unit/Support/NotificationRegistryTest.php b/tests/Unit/Support/NotificationRegistryTest.php index 4bfd0d96..f83d9c80 100644 --- a/tests/Unit/Support/NotificationRegistryTest.php +++ b/tests/Unit/Support/NotificationRegistryTest.php @@ -116,6 +116,47 @@ class NotificationRegistryDispatchPropertySubject extends EloquentModel public $timestamps = false; } +class NotificationRegistrySideEffectSubject extends EloquentModel +{ + public static array $calls = []; + + protected $primaryKey = 'uuid'; + + public $incrementing = false; + + protected $keyType = 'string'; + + public $timestamps = false; + + public function dispatch(): void + { + static::$calls[] = $this->uuid ?? 'new-instance'; + } +} + +class NotificationRegistryLoadingSubject extends NotificationRegistrySideEffectSubject +{ + // Mirrors fleetops Order::resolveDynamicNotifiable(), which loads any property it is given + public function resolveDynamicNotifiable(string $property): mixed + { + if (!$this->relationLoaded($property)) { + $this->load($property); + } + + return $this->{$property}; + } +} + +class NotificationRegistryConstructorProbe +{ + public static int $constructed = 0; + + public function __construct() + { + static::$constructed++; + } +} + class NotificationRegistryDispatchCacheFake { private array $values = []; @@ -218,8 +259,12 @@ function notification_registry_dispatch_database(): Capsule Fleetbase\Models\Group::class, Fleetbase\Models\Role::class, Fleetbase\Models\Company::class, + NotificationRegistryDispatchTarget::class, + NotificationRegistryDispatchGroup::class, ]; - NotificationRegistryDispatchTarget::$sent = []; + NotificationRegistryDispatchTarget::$sent = []; + NotificationRegistrySideEffectSubject::$calls = []; + NotificationRegistryConstructorProbe::$constructed = 0; }); afterEach(function () { @@ -321,6 +366,7 @@ function notification_registry_dispatch_database(): Capsule session(['company' => 'company-1']); NotificationRegistry::register(NotificationRegistryDispatchNotification::class); + NotificationRegistry::registerNotifiable('dynamic:assignee'); Fleetbase\Models\Setting::query()->create([ 'key' => 'company.company-1.notification_settings', @@ -379,6 +425,7 @@ function notification_registry_dispatch_database(): Capsule test('notification registry dispatches by definition name with dynamic subject context', function () { notification_registry_dispatch_database(); + NotificationRegistry::registerNotifiable('dynamic:dispatcher'); Fleetbase\Models\Setting::query()->create([ 'key' => 'company.company-1.notification_settings', @@ -505,6 +552,7 @@ function notification_registry_dispatch_database(): Capsule test('notification registry resolves dynamic notifiables from subject properties', function () { notification_registry_dispatch_database(); + NotificationRegistry::registerNotifiable('dynamic:assignee'); Fleetbase\Models\Setting::query()->create([ 'key' => 'company.company-1.notification_settings', @@ -565,6 +613,106 @@ function notification_registry_dispatch_database(): Capsule expect(NotificationRegistryDispatchTarget::$sent)->toBe([]); }); +function notification_registry_settings_with(array $notifiables): void +{ + Fleetbase\Models\Setting::query()->create([ + 'key' => 'company.company-1.notification_settings', + 'value' => [ + 'notificationRegistryDispatchNotification__manualNotice' => [ + 'notifiables' => [ + ...$notifiables, + [ + 'definition' => NotificationRegistryDispatchTarget::class, + 'primaryKey' => 'uuid', + 'key' => 'target-2', + ], + ], + ], + ], + ]); +} + +test('notification registry recognises only registered notifiable definitions', function () { + NotificationRegistry::registerNotifiable('dynamic:driver'); + + expect(NotificationRegistry::isRegisteredNotifiable('dynamic:driver'))->toBeTrue() + ->and(NotificationRegistry::isRegisteredNotifiable(Fleetbase\Models\User::class))->toBeTrue() + ->and(NotificationRegistry::isRegisteredNotifiable('\\' . Fleetbase\Models\User::class))->toBeTrue() + ->and(NotificationRegistry::isRegisteredNotifiable('dynamic:dispatch'))->toBeFalse() + ->and(NotificationRegistry::isRegisteredNotifiable('dynamic:Driver'))->toBeFalse() + ->and(NotificationRegistry::isRegisteredNotifiable(NotificationRegistryConstructorProbe::class))->toBeFalse() + ->and(NotificationRegistry::isRegisteredNotifiable(''))->toBeFalse() + ->and(NotificationRegistry::isRegisteredNotifiable(null))->toBeFalse() + ->and(NotificationRegistry::isRegisteredNotifiable(['dynamic:driver']))->toBeFalse(); +}); + +test('notification registry does not read an unregistered dynamic property from the subject', function () { + notification_registry_dispatch_database(); + notification_registry_settings_with([ + ['definition' => 'dynamic:dispatch', 'primaryKey' => 'uuid', 'key' => 'ignored-for-dynamic'], + ]); + + $subject = new NotificationRegistrySideEffectSubject(); + $subject->uuid = 'subject-side-effect'; + $subject->company_uuid = 'company-1'; + + NotificationRegistry::notifyUsingDefinitionName(NotificationRegistryDispatchNotification::class, 'Manual Notice', $subject, 'skipped'); + + expect(NotificationRegistrySideEffectSubject::$calls)->toBe([]) + ->and(array_column(NotificationRegistryDispatchTarget::$sent, 'target'))->toBe(['target-2']); +}); + +test('notification registry does not pass an unregistered dynamic property to the subject resolver', function () { + notification_registry_dispatch_database(); + notification_registry_settings_with([ + ['definition' => 'dynamic:dispatch', 'primaryKey' => 'uuid', 'key' => 'ignored-for-dynamic'], + ]); + + $subject = new NotificationRegistryLoadingSubject(); + $subject->uuid = 'subject-loading'; + $subject->company_uuid = 'company-1'; + + NotificationRegistry::notifyUsingDefinitionName(NotificationRegistryDispatchNotification::class, 'Manual Notice', $subject, 'skipped'); + + expect(NotificationRegistrySideEffectSubject::$calls)->toBe([]) + ->and(array_column(NotificationRegistryDispatchTarget::$sent, 'target'))->toBe(['target-2']); +}); + +test('notification registry still resolves a registered dynamic property through the subject resolver', function () { + notification_registry_dispatch_database(); + NotificationRegistry::registerNotifiable('dynamic:dispatcher'); + notification_registry_settings_with([ + ['definition' => 'dynamic:dispatcher', 'primaryKey' => 'uuid', 'key' => 'ignored-for-dynamic'], + ]); + + $subject = new NotificationRegistryDispatchSubject(); + $subject->uuid = 'subject-registered'; + $subject->company_uuid = 'company-1'; + + NotificationRegistry::notifyUsingDefinitionName(NotificationRegistryDispatchNotification::class, 'Manual Notice', $subject, 'registered'); + + expect(array_column(NotificationRegistryDispatchTarget::$sent, 'target'))->toBe(['target-1', 'target-2']); +}); + +test('notification registry does not build an unregistered notifiable class', function () { + notification_registry_dispatch_database(); + NotificationRegistry::$notifiables = array_values(array_diff(NotificationRegistry::$notifiables, [NotificationRegistryDispatchGroup::class])); + notification_registry_settings_with([ + ['definition' => NotificationRegistryConstructorProbe::class, 'primaryKey' => 'uuid', 'key' => 'probe'], + ['definition' => NotificationRegistryDispatchGroup::class, 'primaryKey' => 'uuid', 'key' => 'group-1'], + ['definition' => ['not', 'a', 'class'], 'primaryKey' => 'uuid', 'key' => 'array'], + ]); + + $subject = new NotificationRegistryDispatchSubject(); + $subject->uuid = 'subject-class'; + $subject->company_uuid = 'company-1'; + + NotificationRegistry::notifyUsingDefinitionName(NotificationRegistryDispatchNotification::class, 'Manual Notice', $subject, 'class'); + + expect(NotificationRegistryConstructorProbe::$constructed)->toBe(0) + ->and(array_column(NotificationRegistryDispatchTarget::$sent, 'target'))->toBe(['target-2']); +}); + function notification_registry_company_settings(string $companyUuid, string $settingsKey, string $targetUuid = 'target-1'): void { Fleetbase\Models\Setting::query()->create([