Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] IBX-9379 Added Grace Period for archived versions #515

Open
wants to merge 9 commits into
base: 4.6
Choose a base branch
from
11 changes: 3 additions & 8 deletions phpstan-baseline.neon
Original file line number Diff line number Diff line change
@@ -23353,7 +23353,7 @@ parameters:
path: src/lib/Persistence/Legacy/User/Handler.php

-
message: '#^Cannot access offset mixed on non\-empty\-array\|Ibexa\\Contracts\\Core\\Persistence\\User\\RoleAssignment\.$#'
message: '#^Cannot access offset mixed on non\-empty\-array\<non\-empty\-array\<int, Ibexa\\Contracts\\Core\\Persistence\\User\\RoleAssignment\>\>\|Ibexa\\Contracts\\Core\\Persistence\\User\\RoleAssignment\.$#'
identifier: offsetAccess.nonOffsetAccessible
count: 1
path: src/lib/Persistence/Legacy/User/Mapper.php
@@ -42335,6 +42335,7 @@ parameters:
identifier: argument.type
count: 1
path: tests/integration/Core/Repository/LocationServiceTest.php

-
message: '#^Parameter \#1 \$expectedLocations of method Ibexa\\Tests\\Integration\\Core\\Repository\\LocationServiceTest\:\:assertContentHasExpectedLocations\(\) expects array\<Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location\>, array\<int, Ibexa\\Contracts\\Core\\Repository\\Values\\Content\\Location\|null\> given\.$#'
identifier: argument.type
@@ -67344,7 +67345,7 @@ parameters:
path: tests/lib/Persistence/Legacy/SharedGateway/GatewayFactoryTest.php

-
message: '#^PHPDoc tag @var with type Doctrine\\DBAL\\Platforms\\AbstractPlatform is not subtype of native type Doctrine\\DBAL\\Platforms\\MySqlPlatform\|Doctrine\\DBAL\\Platforms\\PostgreSqlPlatform\|Doctrine\\DBAL\\Platforms\\SqlitePlatform\.$#'
message: '#^PHPDoc tag @var with type Doctrine\\DBAL\\Platforms\\AbstractPlatform is not subtype of native type Doctrine\\DBAL\\Platforms\\MySQL80Platform\|Doctrine\\DBAL\\Platforms\\MySqlPlatform\|Doctrine\\DBAL\\Platforms\\PostgreSqlPlatform\|Doctrine\\DBAL\\Platforms\\SqlitePlatform\.$#'
identifier: varTag.nativeType
count: 1
path: tests/lib/Persistence/Legacy/SharedGateway/GatewayFactoryTest.php
@@ -70841,12 +70842,6 @@ parameters:
count: 1
path: tests/lib/Repository/Service/Mock/ContentTest.php

-
message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\ContentTest\:\:getPartlyMockedContentService\(\) invoked with 2 parameters, 0\-1 required\.$#'
identifier: arguments.count
count: 1
path: tests/lib/Repository/Service/Mock/ContentTest.php

-
message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\ContentTest\:\:getPartlyMockedContentService\(\) should return Ibexa\\Core\\Repository\\ContentService&PHPUnit\\Framework\\MockObject\\MockObject but returns Ibexa\\Core\\Repository\\ContentService\.$#'
identifier: return.type
7 changes: 7 additions & 0 deletions src/contracts/Persistence/Content/Handler.php
Original file line number Diff line number Diff line change
@@ -131,6 +131,13 @@ public function loadContentInfoByRemoteId($remoteId);
*/
public function loadVersionInfo($contentId, $versionNo = null);

/**
* @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException
*
* @return int[]
*/
public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array;

/**
* Returns the number of versions with draft status created by the given $userId.
*
14 changes: 14 additions & 0 deletions src/lib/Persistence/Cache/ContentHandler.php
Original file line number Diff line number Diff line change
@@ -266,6 +266,20 @@ public function loadVersionInfo($contentId, $versionNo = null)
return $versionInfo;
}

/**
* @return int[]
*/
public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array
{
$versionNos = $this->persistenceHandler->contentHandler()->loadVersionNoArchivedWithin($contentId, $seconds);

$this->cache->invalidateTags([
$this->cacheIdentifierGenerator->generateTag(self::CONTENT_IDENTIFIER, [$contentId]),
]);

return $versionNos;
}

