From ff47e1a33807d93cf3a85254524fb986c1e4e99a Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 4 Nov 2026 13:00:50 +0100 Subject: [PATCH 1/3] fix(contacts): accept a contact person only from someone who may add members to its organisation Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/AppInfo/Application.php | 6 + .../ContactPersonOrganisationListener.php | 105 ++++++ .../ContactPersonOrganisationGuard.php | 193 +++++++++++ openspec/specs/contactpersoon-sync/spec.md | 20 ++ tests/Stubs/Db/Organisation.php | 25 ++ .../Event/ObjectUpdatingEvent.php | 59 ++++ .../ContactPersonOrganisationListenerTest.php | 305 ++++++++++++++++++ 7 files changed, 713 insertions(+) create mode 100644 lib/EventListener/ContactPersonOrganisationListener.php create mode 100644 lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php create mode 100644 tests/Stubs/OpenRegister/Event/ObjectUpdatingEvent.php create mode 100644 tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php 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/EventListener/ContactPersonOrganisationListener.php b/lib/EventListener/ContactPersonOrganisationListener.php new file mode 100644 index 000000000..8a100887a --- /dev/null +++ b/lib/EventListener/ContactPersonOrganisationListener.php @@ -0,0 +1,105 @@ + + * @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\Event\ObjectCreatingEvent; +use OCA\OpenRegister\Event\ObjectUpdatingEvent; +use OCA\Stackiq\Service\Contactpersoon\ContactPersonOrganisationGuard; +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 + * the organisation and the e-mail address is not checked again. A write from the + * command line (`occ`, cron) is not checked: installs, imports and the sync run there. + * + * @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 bool|null $commandLine Whether this runs from the command line; null reads PHP_SAPI. + */ + public function __construct( + private readonly ContactPersonOrganisationGuard $guard, + private readonly SettingsService $settingsService, + private ?bool $commandLine = null, + ) { + $this->commandLine ??= (PHP_SAPI === 'cli'); + }//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->commandLine === true) { + return; + } + + $object = null; + $oldData = null; + if ($event instanceof ObjectCreatingEvent) { + $object = $event->getObject(); + } else if ($event instanceof ObjectUpdatingEvent) { + $object = $event->getNewObject(); + $oldData = $event->getOldObject()?->getObject(); + } + + if ($object === null || $this->isContactPerson(schema: (string)$object->getSchema()) === false) { + return; + } + + $data = array_merge(($object->getObject() ?? []), $event->getModifiedData()); + if (ContactPersonOrganisationGuard::needsCheck(contactData: $data, oldData: $oldData) === false + || $this->guard->allows(contactData: $data) === true + ) { + return; + } + + $event->setErrors(['message' => 'This contact person could not be saved.']); + $event->stopPropagation(); + }//end handle() + + /** + * Whether this schema id is the contact person schema. + * + * @param string $schema The object's schema id. + * + * @return bool + */ + private function isContactPerson(string $schema): bool { + $configured = (string)($this->settingsService->getVoorzieningenConfig()['contactpersoon_schema'] ?? ''); + return $configured !== '' && $schema === $configured; + }//end isContactPerson() +}//end class diff --git a/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php b/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php new file mode 100644 index 000000000..cbe8b7ab4 --- /dev/null +++ b/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php @@ -0,0 +1,193 @@ + + * @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, and resets the role group of an existing account named by its e-mail + * address. So a contact person is accepted only from an administrator, or from a member + * of its organisation who may add members, and it may name an existing account only + * when that account already belongs to the same organisation. + * + * @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']; + + /** + * Constructor. + * + * @param IUserSession $userSession Names the caller. + * @param IGroupManager $groupManager Tells administrators and member managers apart. + * @param IUserManager $userManager Tells whether the e-mail address names 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. + * + * @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): bool { + $caller = $this->userSession->getUser()?->getUID(); + if ($caller !== null && $this->groupManager->isAdmin($caller) === true) { + return true; + } + + $organisation = $this->findOrganisation(uuid: self::organisationUuid(contactData: $contactData)); + $account = $this->existingAccount(contactData: $contactData, caller: $caller); + + if ($organisation === null) { + return $account === null; + } + + if ($account !== null && $organisation->hasUser($account) === false) { + return false; + } + + return $caller !== null && $organisation->hasUser($caller) === true && $this->managesMembers(uid: $caller) === true; + }//end allows() + + /** + * Whether the change from the stored contact person needs the check again. + * + * Only the organisation and the e-mail address decide which account joins which organisation. + * + * @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 static function needsCheck(array $contactData, ?array $oldData): bool { + return $oldData === null + || self::organisationUuid(contactData: $contactData) !== self::organisationUuid(contactData: $oldData) + || self::email(contactData: $contactData) !== self::email(contactData: $oldData); + }//end needsCheck() + + /** + * The uuid of the organisation the contact person names, or an empty string. + * + * @param array $contactData The contact person's data. + * + * @return string + */ + private static function organisationUuid(array $contactData): string { + $value = ($contactData['organisation'] ?? $contactData['organization'] ?? ''); + if (is_array($value) === true) { + $value = ($value['uuid'] ?? $value['id'] ?? ''); + } + + return is_string($value) === true ? trim($value) : ''; + }//end organisationUuid() + + /** + * The e-mail address the sync uses as the account name, or an empty string. + * + * @param array $contactData The contact person's data. + * + * @return string + */ + private static function email(array $contactData): string { + $value = ($contactData['email'] ?? $contactData['e-mailadres'] ?? ''); + return is_string($value) === true ? trim($value) : ''; + }//end email() + + /** + * 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 the e-mail address names, when it is not the caller's own. + * + * @param array $contactData The contact person's data. + * @param string|null $caller The caller's uid. + * + * @return string|null + */ + private function existingAccount(array $contactData, ?string $caller): ?string { + $email = self::email(contactData: $contactData); + if ($email === '') { + return null; + } + + $uid = $this->userManager->get($email)?->getUID(); + if ($uid === null || $uid === $caller) { + return null; + } + + return $uid; + }//end existingAccount() + + /** + * 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 { + foreach (self::MEMBER_MANAGER_GROUPS as $group) { + if ($this->groupManager->isInGroup($uid, $group) === true) { + return true; + } + } + + return false; + }//end managesMembers() +}//end class diff --git a/openspec/specs/contactpersoon-sync/spec.md b/openspec/specs/contactpersoon-sync/spec.md index cb9f8136c..ec9752328 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 and resets the role group of an existing account named by its e-mail address. A create, or an update that changes the organisation or the e-mail address, SHALL be refused unless the caller is an administrator, or a member of the contact's OpenRegister organisation in `gebruik-beheerder` or `aanbod-beheerder`. A contact person SHALL NOT name an existing account other than the caller's own unless that account already belongs to the contact's organisation. A contact for an organisation that has no OpenRegister organisation yet (a registration under review) is accepted when it names no existing account. Writes from the command line are not checked. + +@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/EventListener/ContactPersonOrganisationListenerTest.php b/tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php new file mode 100644 index 000000000..5a1b0db14 --- /dev/null +++ b/tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php @@ -0,0 +1,305 @@ + + * @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\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 + */ +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 testTheCommandLineIsNotChecked(): void { + $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'victim-a']); + + $this->listener(caller: null, commandLine: true)->handle($event); + + self::assertFalse($event->isPropagationStopped()); + }//end testTheCommandLineIsNotChecked() + + 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, bool $commandLine = false): 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[$uid]) === true ? $this->user(uid: $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, + $commandLine + ); + }//end listener() + + private function user(string $uid): IUser { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + return $user; + }//end user() +}//end class From 09f476e142ef0b544eca3e8c05867f49f015a926 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 4 Nov 2026 13:05:50 +0100 Subject: [PATCH 2/3] fix(contacts): type the pre-save event and keep the guard free of static calls Assisted-by: ClaudeCode:claude-opus-5-5 --- .../ContactPersonOrganisationListener.php | 50 +++++++++---------- .../ContactPersonOrganisationGuard.php | 25 ++++++---- 2 files changed, 41 insertions(+), 34 deletions(-) diff --git a/lib/EventListener/ContactPersonOrganisationListener.php b/lib/EventListener/ContactPersonOrganisationListener.php index 8a100887a..6df9c740c 100644 --- a/lib/EventListener/ContactPersonOrganisationListener.php +++ b/lib/EventListener/ContactPersonOrganisationListener.php @@ -20,6 +20,7 @@ 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; @@ -43,14 +44,13 @@ class ContactPersonOrganisationListener implements IEventListener { * * @param ContactPersonOrganisationGuard $guard Decides whether the caller may write the contact person. * @param SettingsService $settingsService Names the contact person schema. - * @param bool|null $commandLine Whether this runs from the command line; null reads PHP_SAPI. + * @param bool $fromCli True when PHP runs from the command line. */ public function __construct( private readonly ContactPersonOrganisationGuard $guard, private readonly SettingsService $settingsService, - private ?bool $commandLine = null, + private readonly bool $fromCli = PHP_SAPI === 'cli', ) { - $this->commandLine ??= (PHP_SAPI === 'cli'); }//end __construct() /** @@ -63,25 +63,37 @@ public function __construct( * @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->commandLine === true) { + if ($this->fromCli === true) { return; } - $object = null; - $oldData = null; if ($event instanceof ObjectCreatingEvent) { - $object = $event->getObject(); - } else if ($event instanceof ObjectUpdatingEvent) { - $object = $event->getNewObject(); - $oldData = $event->getOldObject()?->getObject(); + $this->check(event: $event, object: $event->getObject(), oldData: null); + return; } - if ($object === null || $this->isContactPerson(schema: (string)$object->getSchema()) === false) { + 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 (ContactPersonOrganisationGuard::needsCheck(contactData: $data, oldData: $oldData) === false + if ($this->guard->needsCheck(contactData: $data, oldData: $oldData) === false || $this->guard->allows(contactData: $data) === true ) { return; @@ -89,17 +101,5 @@ public function handle(Event $event): void { $event->setErrors(['message' => 'This contact person could not be saved.']); $event->stopPropagation(); - }//end handle() - - /** - * Whether this schema id is the contact person schema. - * - * @param string $schema The object's schema id. - * - * @return bool - */ - private function isContactPerson(string $schema): bool { - $configured = (string)($this->settingsService->getVoorzieningenConfig()['contactpersoon_schema'] ?? ''); - return $configured !== '' && $schema === $configured; - }//end isContactPerson() + }//end check() }//end class diff --git a/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php b/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php index cbe8b7ab4..1f53939ca 100644 --- a/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php +++ b/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php @@ -99,7 +99,7 @@ public function allows(array $contactData): 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 static function needsCheck(array $contactData, ?array $oldData): bool { + public function needsCheck(array $contactData, ?array $oldData): bool { return $oldData === null || self::organisationUuid(contactData: $contactData) !== self::organisationUuid(contactData: $oldData) || self::email(contactData: $contactData) !== self::email(contactData: $oldData); @@ -118,7 +118,11 @@ private static function organisationUuid(array $contactData): string { $value = ($value['uuid'] ?? $value['id'] ?? ''); } - return is_string($value) === true ? trim($value) : ''; + if (is_string($value) === false) { + return ''; + } + + return trim($value); }//end organisationUuid() /** @@ -130,7 +134,11 @@ private static function organisationUuid(array $contactData): string { */ private static function email(array $contactData): string { $value = ($contactData['email'] ?? $contactData['e-mailadres'] ?? ''); - return is_string($value) === true ? trim($value) : ''; + if (is_string($value) === false) { + return ''; + } + + return trim($value); }//end email() /** @@ -182,12 +190,11 @@ private function existingAccount(array $contactData, ?string $caller): ?string { * @return bool */ private function managesMembers(string $uid): bool { - foreach (self::MEMBER_MANAGER_GROUPS as $group) { - if ($this->groupManager->isInGroup($uid, $group) === true) { - return true; - } - } + $memberOf = array_filter( + self::MEMBER_MANAGER_GROUPS, + fn (string $group): bool => $this->groupManager->isInGroup($uid, $group) + ); - return false; + return $memberOf !== []; }//end managesMembers() }//end class From eb8c1e82c99ed27484f9382cd15dc8194b474fab Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 4 Nov 2026 13:20:25 +0100 Subject: [PATCH 3/3] fix(contacts): check every contact key the sync reads, username, and only skip stackiq's own writes Assisted-by: ClaudeCode:claude-opus-5-5 --- .../OrganizationContactSyncJob.php | 14 +- .../ContactPersonOrganisationListener.php | 14 +- lib/Repair/MigrateContactsToNc.php | 16 +- .../ContactPersonOrganisationGuard.php | 168 +++++++++++++----- .../Contactpersoon/SystemContactWrites.php | 67 +++++++ lib/Service/Stackiq/ContactPersonHandler.php | 2 +- openspec/specs/contactpersoon-sync/spec.md | 2 +- .../OrganizationContactSyncJobTest.php | 25 +++ .../ContactPersonOrganisationListenerTest.php | 156 +++++++++++++++- .../Unit/Service/ContactPersonHandlerTest.php | 17 ++ 10 files changed, 421 insertions(+), 60 deletions(-) create mode 100644 lib/Service/Contactpersoon/SystemContactWrites.php 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 index 6df9c740c..4c66d9d8c 100644 --- a/lib/EventListener/ContactPersonOrganisationListener.php +++ b/lib/EventListener/ContactPersonOrganisationListener.php @@ -24,6 +24,7 @@ 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; @@ -31,8 +32,9 @@ /** * On OpenRegister's creating and updating events for a contact person: the write is * refused unless {@see ContactPersonOrganisationGuard} allows it. An update that keeps - * the organisation and the e-mail address is not checked again. A write from the - * command line (`occ`, cron) is not checked: installs, imports and the sync run there. + * 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 * @@ -44,12 +46,12 @@ class ContactPersonOrganisationListener implements IEventListener { * * @param ContactPersonOrganisationGuard $guard Decides whether the caller may write the contact person. * @param SettingsService $settingsService Names the contact person schema. - * @param bool $fromCli True when PHP runs from the command line. + * @param SystemContactWrites $systemWrites Tells whether stackiq itself is writing. */ public function __construct( private readonly ContactPersonOrganisationGuard $guard, private readonly SettingsService $settingsService, - private readonly bool $fromCli = PHP_SAPI === 'cli', + private readonly SystemContactWrites $systemWrites, ) { }//end __construct() @@ -63,7 +65,7 @@ public function __construct( * @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->fromCli === true) { + if ($this->systemWrites->isActive() === true) { return; } @@ -94,7 +96,7 @@ private function check(ObjectCreatingEvent|ObjectUpdatingEvent $event, ObjectEnt $data = array_merge(($object->getObject() ?? []), $event->getModifiedData()); if ($this->guard->needsCheck(contactData: $data, oldData: $oldData) === false - || $this->guard->allows(contactData: $data) === true + || $this->guard->allows(contactData: $data, oldData: $oldData) === true ) { return; } 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 index 1f53939ca..c1a98845c 100644 --- a/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php +++ b/lib/Service/Contactpersoon/ContactPersonOrganisationGuard.php @@ -29,10 +29,13 @@ /** * The contact sync turns a contact person into an account that joins the contact's - * organisation, and resets the role group of an existing account named by its e-mail - * address. So a contact person is accepted only from an administrator, or from a member - * of its organisation who may add members, and it may name an existing account only - * when that account already belongs to the same organisation. + * 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 */ @@ -42,12 +45,27 @@ class ContactPersonOrganisationGuard { */ 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 the e-mail address names an existing account. + * @param IUserManager $userManager Tells whether a name is an existing account. * @param OrganisationMapper $organisationMapper Finds the OpenRegister organisation and its members. */ public function __construct( @@ -61,36 +79,69 @@ public function __construct( /** * Whether the caller may write this contact person. * - * @param array $contactData The contact person's data. + * @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): bool { + public function allows(array $contactData, ?array $oldData = null): bool { $caller = $this->userSession->getUser()?->getUID(); if ($caller !== null && $this->groupManager->isAdmin($caller) === true) { return true; } - $organisation = $this->findOrganisation(uuid: self::organisationUuid(contactData: $contactData)); - $account = $this->existingAccount(contactData: $contactData, caller: $caller); - - if ($organisation === null) { - return $account === null; + if ($this->isAmbiguousOrForeign(contactData: $contactData, caller: $caller) === true) { + return false; } - if ($account !== null && $organisation->hasUser($account) === 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 $caller !== null && $organisation->hasUser($caller) === true && $this->managesMembers(uid: $caller) === true; + 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. * - * Only the organisation and the e-mail address decide which account joins which organisation. + * 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. @@ -100,46 +151,64 @@ public function allows(array $contactData): 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 { - return $oldData === null - || self::organisationUuid(contactData: $contactData) !== self::organisationUuid(contactData: $oldData) - || self::email(contactData: $contactData) !== self::email(contactData: $oldData); + 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 uuid of the organisation the contact person names, or an empty string. + * 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 string + * @return array */ - private static function organisationUuid(array $contactData): string { - $value = ($contactData['organisation'] ?? $contactData['organization'] ?? ''); - if (is_array($value) === true) { - $value = ($value['uuid'] ?? $value['id'] ?? ''); - } + 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_string($value) === false) { - return ''; + if (is_scalar($value) === true && trim((string)$value) !== '') { + $values[] = strtolower(trim((string)$value)); + } } - return trim($value); - }//end organisationUuid() + return array_values(array_unique($values)); + }//end values() /** - * The e-mail address the sync uses as the account name, or an empty string. + * Whether the caller edits their own contact person and keeps its organisation. * - * @param array $contactData The contact person's data. + * @param array $contactData The new data. + * @param array|null $oldData The stored data. + * @param string|null $caller The caller's uid. * - * @return string + * @return bool */ - private static function email(array $contactData): string { - $value = ($contactData['email'] ?? $contactData['e-mailadres'] ?? ''); - if (is_string($value) === false) { - return ''; + private function isOwnContactInSameOrganisation(array $contactData, ?array $oldData, ?string $caller): bool { + if ($oldData === null || $caller === null) { + return false; } - return trim($value); - }//end email() + $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. @@ -161,27 +230,38 @@ private function findOrganisation(string $uuid): ?Organisation { }//end findOrganisation() /** - * The uid of the existing account the e-mail address names, when it is not the caller's own. + * The uid of the existing account this e-mail address names, when it is not the caller's own. * - * @param array $contactData The contact person's data. - * @param string|null $caller The caller's uid. + * @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(array $contactData, ?string $caller): ?string { - $email = self::email(contactData: $contactData); + private function existingAccount(string $email, ?string $caller): ?string { if ($email === '') { return null; } $uid = $this->userManager->get($email)?->getUID(); - if ($uid === null || $uid === $caller) { + 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. * 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 ec9752328..a92d5f08a 100644 --- a/openspec/specs/contactpersoon-sync/spec.md +++ b/openspec/specs/contactpersoon-sync/spec.md @@ -36,7 +36,7 @@ Synchronises contact-person records into managed Nextcloud user accounts, creati ### 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 and resets the role group of an existing account named by its e-mail address. A create, or an update that changes the organisation or the e-mail address, SHALL be refused unless the caller is an administrator, or a member of the contact's OpenRegister organisation in `gebruik-beheerder` or `aanbod-beheerder`. A contact person SHALL NOT name an existing account other than the caller's own unless that account already belongs to the contact's organisation. A contact for an organisation that has no OpenRegister organisation yet (a registration under review) is accepted when it names no existing account. Writes from the command line are not checked. +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. 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 index 5a1b0db14..df6927529 100644 --- a/tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php +++ b/tests/Unit/EventListener/ContactPersonOrganisationListenerTest.php @@ -25,6 +25,7 @@ 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; @@ -39,6 +40,7 @@ * * @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'; @@ -204,13 +206,155 @@ public function testDataAnotherListenerChangedIsChecked(): void { self::assertTrue($event->isPropagationStopped()); }//end testDataAnotherListenerChangedIsChecked() - public function testTheCommandLineIsNotChecked(): void { + public function testAWriteStackiqMarksAsItsOwnIsNotChecked(): void { $event = $this->creating(data: ['organisation' => self::ORG_X, 'email' => 'victim-a']); - $this->listener(caller: null, commandLine: true)->handle($event); + (new SystemContactWrites())->run(fn () => $this->listener(caller: null)->handle($event)); self::assertFalse($event->isPropagationStopped()); - }//end testTheCommandLineIsNotChecked() + }//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'); @@ -262,7 +406,7 @@ private function entity(array $data, string $schema): ObjectEntity { return $entity; }//end entity() - private function listener(?string $caller, bool $commandLine = false): ContactPersonOrganisationListener { + private function listener(?string $caller): ContactPersonOrganisationListener { $session = $this->createMock(IUserSession::class); $session->method('getUser')->willReturn($caller === null ? null : $this->user(uid: $caller)); @@ -271,7 +415,7 @@ private function listener(?string $caller, bool $commandLine = false): ContactPe $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[$uid]) === true ? $this->user(uid: $uid) : null); + $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( @@ -293,7 +437,7 @@ function (string $uuid): Organisation { return new ContactPersonOrganisationListener( new ContactPersonOrganisationGuard($session, $groupManager, $userManager, $mapper), $settings, - $commandLine + new SystemContactWrites() ); }//end listener() 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. *