-
Notifications
You must be signed in to change notification settings - Fork 344
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
Implementation of Bearer Token Authorization Support #1483
Implementation of Bearer Token Authorization Support #1483
Conversation
da432e5
to
17893c4
Compare
Codecov Report
@@ Coverage Diff @@
## master #1483 +/- ##
=========================================
Coverage 97.34% 97.34%
- Complexity 2830 2831 +1
=========================================
Files 175 175
Lines 9418 9420 +2
=========================================
+ Hits 9168 9170 +2
Misses 250 250
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
17893c4
to
fdc4bc0
Compare
@staabm we are starting to use the Client part of Please review. I don't want to "just merge" this because I have been involved with Amrita in discussing this. |
if (isset($settings['accessToken'])) { | ||
$this->addCurlSetting(CURLOPT_HTTPHEADER, ["Authorization: Bearer {$settings['accessToken']}"]); | ||
} | ||
|
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.
setting headers here will get replaced if we do requests with some headers. So, this doesn't work as we expected.
Example:
$webdav = new Client([
"baseUri" => "https://localhost:9200/remote.php/webdav/",
"accessToken" => "eJashdksjaausb.asdohowkawrkas.adhdiuqwiduwqgwdi"
]);
// propfind() sets headers internally
$res = $webdav->propfind("", []); // 401 Unauthorized ('Authorization' header gets removed)
Here, the headers are replaced: https://github.com/sabre-io/http/blob/d7c809503c1cb880622659095fc73d513e9ef23c/lib/Client.php#L409-L411
This will work but can't say if the Http\Client
wants to do it.
if ([] !== $nHeaders) {
$settings[CURLOPT_HTTPHEADER] = array_merge($settings[CURLOPT_HTTPHEADER], $nHeaders);
}
CC @phil-davis
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.
Also, we can do like this. So, I wonder if we want to have it in DAV\Client
's settings
but would be great if we have.
$webdav = new Client([
"baseUri" => "https://localhost:9200/remote.php/webdav/"
]);
$webdav->addCurlSetting(CURLOPT_HTTPHEADER, ["Authorization: Bearer $accessToken"]);
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.
If we want to have it then I found some information to set it curl way instead of in header directly
// lib/DAV/Client.php
...
$this->addCurlSetting(CURLOPT_USERPWD, $userName.':'.$password);
+} else if (isset($settings['accessToken'])) {
+ $this->addCurlSetting(CURLOPT_HTTPAUTH, CURLAUTH_BEARER);
+ $this->addCurlSetting(CURLOPT_XOAUTH2_BEARER, isset($settings['accessToken']));
+}
See https://curl.se/libcurl/c/CURLOPT_HTTPAUTH.html, https://curl.se/libcurl/c/CURLOPT_XOAUTH2_BEARER.html
PHP: https://www.php.net/manual/en/function.curl-setopt.php, https://www.php.net/manual/en/curl.constants.php
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.
Using CURLOPT_XOAUTH2_BEARER
works without having to fix anything in Http\Client.
@@ -102,6 +102,7 @@ class Client extends HTTP\Client | |||
* * baseUri | |||
* * userName (optional) | |||
* * password (optional) | |||
* * accessToken (optional) |
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.
IMO this name is too generic. Couldn't we name it bearerToken
?
Alternatively couldn't we add a method to pass any raw http header thru to curl?
I am not sure why we need this PR. have a look at the examples of sabre/http client in https://github.com/sabre-io/http/blob/d7c809503c1cb880622659095fc73d513e9ef23c/examples/basicauth.php and we have a BearerAuth class already |
After discussion, we don't actually need this.
After discussion, we have realized that we can achieve this in other ways, without needing to add any feature to dav-client. |
Description
With this PR, we are enhancing our HTTP client's capability by introducing support for Bearer token-based authorization.
Related to this PR
Test Added