From 67835daefca937e24c445b61a3aaa35fd530d9f2 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 24 Sep 2026 16:25:30 +0200 Subject: [PATCH] fix: a public link needs the permission to view the password it shares --- .../Account/Services/PublicLink.php | 31 +++++++++++++- .../Application/Account/AccountAccessTest.php | 42 ++++++++++++++++--- .../Account/PublicLinkRoundTripTest.php | 4 ++ .../Account/Services/PublicLinkTest.php | 41 +++++++++++++++++- 4 files changed, 111 insertions(+), 7 deletions(-) diff --git a/src/Application/Account/Services/PublicLink.php b/src/Application/Account/Services/PublicLink.php index 188084bc0..6c96b314b 100644 --- a/src/Application/Account/Services/PublicLink.php +++ b/src/Application/Account/Services/PublicLink.php @@ -34,6 +34,9 @@ use SP\Domain\Account\Models\PublicLink as PublicLinkModel; use SP\Domain\Account\Models\PublicLinkList; use SP\Application\Account\Ports\AccountService; +use SP\Domain\Core\Acl\AccountPermissionException; +use SP\Domain\Core\Acl\AclActionsInterface; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\Account\Ports\PublicLinkRepository; use SP\Application\Account\Ports\PublicLinkService; use SP\Domain\Common\Adapters\Serde; @@ -72,7 +75,8 @@ public function __construct( private readonly PublicLinkRepository $publicLinkRepository, private readonly RequestService $request, private readonly AccountService $accountService, - private readonly CryptInterface $crypt + private readonly CryptInterface $crypt, + private readonly AclInterface $acl ) { parent::__construct($application); } @@ -209,6 +213,29 @@ private function getSecuredLinkData(int $itemId, PublicLinkKey $key): string ->getSerialized(); } + /** + * Refuse unless the signed-in user's profile may view account passwords. + * + * A public link *is* the account's password, handed to whoever holds the URL: creating one + * decrypts it with the master key and seals it into the link's vault, and the creator can open + * it like anybody else. So minting one needs what reading the password needs. Which accounts + * is already settled — `getDataForLink()` reads through `AccountFilterUser`, as + * `getPasswordForId()` does — but the profile's own view-password permission was asked only + * where the password is shown directly, and the account view offers the link button on + * exactly `isShowLink() && isShowViewPass()`. `PUBLICLINK_CREATE` (the "public links" profile + * switch) says nothing about it, so a profile allowed to publish links and not to see + * passwords could read every password it could list by linking it, and on the API — which + * answers with the new hash — without even opening the account. + * + * @throws AccountPermissionException + */ + private function requirePasswordPermission(): void + { + if (!$this->acl->checkUserAccess(AclActionsInterface::ACCOUNT_VIEW_PASS)) { + throw new AccountPermissionException(SPException::ERROR); + } + } + /** * Return the link's expiration time */ @@ -265,6 +292,8 @@ public function deleteByIdBatch(array $ids): int */ public function create(PublicLinkModel $publicLink): int { + $this->requirePasswordPermission(); + return $this->publicLinkRepository->create($this->buildPublicLink($publicLink))->getLastId(); } diff --git a/tests/Integration/Application/Account/AccountAccessTest.php b/tests/Integration/Application/Account/AccountAccessTest.php index 0b1849688..7d45346c5 100644 --- a/tests/Integration/Application/Account/AccountAccessTest.php +++ b/tests/Integration/Application/Account/AccountAccessTest.php @@ -47,7 +47,6 @@ use SP\Application\Account\Ports\PublicLinkService; use SP\Domain\Account\PublicLinkType; use SP\Domain\Account\Models\PublicLink as PublicLinkModel; -use SP\Domain\Core\Exceptions\SPException; use SP\Domain\Core\Acl\AclActionsInterface; use SP\Domain\Core\Bootstrap\Path; use SP\Domain\Core\Context\Context; @@ -57,6 +56,7 @@ use SP\Domain\Database\Ports\DbStorageHandler; use SP\Domain\File\FileSystem; use SP\Domain\User\Dtos\UserDto; +use SP\Domain\Core\Acl\AccountPermissionException; use SP\Domain\User\Models\ProfileData; use SP\Domain\User\Models\User as UserModel; use SP\Domain\User\Models\UserGroup as UserGroupModel; @@ -106,9 +106,10 @@ * AccountAclService outright -- neither leaves anything real to test here. 'cli' keeps Context bound * to the plain Stateless implementation, so "logging in" as a different user between assertions is * just Context::setUserData() with a different UserDto, the same technique those two tests use. - * Every user profile bit is left unset throughout (Context::setUserProfile() is never called): every - * assertion here goes through AccountAclService/AccountService directly rather than through - * AclInterface::checkUserAccess(), and AccountAcl::compileAccountAccess() -- the half that decides + * Every user profile bit is left unset (Context::setUserProfile() is not called) except by the + * public-link tests, since minting a link asks AclInterface::checkUserAccess() for the profile's + * view-password permission. Every other assertion here goes through AccountAclService/AccountService + * directly rather than through AclInterface::checkUserAccess(), and AccountAcl::compileAccountAccess() -- the half that decides * resultView/resultEdit, i.e. what actually gates reading an account -- never consults the profile at * all. A profile row still has to exist for each created user purely to satisfy User.userProfileId's * foreign key; its bits are never read. @@ -554,13 +555,15 @@ public function testAPublicLinkCannotBeMintedForAnAccountTheUserCannotRead(): vo $accountId = $this->createAccount('link', $password, $ownerId, $ownerGroupId); $this->setContextUser($otherId, $otherGroupId, 'link-other'); + // Allowed to view passwords, so the refusal below can only be the account being out of reach. + $this->context->setUserProfile(new ProfileData(['accViewPass' => true])); self::assertFalse( $this->canAccess(AclActionsInterface::ACCOUNT_VIEW, $accountId), 'the account was reachable, so this proves nothing' ); - $this->expectException(SPException::class); + $this->expectException(NoSuchItemException::class); $this->dic->get(PublicLinkService::class)->create( new PublicLinkModel(['itemId' => $accountId, 'typeId' => PublicLinkType::Account->value]) @@ -585,6 +588,7 @@ public function testTheOwnerCanStillMintAPublicLinkForTheirOwnAccount(): void ); $this->setContextUser($ownerId, $ownerGroupId, 'linkown-owner'); + $this->context->setUserProfile(new ProfileData(['accViewPass' => true])); self::assertGreaterThan( 0, @@ -595,6 +599,34 @@ public function testTheOwnerCanStillMintAPublicLinkForTheirOwnAccount(): void ); } + /** + * But not when their profile may not view passwords: the link is the password, and whoever + * mints it can open it, so owning the account is not enough on its own. + * + * @throws Exception + */ + public function testTheOwnerCannotMintALinkWhenTheirProfileMayNotViewPasswords(): void + { + $ownerGroupId = $this->createGroup('linknopass-owner'); + $ownerId = $this->createUser('linknopass-owner', $ownerGroupId); + + $accountId = $this->createAccount( + 'linknopass', + 'LinkNoPass!' . bin2hex(random_bytes(4)), + $ownerId, + $ownerGroupId + ); + + $this->setContextUser($ownerId, $ownerGroupId, 'linknopass-owner'); + $this->context->setUserProfile(new ProfileData(['accViewPass' => false, 'accPublicLinks' => true])); + + $this->expectException(AccountPermissionException::class); + + $this->dic->get(PublicLinkService::class)->create( + new PublicLinkModel(['itemId' => $accountId, 'typeId' => PublicLinkType::Account->value]) + ); + } + /** * An accounts administrator reads any account too -- the same bypass, granted through isAdminAcc * rather than isAdminApp. diff --git a/tests/Integration/Application/Account/PublicLinkRoundTripTest.php b/tests/Integration/Application/Account/PublicLinkRoundTripTest.php index 0db7b076d..132ef1745 100644 --- a/tests/Integration/Application/Account/PublicLinkRoundTripTest.php +++ b/tests/Integration/Application/Account/PublicLinkRoundTripTest.php @@ -48,6 +48,7 @@ use SP\Domain\Database\Ports\DbStorageHandler; use SP\Domain\File\FileSystem; use SP\Domain\User\Dtos\UserDto; +use SP\Domain\User\Models\ProfileData; use SP\Infrastructure\Crypt\Crypt; use SP\Infrastructure\Definitions\CoreDefinitions; use SP\Infrastructure\Definitions\DomainDefinitions; @@ -136,6 +137,9 @@ protected function setUp(): void isAdminAcc: true, ) ); + // Minting a link asks the ACL whether the user may view passwords, and the ACL refuses + // anybody — an administrator included — whose session carries no profile at all. + $context->setUserProfile(new ProfileData()); // AccountCryptService::getPasswordEncrypted() and PublicLink::getSecuredLinkData() both pull // the master password from here (Service::getMasterKeyFromContext()). A real login sets this // after checking the master password against the stored hash; done directly since this diff --git a/tests/Unit/Application/Account/Services/PublicLinkTest.php b/tests/Unit/Application/Account/Services/PublicLinkTest.php index 2b70ac6c4..e8bc5cc11 100644 --- a/tests/Unit/Application/Account/Services/PublicLinkTest.php +++ b/tests/Unit/Application/Account/Services/PublicLinkTest.php @@ -34,6 +34,9 @@ use PHPUnit\Framework\MockObject\MockObject; use SP\Domain\Account\Models\PublicLink as PublicLinkModel; use SP\Application\Account\Ports\AccountService; +use SP\Domain\Core\Acl\AccountPermissionException; +use SP\Domain\Core\Acl\AclActionsInterface; +use SP\Domain\Core\Acl\AclInterface; use SP\Domain\Account\Ports\PublicLinkRepository; use SP\Application\Account\Services\PublicLink; use SP\Domain\Common\Models\Simple; @@ -63,6 +66,9 @@ class PublicLinkTest extends UnitaryTestCase private PublicLink $publicLink; private CryptInterface|MockObject $crypt; private MockObject|AccountService $accountService; + private bool $mayViewPasswords = true; + /** @var int[] */ + private array $aclAskedFor = []; /** * @throws QueryException @@ -605,6 +611,29 @@ public function testCreate() $actual = $this->publicLink->create($publicLinkData); $this->assertEquals($result->getLastId(), $actual); + $this->assertSame([AclActionsInterface::ACCOUNT_VIEW_PASS], $this->aclAskedFor); + } + + /** + * A public link is the account's password, handed to whoever has the URL, so a user whose + * profile may not view passwords cannot mint one — whatever `PUBLICLINK_CREATE` says. + * + * @throws CryptoException + * @throws ConstraintException + * @throws QueryException + * @throws SPException + */ + public function testCreateIsRefusedToAUserWhoMayNotViewPasswords() + { + $this->mayViewPasswords = false; + + $this->publicLinkRepository->expects(self::never())->method('create'); + $this->accountService->expects(self::never())->method('getDataForLink'); + $this->crypt->expects(self::never())->method('decrypt'); + + $this->expectException(AccountPermissionException::class); + + $this->publicLink->create(PublicLinkDataGenerator::factory()->buildPublicLink()); } /** @@ -667,6 +696,15 @@ protected function setUp(): void $this->accountService = $this->createMock(AccountService::class); $this->crypt = $this->createMock(CryptInterface::class); + // Read when called rather than when configured, so each test can say who is signed in. + $acl = $this->createStub(AclInterface::class); + $acl->method('checkUserAccess') + ->willReturnCallback(function (int $actionId) { + $this->aclAskedFor[] = $actionId; + + return $actionId === AclActionsInterface::ACCOUNT_VIEW_PASS && $this->mayViewPasswords; + }); + $this->context->setTrasientKey(Context::MASTER_PASSWORD_KEY, self::$faker->password()); $this->publicLink = @@ -675,7 +713,8 @@ protected function setUp(): void $this->publicLinkRepository, $request, $this->accountService, - $this->crypt + $this->crypt, + $acl ); } }