-
Notifications
You must be signed in to change notification settings - Fork 359
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
Support alternative token location #271
Changes from all commits
b4d58b2
1472e65
f2432d1
ce024c9
91cf8bf
6cc7e65
55c8eff
815e2bc
5b0bd05
e710db1
594194e
27669e3
937fd44
e68a10d
4f51c8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,9 +25,25 @@ import ( | |
"strings" | ||
) | ||
|
||
func BearerTokenFromRequest(r *http.Request) string { | ||
auth := r.Header.Get("Authorization") | ||
split := strings.SplitN(auth, " ", 2) | ||
const ( | ||
defaultAuthorizationHeader = "Authorization" | ||
) | ||
|
||
type BearerTokenLocation struct { | ||
Header *string `json:"header"` | ||
QueryParameter *string `json:"query_parameter"` | ||
} | ||
|
||
func BearerTokenFromRequest(r *http.Request, tokenLocation *BearerTokenLocation) string { | ||
if tokenLocation != nil { | ||
if tokenLocation.Header != nil { | ||
return r.Header.Get(*tokenLocation.Header) | ||
} else if tokenLocation.QueryParameter != nil { | ||
return r.FormValue(*tokenLocation.QueryParameter) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible that both if / else if do not get executed? Then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is not possible, as JSON schema for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this explicit in the code by simply doing a if/else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can, but if somehow QueryParameter would be a nil, than the program will crash on dereferencing a nil pointer. With "else if" the function will simply return an empty string, like if there was no token in the configured header/query parameter (what may be a proper behavior if there was "alternative token location" set, but not configured - even though with the current schema it is not possible). I see an alternative solution here: returning an error saying that rule is misconfigured. But I would have to add error handling in the places where this function is called. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, how about simply falling back to the authorization header in the case where both are nil? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, done |
||
} | ||
token := r.Header.Get(defaultAuthorizationHeader) | ||
split := strings.SplitN(token, " ", 2) | ||
if len(split) != 2 || !strings.EqualFold(split[0], "bearer") { | ||
return "" | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
package helper_test | ||
|
||
import ( | ||
"net/http" | ||
"testing" | ||
|
||
"github.com/ory/oathkeeper/helper" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
const ( | ||
defaultHeaderName = "Authorization" | ||
) | ||
|
||
func TestBearerTokenFromRequest(t *testing.T) { | ||
t.Run("case=token should be received from default header if custom location is not set and token is present", func(t *testing.T) { | ||
expectedToken := "token" | ||
request := &http.Request{Header: http.Header{defaultHeaderName: {"bearer " + expectedToken}}} | ||
token := helper.BearerTokenFromRequest(request, nil) | ||
assert.Equal(t, expectedToken, token) | ||
}) | ||
t.Run("case=should return empty string if custom location is not set and token is not present in default header", func(t *testing.T) { | ||
request := &http.Request{} | ||
token := helper.BearerTokenFromRequest(request, nil) | ||
assert.Empty(t, token) | ||
}) | ||
t.Run("case=should return empty string if custom location is set to header and token is not present in that header", func(t *testing.T) { | ||
customHeaderName := "Custom-Auth-Header" | ||
request := &http.Request{Header: http.Header{defaultHeaderName: {"bearer token"}}} | ||
tokenLocation := helper.BearerTokenLocation{Header: &customHeaderName} | ||
token := helper.BearerTokenFromRequest(request, &tokenLocation) | ||
assert.Empty(t, token) | ||
}) | ||
t.Run("case=should return empty string if custom location is set to query parameter and token is not present in that query parameter", func(t *testing.T) { | ||
customQueryParameterName := "Custom-Auth" | ||
request := &http.Request{Header: http.Header{defaultHeaderName: {"bearer token"}}} | ||
tokenLocation := helper.BearerTokenLocation{QueryParameter: &customQueryParameterName} | ||
token := helper.BearerTokenFromRequest(request, &tokenLocation) | ||
assert.Empty(t, token) | ||
}) | ||
t.Run("case=token should be received from custom header if custom location is set to header and token is present", func(t *testing.T) { | ||
expectedToken := "token" | ||
customHeaderName := "Custom-Auth-Header" | ||
request := &http.Request{Header: http.Header{customHeaderName: {expectedToken}}} | ||
tokenLocation := helper.BearerTokenLocation{Header: &customHeaderName} | ||
token := helper.BearerTokenFromRequest(request, &tokenLocation) | ||
assert.Equal(t, expectedToken, token) | ||
}) | ||
t.Run("case=token should be received from custom header if custom location is set to query parameter and token is present", func(t *testing.T) { | ||
expectedToken := "token" | ||
customQueryParameterName := "Custom-Auth" | ||
request := &http.Request{ | ||
Form: map[string][]string{ | ||
customQueryParameterName: []string{expectedToken}, | ||
}, | ||
} | ||
tokenLocation := helper.BearerTokenLocation{QueryParameter: &customQueryParameterName} | ||
token := helper.BearerTokenFromRequest(request, &tokenLocation) | ||
assert.Equal(t, expectedToken, token) | ||
}) | ||
t.Run("case=token should be received from default header if custom token location is set, but neither Header nor Query Param is configured", func(t *testing.T) { | ||
expectedToken := "token" | ||
request := &http.Request{Header: http.Header{defaultHeaderName: {"bearer " + expectedToken}}} | ||
tokenLocation := helper.BearerTokenLocation{} | ||
token := helper.BearerTokenFromRequest(request, &tokenLocation) | ||
assert.Equal(t, expectedToken, 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.
Would it make sense to test this independently?
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.
Yeah. That might be a good idea :)
I added tests.