fix: If withProxy(apiKey) is set the client does not set custom headers #191
Conversation
Since our README explicitly states that using withProxy($apiKey) is in lieu of using custom headers for the Authorization header, our SDK should make sure to check if the proxyApiKey is set, and if so, use it to set the Authorization header also for client registration and client metrics.
src/Metrics/DefaultMetricsSender.php
Outdated
| foreach ($this->configuration->getHeaders() as $name => $value) { | ||
| $request = $request->withHeader($name, $value); | ||
| } | ||
|
|
||
| try { | ||
| $this->httpClient->sendRequest($request); | ||
| $response = $this->httpClient->sendRequest($request); |
There was a problem hiding this comment.
Why the variable assignment?
There was a problem hiding this comment.
Ah, thanks for the spot, because, I needed to see the status code, it wasn't supposed to be part of the PR, reverting :)
| if ($this->configuration->getProxyKey() !== null) { | ||
| $request = $request->withHeader('Authorization', $this->configuration->getProxyKey()); | ||
| } |
There was a problem hiding this comment.
Well, this is rather unfortunate solution, but sadly I don't think we have any other quick fix. The proxy key should be directly tied to the headers in configuration.
Can you add a comment with TODO: refactor proxy key into headers or something similar, so that I'll pick it up next time I'm doing clean-up?
There was a problem hiding this comment.
I can have another stab before converting this to a proper PR.
There was a problem hiding this comment.
Totally agree that this should be moved to the headers list when we build the configuration.
src/Metrics/DefaultMetricsSender.php
Outdated
| if ($this->configuration->getProxyKey() !== null) { | ||
| $request = $request->withHeader('Authorization', $this->configuration->getProxyKey()); | ||
| } |
There was a problem hiding this comment.
Add the same todo comment here, please.
There was a problem hiding this comment.
I'm going to have one more stab at the builder before converting this to a proper PR. Thanks for the feedback 👍
|
Hey @RikudouSage - could I get a stop or a go-ahead here, I added the api key to the headers in the builder's build method instead, which meant I could remove the unfortunate if checks in our request handling. |
|
Please merge, sorry, I see I owed you this answer. |
chriswk
changed the title
chriswk
changed the title
…rs (#191) Since our README explicitly states that using withProxy($apiKey) is in lieu of using custom headers for the Authorization header, our SDK now makes sure to check if the proxyApiKey is set, and if so, uses it to set the Authorization header also for client registration and client metrics.
Since our README explicitly states that using withProxy($apiKey) is in lieu of using custom headers for the Authorization header, our SDK should make sure to check if the proxyApiKey is set, and if so, use it to set the Authorization header also for client registration and client metrics.
Description
We had clients complaining that when they used the PHP client in proxy mode (using the withProxy method), they struggled with 403 responses when trying to register/post metrics. After diving into the code I found that we were not using the key as an authorization header for any other request but the request to /api/frontend.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: