Skip to content

Commit a06cb42

Browse files
fix!: GitLab repository branch prefix can be updated by any user
This closes request #28848 GitLab repository branch prefix can be updated by any user Any user is able to update the branch prefix of any GitLab repository integration using the REST route PATCH /gitlab_repositories/{id}. This is wrong, only Git adminsitrators must be allowed to do that. Change-Id: I70febc7cc3b49d5687d8f6a3f1a061710f6cf5ef
1 parent ee5b4e1 commit a06cb42

File tree

3 files changed

+68
-7
lines changed

3 files changed

+68
-7
lines changed

Diff for: plugins/gitlab/include/Artifact/Action/CreateBranchPrefixUpdater.php

+11-1
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121

2222
namespace Tuleap\Gitlab\Artifact\Action;
2323

24+
use GitPermissionsManager;
25+
use GitUserNotAdminException;
26+
use PFUser;
2427
use Tuleap\Git\Branch\BranchName;
2528
use Tuleap\Git\Branch\InvalidBranchNameException;
2629
use Tuleap\Gitlab\Repository\GitlabRepositoryIntegrationFactory;
@@ -32,21 +35,28 @@ final class CreateBranchPrefixUpdater
3235

3336
public function __construct(
3437
private GitlabRepositoryIntegrationFactory $integration_factory,
38+
private GitPermissionsManager $git_permissions_manager,
3539
private SaveIntegrationBranchPrefix $branch_prefix_saver,
3640
) {
3741
}
3842