public function countDraftsForUser(int $userId): int
{
$this->logger->logCall(__METHOD__, ['user' => $userId]);
5 changes: 5 additions & 0 deletions src/lib/Persistence/Legacy/Content/Gateway.php
Original file line number Diff line number Diff line change
@@ -212,6 +212,11 @@ abstract public function loadContentInfoList(array $contentIds): array;
*/
abstract public function loadVersionInfo(int $contentId, ?int $versionNo = null): array;

/**
* @return array<mixed>
*/
abstract public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array;

/**
* Return the number of all versions with given status created by the given $userId.
*/
45 changes: 45 additions & 0 deletions src/lib/Persistence/Legacy/Content/Gateway/DoctrineDatabase.php
Original file line number Diff line number Diff line change
@@ -910,6 +910,51 @@ public function loadVersionInfo(int $contentId, ?int $versionNo = null): array
return $queryBuilder->execute()->fetchAll(FetchMode::ASSOCIATIVE);
}

/**
* @return array<mixed>
*/
public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array
{
$cutoffTimestamp = strtotime(sprintf('-%d seconds', $seconds));
if (false === $cutoffTimestamp) {
return [];
}
$queryBuilder = $this->queryBuilder->createVersionInfoFindQueryBuilder();
$expr = $queryBuilder->expr();

$queryBuilder
->andWhere(
$expr->eq(
'v.contentobject_id',
$queryBuilder->createNamedParameter(
$contentId,
ParameterType::INTEGER,
':content_id'
)
)
)->andWhere(
$expr->eq(
'v.status',
$queryBuilder->createNamedParameter(
VersionInfo::STATUS_ARCHIVED,
ParameterType::INTEGER,
':status'
)
)
)->andWhere(
$expr->gt(
'v.modified',
$queryBuilder->createNamedParameter(
$cutoffTimestamp,
ParameterType::INTEGER,
':modified'
)
)
)->orderBy('v.modified', 'DESC');

return $queryBuilder->execute()->fetchAllAssociative();
}

public function countVersionsForUser(int $userId, int $status = VersionInfo::STATUS_DRAFT): int
{
$query = $this->connection->createQueryBuilder();
12 changes: 12 additions & 0 deletions src/lib/Persistence/Legacy/Content/Gateway/ExceptionConversion.php
Original file line number Diff line number Diff line change
@@ -217,6 +217,18 @@ public function loadVersionInfo(int $contentId, ?int $versionNo = null): array
}
}

/**
* @return int[]
*/
public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array
{
try {
return $this->innerGateway->loadVersionNoArchivedWithin($contentId, $seconds);
} catch (DBALException | PDOException $e) {
throw DatabaseException::wrap($e);
}
}

