Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion src/Application/Account/Services/PublicLink.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -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();
}

Expand Down
42 changes: 37 additions & 5 deletions tests/Integration/Application/Account/AccountAccessTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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])
Expand All @@ -585,6 +588,7 @@ public function testTheOwnerCanStillMintAPublicLinkForTheirOwnAccount(): void
);

$this->setContextUser($ownerId, $ownerGroupId, 'linkown-owner');
$this->context->setUserProfile(new ProfileData(['accViewPass' => true]));

self::assertGreaterThan(
0,
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
41 changes: 40 additions & 1 deletion tests/Unit/Application/Account/Services/PublicLinkTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
}

/**
Expand Down Expand Up @@ -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 =
Expand All @@ -675,7 +713,8 @@ protected function setUp(): void
$this->publicLinkRepository,
$request,
$this->accountService,
$this->crypt
$this->crypt,
$acl
);
}
}
Loading