Skip to content
Permalink
Browse files Browse the repository at this point in the history
fix: Fine grained permissions must be well handled in POST git/:id/br…
…anches

This fixes request #27538 Fine grained permissions are not checked when creating a branch with REST API

The fine grained permissions were not taken into account in the previous
checks.

Change-Id: I1fa72ab34e4f41d5ea3ec9dd21e990ce330d780f
  • Loading branch information
yannis-rossetto committed Jul 5, 2022
1 parent ecbfaa5 commit 58ecb1d
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 27 deletions.
30 changes: 17 additions & 13 deletions plugins/git/include/REST/v1/Branch/BranchCreator.php
Expand Up @@ -21,38 +21,31 @@

namespace Tuleap\Git\REST\v1\Branch;

use Git;
use Git_Command_Exception;
use Luracast\Restler\RestException;
use Tuleap\Git\Branch\BranchCreationExecutor;
use Tuleap\Git\Branch\BranchName;
use Tuleap\Git\Branch\CannotCreateNewBranchException;
use Tuleap\Git\Branch\InvalidBranchNameException;
use Tuleap\Git\Permissions\AccessControlVerifier;
use Tuleap\Git\REST\v1\GitBranchPOSTRepresentation;

class BranchCreator
{
private const BRANCH_PREFIX = "refs/heads/";

public function __construct(private \Git_Exec $git_exec, private BranchCreationExecutor $branch_creation_executor)
{
public function __construct(
private \Git_Exec $git_exec,
private BranchCreationExecutor $branch_creation_executor,
private AccessControlVerifier $access_control_verifier,
) {
}

/**
* @throws RestException
*/
public function createBranch(\PFUser $user, \GitRepository $repository, GitBranchPOSTRepresentation $representation): void
{
if (
! $user->hasPermission(Git::PERM_WRITE, $repository->getId(), $repository->getProjectId())
&& ! $user->hasPermission(Git::PERM_WPLUS, $repository->getId(), $repository->getProjectId())
) {
throw new RestException(
403,
"User cannot update the content of the repository"
);
}

try {
BranchName::fromBranchNameShortHand($representation->branch_name);
} catch (InvalidBranchNameException $exception) {
Expand All @@ -65,6 +58,17 @@ public function createBranch(\PFUser $user, \GitRepository $repository, GitBranc
);
}

if (! $this->access_control_verifier->canWrite($user, $repository, self::BRANCH_PREFIX . $representation->branch_name)) {
throw new RestException(
403,
sprintf(
"You are not allowed to create the branch %s in repository %s",
$representation->branch_name,
$repository->getName()
)
);
}

$all_branches_names = $this->git_exec->getAllBranchesSortedByCreationDate();
if (in_array($representation->branch_name, $all_branches_names)) {
throw new RestException(
Expand Down
14 changes: 13 additions & 1 deletion plugins/git/include/REST/v1/RepositoryResource.class.php
Expand Up @@ -72,6 +72,7 @@
use Tuleap\Git\GitPHP\ProjectProvider;
use Tuleap\Git\GitPHP\RepositoryAccessException;
use Tuleap\Git\GitPHP\RepositoryNotExistingException;
use Tuleap\Git\Permissions\AccessControlVerifier;
use Tuleap\Git\Permissions\DefaultFineGrainedPermissionFactory;
use Tuleap\Git\Permissions\FineGrainedDao;
use Tuleap\Git\Permissions\FineGrainedPatternValidator;
Expand Down Expand Up @@ -919,7 +920,18 @@ public function createBranch($id, GitBranchPOSTRepresentation $representation):

$repository = $this->getRepositoryForCurrentUser($id);

(new BranchCreator(new Git_Exec($repository->getFullPath(), $repository->getFullPath()), new BranchCreationExecutor()))->createBranch(
$branch_creator = new BranchCreator(
new Git_Exec($repository->getFullPath(), $repository->getFullPath()),
new BranchCreationExecutor(),
new AccessControlVerifier(
new FineGrainedRetriever(
new FineGrainedDao(),
),
new \System_Command(),
)
);

$branch_creator->createBranch(
$this->getCurrentUser(),
$repository,
$representation
Expand Down
27 changes: 14 additions & 13 deletions plugins/git/tests/unit/REST/v1/Branch/BranchCreatorTest.php
Expand Up @@ -23,12 +23,13 @@

namespace Tuleap\Git\REST\v1\Branch;

use Git;
use Git_Command_Exception;
use Luracast\Restler\RestException;
use Tuleap\Git\Branch\BranchCreationExecutor;
use Tuleap\Git\Branch\CannotCreateNewBranchException;
use Tuleap\Git\Permissions\AccessControlVerifier;
use Tuleap\Git\REST\v1\GitBranchPOSTRepresentation;
use Tuleap\Test\Builders\UserTestBuilder;
use Tuleap\Test\PHPUnit\TestCase;

final class BranchCreatorTest extends TestCase
Expand All @@ -45,17 +46,23 @@ final class BranchCreatorTest extends TestCase
* @var \PHPUnit\Framework\MockObject\MockObject&BranchCreationExecutor
*/
private $branch_creation_executor;
/**
* @var AccessControlVerifier&\PHPUnit\Framework\MockObject\MockObject
*/
private $access_control_verifier;

protected function setUp(): void
{
parent::setUp();

$this->git_exec = $this->createMock(\Git_Exec::class);
$this->branch_creation_executor = $this->createMock(BranchCreationExecutor::class);
$this->access_control_verifier = $this->createMock(AccessControlVerifier::class);

$this->creator = new BranchCreator(
$this->git_exec,
$this->branch_creation_executor
$this->branch_creation_executor,
$this->access_control_verifier
);

$this->git_exec->method('getAllBranchesSortedByCreationDate')->willReturn([
Expand Down Expand Up @@ -90,7 +97,7 @@ public function testItThrowsAnExceptionIfUserCannotWriteInRepository(): void
->method("createNewBranch");

$this->expectException(RestException::class);
$this->expectExceptionMessage("User cannot update the content of the repository");
$this->expectExceptionMessage("You are not allowed to create the branch new_branch in repository repo01");

$this->creator->createBranch(
$this->buildMockUserWithoutPermissions(),
Expand Down Expand Up @@ -201,22 +208,16 @@ public function testItThrowsAnExceptionIfGitCommandFailed(): void

private function buildMockUserWithPermissions(): \PFUser
{
$user = $this->createMock(\PFUser::class);
$user->method('hasPermission')->willReturnMap([
[Git::PERM_WRITE, self::REPO_ID, self::PROJECT_ID, true],
[Git::PERM_WPLUS, self::REPO_ID, self::PROJECT_ID, true],
]);
$user = UserTestBuilder::anActiveUser()->build();
$this->access_control_verifier->method('canWrite')->willReturn(true);

return $user;
}

private function buildMockUserWithoutPermissions(): \PFUser
{
$user = $this->createMock(\PFUser::class);
$user->method('hasPermission')->willReturnMap([
[Git::PERM_WRITE, self::REPO_ID, self::PROJECT_ID, false],
[Git::PERM_WPLUS, self::REPO_ID, self::PROJECT_ID, false],
]);
$user = UserTestBuilder::anActiveUser()->build();
$this->access_control_verifier->method('canWrite')->willReturn(false);

return $user;
}
Expand Down

0 comments on commit 58ecb1d

Please sign in to comment.