diff --git a/app/Http/Controllers/Webhook/Gitlab.php b/app/Http/Controllers/Webhook/Gitlab.php index 0a7efed99..9371d9d91 100644 --- a/app/Http/Controllers/Webhook/Gitlab.php +++ b/app/Http/Controllers/Webhook/Gitlab.php @@ -40,6 +40,11 @@ class Gitlab extends Controller $gitlabApp = GitlabApp::whereKey(data_get($payload, 'gitlab_app_id'))->firstOrFail(); + // Only users who may administer the source can complete OAuth and store tokens. + if (! $request->user()->can('update', $gitlabApp)) { + return redirect()->route('source.all')->with('error', 'You are not authorized to connect this GitLab App.'); + } + $baseUrl = rtrim($gitlabApp->html_url, '/'); $response = Http::asForm()->post("{$baseUrl}/oauth/token", [ diff --git a/app/Livewire/Project/New/GitlabPrivateRepository.php b/app/Livewire/Project/New/GitlabPrivateRepository.php index 798a0dc50..a0ff15304 100644 --- a/app/Livewire/Project/New/GitlabPrivateRepository.php +++ b/app/Livewire/Project/New/GitlabPrivateRepository.php @@ -7,11 +7,14 @@ use App\Models\GitlabApp; use App\Models\Project; use App\Rules\ValidGitBranch; use App\Support\ValidationPatterns; +use Illuminate\Foundation\Auth\Access\AuthorizesRequests; use Illuminate\Support\Facades\Route; use Livewire\Component; class GitlabPrivateRepository extends Component { + use AuthorizesRequests; + public $current_step = 'gitlab_apps'; public $gitlab_apps; @@ -158,6 +161,8 @@ class GitlabPrivateRepository extends Component public function submit() { try { + $this->authorize('create', Application::class); + $validator = validator([ 'selected_repository_path' => $this->selected_repository_path, 'selected_branch_name' => $this->selected_branch_name, diff --git a/app/Policies/GitlabAppPolicy.php b/app/Policies/GitlabAppPolicy.php index 596e7e15a..56861e89b 100644 --- a/app/Policies/GitlabAppPolicy.php +++ b/app/Policies/GitlabAppPolicy.php @@ -7,44 +7,69 @@ use App\Models\User; class GitlabAppPolicy { + /** + * Determine whether the user can view any models. + */ public function viewAny(User $user): bool { return true; } + /** + * Determine whether the user can view the model. + */ public function view(User $user, GitlabApp $gitlabApp): bool { - return true; + if ($gitlabApp->is_system_wide) { + return true; + } + + return $user->teams->contains('id', $gitlabApp->team_id); } + /** + * Determine whether the user can create models. + */ public function create(User $user): bool { - return true; + return $user->isAdmin(); } + /** + * Determine whether the user can update the model. + */ public function update(User $user, GitlabApp $gitlabApp): bool { if ($gitlabApp->is_system_wide) { - return true; + return $user->canAccessSystemResources(); } - return true; + return $user->isAdminOfTeam($gitlabApp->team_id); } + /** + * Determine whether the user can delete the model. + */ public function delete(User $user, GitlabApp $gitlabApp): bool { if ($gitlabApp->is_system_wide) { - return true; + return $user->canAccessSystemResources(); } - return true; + return $user->isAdminOfTeam($gitlabApp->team_id); } + /** + * Determine whether the user can restore the model. + */ public function restore(User $user, GitlabApp $gitlabApp): bool { return false; } + /** + * Determine whether the user can permanently delete the model. + */ public function forceDelete(User $user, GitlabApp $gitlabApp): bool { return false; diff --git a/tests/Feature/GitlabAppAuthorizationTest.php b/tests/Feature/GitlabAppAuthorizationTest.php new file mode 100644 index 000000000..7fc20d07f --- /dev/null +++ b/tests/Feature/GitlabAppAuthorizationTest.php @@ -0,0 +1,92 @@ +team = Team::factory()->create(); + $this->owner = User::factory()->create(); + $this->member = User::factory()->create(); + $this->team->members()->attach($this->owner->id, ['role' => 'owner']); + $this->team->members()->attach($this->member->id, ['role' => 'member']); + + InstanceSettings::forceCreate([ + 'id' => 0, + 'fqdn' => null, + 'public_ipv4' => null, + 'public_ipv6' => null, + ]); + + $this->gitlabApp = GitlabApp::create([ + 'name' => 'Self-hosted GitLab', + 'api_url' => 'https://gitlab.example.com/api/v4', + 'html_url' => 'https://gitlab.example.com', + 'custom_user' => 'git', + 'custom_port' => 22, + 'client_id' => 'client-id', + 'client_secret' => 'client-secret', + 'webhook_token' => 'secret-webhook-token', + 'access_token' => 'access-token', + 'refresh_token' => 'refresh-token', + 'expires_at' => time() + 3600, + 'redirect_uri' => 'https://coolify.example.com/webhooks/source/gitlab/redirect', + 'team_id' => $this->team->id, + 'is_system_wide' => false, + 'is_public' => false, + ]); +}); + +describe('GitLab App authorization', function () { + test('team member cannot update a gitlab app via instantSave', function () { + $this->actingAs($this->member); + session(['currentTeam' => $this->team]); + + Livewire::withQueryParams(['gitlab_app_uuid' => $this->gitlabApp->uuid]) + ->test(Change::class) + ->set('isSystemWide', true) + ->call('instantSave') + ->assertDispatched('error'); + + expect($this->gitlabApp->refresh()->is_system_wide)->toBeFalse(); + }); + + test('team owner can update a gitlab app via instantSave', function () { + $this->actingAs($this->owner); + session(['currentTeam' => $this->team]); + + Livewire::withQueryParams(['gitlab_app_uuid' => $this->gitlabApp->uuid]) + ->test(Change::class) + ->set('isSystemWide', true) + ->call('instantSave') + ->assertDispatched('success'); + + expect($this->gitlabApp->refresh()->is_system_wide)->toBeTrue(); + }); + + test('team member cannot create an application from a private gitlab repository', function () { + $this->actingAs($this->member); + session(['currentTeam' => $this->team]); + + $applicationsBefore = Application::count(); + + // Avoid setting selected_project_id — its updated* hook loads branches and is unrelated to this auth check. + Livewire::test(GitlabPrivateRepository::class, ['type' => 'private-gitlab-app']) + ->set('selected_repository_path', 'group/repo') + ->set('selected_branch_name', 'main') + ->set('selected_gitlab_app_id', $this->gitlabApp->id) + ->set('gitlab_app_id', $this->gitlabApp->id) + ->call('submit') + ->assertDispatched('error'); + + expect(Application::count())->toBe($applicationsBefore); + }); +}); diff --git a/tests/Feature/GitlabOAuthCallbackStateTest.php b/tests/Feature/GitlabOAuthCallbackStateTest.php index 2bd8e2aa8..c9d693dfe 100644 --- a/tests/Feature/GitlabOAuthCallbackStateTest.php +++ b/tests/Feature/GitlabOAuthCallbackStateTest.php @@ -105,4 +105,25 @@ describe('GitLab OAuth callback state validation', function () { $response->assertRedirect(route('login')); }); + + test('rejects a callback from a team member who cannot administer the source', function () { + Http::fake(); + + $member = User::factory()->create(); + $this->team->members()->attach($member->id, ['role' => 'member']); + $this->actingAs($member); + session(['currentTeam' => $this->team]); + + $state = 'member-state'; + Cache::put(GitlabSource::oauthStateCacheKey($state), [ + 'gitlab_app_id' => $this->gitlabApp->id, + 'team_id' => $this->team->id, + ], now()->addMinutes(60)); + + $response = $this->get('/webhooks/source/gitlab/redirect?code=any&state='.$state); + + $response->assertRedirect(route('source.all')); + Http::assertNothingSent(); + expect($this->gitlabApp->refresh()->access_token)->toBeNull(); + }); }); diff --git a/tests/Unit/Policies/GitlabAppPolicyTest.php b/tests/Unit/Policies/GitlabAppPolicyTest.php new file mode 100644 index 000000000..a2cd2a091 --- /dev/null +++ b/tests/Unit/Policies/GitlabAppPolicyTest.php @@ -0,0 +1,172 @@ +makePartial(); + + $policy = new GitlabAppPolicy; + expect($policy->viewAny($user))->toBeTrue(); +}); + +it('allows any user to view system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->view($user, $model))->toBeTrue(); +}); + +it('allows team member to view non-system-wide gitlab app', function () { + $teams = collect([ + (object) ['id' => 1, 'pivot' => (object) ['role' => 'member']], + ]); + + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->view($user, $model))->toBeTrue(); +}); + +it('denies non-team member to view non-system-wide gitlab app', function () { + $teams = collect([ + (object) ['id' => 2, 'pivot' => (object) ['role' => 'member']], + ]); + + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('getAttribute')->with('teams')->andReturn($teams); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->view($user, $model))->toBeFalse(); +}); + +it('allows admin to create gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdmin')->andReturn(true); + + $policy = new GitlabAppPolicy; + expect($policy->create($user))->toBeTrue(); +}); + +it('denies non-admin to create gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdmin')->andReturn(false); + + $policy = new GitlabAppPolicy; + expect($policy->create($user))->toBeFalse(); +}); + +it('allows user with system access to update system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('canAccessSystemResources')->andReturn(true); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->update($user, $model))->toBeTrue(); +}); + +it('denies user without system access to update system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('canAccessSystemResources')->andReturn(false); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->update($user, $model))->toBeFalse(); +}); + +it('allows team admin to update non-system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdminOfTeam')->with(1)->andReturn(true); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->update($user, $model))->toBeTrue(); +}); + +it('denies team member to update non-system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdminOfTeam')->with(1)->andReturn(false); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->update($user, $model))->toBeFalse(); +}); + +it('allows user with system access to delete system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('canAccessSystemResources')->andReturn(true); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->delete($user, $model))->toBeTrue(); +}); + +it('denies user without system access to delete system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('canAccessSystemResources')->andReturn(false); + + $model = mockGitlabApp(teamId: 1, isSystemWide: true); + + $policy = new GitlabAppPolicy; + expect($policy->delete($user, $model))->toBeFalse(); +}); + +it('allows team admin to delete non-system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdminOfTeam')->with(1)->andReturn(true); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->delete($user, $model))->toBeTrue(); +}); + +it('denies team member to delete non-system-wide gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + $user->shouldReceive('isAdminOfTeam')->with(1)->andReturn(false); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->delete($user, $model))->toBeFalse(); +}); + +it('denies restore of gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->restore($user, $model))->toBeFalse(); +}); + +it('denies force delete of gitlab app', function () { + $user = Mockery::mock(User::class)->makePartial(); + + $model = mockGitlabApp(teamId: 1, isSystemWide: false); + + $policy = new GitlabAppPolicy; + expect($policy->forceDelete($user, $model))->toBeFalse(); +}); + +function mockGitlabApp(int $teamId, bool $isSystemWide): GitlabApp +{ + $gitlabApp = Mockery::mock(GitlabApp::class)->makePartial(); + $gitlabApp->team_id = $teamId; + $gitlabApp->is_system_wide = $isSystemWide; + + return $gitlabApp; +}