-
Notifications
You must be signed in to change notification settings - Fork 6
apply http data source service config to request #492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3290192
to
4dc0902
Compare
Test this PR in WordPress Playground. |
The generic http data source supports authentication config in the service config. This was not applied to the request, which now happens.
4dc0902
to
f7e3291
Compare
$auth_headers[ $auth['key'] ] = $auth['value']; | ||
} | ||
|
||
return $auth_headers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding a filter here to allow customizing these headers. I know we only allow 3 types of authorization schemes, but there are more schemes possible.. This might be a good way to support those without expanding the dropdown?
|
||
$query_string = $service_config['auth']['key'] . '=' . $service_config['auth']['value']; | ||
|
||
return $endpoint . ( strpos( $endpoint, '?' ) === false ? '?' : '&' ) . $query_string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be time to use some built in methods from WordPress or PHP to do URL construction rather than doing it by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected static function map_service_config( array $service_config ): array { | ||
$request_headers = $service_config['request_headers'] ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about trying to use the existing request_headers function to add these auth headers in? My thought was that, it should all happen in one place because they are headers for the request at the end of the day.
This is superseded by #495 |
The generic HTTP data source supports authentication configuration in the service config, including Bearer Token, Basic Auth, and API Key. Authentication headers are now properly applied to outgoing requests based on the configured authentication type -
Authorization
header with valueBearer {token}
Authorization
header with valueBasic {base64_encoded_credentials}
Authorization
header ifadd_to
isheader
, or as a query parameter ifadd_to
isqueryparams