public function countVersionsForUser(int $userId, int $status = VersionInfo::STATUS_DRAFT): int
{
try {
15 changes: 15 additions & 0 deletions src/lib/Persistence/Legacy/Content/Handler.php
Original file line number Diff line number Diff line change
@@ -483,6 +483,21 @@ public function loadVersionInfo($contentId, $versionNo = null)
return reset($versionInfo);
}

public function loadVersionNoArchivedWithin(int $contentId, int $seconds): array
{
$rows = $this->contentGateway->loadVersionNoArchivedWithin($contentId, $seconds);
if (empty($rows)) {
throw new NotFound('content', $contentId);
}

$archivedVersionNos = [];
foreach ($rows as $row) {
$archivedVersionNos[] = $row['ezcontentobject_version_version'];
}

return $archivedVersionNos;
}

public function countDraftsForUser(int $userId): int
{
return $this->contentGateway->countVersionsForUser($userId, VersionInfo::STATUS_DRAFT);
24 changes: 22 additions & 2 deletions src/lib/Repository/ContentService.php
Original file line number Diff line number Diff line change
@@ -129,6 +129,7 @@ public function __construct(
// Version archive limit (0-50), only enforced on publish, not on un-publish.
'default_version_archive_limit' => 5,
'remove_archived_versions_on_publish' => true,
'grace_period_in_seconds' => 30,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a plan, yes.

];
$this->contentFilteringHandler = $contentFilteringHandler;
$this->permissionResolver = $permissionService;
@@ -381,20 +382,39 @@ public function loadContentByVersionInfo(APIVersionInfo $versionInfo, array $lan
public function loadContent(int $contentId, array $languages = null, ?int $versionNo = null, bool $useAlwaysAvailable = true): APIContent
{
$content = $this->internalLoadContentById($contentId, $languages, $versionNo, $useAlwaysAvailable);

if (!$this->permissionResolver->canUser('content', 'read', $content)) {
throw new UnauthorizedException('content', 'read', ['contentId' => $contentId]);
}
if (
!$content->getVersionInfo()->isPublished()
&& !$this->permissionResolver->canUser('content', 'versionread', $content)
) {
throw new UnauthorizedException('content', 'versionread', ['contentId' => $contentId, 'versionNo' => $versionNo]);
if (!$this->isInGracePeriod($content, $this->settings['grace_period_in_seconds'], $versionNo)) {
throw new UnauthorizedException('content', 'versionread', ['contentId' => $contentId, 'versionNo' => $versionNo]);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. That check was not only to disallow viewing "unpublished" versions, but also other drafts, even for content that hasn't been published at all. While I see that your implementation of isInGracePeriod checks indeed for archived versions, you should probably check first if content has been archived. If it hasn't then that's a normal draft someone is working on before publishing it, so it should throw unauthorized exception right away if there's no versionread policy.
  2. Try combining that nested if-if into a single chained expression following code style of L389-390.

Ad. 2. It's really useful to review SonarCloud / SonarQube analysis. https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=515&id=ibexa_core&open=AZWkhRnq4uuA193_QeJw
or install SonarQube plugin for PHPStorm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain point 1 in more detail? I don't see a way that the grace period will grant access to a draft for a person that should be blocked by a versionread on the first hand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain point 1 in more detail? I don't see a way that the grace period will grant access to a draft for a person that should be blocked by a versionread on the first hand.

Essentially

|| ($content->getVersionInfo()->isArchived() && $this->isInGracePeriod())

costs less, performance-wise, than isInGracePeriod alone, for the cases if we're dealing with draft, not an older published version.
Unless I'm missing something...

Copy link
Contributor

@ViniTou ViniTou Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alongosz
Did you not meant

(
 $content->getVersionInfo()->isArchived()
 && --->**!**<--- $this->isInGracePeriod($content, $this->settings['grace_period_in_seconds'], $versionNo)
)

?

update: either way, this breaks the solution for some reason, hard to debug why, I guess sometimes the old version is not yet in archived state?


return $content;
}

private function isInGracePeriod(APIContent $content, int $graceSeconds, ?int $versionNo): bool
{
if ($graceSeconds <= 0 || $versionNo === null) {
return false;
}

try {
$lastArchivedVersionNos = $this->persistenceHandler->contentHandler()->loadVersionNoArchivedWithin(
$content->getId(),
$graceSeconds
);
} catch (APINotFoundException $e) {
return false;
}

return in_array($versionNo, $lastArchivedVersionNos, true);
}

public function internalLoadContentById(
int $id,
?array $languages = null,
15 changes: 9 additions & 6 deletions tests/lib/Repository/Service/Mock/ContentTest.php
Original file line number Diff line number Diff line change
@@ -491,9 +491,11 @@ public function testLoadContentUnauthorized()
$contentService->loadContent($contentId);
}

public function testLoadContentNotPublishedStatusUnauthorized()
public function testLoadContentNotPublishedStatusUnauthorized(bool $expectException = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly does this work? Seems this is always true, since there's no data provider or test dependency here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is leftover from when I wanted to reuse this test case with a grace period enabled, please ignore it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is leftover from when I wanted to reuse this test case with a grace period enabled, please ignore it.

ok, just make sure it's dropped in the final version. I'd rather move coverage of grace period to integration tests layer.

{
$this->expectException(UnauthorizedException::class);
if ($expectException) {
$this->expectException(UnauthorizedException::class);
}

$permissionResolver = $this->getPermissionResolverMock();
$contentService = $this->getPartlyMockedContentService(['internalLoadContentById']);
@@ -3317,8 +3319,7 @@ protected function assertForTestUpdateContentNonRedundantFieldSet(
->expects($this->once())
->method('getCurrentUserReference')
->willReturn(new UserReference(169));
$mockedService = $this->getPartlyMockedContentService(['internalLoadContentById'], $permissionResolverMock);
$permissionResolverMock = $this->getPermissionResolverMock();
$mockedService = $this->getPartlyMockedContentService(['internalLoadContentById']);
/** @var \PHPUnit\Framework\MockObject\MockObject $contentHandlerMock */
$contentHandlerMock = $this->getPersistenceMock()->contentHandler();
/** @var \PHPUnit\Framework\MockObject\MockObject $languageHandlerMock */
@@ -6271,7 +6272,7 @@ protected function getLocationServiceMock()
*
* @return \Ibexa\Core\Repository\ContentService|\PHPUnit\Framework\MockObject\MockObject
*/
protected function getPartlyMockedContentService(array $methods = null)
protected function getPartlyMockedContentService(array $methods = null, int $gracePeriodInSeconds = 0)
{
if (!isset($this->partlyMockedContentService)) {
$this->partlyMockedContentService = $this->getMockBuilder(ContentService::class)
@@ -6288,7 +6289,9 @@ protected function getPartlyMockedContentService(array $methods = null)
$this->getContentMapper(),
$this->getContentValidatorStrategy(),
$this->getContentFilteringHandlerMock(),
[],
[
'grace_period_in_seconds' => $gracePeriodInSeconds,
],
]
)
->getMock();