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

EZP-30997: Fixed permission checks when copying translations during publishing #2858

Merged
merged 11 commits into from
Dec 5, 2019

Conversation

pawbuj
Copy link
Contributor

@pawbuj pawbuj commented Nov 4, 2019

Question Answer
JIRA issue EZP-30997
Bug/Improvement yes
New feature no
Target version 7.5
BC breaks no
Tests pass no
Doc needed no

TODO:

  • Implement feature / fix a bug.
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@pawbuj pawbuj requested review from alongosz and adamwojs November 4, 2019 12:39
@pawbuj pawbuj self-assigned this Nov 4, 2019
@pawbuj pawbuj added the Bug label Nov 4, 2019
@pawbuj pawbuj changed the base branch from master to 7.5 November 4, 2019 12:44
@pawbuj pawbuj changed the title added integration test EZP-30997 Nov 4, 2019
@adamwojs adamwojs changed the title EZP-30997 EZP-30997: Adding a new translation to content fails when language limitation is used Nov 7, 2019
@pawbuj pawbuj marked this pull request as ready for review November 7, 2019 11:56
@alongosz alongosz changed the title EZP-30997: Adding a new translation to content fails when language limitation is used EZP-30997: Fixed permission checks when copying translations during publishing Nov 12, 2019
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Overall looks good, some minor adjustments are required:

eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentService.php Outdated Show resolved Hide resolved
eZ/Publish/Core/Repository/ContentService.php Show resolved Hide resolved
@pawbuj pawbuj requested a review from alongosz November 13, 2019 14:54
@alongosz
Copy link
Member

While reviewing once again #2846 I've discovered yet another bug here. Consider the following use case:

public function testPublishVersionTranslationIsNotAllowed(): void
{
    $repository = $this->getRepository();
    $contentService = $repository->getContentService();
    $permissionResolver = $repository->getPermissionResolver();

    $britishEditor = $this->createEditorUserWithLanguageLimitation(['eng-GB'], 'editor-uk');
    $americanEditor = $this->createEditorUserWithLanguageLimitation(['eng-US'], 'editor-us');

    // British editor publishes BrE content
    $permissionResolver->setCurrentUserReference($britishEditor);

    $folder = $this->createFolder(['eng-GB' => 'BrE Folder'], 2);

    // American editor creates and saves AmE draft
    $permissionResolver->setCurrentUserReference($americanEditor);

    $folder = $contentService->loadContent($folder->id);
    $folderDraft = $contentService->createContentDraft($folder->contentInfo);
    $folderUpdateStruct = $contentService->newContentUpdateStruct();
    $folderUpdateStruct->setField('name', 'AmE Folder', 'eng-US');
    $folderDraft = $contentService->updateContent(
        $folderDraft->versionInfo,
        $folderUpdateStruct
    );

    // British editor tries to publish AmE translation
    $permissionResolver->setCurrentUserReference($britishEditor);
    $folderDraftVersionInfo = $contentService->loadVersionInfo(
        $folderDraft->contentInfo,
        $folderDraft->versionInfo->versionNo
    );
    self::assertTrue($folderDraftVersionInfo->isDraft());
    $this->expectException(UnauthorizedException::class);
    $this->expectExceptionMessage("User does not have access to 'publish' 'content'");
    $contentService->publishVersion($folderDraftVersionInfo, ['eng-US']);
}

private function createEditorUserWithLanguageLimitation(
    array $allowedTranslationsList,
    string $login = 'editor'
): User {
    $limitations = [
        // limitation for specific translations
        new LanguageLimitation(['limitationValues' => $allowedTranslationsList]),
    ];

    return $this->createUserWithPolicies(
        $login,
        [
            ['module' => 'content', 'function' => 'read'],
            ['module' => 'content', 'function' => 'versionread'],
            ['module' => 'content', 'function' => 'view_embed'],
            ['module' => 'content', 'function' => 'create', 'limitations' => $limitations],
            ['module' => 'content', 'function' => 'edit', 'limitations' => $limitations],
            ['module' => 'content', 'function' => 'publish', 'limitations' => $limitations],
        ]
    );
}

I'm not sure if this is reproducible with AdminUI (depends if ATM it's possible to edit someone else's draft - don't remember). However for the API it's an issue.

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Almost ok ;)

@alongosz
Copy link
Member

@mikadamczyk one more request - could you incorporate #2846 here as well, as it touches the same portion of code? Would be easier to test.

@mikadamczyk
Copy link
Contributor

Sure @alongosz I thought that they ware divided by some purpose, but definitely it touches the same part of code

@alongosz
Copy link
Member

Sure @alongosz I thought that they were divided by some purpose, but definitely it touches the same part of code

Yes, initially they were, but given the changes here it no longer makes sense :)

mikadamczyk and others added 2 commits November 29, 2019 11:19
Co-Authored-By: Andrew Longosz <alongosz@users.noreply.github.com>
@mikadamczyk mikadamczyk requested a review from alongosz November 29, 2019 10:25
@alongosz alongosz merged commit d9c8926 into 7.5 Dec 5, 2019
@alongosz alongosz deleted the EZP-30997 branch December 5, 2019 12:49
alongosz added a commit that referenced this pull request Dec 5, 2019
Failure was caused by wrong conflict resolution when merging #2858 into master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants