diff --git a/src/Io/Transaction.php b/src/Io/Transaction.php index 330ffed0..b64622a8 100644 --- a/src/Io/Transaction.php +++ b/src/Io/Transaction.php @@ -5,6 +5,7 @@ use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\UriInterface; +use React\Http\Message\Response; use RingCentral\Psr7\Request; use RingCentral\Psr7\Uri; use React\EventLoop\LoopInterface; @@ -234,6 +235,8 @@ public function onResponse(ResponseInterface $response, RequestInterface $reques /** * @param ResponseInterface $response * @param RequestInterface $request + * @param Deferred $deferred + * @param ClientRequestState $state * @return PromiseInterface * @throws \RuntimeException */ @@ -242,7 +245,7 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac // resolve location relative to last request URI $location = Uri::resolve($request->getUri(), $response->getHeaderLine('Location')); - $request = $this->makeRedirectRequest($request, $location); + $request = $this->makeRedirectRequest($request, $location, $response->getStatusCode()); $this->progress('redirect', array($request)); if ($state->numRequests >= $this->maxRedirects) { @@ -255,25 +258,33 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac /** * @param RequestInterface $request * @param UriInterface $location + * @param int $statusCode * @return RequestInterface + * @throws \RuntimeException */ - private function makeRedirectRequest(RequestInterface $request, UriInterface $location) + private function makeRedirectRequest(RequestInterface $request, UriInterface $location, $statusCode) { - $originalHost = $request->getUri()->getHost(); - $request = $request - ->withoutHeader('Host') - ->withoutHeader('Content-Type') - ->withoutHeader('Content-Length'); - // Remove authorization if changing hostnames (but not if just changing ports or protocols). + $originalHost = $request->getUri()->getHost(); if ($location->getHost() !== $originalHost) { $request = $request->withoutHeader('Authorization'); } - // naïve approach.. - $method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET'; + $request = $request->withoutHeader('Host')->withUri($location); + + if ($statusCode === Response::STATUS_TEMPORARY_REDIRECT || $statusCode === Response::STATUS_PERMANENT_REDIRECT) { + if ($request->getBody() instanceof ReadableStreamInterface) { + throw new \RuntimeException('Unable to redirect request with streaming body'); + } + } else { + $request = $request + ->withMethod($request->getMethod() === 'HEAD' ? 'HEAD' : 'GET') + ->withoutHeader('Content-Type') + ->withoutHeader('Content-Length') + ->withBody(new EmptyBodyStream()); + } - return new Request($method, $location, $request->getHeaders()); + return $request; } private function progress($name, array $args = array()) diff --git a/tests/Io/TransactionTest.php b/tests/Io/TransactionTest.php index d9ac2178..05022009 100644 --- a/tests/Io/TransactionTest.php +++ b/tests/Io/TransactionTest.php @@ -663,7 +663,7 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting() array($this->callback(function (RequestInterface $request) use ($that) { $that->assertFalse($request->hasHeader('Content-Type')); $that->assertFalse($request->hasHeader('Content-Length')); - return true;; + return true; })) )->willReturnOnConsecutiveCalls( Promise\resolve($redirectResponse), @@ -674,6 +674,122 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting() $transaction->send($requestWithCustomHeaders); } + public function testRequestMethodShouldBeChangedWhenRedirectingWithSeeOther() + { + $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + + $customHeaders = array( + 'Content-Type' => 'text/html; charset=utf-8', + 'Content-Length' => '111', + ); + + $request = new Request('POST', 'http://example.com', $customHeaders); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + // response to the given $request + $redirectResponse = new Response(303, array('Location' => 'http://example.com/new')); + + // mock sender to resolve promise with the given $okResponse in + // response to the given $request + $okResponse = new Response(200); + $that = $this; + $sender->expects($this->exactly(2))->method('send')->withConsecutive( + array($this->anything()), + array($this->callback(function (RequestInterface $request) use ($that) { + $that->assertEquals('GET', $request->getMethod()); + $that->assertFalse($request->hasHeader('Content-Type')); + $that->assertFalse($request->hasHeader('Content-Length')); + return true; + })) + )->willReturnOnConsecutiveCalls( + Promise\resolve($redirectResponse), + Promise\resolve($okResponse) + ); + + $transaction = new Transaction($sender, $loop); + $transaction->send($request); + } + + public function testRequestMethodAndBodyShouldNotBeChangedWhenRedirectingWith307Or308() + { + $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + + $customHeaders = array( + 'Content-Type' => 'text/html; charset=utf-8', + 'Content-Length' => '111', + ); + + $request = new Request('POST', 'http://example.com', $customHeaders, '{"key":"value"}'); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + // response to the given $request + $redirectResponse = new Response(307, array('Location' => 'http://example.com/new')); + + // mock sender to resolve promise with the given $okResponse in + // response to the given $request + $okResponse = new Response(200); + $that = $this; + $sender->expects($this->exactly(2))->method('send')->withConsecutive( + array($this->anything()), + array($this->callback(function (RequestInterface $request) use ($that) { + $that->assertEquals('POST', $request->getMethod()); + $that->assertEquals('{"key":"value"}', (string)$request->getBody()); + $that->assertEquals( + array( + 'Content-Type' => array('text/html; charset=utf-8'), + 'Content-Length' => array('111'), + 'Host' => array('example.com') + ), + $request->getHeaders() + ); + return true; + })) + )->willReturnOnConsecutiveCalls( + Promise\resolve($redirectResponse), + Promise\resolve($okResponse) + ); + + $transaction = new Transaction($sender, $loop); + $transaction->send($request); + } + + public function testRedirectingStreamingBodyWith307Or308ShouldThrowCantRedirectStreamException() + { + $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + + $customHeaders = array( + 'Content-Type' => 'text/html; charset=utf-8', + 'Content-Length' => '111', + ); + + $stream = new ThroughStream(); + $request = new Request('POST', 'http://example.com', $customHeaders, new ReadableBodyStream($stream)); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + // response to the given $request + $redirectResponse = new Response(307, array('Location' => 'http://example.com/new')); + + $sender->expects($this->once())->method('send')->withConsecutive( + array($this->anything()) + )->willReturnOnConsecutiveCalls( + Promise\resolve($redirectResponse) + ); + + $transaction = new Transaction($sender, $loop); + $promise = $transaction->send($request); + + $exception = null; + $promise->then(null, function ($reason) use (&$exception) { + $exception = $reason; + }); + + assert($exception instanceof \RuntimeException); + $this->assertEquals('Unable to redirect request with streaming body', $exception->getMessage()); + } + public function testCancelTransactionWillCancelRequest() { $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();