diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 801336b2b..29a085d28 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -484,6 +484,9 @@ 'PrettierLinter' => 'lint/linter/PrettierLinter.php', 'PytestTestEngine' => 'unit/engine/PytestTestEngine.php', 'SubmitQueueMockClient' => 'land/__tests__/mocks/MockSubmitQueueClient.php', + 'SubmitQueueMockClientWithUSSOTracking' => 'land/__tests__/mocks/MockSubmitQueueClient.php', + 'SubmitQueueMockFuture' => 'land/__tests__/mocks/MockSubmitQueueClient.php', + 'SubmitQueueMockHTTPSFuture' => 'land/__tests__/mocks/MockSubmitQueueClient.php', 'SubmitQueueMockRepositoryAPI' => 'land/__tests__/mocks/MockRepositoryAPI.php', 'SubmitQueueMockWorkflow' => 'land/__tests__/mocks/MockWorkflow.php', 'TAPTestEngine' => 'unit/engine/TAPTestEngine.php', @@ -1016,6 +1019,9 @@ 'PrettierLinter' => 'ArcanistExternalLinter', 'PytestTestEngine' => 'ArcanistUnitTestEngine', 'SubmitQueueMockClient' => 'stdClass', + 'SubmitQueueMockClientWithUSSOTracking' => 'UberSubmitQueueClient', + 'SubmitQueueMockFuture' => 'Phobject', + 'SubmitQueueMockHTTPSFuture' => 'Phobject', 'SubmitQueueMockRepositoryAPI' => 'ArcanistRepositoryAPI', 'SubmitQueueMockWorkflow' => 'ArcanistWorkflow', 'TAPTestEngine' => 'ArcanistUnitTestEngine', diff --git a/src/land/UberSubmitQueueClient.php b/src/land/UberSubmitQueueClient.php index eb810df9a..137c724d1 100644 --- a/src/land/UberSubmitQueueClient.php +++ b/src/land/UberSubmitQueueClient.php @@ -1,11 +1,11 @@ uri = new PhutilURI($uri); @@ -42,7 +42,7 @@ public function submitPriorityMergeRequest($revisionId) { 'revisionId' => $revisionId, 'conduitToken' => $this->conduitToken, ); - return $this->callMethodSynchronous("POST", "/v2/priority_merge_request", $params); + return $this->callMethodSynchronous("POST", "/v2/priority_merge_request", $params, true); } public function submitMergeStackRequest($remoteUrl, $stack, $shouldShadow, $targetOnto) { @@ -58,25 +58,27 @@ public function submitMergeStackRequest($remoteUrl, $stack, $shouldShadow, $targ return $this->callMethodSynchronous("POST", "/merge_requests", $params); } - private function callMethodSynchronous($method, $api, array $params) { - return $this->callMethod($method, $api, $params)->resolve(); + private function callMethodSynchronous($method, $api, array $params, $use_usso_token = false) { + return $this->callMethod($method, $api, $params, $use_usso_token)->resolve(); } - private function callMethod($method, $api, array $params) { + protected function callMethod($method, $api, array $params, $use_usso_token = false) { $req = id(clone $this->uri)->setPath('/api'.$api.'?'.http_build_query($params)); // Always use the cURL-based HTTPSFuture, for proxy support and other // protocol edge cases that HTTPFuture does not support. $core_future = new HTTPSFuture($req); $core_future->addHeader('Host', $this->getHost()); - // Add uSSO token to the request - $usso = new UberUSSO(); - $hostname = parse_url($this->uri, PHP_URL_HOST); - $token = $usso->maybeUseUSSOToken($hostname); - if (!$token) { - $token = $usso->getUSSOToken($hostname); + // Add uSSO token to the request only when using --skip-submitqueue-checks flag + if ($use_usso_token) { + $usso = new UberUSSO(); + $hostname = parse_url($this->uri, PHP_URL_HOST); + $token = $usso->maybeUseUSSOToken($hostname); + if (!$token) { + $token = $usso->getUSSOToken($hostname); + } + $core_future->addHeader('Authorization', "Bearer {$token}"); } - $core_future->addHeader('Authorization', "Bearer {$token}"); $core_future->setMethod($method); $core_future->setTimeout($this->timeout); diff --git a/src/land/__tests__/UberArcanistSubmitQueueEngineTestCase.php b/src/land/__tests__/UberArcanistSubmitQueueEngineTestCase.php index 41e799731..db15c7220 100644 --- a/src/land/__tests__/UberArcanistSubmitQueueEngineTestCase.php +++ b/src/land/__tests__/UberArcanistSubmitQueueEngineTestCase.php @@ -154,4 +154,61 @@ public function testPushChangeToSubmitQueue_PriorityMerge() { $this->assertEqual('111', $mock_client->last_call_params['revisionId']); $this->assertEqual(1, count($mock_client->last_call_params), 'Should only have one parameter'); } + + /** + * Test that submitMergeRequest does NOT add USSO token. + */ + public function testSubmitMergeRequest_NoUSSOToken() { + $client = new SubmitQueueMockClientWithUSSOTracking( + 'https://submit-queue.example.com', + 'test-conduit-token' + ); + + // Call submitMergeRequest (regular merge, no --skip-submitqueue-checks flag) + $client->submitMergeRequest( + 'git@github.com:test/repo.git', + '123', + '456', + false, + 'master' + ); + + // Verify that use_usso_token was false + $this->assertEqual(false, $client->last_use_usso_token, + 'Regular merge requests should NOT use USSO token'); + $this->assertEqual(0, count($client->added_headers), + 'No Authorization headers should be added for regular merge requests'); + } + + /** + * Test that submitPriorityMergeRequest DOES add USSO token. + */ + public function testSubmitPriorityMergeRequest_WithUSSOToken() { + $client = new SubmitQueueMockClientWithUSSOTracking( + 'https://submit-queue.example.com', + 'test-conduit-token' + ); + + // Call submitPriorityMergeRequest (with --skip-submitqueue-checks flag) + $client->submitPriorityMergeRequest('789'); + + // Verify that use_usso_token was true + $this->assertEqual(true, $client->last_use_usso_token, + 'Priority merge requests should use USSO token'); + $this->assertEqual(1, count($client->added_headers), + 'Authorization header should be added for priority merge requests'); + + // Verify Authorization header was added with correct format + $auth_header_found = false; + foreach ($client->added_headers as $header) { + if (strpos($header[0], 'Authorization') === 0) { + $auth_header_found = true; + $this->assertEqual('Authorization', $header[0]); + $this->assertTrue(strpos($header[1], 'Bearer ') === 0, + 'Authorization header should start with "Bearer "'); + } + } + $this->assertTrue($auth_header_found, + 'Authorization header should be present for priority merge requests'); + } } diff --git a/src/land/__tests__/mocks/MockSubmitQueueClient.php b/src/land/__tests__/mocks/MockSubmitQueueClient.php index fb9b734ef..427b94d7c 100644 --- a/src/land/__tests__/mocks/MockSubmitQueueClient.php +++ b/src/land/__tests__/mocks/MockSubmitQueueClient.php @@ -27,3 +27,89 @@ public function submitPriorityMergeRequest($revisionId) { return 'http://submit-queue.example.com/priority/456'; } } + +/** + * Testable version of UberSubmitQueueClient that tracks USSO token usage. + */ +class SubmitQueueMockClientWithUSSOTracking extends UberSubmitQueueClient { + public $last_use_usso_token = null; + public $added_headers = array(); + + protected function callMethod($method, $api, array $params, $use_usso_token = false) { + // Track whether USSO token was requested + $this->last_use_usso_token = $use_usso_token; + $this->added_headers = array(); + + // Create a mock future + $mock_future = new SubmitQueueMockHTTPSFuture($this); + + // Simulate adding headers + $mock_future->addHeader('Host', $this->getHost()); + + // Simulate USSO token logic + if ($use_usso_token) { + // For testing, use a simple mock token + $token = 'mock-usso-token'; + + // Check if environment variable is set (matching real behavior) + $env_token = getenv('ARC_USSO_TOKEN'); + if ($env_token) { + $token = $env_token; + } + + $mock_future->addHeader('Authorization', "Bearer {$token}"); + } + + $mock_future->setMethod($method); + $mock_future->setTimeout($this->timeout); + + // Return a mock future that resolves to a success response + return new SubmitQueueMockFuture($mock_future); + } +} + +/** + * Mock HTTPSFuture for testing. + */ +class SubmitQueueMockHTTPSFuture { + private $client; + + public function __construct($client) { + $this->client = $client; + } + + public function addHeader($name, $value) { + // Track added headers in the testable client + if ($name === 'Authorization') { + $this->client->added_headers[] = array($name, $value); + } + return $this; + } + + public function setMethod($method) { + return $this; + } + + public function setTimeout($timeout) { + return $this; + } +} + +/** + * Mock UberSubmitQueueFuture for testing. + */ +class SubmitQueueMockFuture { + private $future; + + public function __construct($future) { + $this->future = $future; + } + + public function isReady() { + return true; + } + + public function resolve() { + return 'http://submit-queue.example.com/status/mock'; + } +}