-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added Authorization Bearer token support as per RFC6750 #1
Conversation
mbag
commented
Aug 7, 2018
- appended Authorization header token parsing after X-Consul-Token
- added test cases
- updated website documentation to mention Authorization header
* appended Authorization header token parsing after X-Consul-Token * added test cases * updated website documentation to mention Authorization header
@splashx please review changes made to the Consul. If you are OK with them, create PR to consul master repo. I can continue discussion with consul maintainers on hashicorp#4483 |
* improve tests * improve Bearer parsing
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.
LGTM
agent/http_test.go
Outdated
@@ -736,6 +736,19 @@ func TestACLResolution(t *testing.T) { | |||
reqBothTokens, _ := http.NewRequest("GET", "/v1/catalog/nodes?token=baz", nil) | |||
reqBothTokens.Header.Add("X-Consul-Token", "zap") | |||
|
|||
// Request with Authorization token |
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.
Add on: Authorization *Bearer* token
agent/http_test.go
Outdated
reqAuthAndXToken, _ := http.NewRequest("GET", "/v1/catalog/nodes", nil) | ||
reqAuthAndXToken.Header.Add("X-Consul-Token", "xtoken") | ||
reqAuthAndXToken.Header.Add("Authorization", "Bearer notparsed") | ||
|
||
a := NewTestAgent(t.Name(), "") |
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.
Add a test for when Authorization: Bearer
is passed (we expect the test to fail because of this line )
LGTM please merge |