3943
/**
4044
* @throws GitlabRepositoryIntegrationNotFoundException
4145
* @throws InvalidBranchNameException
46+
* @throws GitUserNotAdminException
4247
*/
43-
public function updateBranchPrefix(int $integration_id, string $prefix): void
48+
public function updateBranchPrefix(PFUser $user, int $integration_id, string $prefix): void
4449
{
4550
$gitlab_repository = $this->integration_factory->getIntegrationById($integration_id);
4651
if (! $gitlab_repository) {
4752
throw new GitlabRepositoryIntegrationNotFoundException($integration_id);
4853
}
4954

55+
$project = $gitlab_repository->getProject();
56+
if (! $this->git_permissions_manager->userIsGitAdmin($user, $project)) {
57+
throw new GitUserNotAdminException();
58+
}
59+
5060
BranchName::fromBranchNameShortHand($prefix . self::FAKE_BRANCH_NAME);
5161

5262
$this->branch_prefix_saver->setCreateBranchPrefixForIntegration(

Diff for: plugins/gitlab/include/REST/v1/GitlabRepositoryResource.php

+6-2
Original file line numberDiff line numberDiff line change
@@ -473,13 +473,15 @@ protected function patchId(
473473
$prefix_updater = new CreateBranchPrefixUpdater(
474474
new GitlabRepositoryIntegrationFactory(
475475
new GitlabRepositoryIntegrationDao(),
476-
ProjectManager::instance()
476+
ProjectManager::instance(),
477477
),
478-
new CreateBranchPrefixDao()
478+
$this->getGitPermissionsManager(),
479+
new CreateBranchPrefixDao(),
479480
);
480481

481482
try {
482483
$prefix_updater->updateBranchPrefix(
484+
$current_user,
483485
$id,
484486
$patch_representation->create_branch_prefix
485487
);
@@ -490,6 +492,8 @@ protected function patchId(
490492
400,
491493
$exception->getMessage()
492494
);
495+
} catch (GitUserNotAdminException $exception) {
496+
throw new RestException(401, "User must be Git administrator.");
493497
}
494498
}
495499

Diff for: plugins/gitlab/tests/unit/Artifact/Action/CreateBranchPrefixUpdaterTest.php

+51-4
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,14 @@
2222

2323
namespace Tuleap\Gitlab\Artifact\Action;
2424

25+
use GitPermissionsManager;
26+
use GitUserNotAdminException;
2527
use Tuleap\Git\Branch\InvalidBranchNameException;
2628
use Tuleap\Gitlab\Repository\GitlabRepositoryIntegrationFactory;
2729
use Tuleap\Gitlab\Repository\GitlabRepositoryIntegrationNotFoundException;
2830
use Tuleap\Gitlab\Test\Builder\RepositoryIntegrationBuilder;
2931
use Tuleap\Gitlab\Test\Stubs\SaveIntegrationBranchPrefixStub;
32+
use Tuleap\Test\Builders\UserTestBuilder;
3033
use Tuleap\Test\PHPUnit\TestCase;
3134

3235
final class CreateBranchPrefixUpdaterTest extends TestCase
@@ -38,11 +41,16 @@ final class CreateBranchPrefixUpdaterTest extends TestCase
3841
private $integration_factory;
3942
private SaveIntegrationBranchPrefixStub $branch_prefix_saver;
4043
private string $branch_prefix;
44+
/**
45+
* @var GitPermissionsManager&\PHPUnit\Framework\MockObject\MockObject
46+
*/
47+
private $git_permissions_manager;
4148

4249
protected function setUp(): void
4350
{
44-
$this->integration_factory = $this->createMock(GitlabRepositoryIntegrationFactory::class);
45-
$this->branch_prefix_saver = SaveIntegrationBranchPrefixStub::withCallCount();
51+
$this->integration_factory = $this->createMock(GitlabRepositoryIntegrationFactory::class);
52+
$this->git_permissions_manager = $this->createMock(GitPermissionsManager::class);
53+
$this->branch_prefix_saver = SaveIntegrationBranchPrefixStub::withCallCount();
4654

4755
$this->branch_prefix = 'dev-';
4856
}
@@ -51,10 +59,15 @@ private function updateBranchPrefix(): void
5159
{
5260
$updater = new CreateBranchPrefixUpdater(
5361
$this->integration_factory,
54-
$this->branch_prefix_saver
62+
$this->git_permissions_manager,
63+
$this->branch_prefix_saver,
5564
);
5665

57-
$updater->updateBranchPrefix(self::INTEGRATION_ID, $this->branch_prefix);
66+
$updater->updateBranchPrefix(
67+
UserTestBuilder::anActiveUser()->build(),
68+
self::INTEGRATION_ID,
69+
$this->branch_prefix,
70+
);
5871
}
5972

6073
public function testItStoresTheBranchPrefix(): void
@@ -65,6 +78,11 @@ public function testItStoresTheBranchPrefix(): void
6578
->with(self::INTEGRATION_ID)
6679
->willReturn(RepositoryIntegrationBuilder::aGitlabRepositoryIntegration(self::INTEGRATION_ID)->build());
6780

81+
$this->git_permissions_manager
82+
->expects(self::once())
83+
->method('userIsGitAdmin')
84+
->willReturn(true);
85+
6886
$this->updateBranchPrefix();
6987

7088
self::assertSame(1, $this->branch_prefix_saver->getCallCount());
@@ -78,6 +96,11 @@ public function testItStoresTheBranchPrefixWithSomeSpecialChars(): void
7896
->with(self::INTEGRATION_ID)
7997
->willReturn(RepositoryIntegrationBuilder::aGitlabRepositoryIntegration(self::INTEGRATION_ID)->build());
8098

99+
$this->git_permissions_manager
100+
->expects(self::once())
101+
->method('userIsGitAdmin')
102+
->willReturn(true);
103+
81104
$this->branch_prefix = 'dev/';
82105
$this->updateBranchPrefix();
83106

@@ -98,6 +121,25 @@ public function testItThrowsAnExceptionIfIntegrationNotFoundTheBranchPrefix(): v
98121
self::assertSame(0, $this->branch_prefix_saver->getCallCount());
99122
}
100123

124+
public function testItThrowsAnExceptionIfUserIsNotGitAdministrator(): void
125+
{
126+
$this->integration_factory
127+
->expects(self::once())
128+
->method('getIntegrationById')
129+
->with(self::INTEGRATION_ID)
130+
->willReturn(RepositoryIntegrationBuilder::aGitlabRepositoryIntegration(self::INTEGRATION_ID)->build());
131+
132+
$this->git_permissions_manager
133+
->expects(self::once())
134+
->method('userIsGitAdmin')
135+
->willReturn(false);
136+
137+
$this->expectException(GitUserNotAdminException::class);
138+
$this->updateBranchPrefix();
139+
140+
self::assertSame(0, $this->branch_prefix_saver->getCallCount());
141+
}
142+
101143
public function testItThrowsAnExceptionIfBranchPrefixIsNotValid(): void
102144
{
103145
$this->integration_factory
@@ -106,6 +148,11 @@ public function testItThrowsAnExceptionIfBranchPrefixIsNotValid(): void
106148
->with(self::INTEGRATION_ID)
107149
->willReturn(RepositoryIntegrationBuilder::aGitlabRepositoryIntegration(self::INTEGRATION_ID)->build());
108150

151+
$this->git_permissions_manager
152+
->expects(self::once())
153+
->method('userIsGitAdmin')
154+
->willReturn(true);
155+
109156
$this->branch_prefix = 'dev:';
110157

111158
$this->expectException(InvalidBranchNameException::class);

0 commit comments

Comments
 (0)