diff --git a/app/Actions/UpdateMentionedOrganizations.php b/app/Actions/UpdateMentionedOrganizations.php index 31546db70..957533808 100644 --- a/app/Actions/UpdateMentionedOrganizations.php +++ b/app/Actions/UpdateMentionedOrganizations.php @@ -9,6 +9,7 @@ use App\Models\Profile; use Carbon\CarbonImmutable; use Closure; +use Illuminate\Http\Client\RequestException; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\DB; use Illuminate\Support\Str; @@ -39,8 +40,9 @@ public function __construct( /** * Asks GitHub about the lowercase handles that are neither a user's nor a known organization's, and refreshes the * known organizations among them once their profile is a day old. A handle that isn't an organization's isn't asked - * about again for a day, and an organization GitHub no longer has is deleted, with the links to it. Returns when to - * try again if the rate limit cut the lookups short. + * about again for a day, and an organization GitHub no longer has is deleted, with the links to it. An + * organization that keeps GitHub from showing it to the app, with an IP allow list for one, is skipped. Returns when + * to try again if the rate limit cut the lookups short. * * @param list $handles */ @@ -89,7 +91,20 @@ function () use ($organization): void { } foreach ($lookups as [$lookUp, $notFound]) { - $account = $lookUp(); + try { + $account = $lookUp(); + } catch (RequestException $exception) { + // The refusals an organization sets up, as in HttpGitHubProvider::organizationMembership(): an IP + // allow list, SAML single sign-on or OAuth App access restrictions. Any other one, a rate limit among + // them, is retried. + $response = $exception->response; + + if (! $response->forbidden() || (! $response->hasHeader('X-GitHub-SSO') && ! Str::contains($response->fluent()->string('message')->value(), ['IP allow list', 'OAuth App access restrictions']))) { + throw $exception; + } + + continue; + } if (($account['type'] ?? null) !== 'Organization') { $notFound(); diff --git a/tests/Unit/Actions/UpdateCommitTopicsTest.php b/tests/Unit/Actions/UpdateCommitTopicsTest.php index 40bb954d3..a0cfbd39c 100644 --- a/tests/Unit/Actions/UpdateCommitTopicsTest.php +++ b/tests/Unit/Actions/UpdateCommitTopicsTest.php @@ -230,12 +230,18 @@ function gitHubOrganization(array $account = []): array expect(Cache::has('github-organizations:paused'))->toBeFalse(); }); -it('throws when GitHub fails, so the job tries again', function (): void { +it('throws when GitHub fails, rate limits or forbids anything else, so the job tries again', function (Psr7Response $response): void { $github = $this->mock(GitHubProvider::class); $github->shouldReceive('rateLimit')->andReturnNull(); - $github->shouldReceive('accountByLogin')->with('laravel')->andThrow(new RequestException(new Response(new Psr7Response(502)))); + $github->shouldReceive('accountByLogin')->with('laravel')->andThrow(new RequestException(new Response($response))); $commit = Commit::factory()->create(['body' => '@laravel', 'mentions' => []]); expect(fn () => resolve(UpdateCommitTopics::class)->handle($commit))->toThrow(RequestException::class); -}); +})->with([ + 'an error' => fn (): Psr7Response => new Psr7Response(502), + 'the primary rate limit' => fn (): Psr7Response => new Psr7Response(403, ['X-RateLimit-Remaining' => '0']), + 'a secondary rate limit' => fn (): Psr7Response => new Psr7Response(403, ['Retry-After' => '60']), + 'a secondary rate limit without headers' => fn (): Psr7Response => new Psr7Response(403, [], '{"message":"You have exceeded a secondary rate limit. Please wait a few minutes before you try again."}'), + 'any other refusal' => fn (): Psr7Response => new Psr7Response(403, [], '{"message":"Resource not accessible by integration"}'), +]); diff --git a/tests/Unit/Jobs/LookUpUserOrganizationsTest.php b/tests/Unit/Jobs/LookUpUserOrganizationsTest.php index 8abf976c0..9a01224dd 100644 --- a/tests/Unit/Jobs/LookUpUserOrganizationsTest.php +++ b/tests/Unit/Jobs/LookUpUserOrganizationsTest.php @@ -8,6 +8,9 @@ use App\Jobs\LookUpUserOrganizations; use App\Models\Profile; use App\Models\User; +use GuzzleHttp\Psr7\Response as Psr7Response; +use Illuminate\Http\Client\RequestException; +use Illuminate\Http\Client\Response; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Queue; @@ -25,6 +28,31 @@ expect(Profile::query()->where('type', ProfileType::Organization)->pluck('handle')->all())->toBe(['laravel']); }); +it('skips the organizations that keep GitHub from showing them, and looks the others up', function (Psr7Response $response): void { + $forbidden = new RequestException(new Response($response)); + $github = $this->mock(GitHubProvider::class); + $github->shouldReceive('rateLimit')->andReturnNull(); + $github->shouldReceive('accountByLogin')->with('restricted')->once()->andThrow($forbidden); + $github->shouldReceive('accountByLogin')->with('laravel')->once()->andReturn(['login' => 'laravel', 'id' => 958072, 'type' => 'Organization', 'name' => 'Laravel', 'avatar_url' => null, 'bio' => null, 'location' => null, 'blog' => '', 'company' => null]); + $github->shouldReceive('account')->with('1')->once()->andThrow($forbidden); + $github->shouldReceive('account')->with('6764390')->once()->andReturn(['login' => 'symfony', 'id' => 6764390, 'type' => 'Organization', 'name' => 'Symfony, SAS', 'avatar_url' => null, 'bio' => null, 'location' => null, 'blog' => '', 'company' => null]); + + $private = Profile::factory()->organization()->create(['github_id' => '1', 'handle' => 'private', 'name' => 'Private', 'github_synced_at' => now()->subDays(2)]); + $symfony = Profile::factory()->organization()->create(['github_id' => '6764390', 'handle' => 'symfony', 'name' => 'Symfony', 'github_synced_at' => now()->subDays(2)]); + $user = User::factory()->create(['bio' => 'Thanks @restricted and @laravel', 'company' => '@private @symfony']); + + dispatch_sync(new LookUpUserOrganizations($user)); + + expect(Profile::query()->where('type', ProfileType::Organization)->orderBy('handle')->pluck('handle')->all())->toBe(['laravel', 'private', 'symfony']) + ->and($private->refresh()->name)->toBe('Private') + ->and($symfony->refresh()->name)->toBe('Symfony, SAS') + ->and(Cache::has('github-organizations:not-found:restricted'))->toBeFalse(); +})->with([ + 'an IP allow list' => fn (): Psr7Response => new Psr7Response(403, [], '{"message":"Although you appear to have the correct authorization credentials, the `acme` organization has an IP allow list enabled, and your IP address is not permitted to access this resource."}'), + 'SAML single sign-on' => fn (): Psr7Response => new Psr7Response(403, ['X-GitHub-SSO' => 'required; url=https://github.com/orgs/acme/sso'], '{"message":"Resource protected by organization SAML enforcement."}'), + 'OAuth app access restrictions' => fn (): Psr7Response => new Psr7Response(403, [], '{"message":"Although you appear to have the correct authorization credentials, the `acme` organization has enabled OAuth App access restrictions, meaning that data access to third-parties is limited."}'), +]); + it('runs again once the rate limit resets when it cut the lookups short', function (): void { $github = $this->mock(GitHubProvider::class); $github->shouldNotReceive('accountByLogin');