fix(organisations): check that an organisation belongs to the organisatie before linking (REVIEW)

lib/Command/OrganisationLinkCommand.php

@@ -59,6 +59,7 @@ class OrganisationLinkCommand extends Command {
 		'no-organisation'     => '<error>No OpenRegister organisation has this uuid.</error>',
 		'organisation-in-use' => '<error>This OpenRegister organisation belongs to another organisatie.</error>',
 		'default-organisation' => '<error>This is the OpenRegister default organisation, which is never linked.</error>',
+		'default-unknown'     => '<error>OpenRegister cannot say which organisation is its default, so nothing is linked.</error>',
 		'not-confirmed'       => '<comment>Not linked.</comment> Confirm, or run again with --yes.',
 	];
 

lib/EventListener/AnonymousRegistrationListener.php

@@ -20,8 +20,10 @@
 
 namespace OCA\Stackiq\EventListener;
 
+use OCA\OpenRegister\Db\OrganisationMapper;
 use OCA\OpenRegister\Event\ObjectCreatingEvent;
 use OCA\Stackiq\Service\SettingsService;
+use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\EventDispatcher\Event;
 use OCP\EventDispatcher\IEventListener;
 use OCP\IUserSession;
@@ -36,6 +38,9 @@
  * administrators set them on purpose. So does a create from the command line
  * (`occ`, cron), where installs, imports and seed data run without a session.
  *
+ * An anonymous create that carries the uuid of an existing OpenRegister organisation
+ * is refused, because the sync links an organisatie to the organisation with its uuid.
+ *
  * @template-implements IEventListener<Event>
  *
  * @spec openspec/specs/organisation-registration/spec.md#requirement-req-orr-001-an-organisation-registered-without-an-account-is-pending-until-reviewed
@@ -46,18 +51,21 @@ class AnonymousRegistrationListener implements IEventListener {
 	 *
 	 * @param IUserSession $userSession Tells whether someone is signed in.
 	 * @param SettingsService $settingsService Names the organisation schema.
+	 * @param OrganisationMapper $organisationMapper Tells whether an OpenRegister organisation holds a uuid.
 	 * @param bool|null $commandLine Whether this runs from the command line; null reads PHP_SAPI.
 	 */
 	public function __construct(
 		private readonly IUserSession $userSession,
 		private readonly SettingsService $settingsService,
+		private readonly OrganisationMapper $organisationMapper,
 		private ?bool $commandLine = null,
 	) {
 		$this->commandLine ??= (PHP_SAPI === 'cli');
 	}//end __construct()
 
 	/**
-	 * Stamp an anonymous organisation create as pending and unpublished.
+	 * Stamp an anonymous organisation create as pending and unpublished, or refuse
+	 * it when it carries the uuid of an existing OpenRegister organisation.
 	 *
 	 * @param Event $event The dispatched event.
 	 *
@@ -75,6 +83,12 @@ public function handle(Event $event): void {
 			return;
 		}
 
+		if ($this->organisationHolds(uuid: $event->getObject()->getUuid()) === true) {
+			$event->setErrors(['message' => 'This organisation could not be registered.']);
+			$event->stopPropagation();
+			return;
+		}
+
 		// Merge, never replace: another listener may already have modified the data.
 		$event->setModifiedData(
 			array_merge(
@@ -83,4 +97,24 @@ public function handle(Event $event): void {
 			)
 		);
 	}//end handle()
+
+	/**
+	 * Whether an OpenRegister organisation holds this uuid.
+	 *
+	 * @param string|null $uuid The uuid the new object carries, when it has one yet.
+	 *
+	 * @return bool
+	 */
+	private function organisationHolds(?string $uuid): bool {
+		if ($uuid === null || $uuid === '') {
+			return false;
+		}
+
+		try {
+			$this->organisationMapper->findByUuid($uuid);
+			return true;
+		} catch (DoesNotExistException $e) {
+			return false;
+		}
+	}//end organisationHolds()
 }//end class

lib/Service/OrganizationSyncService.php

