Throw exception on 403 response

[samdark]

What I do

In case you deny access in AWS policies, user would get 403 response with "User: anonymous is not authorized to perform" message.

Expected

Exception containing a response code and a message.

Actual

Elastica ignores the fact so I have to check each call to the library like the following:

$response = $index->create($mapping);
if (!$response->isOk()) {
    throw new \Exception(sprintf('Unable to create index. Server responded with %s: %s', $response->getStatus(), json_encode($response->getData())));
}

[jandom]

@samdark completely agreed!

Now, in case of a typo in AWS credentials, 403 are silently ignored and that's just not a great "feature"

The code below is fair enough...

# lib/Elastica//Transport/Guzzle.php

$client = $this->_getGuzzleClient($this->_getBaseUrl($connection), $connection->isPersistent(), $request);

$options = [
    'exceptions' => false, // 4xx and 5xx is expected and NOT an exceptions in this context
];

// options then passed to $res = $client->send($req, $options);

...because a few lines later, response is re-packaged and create a new Response object

$response = new Response((string) $res->getBody(), $res->getStatusCode());
...
if ($response->hasError()) {
    throw new ResponseException($request, $response);
}

However, that's really limited because Response hasError assumes that the response comes from ES cluster. If AWS credentials are wrong (eg a because of a typo) this will never be transparent to the user

/**
 * True if response has error.
 *
 * @return bool True if response has error
 */
public function hasError()
{
    $response = $this->getData();

    return isset($response['error']);
}

In case of invalid AWS credentials the status code is 403 and the response looks like

{"message":"The security token included in the request is invalid."}

[jandom]

This is the commit that introduced the problem

47e6d70#diff-77a5691dc6944986e3f43e8d562514d3

4xx and 5xx are not crashing to be handled later, except for some of them that are not, because hasErrors assumes $response['error'] is the source of truth

[ruflin]

Any suggestions on how to fix it?

[jandom]

Setup the docker container via the makefile and had like 3 ideas, all ugly. So will get back with something more "kosher" in the next few days. Thanks for getting back!

[Destroy666x]

Very annoying issue that lost me a bunch of time. Also when attempting AWS connection, but this time a 400 for HTTP/HTTPS incompatibility was ignored because I didn't realize transport: https was needed. No error in the console whatsoever and progress bars went on quite slowly, so I assumed that ES service is failing for some reason. BTW, interestingly, when doing it the other way round, so using https when http was needed, an error was thrown that time... Looking forward to a fix, if I find any time (I doubt) I might try.

[jandom]

@Destroy666x agreed, this is not a great behaviour at present.

Spent a some time last night how it all fits together

• this is actually a feature not a bug
• the Client class from ruflin/Elastica gobbles up all the exceptions into transportInfo
• there is even a test for this

Thus, it is instead down to downstream client to implement an error check. in this case FOSElasticaBundle extends the base client class and should do the check.

The existing tests in FosElasticaBundle rely on NullTransport from ruflin/Elastica to send a mock response. This class could be either duplicated (easy) or refactored (harder) to be more generic and test different use-cases.

The PR is available here #1529

I'm keen to hear your comments – this is my first contribution here and i'm sure this may not be 100% correct @ruflin

[jandom]

This is now resolved by two PRs

• Implement an ErrorTransport #1529
• Handle Elastica Transport errors FriendsOfSymfony/FOSElasticaBundle#1465

[NathanBaulch]

What about those of us who are using AWS Elasticsearch without FOSElasticaBundle? Do we need to wrap the Elastica client just so we can make sure no transport errors have occurred?

In my particular case I'm using AWS v4 auth and my server's date drifted 5 minutes into the future. I started getting weird errors from Elastica because the response from AWS doesn't contain an error field (so Response::hasError() is false even though the HTTP status code was 403) so it just assumed nothing was wrong. For example, aggregation queries started throwing "This result set does not contain an aggregation named xxx" instead of a meaningful error.

[ruflin]

@NathanBaulch What would the changes to Elastica be to support this?

[ebernhardson]

Following the principle of least surprise, I would expect Response::hasError to return true for anything other than a successful response.

[NathanBaulch]

I agree, though it looks like there's already an open discussion on this in #1396.

One exception to this (which bit me yesterday) are the 404 responses from the HEAD requests made by various exists functions (Type, Index, IndexTemplate).

I don't know what the best long term solution is, but for now I've sub-classed Client in my application.

class Client extends \Elastica\Client
{
    public function request($path, $method = Request::GET, $data = [], array $query = []) {
        $res = parent::request($path, $method, $data, $query);
        // workaround for Elastica issue #1508
        $info = $res->getTransferInfo();
        if (($code = $info['http_code'] ?? null) && $code >= 400 && !($code === 404 && $method === Request::HEAD)) {
            $data = $res->getData();
            $body = is_array($data) ? ($data['message'] ?? json_encode($data)) : $data;
            throw new ClientException("HTTP $code error: $body");
        }
        return $res;
    }
}

[ruflin]

As we started to do breaking changes in Elastica for 7.0 we can also tackled this with potential breaking changes. Should we move the discussion to #1396 ?