diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 0686f541a..9fe989bad 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -25,6 +25,7 @@ use OCA\OpenRegister\Event\ObjectCreatingEvent; use OCA\OpenRegister\Event\ObjectDeletedEvent; use OCA\OpenRegister\Event\ObjectUpdatedEvent; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; use OCA\OpenRegister\Event\UserProfileUpdatedEvent; use OCA\OpenRegister\Service\OrganisationService as OpenRegisterOrganisationService; use OCA\Stackiq\BackgroundJob\ContractStatusJob; @@ -38,6 +39,7 @@ use OCA\Stackiq\EventListener\ModuleVersionPublicationListener; use OCA\Stackiq\EventListener\ModuleComplianceSubscriber; use OCA\Stackiq\EventListener\AnonymousRegistrationListener; +use OCA\Stackiq\EventListener\ContactPersonOrganisationListener; use OCA\Stackiq\EventListener\ModuleRegistrationSubscriber; use OCA\Stackiq\EventListener\TestEventListener; use OCA\Stackiq\EventListener\UserProfileUpdatedEventListener; @@ -818,6 +820,10 @@ private function registerEventListeners(IRegistrationContext $context): void { // An organisation registered without an account stays pending until reviewed. $context->registerEventListener(ObjectCreatingEvent::class, AnonymousRegistrationListener::class); + + // A contact person only from someone who may add members to its organisation. + $context->registerEventListener(ObjectCreatingEvent::class, ContactPersonOrganisationListener::class); + $context->registerEventListener(ObjectUpdatingEvent::class, ContactPersonOrganisationListener::class); $context->registerEventListener(ObjectUpdatedEvent::class, ModuleRegistrationSubscriber::class); // Queue the owner resolution when a supplier announces maintenance (lifecycle-maintenance-and-supplier-roadmap). diff --git a/lib/BackgroundJob/OrganizationContactSyncJob.php b/lib/BackgroundJob/OrganizationContactSyncJob.php index 880a34679..1ecaa2c1b 100644 --- a/lib/BackgroundJob/OrganizationContactSyncJob.php +++ b/lib/BackgroundJob/OrganizationContactSyncJob.php @@ -19,6 +19,7 @@ namespace OCA\Stackiq\BackgroundJob; +use OCA\Stackiq\Service\Contactpersoon\SystemContactWrites; use OCA\Stackiq\Service\OrganizationSyncService; use OCA\Stackiq\Service\SettingsService; use OCA\Stackiq\Service\StackiqContactSyncService; @@ -104,6 +105,17 @@ public function __construct( * @spec openspec/specs/softwarecatalog-contacts-to-nc/spec.md */ protected function run($argument): void { + (new SystemContactWrites())->run(fn () => $this->runSync()); + }//end run() + + /** + * The scheduled sync and link refresh. + * + * @return void + * + * @spec openspec/specs/softwarecatalog-contacts-to-nc/spec.md + */ + private function runSync(): void { if (in_array('openregister', $this->appManager->getInstalledApps(), true) === false) { $this->logger->info('[OrganizationContactSyncJob] OpenRegister not installed, skipping sync'); return; @@ -143,7 +155,7 @@ protected function run($argument): void { ] ); }//end try - }//end run() + }//end runSync() /** * Refresh the `contactsUid` link on every contactpersoon/organisatie record. diff --git a/lib/EventListener/ContactPersonOrganisationListener.php b/lib/EventListener/ContactPersonOrganisationListener.php new file mode 100644 index 000000000..4c66d9d8c --- /dev/null +++ b/lib/EventListener/ContactPersonOrganisationListener.php @@ -0,0 +1,107 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://github.com/ConductionNL/stackiq + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\EventListener; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCA\OpenRegister\Event\ObjectCreatingEvent; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; +use OCA\Stackiq\Service\Contactpersoon\ContactPersonOrganisationGuard; +use OCA\Stackiq\Service\Contactpersoon\SystemContactWrites; +use OCA\Stackiq\Service\SettingsService; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; + +/** + * On OpenRegister's creating and updating events for a contact person: the write is + * refused unless {@see ContactPersonOrganisationGuard} allows it. An update that keeps + * every key the guard reads is not checked again. Writes stackiq's own contact sync + * and migration make, marked through {@see SystemContactWrites}, are not checked. + * Other command line writes, such as OpenRegister bulk jobs, are. + * + * @template-implements IEventListener + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + */ +class ContactPersonOrganisationListener implements IEventListener { + /** + * Constructor. + * + * @param ContactPersonOrganisationGuard $guard Decides whether the caller may write the contact person. + * @param SettingsService $settingsService Names the contact person schema. + * @param SystemContactWrites $systemWrites Tells whether stackiq itself is writing. + */ + public function __construct( + private readonly ContactPersonOrganisationGuard $guard, + private readonly SettingsService $settingsService, + private readonly SystemContactWrites $systemWrites, + ) { + }//end __construct() + + /** + * Refuse a contact person write the caller may not make. + * + * @param Event $event The dispatched event. + * + * @return void + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + */ + public function handle(Event $event): void { + if ($this->systemWrites->isActive() === true) { + return; + } + + if ($event instanceof ObjectCreatingEvent) { + $this->check(event: $event, object: $event->getObject(), oldData: null); + return; + } + + if ($event instanceof ObjectUpdatingEvent) { + $this->check(event: $event, object: $event->getNewObject(), oldData: $event->getOldObject()?->getObject()); + } + }//end handle() + + /** + * Stop the write when it is a contact person the guard does not allow. + * + * @param ObjectCreatingEvent|ObjectUpdatingEvent $event The pre-save event. + * @param ObjectEntity $object The object being written. + * @param array|null $oldData The stored data, or null on a create. + * + * @return void + */ + private function check(ObjectCreatingEvent|ObjectUpdatingEvent $event, ObjectEntity $object, ?array $oldData): void { + $configured = (string)($this->settingsService->getVoorzieningenConfig()['contactpersoon_schema'] ?? ''); + if ($configured === '' || (string)$object->getSchema() !== $configured) { + return; + } + + $data = array_merge(($object->getObject() ?? []), $event->getModifiedData()); + if ($this->guard->needsCheck(contactData: $data, oldData: $oldData) === false + || $this->guard->allows(contactData: $data, oldData: $oldData) === true + ) { + return; + } + + $event->setErrors(['message' => 'This contact person could not be saved.']); + $event->stopPropagation(); + }//end check() +}//end class diff --git a/lib/Repair/MigrateContactsToNc.php b/lib/Repair/MigrateContactsToNc.php index 76736a520..c1a946dcc 100644 --- a/lib/Repair/MigrateContactsToNc.php +++ b/lib/Repair/MigrateContactsToNc.php @@ -31,6 +31,7 @@ namespace OCA\Stackiq\Repair; +use OCA\Stackiq\Service\Contactpersoon\SystemContactWrites; use OCA\Stackiq\AppInfo\AppIdentity; use OCA\Stackiq\Service\SettingsService; use OCA\Stackiq\Service\StackiqContactSyncService; @@ -105,6 +106,19 @@ public function getName(): string { * @spec openspec/specs/softwarecatalog-contacts-to-nc/spec.md */ public function run(IOutput $output): void { + (new SystemContactWrites())->run(fn () => $this->migrate(output: $output)); + }//end run() + + /** + * Migrate the contacts. + * + * @param IOutput $output The output interface for progress reporting. + * + * @return void + * + * @spec openspec/specs/softwarecatalog-contacts-to-nc/spec.md + */ + private function migrate(IOutput $output): void { $output->startProgress(2); // Converged on a previous run — skip quietly. This step is registered @@ -169,7 +183,7 @@ public function run(IOutput $output): void { } $output->finishProgress(); - }//end run() + }//end migrate() /** * Migrate every object of one type. diff --git a/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php b/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php new file mode 100644 index 000000000..c1a98845c --- /dev/null +++ b/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php @@ -0,0 +1,280 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://github.com/ConductionNL/stackiq + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Service\Contactpersoon; + +use OCA\OpenRegister\Db\Organisation; +use OCA\OpenRegister\Db\OrganisationMapper; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\IGroupManager; +use OCP\IUserManager; +use OCP\IUserSession; + +/** + * The contact sync turns a contact person into an account that joins the contact's + * organisation, resets the role group of an existing account named by its e-mail + * address, and writes names and e-mail address to the account named by `username`. + * So a contact person is accepted only from an administrator, or from a member of its + * organisation who may add members; it may name an existing account only when that + * account already belongs to the same organisation, and `username` only when it is the + * caller's own account. The sync reads several keys for the same value, so every key + * is checked on its own and two keys that disagree are refused. + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + */ +class ContactPersonOrganisationGuard { + /** + * The groups whose members may add members to their own organisation. + */ + public const MEMBER_MANAGER_GROUPS = ['gebruik-beheerder', 'aanbod-beheerder']; + + /** + * The keys the sync reads the organisation uuid from. + */ + public const ORGANISATION_KEYS = ['organisation', 'organization']; + + /** + * The keys the sync reads the e-mail address from. + */ + public const EMAIL_KEYS = ['email', 'e-mailadres']; + + /** + * The key the name sync reads the target account from. + */ + public const USERNAME_KEY = 'username'; + + /** + * Constructor. + * + * @param IUserSession $userSession Names the caller. + * @param IGroupManager $groupManager Tells administrators and member managers apart. + * @param IUserManager $userManager Tells whether a name is an existing account. + * @param OrganisationMapper $organisationMapper Finds the OpenRegister organisation and its members. + */ + public function __construct( + private readonly IUserSession $userSession, + private readonly IGroupManager $groupManager, + private readonly IUserManager $userManager, + private readonly OrganisationMapper $organisationMapper, + ) { + }//end __construct() + + /** + * Whether the caller may write this contact person. + * + * @param array $contactData The contact person's data. + * @param array|null $oldData The stored data, or null on a create. + * + * @return bool + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + */ + public function allows(array $contactData, ?array $oldData = null): bool { + $caller = $this->userSession->getUser()?->getUID(); + if ($caller !== null && $this->groupManager->isAdmin($caller) === true) { + return true; + } + + if ($this->isAmbiguousOrForeign(contactData: $contactData, caller: $caller) === true) { + return false; + } + + $organisation = $this->findOrganisation(uuid: $this->values(contactData: $contactData, keys: self::ORGANISATION_KEYS)[0] ?? ''); + $account = $this->existingAccount(email: $this->values(contactData: $contactData, keys: self::EMAIL_KEYS)[0] ?? '', caller: $caller); + if ($account !== null && $organisation?->hasUser($account) !== true) { + return false; + } + + return $this->mayAddTo(organisation: $organisation, contactData: $contactData, oldData: $oldData, caller: $caller); + }//end allows() + + /** + * Whether two keys disagree, or `username` names an account other than the caller's. + * + * @param array $contactData The contact person's data. + * @param string|null $caller The caller's uid. + * + * @return bool + */ + private function isAmbiguousOrForeign(array $contactData, ?string $caller): bool { + return count($this->values(contactData: $contactData, keys: self::ORGANISATION_KEYS)) > 1 + || count($this->values(contactData: $contactData, keys: self::EMAIL_KEYS)) > 1 + || $this->namesAnotherAccount(name: $this->values(contactData: $contactData, keys: [self::USERNAME_KEY])[0] ?? '', caller: $caller) === true; + }//end isAmbiguousOrForeign() + + /** + * Whether the caller may add this contact to the organisation. + * + * @param Organisation|null $organisation The OpenRegister organisation, or null when it has none yet. + * @param array $contactData The new data. + * @param array|null $oldData The stored data, or null on a create. + * @param string|null $caller The caller's uid. + * + * @return bool + */ + private function mayAddTo(?Organisation $organisation, array $contactData, ?array $oldData, ?string $caller): bool { + if ($organisation === null || $this->isOwnContactInSameOrganisation(contactData: $contactData, oldData: $oldData, caller: $caller) === true) { + return true; + } + + return $caller !== null && $organisation->hasUser($caller) === true && $this->managesMembers(uid: $caller) === true; + }//end mayAddTo() + + /** + * Whether the change from the stored contact person needs the check again. + * + * Every key that decides which account joins which organisation, or which account + * the name sync writes to, is compared on its own. + * + * @param array $contactData The new data. + * @param array|null $oldData The stored data, or null when there is none. + * + * @return bool + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + */ + public function needsCheck(array $contactData, ?array $oldData): bool { + if ($oldData === null) { + return true; + } + + foreach ([...self::ORGANISATION_KEYS, ...self::EMAIL_KEYS, self::USERNAME_KEY] as $key) { + if ($this->values(contactData: $contactData, keys: [$key]) !== $this->values(contactData: $oldData, keys: [$key])) { + return true; + } + } + + return false; + }//end needsCheck() + + /** + * The distinct non-empty values under these keys, trimmed and lower-cased. + * + * Account names compare case-insensitively in Nextcloud, and uuids are lower case. + * + * @param array $contactData The contact person's data. + * @param array $keys The keys to read. + * + * @return array + */ + private function values(array $contactData, array $keys): array { + $values = []; + foreach ($keys as $key) { + $value = ($contactData[$key] ?? ''); + if (is_array($value) === true) { + $value = ($value['uuid'] ?? $value['id'] ?? ''); + } + + if (is_scalar($value) === true && trim((string)$value) !== '') { + $values[] = strtolower(trim((string)$value)); + } + } + + return array_values(array_unique($values)); + }//end values() + + /** + * Whether the caller edits their own contact person and keeps its organisation. + * + * @param array $contactData The new data. + * @param array|null $oldData The stored data. + * @param string|null $caller The caller's uid. + * + * @return bool + */ + private function isOwnContactInSameOrganisation(array $contactData, ?array $oldData, ?string $caller): bool { + if ($oldData === null || $caller === null) { + return false; + } + + $own = [strtolower($caller)]; + return $this->values(contactData: $oldData, keys: [self::USERNAME_KEY]) === $own + && $this->values(contactData: $contactData, keys: [self::USERNAME_KEY]) === $own + && $this->values(contactData: $contactData, keys: self::ORGANISATION_KEYS) === $this->values(contactData: $oldData, keys: self::ORGANISATION_KEYS); + }//end isOwnContactInSameOrganisation() + + /** + * The OpenRegister organisation with this uuid, or null. + * + * @param string $uuid The organisation uuid. + * + * @return Organisation|null + */ + private function findOrganisation(string $uuid): ?Organisation { + if ($uuid === '') { + return null; + } + + try { + return $this->organisationMapper->findByUuid($uuid); + } catch (DoesNotExistException $e) { + return null; + } + }//end findOrganisation() + + /** + * The uid of the existing account this e-mail address names, when it is not the caller's own. + * + * @param string $email The e-mail address the sync uses as the account name. + * @param string|null $caller The caller's uid. + * + * @return string|null + */ + private function existingAccount(string $email, ?string $caller): ?string { + if ($email === '') { + return null; + } + + $uid = $this->userManager->get($email)?->getUID(); + if ($uid === null || strtolower($uid) === strtolower((string)$caller)) { + return null; + } + + return $uid; + }//end existingAccount() + + /** + * Whether this name is an existing account other than the caller's own. + * + * @param string $name The account name. + * @param string|null $caller The caller's uid. + * + * @return bool + */ + private function namesAnotherAccount(string $name, ?string $caller): bool { + return $this->existingAccount(email: $name, caller: $caller) !== null; + }//end namesAnotherAccount() + + /** + * Whether this user is in a group that may add members. + * + * @param string $uid The user id. + * + * @return bool + */ + private function managesMembers(string $uid): bool { + $memberOf = array_filter( + self::MEMBER_MANAGER_GROUPS, + fn (string $group): bool => $this->groupManager->isInGroup($uid, $group) + ); + + return $memberOf !== []; + }//end managesMembers() +}//end class diff --git a/lib/Service/Contactpersoon/SystemContactWrites.php b/lib/Service/Contactpersoon/SystemContactWrites.php new file mode 100644 index 000000000..bbed237ce --- /dev/null +++ b/lib/Service/Contactpersoon/SystemContactWrites.php @@ -0,0 +1,67 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://github.com/ConductionNL/stackiq + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Service\Contactpersoon; + +/** + * The contact sync job and the contact migration run their writes through + * {@see run()}; the contact person listener does not check a write made while one + * is running. The mark is process-wide and ends when the callable returns or throws, + * so a later job in the same cron run is checked again. + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + */ +class SystemContactWrites { + /** + * How many system writes are running in this process. + * + * @var integer + */ + private static int $depth = 0; + + /** + * Run a callable as a system write. + * + * @param callable $write The writes to make. + * + * @return mixed What the callable returns. + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + */ + public function run(callable $write): mixed { + self::$depth++; + try { + return $write(); + } finally { + self::$depth--; + } + }//end run() + + /** + * Whether a system write is running. + * + * @return bool + * + * @spec openspec/specs/contactpersoon-sync/spec.md#requirement-the-system-shall-only-accept-a-contact-person-from-someone-who-may-add-members-to-its-organisation-req-003 + */ + public function isActive(): bool { + return self::$depth > 0; + }//end isActive() +}//end class diff --git a/lib/Service/Stackiq/ContactPersonHandler.php b/lib/Service/Stackiq/ContactPersonHandler.php index ca4717a72..4de540214 100644 --- a/lib/Service/Stackiq/ContactPersonHandler.php +++ b/lib/Service/Stackiq/ContactPersonHandler.php @@ -1211,7 +1211,7 @@ public function storeContactNameFields(\OCP\IUser $user, array $contactData): vo }//end if // Sync e-mailadres to user's email property. - $email = $contactData['e-mailadres'] ?? $contactData['email'] ?? ''; + $email = $contactData['email'] ?? $contactData['e-mailadres'] ?? ''; if (empty($email) === false && $email !== $user->getEMailAddress()) { $user->setEMailAddress($email); $this->_logger->info( diff --git a/openspec/specs/contactpersoon-sync/spec.md b/openspec/specs/contactpersoon-sync/spec.md index cb9f8136c..a92d5f08a 100644 --- a/openspec/specs/contactpersoon-sync/spec.md +++ b/openspec/specs/contactpersoon-sync/spec.md @@ -34,3 +34,23 @@ Synchronises contact-person records into managed Nextcloud user accounts, creati - WHEN `disableUserForContactpersoon(id)` is called - THEN the linked account MUST be disabled + +### Requirement: The system SHALL only accept a contact person from someone who may add members to its organisation (REQ-003) +The contact sync turns a contact person into an account in the contact's organisation, resets the role group of an existing account named by its e-mail address, and writes names and e-mail address to the account named by `username`. A create, or an update that changes any of `organisation`, `organization`, `email`, `e-mailadres` or `username`, SHALL be refused unless the caller is an administrator, or a member of the contact's OpenRegister organisation in `gebruik-beheerder` or `aanbod-beheerder`. Every one of these keys SHALL be checked on its own, and a contact whose two organisation keys or two e-mail keys hold different values SHALL be refused. A contact person SHALL NOT name an existing account other than the caller's own, in its e-mail keys or in `username`, unless that account already belongs to the contact's organisation, and `username` only when it is the caller's own account. A member MAY change the e-mail address of their own contact person in the same organisation. A contact for an organisation that has no OpenRegister organisation yet (a registration under review) is accepted when it names no existing account. Only the writes stackiq's own contact sync job and contact migration mark as system writes are not checked; other command line writes are. + +@e2e exclude Enforced in an OpenRegister pre-save listener; tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php drives it with the real guard. + +#### Scenario: REQ-003 another organisation +- GIVEN a signed-in user who is not a member of organisation X +- WHEN they create a contact person for organisation X +- THEN the create MUST be refused and no account is made + +#### Scenario: REQ-003 an existing account +- GIVEN an existing account that belongs to organisation A +- WHEN a member manager of organisation X creates a contact person for X with that account's e-mail address +- THEN the create MUST be refused and the account's groups are left alone + +#### Scenario: REQ-003 own organisation +- GIVEN a member of organisation X in `gebruik-beheerder` +- WHEN they create a contact person for X with a new e-mail address +- THEN the create MUST be accepted diff --git a/tests/Stubs/Db/Organisation.php b/tests/Stubs/Db/Organisation.php index 6a79b1480..6d24f341b 100644 --- a/tests/Stubs/Db/Organisation.php +++ b/tests/Stubs/Db/Organisation.php @@ -107,4 +107,29 @@ public function isActive(): bool { return $this->active; }//end isActive() + /** + * @var array + */ + private array $users = []; + + /** + * @param array $users The member user ids. + * + * @return void + */ + public function setUsers(array $users): void { + $this->users = $users; + }//end setUsers() + + /** + * Same check as OpenRegister's Organisation::hasUser(). + * + * @param string $userId The user id. + * + * @return bool + */ + public function hasUser(string $userId): bool { + return in_array($userId, $this->users); + }//end hasUser() + }//end class diff --git a/tests/Stubs/OpenRegister/Event/ObjectUpdatingEvent.php b/tests/Stubs/OpenRegister/Event/ObjectUpdatingEvent.php new file mode 100644 index 000000000..6e6c0fcbe --- /dev/null +++ b/tests/Stubs/OpenRegister/Event/ObjectUpdatingEvent.php @@ -0,0 +1,59 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://github.com/ConductionNL/stackiq + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Event; +use OCA\OpenRegister\Db\ObjectEntity; +use OCP\EventDispatcher\Event; +use Psr\EventDispatcher\StoppableEventInterface; +class ObjectUpdatingEvent extends Event implements StoppableEventInterface { + private ObjectEntity $newObject; + private ?ObjectEntity $oldObject; + private bool $propagationStopped = false; + private array $errors = []; + private array $modifiedData = []; + public function __construct(ObjectEntity $newObject, ?ObjectEntity $oldObject = null) { + parent::__construct(); + $this->newObject = $newObject; + $this->oldObject = $oldObject; + }//end __construct() + public function getNewObject(): ObjectEntity { + return $this->newObject; + }//end getNewObject() + public function getOldObject(): ?ObjectEntity { + return $this->oldObject; + }//end getOldObject() + public function isPropagationStopped(): bool { + return $this->propagationStopped; + }//end isPropagationStopped() + public function stopPropagation(): void { + $this->propagationStopped = true; + }//end stopPropagation() + public function setErrors(array $errors): void { + $this->errors = $errors; + }//end setErrors() + public function getErrors(): array { + return $this->errors; + }//end getErrors() + public function setModifiedData(array $data): void { + $this->modifiedData = $data; + }//end setModifiedData() + public function getModifiedData(): array { + return $this->modifiedData; + }//end getModifiedData() +}//end class diff --git a/tests/Unit/BackgroundJob/OrganizationContactSyncJobTest.php b/tests/Unit/BackgroundJob/OrganizationContactSyncJobTest.php index e0b28db43..007ea12e1 100644 --- a/tests/Unit/BackgroundJob/OrganizationContactSyncJobTest.php +++ b/tests/Unit/BackgroundJob/OrganizationContactSyncJobTest.php @@ -29,6 +29,7 @@ namespace OCA\Stackiq\Tests\Unit\BackgroundJob; use OCA\Stackiq\BackgroundJob\OrganizationContactSyncJob; +use OCA\Stackiq\Service\Contactpersoon\SystemContactWrites; use OCA\Stackiq\Service\OrganizationSyncService; use OCA\Stackiq\Service\SettingsService; use OCA\Stackiq\Service\StackiqContactSyncService; @@ -87,6 +88,30 @@ public function testAnEnabledOrUnsavedJobSyncs(): void { } }//end testAnEnabledOrUnsavedJobSyncs() + /** + * The sync writes as stackiq itself, so the contact person check lets them through, + * and the mark is gone once the job returns. + * + * @return void + */ + public function testTheSyncRunsAsASystemWrite(): void { + $store = []; + $active = null; + $sync = $this->createMock(OrganizationSyncService::class); + $sync->method('performScheduledSync')->willReturnCallback( + function () use (&$active): void { + $active = (new SystemContactWrites())->isActive(); + } + ); + $contacts = $this->createMock(StackiqContactSyncService::class); + $contacts->method('isAvailable')->willReturn(false); + + $this->runJob($sync, $contacts, $this->settings($store)); + + $this->assertTrue($active); + $this->assertFalse((new SystemContactWrites())->isActive()); + }//end testTheSyncRunsAsASystemWrite() + /** * Run the job's protected run() once. * diff --git a/tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php b/tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php new file mode 100644 index 000000000..df6927529 --- /dev/null +++ b/tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php @@ -0,0 +1,449 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://github.com/ConductionNL/stackiq + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Stackiq\Tests\Unit\EventListener; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCA\OpenRegister\Db\Organisation; +use OCA\OpenRegister\Db\OrganisationMapper; +use OCA\OpenRegister\Event\ObjectCreatingEvent; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; +use OCA\Stackiq\EventListener\ContactPersonOrganisationListener; +use OCA\Stackiq\Service\Contactpersoon\ContactPersonOrganisationGuard; +use OCA\Stackiq\Service\Contactpersoon\SystemContactWrites; +use OCA\Stackiq\Service\SettingsService; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\IGroupManager; +use OCP\IUser; +use OCP\IUserManager; +use OCP\IUserSession; +use PHPUnit\Framework\TestCase; + +/** + * REQ-003: a contact person only from an administrator, or from a member of its + * organisation who may add members; an existing account only when it is already a member. + * + * @covers \OCA\Stackiq\EventListener\ContactPersonOrganisationListener + * @covers \OCA\Stackiq\Service\Contactpersoon\ContactPersonOrganisationGuard + * @covers \OCA\Stackiq\Service\Contactpersoon\SystemContactWrites + */ +final class ContactPersonOrganisationListenerTest extends TestCase { + private const SCHEMA = '41'; + + private const ORG_X = '6b0c1f8e-0000-4000-8000-00000000000x'; + + private const ORG_A = '6b0c1f8e-0000-4000-8000-00000000000a'; + + /** + * Organisation uuid => member uids. + * + * @var array> + */ + private array $organisations = [ + self::ORG_X => ['beheer-x', 'raadpleger-x', 'victim-x'], + self::ORG_A => ['beheer-a', 'victim-a'], + ]; + + /** + * Uid => groups. + * + * @var array> + */ + private array $groups = [ + 'beheer-x' => ['gebruik-beheerder'], + 'raadpleger-x' => ['gebruik-raadpleger'], + 'beheer-a' => ['aanbod-beheerder'], + 'outsider' => ['software-catalog-users'], + 'root' => ['admin'], + 'victim-x' => ['gebruik-raadpleger'], + 'victim-a' => ['gebruik-beheerder'], + ]; + + public function testASignedInUserOutsideTheOrganisationCannotAddAnAccountToIt(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'new@example.org']); + + $this->listener(caller: 'outsider')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + self::assertSame(['message' => 'This contact person could not be saved.'], $event->getErrors()); + }//end testASignedInUserOutsideTheOrganisationCannotAddAnAccountToIt() + + public function testAMemberManagerOfAnotherOrganisationCannotAddAnAccountToIt(): void { + $event = $this->creating(data: ['organization' => self::ORG_X, 'e-mailadres' => 'new@example.org']); + + $this->listener(caller: 'beheer-a')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAMemberManagerOfAnotherOrganisationCannotAddAnAccountToIt() + + public function testAMemberWithoutTheRightToAddMembersIsRefused(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'new@example.org']); + + $this->listener(caller: 'raadpleger-x')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAMemberWithoutTheRightToAddMembersIsRefused() + + public function testAMemberManagerAddsAContactToTheOwnOrganisation(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'new@example.org']); + + $this->listener(caller: 'beheer-x')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + self::assertSame([], $event->getErrors()); + }//end testAMemberManagerAddsAContactToTheOwnOrganisation() + + public function testAnAdministratorMayWriteAnyContact(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'victim-a']); + + $this->listener(caller: 'root')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testAnAdministratorMayWriteAnyContact() + + public function testAContactCannotNameAnExistingAccountOfAnotherOrganisation(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'victim-a']); + + $this->listener(caller: 'beheer-x')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAContactCannotNameAnExistingAccountOfAnotherOrganisation() + + public function testAContactWithoutOrganisationCannotNameSomeoneElsesAccount(): void { + $event = $this->creating(data: ['email' => 'victim-a']); + + $this->listener(caller: 'outsider')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAContactWithoutOrganisationCannotNameSomeoneElsesAccount() + + public function testAMemberManagerMayNameAnAccountThatIsAlreadyAMember(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'victim-x']); + + $this->listener(caller: 'beheer-x')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testAMemberManagerMayNameAnAccountThatIsAlreadyAMember() + + public function testAnAnonymousContactForAnExistingOrganisationIsRefused(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'new@example.org']); + + $this->listener(caller: null)->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAnAnonymousContactForAnExistingOrganisationIsRefused() + + public function testAContactForAnOrganisationThatIsStillBeingRegisteredIsAccepted(): void { + $event = $this->creating(data: ['organisation' => 'not-yet-an-organisation', 'email' => 'new@example.org']); + + $this->listener(caller: null)->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testAContactForAnOrganisationThatIsStillBeingRegisteredIsAccepted() + + public function testAnUpdateThatMovesTheContactToAnotherOrganisationIsChecked(): void { + $event = $this->updating( + data: ['organisation' => self::ORG_X, 'email' => 'self@example.org'], + oldData: ['organisation' => self::ORG_A, 'email' => 'self@example.org'] + ); + + $this->listener(caller: 'beheer-a')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAnUpdateThatMovesTheContactToAnotherOrganisationIsChecked() + + public function testAnUpdateThatChangesTheEmailAddressIsChecked(): void { + $event = $this->updating( + data: ['organisation' => self::ORG_X, 'email' => 'victim-a'], + oldData: ['organisation' => self::ORG_X, 'email' => 'new@example.org'] + ); + + $this->listener(caller: 'beheer-x')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAnUpdateThatChangesTheEmailAddressIsChecked() + + public function testAnUpdateWithoutAStoredVersionIsChecked(): void { + $event = $this->updating(data: ['organisation' => self::ORG_X, 'email' => 'new@example.org'], oldData: null); + + $this->listener(caller: 'outsider')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAnUpdateWithoutAStoredVersionIsChecked() + + public function testAnUpdateThatKeepsOrganisationAndEmailIsNotCheckedAgain(): void { + $event = $this->updating( + data: ['organisation' => self::ORG_X, 'email' => 'raadpleger-x', 'voornaam' => 'Nieuw'], + oldData: ['organisation' => self::ORG_X, 'email' => 'raadpleger-x', 'voornaam' => 'Oud'] + ); + + $this->listener(caller: 'raadpleger-x')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testAnUpdateThatKeepsOrganisationAndEmailIsNotCheckedAgain() + + public function testDataAnotherListenerChangedIsChecked(): void { + $event = $this->creating(data: ['email' => 'new@example.org']); + $event->setModifiedData(['organisation' => self::ORG_X]); + + $this->listener(caller: 'outsider')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testDataAnotherListenerChangedIsChecked() + + public function testAWriteStackiqMarksAsItsOwnIsNotChecked(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'victim-a']); + + (new SystemContactWrites())->run(fn () => $this->listener(caller: null)->handle($event)); + + self::assertFalse($event->isPropagationStopped()); + }//end testAWriteStackiqMarksAsItsOwnIsNotChecked() + + public function testTheMarkEndsWithTheSystemWrite(): void { + (new SystemContactWrites())->run(fn () => null); + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'victim-a']); + + $this->listener(caller: null)->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testTheMarkEndsWithTheSystemWrite() + + public function testTheMarkEndsWhenTheSystemWriteThrows(): void { + try { + (new SystemContactWrites())->run(fn () => throw new \RuntimeException('sync failed')); + } catch (\RuntimeException $e) { + } + + self::assertFalse((new SystemContactWrites())->isActive()); + }//end testTheMarkEndsWhenTheSystemWriteThrows() + + public function testAnUnmarkedWriteWithoutASessionIsChecked(): void { + $event = $this->updating( + data: ['organization' => self::ORG_X, 'email' => 'x@example.org'], + oldData: ['organization' => self::ORG_A, 'email' => 'x@example.org'] + ); + + $this->listener(caller: null)->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAnUnmarkedWriteWithoutASessionIsChecked() + + public function testAnEmptyOrganisationKeyDoesNotHideTheOtherKey(): void { + $event = $this->creating(data: ['organisation' => '', 'organization' => self::ORG_X, 'email' => 'attacker@example.org']); + + $this->listener(caller: 'outsider')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAnEmptyOrganisationKeyDoesNotHideTheOtherKey() + + public function testTwoOrganisationKeysThatDisagreeAreRefused(): void { + foreach (['00000000-0000-4000-8000-000000000000', self::ORG_A] as $organisation) { + $event = $this->creating(data: ['organisation' => $organisation, 'organization' => self::ORG_X, 'email' => 'attacker@example.org']); + + $this->listener(caller: 'beheer-a')->handle($event); + + self::assertTrue($event->isPropagationStopped(), $organisation); + } + }//end testTwoOrganisationKeysThatDisagreeAreRefused() + + public function testTwoEmailKeysThatDisagreeAreRefused(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'new@example.org', 'e-mailadres' => 'other@example.org']); + + $this->listener(caller: 'beheer-x')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testTwoEmailKeysThatDisagreeAreRefused() + + public function testAnUpdateThatSwapsTheOtherOrganisationKeyIsChecked(): void { + $event = $this->updating( + data: ['organisation' => self::ORG_A, 'organization' => self::ORG_X, 'email' => 'x@example.org'], + oldData: ['organization' => self::ORG_A, 'email' => 'x@example.org'] + ); + + $this->listener(caller: 'outsider')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAnUpdateThatSwapsTheOtherOrganisationKeyIsChecked() + + public function testUsernameCannotNameAnotherAccount(): void { + $event = $this->updating( + data: ['organization' => self::ORG_X, 'e-mailadres' => 'victim-x', 'username' => 'root', 'voornaam' => 'x'], + oldData: ['organization' => self::ORG_X, 'e-mailadres' => 'victim-x', 'username' => 'victim-x'] + ); + + $this->listener(caller: 'beheer-x')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testUsernameCannotNameAnotherAccount() + + public function testAnAdministratorMaySetUsername(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'victim-x', 'username' => 'victim-x']); + + $this->listener(caller: 'root')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testAnAdministratorMaySetUsername() + + public function testAMemberManagesTheirOwnContact(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'beheer-x', 'username' => 'beheer-x']); + + $this->listener(caller: 'beheer-x')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testAMemberManagesTheirOwnContact() + + public function testAMemberChangesTheEmailAddressOfTheirOwnContact(): void { + $event = $this->updating( + data: ['organisation' => self::ORG_X, 'email' => 'new-address@example.org', 'username' => 'raadpleger-x'], + oldData: ['organisation' => self::ORG_X, 'email' => 'raadpleger-x', 'username' => 'raadpleger-x'] + ); + + $this->listener(caller: 'raadpleger-x')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testAMemberChangesTheEmailAddressOfTheirOwnContact() + + public function testTheOwnContactCannotNameAnotherAccount(): void { + $event = $this->updating( + data: ['organisation' => self::ORG_X, 'email' => 'victim-a', 'username' => 'raadpleger-x'], + oldData: ['organisation' => self::ORG_X, 'email' => 'raadpleger-x', 'username' => 'raadpleger-x'] + ); + + $this->listener(caller: 'raadpleger-x')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testTheOwnContactCannotNameAnotherAccount() + + public function testTheOwnContactCannotMoveToAnotherOrganisation(): void { + $event = $this->updating( + data: ['organisation' => self::ORG_A, 'email' => 'raadpleger-x', 'username' => 'raadpleger-x'], + oldData: ['organisation' => self::ORG_X, 'email' => 'raadpleger-x', 'username' => 'raadpleger-x'] + ); + + $this->listener(caller: 'raadpleger-x')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testTheOwnContactCannotMoveToAnotherOrganisation() + + public function testTheSameUuidInAnotherCaseIsNoConflict(): void { + $event = $this->creating(data: ['organisation' => strtoupper(self::ORG_X), 'organization' => self::ORG_X, 'email' => 'new@example.org']); + + $this->listener(caller: 'beheer-x')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testTheSameUuidInAnotherCaseIsNoConflict() + + public function testAnAccountNameInAnotherCaseOrWithSpacesIsTheSameAccount(): void { + $event = $this->creating(data: ['organisation' => self::ORG_A, 'email' => ' VICTIM-X ']); + + $this->listener(caller: 'beheer-a')->handle($event); + + self::assertTrue($event->isPropagationStopped()); + }//end testAnAccountNameInAnotherCaseOrWithSpacesIsTheSameAccount() + + public function testAnotherSchemaIsLeftAlone(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'new@example.org'], schema: '30'); + + $this->listener(caller: 'outsider')->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testAnotherSchemaIsLeftAlone() + + public function testTheListenerIsRegisteredForCreatesAndUpdates(): void { + $source = (string)file_get_contents(__DIR__ . '/../../../lib/AppInfo/Application.php'); + + foreach (['ObjectCreatingEvent', 'ObjectUpdatingEvent'] as $event) { + self::assertMatchesRegularExpression( + '/registerEventListener\(\s*' . $event . '::class,\s*ContactPersonOrganisationListener::class\s*\)/', + $source + ); + } + }//end testTheListenerIsRegisteredForCreatesAndUpdates() + + /** + * @param array $data The object data. + */ + private function creating(array $data, string $schema = self::SCHEMA): ObjectCreatingEvent { + return new ObjectCreatingEvent($this->entity(data: $data, schema: $schema)); + }//end creating() + + /** + * @param array $data The new object data. + * @param array|null $oldData The stored object data. + */ + private function updating(array $data, ?array $oldData): ObjectUpdatingEvent { + $old = null; + if ($oldData !== null) { + $old = $this->entity(data: $oldData, schema: self::SCHEMA); + } + + return new ObjectUpdatingEvent($this->entity(data: $data, schema: self::SCHEMA), $old); + }//end updating() + + /** + * @param array $data The object data. + */ + private function entity(array $data, string $schema): ObjectEntity { + $entity = $this->createMock(ObjectEntity::class); + $entity->method('getSchema')->willReturn($schema); + $entity->method('getObject')->willReturn($data); + + return $entity; + }//end entity() + + private function listener(?string $caller): ContactPersonOrganisationListener { + $session = $this->createMock(IUserSession::class); + $session->method('getUser')->willReturn($caller === null ? null : $this->user(uid: $caller)); + + $groupManager = $this->createMock(IGroupManager::class); + $groupManager->method('isAdmin')->willReturnCallback(fn (string $uid): bool => in_array('admin', ($this->groups[$uid] ?? []), true)); + $groupManager->method('isInGroup')->willReturnCallback(fn (string $uid, string $gid): bool => in_array($gid, ($this->groups[$uid] ?? []), true)); + + $userManager = $this->createMock(IUserManager::class); + $userManager->method('get')->willReturnCallback(fn (string $uid): ?IUser => isset($this->groups[strtolower($uid)]) === true ? $this->user(uid: strtolower($uid)) : null); + + $mapper = $this->createMock(OrganisationMapper::class); + $mapper->method('findByUuid')->willReturnCallback( + function (string $uuid): Organisation { + if (isset($this->organisations[$uuid]) === false) { + throw new DoesNotExistException('no organisation'); + } + + $organisation = new Organisation(); + $organisation->setUuid($uuid); + $organisation->setUsers($this->organisations[$uuid]); + return $organisation; + } + ); + + $settings = $this->createMock(SettingsService::class); + $settings->method('getVoorzieningenConfig')->willReturn(['contactpersoon_schema' => self::SCHEMA, 'organisatie_schema' => '30']); + + return new ContactPersonOrganisationListener( + new ContactPersonOrganisationGuard($session, $groupManager, $userManager, $mapper), + $settings, + new SystemContactWrites() + ); + }//end listener() + + private function user(string $uid): IUser { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + return $user; + }//end user() +}//end class diff --git a/tests/Unit/Service/ContactPersonHandlerTest.php b/tests/Unit/Service/ContactPersonHandlerTest.php index 6cdb30134..1d277a569 100644 --- a/tests/Unit/Service/ContactPersonHandlerTest.php +++ b/tests/Unit/Service/ContactPersonHandlerTest.php @@ -182,6 +182,23 @@ private function addToGroup(IUser $user, string $group): void { (new ReflectionMethod(ContactPersonHandler::class, 'addUserToGroupWithCheck'))->invoke($this->handler, $user, $group, 'test-type'); }//end addToGroup() + /** + * The name sync writes the address under `email` when both keys are set, the same + * key the contact sync uses as the account name. + * + * @return void + */ + public function testTheNameSyncPrefersTheEmailKey(): void { + $user = $this->user(); + $user->method('getEMailAddress')->willReturn('old@example.org'); + $user->expects($this->once())->method('setEMailAddress')->with('account@example.org'); + + $this->handler->storeContactNameFields( + user: $user, + contactData: ['email' => 'account@example.org', 'e-mailadres' => 'other@example.org'] + ); + }//end testTheNameSyncPrefersTheEmailKey() + /** * A user double. *