@@ -61,6 +61,11 @@
  */
 class OrganizationSyncService {
 
+	/**
+	 * App config key prefix under which an administrator's link is recorded, per organisatie uuid.
+	 */
+	private const LINK_KEY_PREFIX = 'linked_organisation_';
+
 	/**
 	 * OrganisatieService instance
 	 *
@@ -990,6 +995,11 @@ private function ensureOrganisationEntity(object $organisationObject, array $run
 			// Try to find existing organisation entity.
 			try {
 				$organisationEntity = $this->organisationMapper->findByUuid($organisationId);
+				if ($this->isBoundOrganisation(organisationEntity: $organisationEntity, run: $run) === false) {
+					$this->refuseUuidConflict(run: $run);
+					return null;
+				}
+
 				return $this->refreshOrganisationEntity(organisationEntity: $organisationEntity, run: $run);
 			} catch (\OCP\AppFramework\Db\DoesNotExistException $e) {
 				if ($this->slugIsTaken(run: $run) === true) {
@@ -1048,6 +1058,48 @@ private function fullOrganisationObject(object $organisationObject): object {
 		return $organisationObject;
 	}//end fullOrganisationObject()
 
+	/**
+	 * Whether this organisation belongs to the organisatie that carries its uuid.
+	 *
+	 * The sync makes an organisation after the organisatie, so one that is older
+	 * only belongs to it when an administrator linked the two. The organisatie's
+	 * stored organisation cannot answer this: OpenRegister stamps a new object
+	 * with the default or active organisation, which may be the very one asked about.
+	 *
+	 * @param object $organisationEntity The organisation with the organisatie's uuid.
+	 * @param array  $run                The ensure run (see ensureOrganisationEntity).
+	 *
+	 * @return bool False when a creation date is unknown and no link is recorded.
+	 */
+	private function isBoundOrganisation(object $organisationEntity, array $run): bool {
+		$entityCreated = $organisationEntity->getCreated();
+		$objectCreated = $run['organisationObject']->getCreated();
+		if ($entityCreated instanceof \DateTimeInterface
+			&& $objectCreated instanceof \DateTimeInterface
+			&& $entityCreated->getTimestamp() >= $objectCreated->getTimestamp()
+		) {
+			return true;
+		}
+
+		return $this->config->getValueString('stackiq', self::LINK_KEY_PREFIX . $run['organisationId'], '', lazy: true) !== '';
+	}//end isBoundOrganisation()
+
+	/**
+	 * Log and count an organisatie left unlinked because it carries the uuid of an
+	 * organisation that is not its own.
+	 *
+	 * @param array $run The ensure run (see ensureOrganisationEntity).
+	 *
+	 * @return void
+	 */
+	private function refuseUuidConflict(array $run): void {
+		$this->logger->warning(
+			'OrganizationSyncService: an organisatie carries the uuid of an older organisation; nothing is bound, for an administrator to review',
+			['app' => 'stackiq', 'organisatieId' => $run['organisationId']]
+		);
+		$run['stats']['organisationIdConflicts'][] = $run['organisationId'];
+	}//end refuseUuidConflict()
+
 	/**
 	 * Bring an existing organisation entity's active flag in line with the object's status,
 	 * mail an organisation that became active, and make the entity the object's owner.
@@ -1107,8 +1159,12 @@ private function refreshOrganisationEntity(object $organisationEntity, array $ru
 	 * Link an organisatie to an existing OpenRegister organisation, as an administrator decides.
 	 *
 	 * The organisation takes the organisatie's uuid, which is what links the two,
-	 * and becomes the organisatie object's owner. Refused for the reasons
-	 * checkOrganisationLink() gives.
+	 * and becomes the organisatie object's owner. The link is recorded so the sync
+	 * keeps the two together although the organisation is older. Refused for the
+	 * reasons checkOrganisationLink() gives.
+	 *
+	 * With both uuids the same it confirms an organisatie the sync refused for
+	 * carrying the uuid of an older organisation.
 	 *
 	 * @param string $organisatieUuid The organisatie object's uuid.
 	 * @param string $entityUuid      The OpenRegister organisation's uuid.
@@ -1124,6 +1180,7 @@ public function linkOrganisationEntity(string $organisatieUuid, string $entityUu
 			return $outcome;
 		}
 
+		$this->config->setValueString('stackiq', self::LINK_KEY_PREFIX . $organisatieUuid, date('Y-m-d H:i:s'), lazy: true);
 		$entity = $this->organisationMapper->findByUuid($entityUuid);
 		$entity->setUuid($organisatieUuid);
 		$this->organisationMapper->save($entity);
@@ -1141,12 +1198,13 @@ public function linkOrganisationEntity(string $organisatieUuid, string $entityUu
 	 *
 	 * Refused when the organisatie is unknown or already has an organisation, when
 	 * the organisation is unknown, is OpenRegister's default organisation, or is
-	 * already another organisatie's. Nothing is written.
+	 * already another organisatie's, and when OpenRegister cannot say which
+	 * organisation is its default. Nothing is written.
 	 *
 	 * @param string $organisatieUuid The organisatie object's uuid.
 	 * @param string $entityUuid      The OpenRegister organisation's uuid.
 	 *
-	 * @return string|null no-organisatie, already-linked, no-organisation,
+	 * @return string|null no-organisatie, already-linked, no-organisation, default-unknown,
 	 *                     default-organisation, organisation-in-use, or null.
 	 *
 	 * @spec openspec/specs/organisatie-service/spec.md
@@ -1163,11 +1221,12 @@ public function checkOrganisationLink(string $organisatieUuid, string $entityUui
 	 *
 	 * @param string $organisatieUuid The organisatie object's uuid.
 	 *
-	 * @return array The run: register, organizationSchema and organisationObject (null when unknown).
+	 * @return array The run: organisationId, register, organizationSchema and organisationObject (null when unknown).
 	 */
 	private function linkRun(string $organisatieUuid): array {
 		$config = $this->settingsService->getVoorzieningenConfig();
 		$run    = [
+			'organisationId'     => $organisatieUuid,
 			'register'           => (string) ($config['register'] ?? ''),
 			'organizationSchema' => (string) ($config['organisatie_schema'] ?? ''),
 		];
@@ -1189,31 +1248,57 @@ private function linkRefusal(array $run, array $uuids): ?string {
 			return 'no-organisatie';
 		}
 
-		if ($this->organisationExists(uuid: $organisatieUuid) === true) {
+		if ($this->organisatieHasOrganisation(run: $run, entityUuid: $entityUuid) === true) {
 			return 'already-linked';
 		}
 
 		return $this->organisationRefusal(run: $run, entityUuid: $entityUuid);
 	}//end linkRefusal()
 
+	/**
+	 * Whether the organisatie already has its organisation.
+	 *
+	 * An organisation with the organisatie's uuid that does not belong to it can
+	 * still be confirmed, by naming that same uuid as the organisation to link.
+	 *
+	 * @param array  $run        The link run (see linkRun()).
+	 * @param string $entityUuid The OpenRegister organisation's uuid.
+	 *
+	 * @return bool
+	 */
+	private function organisatieHasOrganisation(array $run, string $entityUuid): bool {
+		try {
+			$holder = $this->organisationMapper->findByUuid($run['organisationId']);
+		} catch (\OCP\AppFramework\Db\DoesNotExistException $e) {
+			return false;
+		}
+
+		return $entityUuid !== $run['organisationId'] || $this->isBoundOrganisation(organisationEntity: $holder, run: $run) === true;
+	}//end organisatieHasOrganisation()
+
 	/**
 	 * Why this OpenRegister organisation may not take an organisatie's uuid, or null when it may.
 	 *
 	 * @param array  $run        The link run (see linkRun()).
 	 * @param string $entityUuid The OpenRegister organisation's uuid.
 	 *
-	 * @return string|null no-organisation, default-organisation, organisation-in-use, or null.
+	 * @return string|null no-organisation, default-unknown, default-organisation, organisation-in-use, or null.
 	 */
 	private function organisationRefusal(array $run, string $entityUuid): ?string {
 		if ($this->organisationExists(uuid: $entityUuid) === false) {
 			return 'no-organisation';
 		}
 
-		if ($this->container->get('OCA\OpenRegister\Service\OrganisationService')->getDefaultOrganisationUuid() === $entityUuid) {
+		$defaultUuid = $this->container->get('OCA\OpenRegister\Service\OrganisationService')->getDefaultOrganisationUuid();
+		if ($defaultUuid === null || $defaultUuid === '') {
+			return 'default-unknown';
+		}
+
+		if ($defaultUuid === $entityUuid) {
 			return 'default-organisation';
 		}
 
-		if ($this->findOrganisatie(run: $run, uuid: $entityUuid) !== null) {
+		if ($entityUuid !== $run['organisationId'] && $this->findOrganisatie(run: $run, uuid: $entityUuid) !== null) {
 			return 'organisation-in-use';
 		}
 

openspec/specs/organisation-registration/spec.md

@@ -1,7 +1,7 @@
 # organisation-registration Specification
 
 ## Purpose
-How an organisation that registers itself through the public portal (board SkPubliek, "Uw organisatie aanmelden") is stored and reviewed: it stays pending without a publication date until an administrator approves it, and the registration number the form asks is kept. Created by the change organisation-registration-through-the-portal (archived 2026-10-30); holds REQ-ORR-001 and REQ-ORR-002.
+How an organisation that registers itself through the public portal (board SkPubliek, "Uw organisatie aanmelden") is stored and reviewed: it stays pending without a publication date until an administrator approves it, and the registration number the form asks is kept. Created by the change organisation-registration-through-the-portal (archived 2026-10-30); holds REQ-ORR-001 and REQ-ORR-002. REQ-ORR-003 keeps a registration from binding to an organisation that is not its own.
 
 ## Requirements
 
@@ -34,3 +34,29 @@ The organisation schema SHALL hold `website`, `registrationNumber` (a KvK number
 - **GIVEN** an active organisation registered with a contact e-mail address
 - **WHEN** an anonymous visitor reads it
 - **THEN** the contact name and e-mail address are not in the answer
+
+### Requirement: REQ-ORR-003 A registration never binds to an organisation that is not its own
+
+An organisation object SHALL only be linked to the OpenRegister organisation with its uuid when that organisation was made from it, which shows in it being at least as new as the object, or when an administrator linked the two. Otherwise the sync SHALL leave the existing organisation, its members and its active state untouched, process none of the object's contact persons, log a warning and count the object under `organisationIdConflicts`. An anonymous create that carries the uuid of an existing OpenRegister organisation SHALL be refused.
+
+#### Scenario: A crafted registration names an existing organisation's uuid
+@e2e exclude A request crafted outside the browser; tests/Unit/EventListener/AnonymousRegistrationListenerTest.php asserts the refusal.
+
+- **GIVEN** an OpenRegister organisation and a request without a session
+- **WHEN** the request posts an organisation with that organisation's uuid as its `id`
+- **THEN** the create is refused with a message that names no organisation
+
+#### Scenario: The sync meets an object with the uuid of an older organisation
+@e2e exclude The sync's decision on stored dates; tests/Unit/Service/OrganizationSyncEntityGoldenTest.php asserts no writes, no contact processing and the count.
+
+- **GIVEN** a stored organisation object created after the OpenRegister organisation with its uuid, with no link recorded
+- **WHEN** the event path or the scheduled sync processes that object
+- **THEN** nothing is written and its contact persons are not processed
+- **AND** the object is counted under `organisationIdConflicts`
+
+#### Scenario: An administrator confirms a refused pair
+@e2e exclude An occ command; tests/Unit/Service/OrganizationSyncEntityGoldenTest.php asserts the link and what it records.
+
+- **GIVEN** an organisation object the sync refused, and that it does belong with the organisation holding its uuid
+- **WHEN** an administrator runs `occ stackiq:organisation:link` with that uuid twice
+- **THEN** the link is recorded and the sync treats the two as linked

tests/Unit/Command/OrganisationLinkCommandTest.php

@@ -46,6 +46,7 @@ public static function refusals(): array {
 			'no organisation'      => ['no-organisation', 'No OpenRegister organisation has this uuid.'],
 			'organisation in use'  => ['organisation-in-use', 'belongs to another organisatie'],
 			'default organisation' => ['default-organisation', 'default organisation, which is never linked'],
+			'default unknown'      => ['default-unknown', 'cannot say which organisation is its default'],
 			'failure'              => [new RuntimeException('db down'), 'Could not link: db down'],
 		];
 	}//end refusals()