Skip to content
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

oauth2: Introspection should return token type #265

Merged
merged 2 commits into from
Apr 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ bumps (`0.1.0` -> `0.2.0`).

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

## 0.18.0

This release allows the introspection handler to return the token type (e.g. `access_token`, `refresh_token`) of the
introspected token. To achieve that, some breaking API changes have been introduced:

* `OAuth2.IntrospectToken(ctx context.Context, token string, tokenType TokenType, session Session, scope ...string) (AccessRequester, error)` is now `OAuth2.IntrospectToken(ctx context.Context, token string, tokenType TokenType, session Session, scope ...string) (TokenType, AccessRequester, error)`.
* `TokenIntrospector.IntrospectToken(ctx context.Context, token string, tokenType TokenType, accessRequest AccessRequester, scopes []string) (error)` is now `TokenIntrospector.IntrospectToken(ctx context.Context, token string, tokenType TokenType, accessRequest AccessRequester, scopes []string) (TokenType, error)`.

This patch also resolves a misconfigured json key in the `IntrospectionResponse` struct. `AccessRequester AccessRequester json:",extra"` is now properly declared as `AccessRequester AccessRequester json:"extra"`.

## 0.17.0

This release resolves a security issue (reported by [platform.sh](https://www.platform.sh)) related to potential storage implementations.
Expand Down
20 changes: 12 additions & 8 deletions handler/oauth2/introspector.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,28 +37,32 @@ type CoreValidator struct {
DisableRefreshTokenValidation bool
}

func (c *CoreValidator) IntrospectToken(ctx context.Context, token string, tokenType fosite.TokenType, accessRequest fosite.AccessRequester, scopes []string) (err error) {
func (c *CoreValidator) IntrospectToken(ctx context.Context, token string, tokenType fosite.TokenType, accessRequest fosite.AccessRequester, scopes []string) (fosite.TokenType, error) {
if c.DisableRefreshTokenValidation {
return c.introspectAccessToken(ctx, token, accessRequest, scopes)
if err := c.introspectAccessToken(ctx, token, accessRequest, scopes); err != nil {
return "", err
}
return fosite.AccessToken, nil
}

var err error
switch tokenType {
case fosite.RefreshToken:
if err = c.introspectRefreshToken(ctx, token, accessRequest, scopes); err == nil {
return nil
return fosite.RefreshToken, nil
} else if err = c.introspectAccessToken(ctx, token, accessRequest, scopes); err == nil {
return nil
return fosite.AccessToken, nil
}
return err
return "", err
}

if err = c.introspectAccessToken(ctx, token, accessRequest, scopes); err == nil {
return nil
return fosite.AccessToken, nil
} else if err := c.introspectRefreshToken(ctx, token, accessRequest, scopes); err == nil {
return nil
return fosite.RefreshToken, nil
}

return err
return "", err
}

func matchScopes(ss fosite.ScopeStrategy, granted, scopes []string) error {
Expand Down
8 changes: 4 additions & 4 deletions handler/oauth2/introspector_jwt.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,10 @@ type StatelessJWTValidator struct {
ScopeStrategy fosite.ScopeStrategy
}

func (v *StatelessJWTValidator) IntrospectToken(ctx context.Context, token string, tokenType fosite.TokenType, accessRequest fosite.AccessRequester, scopes []string) (err error) {
func (v *StatelessJWTValidator) IntrospectToken(ctx context.Context, token string, tokenType fosite.TokenType, accessRequest fosite.AccessRequester, scopes []string) (fosite.TokenType, error) {
or, err := v.JWTAccessTokenStrategy.ValidateJWT(fosite.AccessToken, token)
if err != nil {
return err
return "", err
}

for _, scope := range scopes {
Expand All @@ -50,10 +50,10 @@ func (v *StatelessJWTValidator) IntrospectToken(ctx context.Context, token strin
}

if !v.ScopeStrategy(or.GetGrantedScopes(), scope) {
return errors.WithStack(fosite.ErrInvalidScope)
return "", errors.WithStack(fosite.ErrInvalidScope)
}
}

accessRequest.Merge(or)
return nil
return "", nil
}
4 changes: 2 additions & 2 deletions handler/oauth2/introspector_jwt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func TestIntrospectJWT(t *testing.T) {
}

areq := fosite.NewAccessRequest(nil)
err := v.IntrospectToken(nil, c.token(), fosite.AccessToken, areq, c.scopes)
_, err := v.IntrospectToken(nil, c.token(), fosite.AccessToken, areq, c.scopes)

if c.expectErr != nil {
require.EqualError(t, err, c.expectErr.Error())
Expand Down Expand Up @@ -145,7 +145,7 @@ func BenchmarkIntrospectJWT(b *testing.B) {
areq := fosite.NewAccessRequest(nil)

for n := 0; n < b.N; n++ {
err = v.IntrospectToken(nil, token, fosite.AccessToken, areq, []string{})
_, err = v.IntrospectToken(nil, token, fosite.AccessToken, areq, []string{})
}

assert.NoError(b, err)
Expand Down
6 changes: 5 additions & 1 deletion handler/oauth2/introspector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/ory/fosite"
"github.com/ory/fosite/internal"
"github.com/pkg/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand All @@ -50,6 +51,7 @@ func TestIntrospectToken(t *testing.T) {
description string
setup func()
expectErr error
expectTT fosite.TokenType
}{
{
description: "should fail because no bearer token set",
Expand Down Expand Up @@ -96,16 +98,18 @@ func TestIntrospectToken(t *testing.T) {
setup: func() {
chgen.EXPECT().ValidateAccessToken(nil, areq, "1234").Return(nil)
},
expectTT: fosite.AccessToken,
},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
c.setup()
err := v.IntrospectToken(nil, fosite.AccessTokenFromRequest(httpreq), fosite.AccessToken, areq, []string{})
tt, err := v.IntrospectToken(nil, fosite.AccessTokenFromRequest(httpreq), fosite.AccessToken, areq, []string{})

if c.expectErr != nil {
require.EqualError(t, err, c.expectErr.Error())
} else {
require.NoError(t, err)
assert.Equal(t, c.expectTT, tt)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion integration/helper_endpoints_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func tokenIntrospectionHandler(t *testing.T, oauth2 fosite.OAuth2Provider, sessi
func tokenInfoHandler(t *testing.T, oauth2 fosite.OAuth2Provider, session fosite.Session) func(rw http.ResponseWriter, req *http.Request) {
return func(rw http.ResponseWriter, req *http.Request) {
ctx := fosite.NewContext()
if _, err := oauth2.IntrospectToken(ctx, fosite.AccessTokenFromRequest(req), fosite.AccessToken, session); err != nil {
if _, _, err := oauth2.IntrospectToken(ctx, fosite.AccessTokenFromRequest(req), fosite.AccessToken, session); err != nil {
t.Logf("Info request failed because `%s`.", err.Error())
// t.Logf("Stack: %s", err.(stackTracer).StackTrace())
http.Error(rw, errors.Cause(err).(*fosite.RFC6749Error).Description, errors.Cause(err).(*fosite.RFC6749Error).Code)
Expand Down
7 changes: 4 additions & 3 deletions internal/introspector.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,11 @@ func (_m *MockTokenIntrospector) EXPECT() *_MockTokenIntrospectorRecorder {
return _m.recorder
}

func (_m *MockTokenIntrospector) IntrospectToken(_param0 context.Context, _param1 string, _param2 fosite.TokenType, _param3 fosite.AccessRequester, _param4 []string) error {
func (_m *MockTokenIntrospector) IntrospectToken(_param0 context.Context, _param1 string, _param2 fosite.TokenType, _param3 fosite.AccessRequester, _param4 []string) (fosite.TokenType, error) {
ret := _m.ctrl.Call(_m, "IntrospectToken", _param0, _param1, _param2, _param3, _param4)
ret0, _ := ret[0].(error)
return ret0
ret0, _ := ret[0].(fosite.TokenType)
ret1, _ := ret[1].(error)
return ret0, ret1
}

func (_mr *_MockTokenIntrospectorRecorder) IntrospectToken(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call {
Expand Down
15 changes: 9 additions & 6 deletions introspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
)

type TokenIntrospector interface {
IntrospectToken(ctx context.Context, token string, tokenType TokenType, accessRequest AccessRequester, scopes []string) error
IntrospectToken(ctx context.Context, token string, tokenType TokenType, accessRequest AccessRequester, scopes []string) (TokenType, error)
}

func AccessTokenFromRequest(req *http.Request) string {
Expand All @@ -54,24 +54,27 @@ func AccessTokenFromRequest(req *http.Request) string {
return split[1]
}

func (f *Fosite) IntrospectToken(ctx context.Context, token string, tokenType TokenType, session Session, scopes ...string) (AccessRequester, error) {
func (f *Fosite) IntrospectToken(ctx context.Context, token string, tokenType TokenType, session Session, scopes ...string) (TokenType, AccessRequester, error) {
var found bool = false
var foundTokenType TokenType = ""

ar := NewAccessRequest(session)
for _, validator := range f.TokenIntrospectionHandlers {
if err := errors.Cause(validator.IntrospectToken(ctx, token, tokenType, ar, scopes)); err == nil {
tt, err := validator.IntrospectToken(ctx, token, tokenType, ar, scopes)
if err := errors.Cause(err); err == nil {
found = true
foundTokenType = tt
} else if err.Error() == ErrUnknownRequest.Error() {
// Nothing to do
} else if err != nil {
rfcerr := ErrorToRFC6749Error(err)
return nil, errors.WithStack(rfcerr.WithDebug("A validator returned an error"))
return "", nil, errors.WithStack(rfcerr.WithDebug("A validator returned an error"))
}
}

if !found {
return nil, errors.WithStack(ErrRequestUnauthorized.WithDebug("No validator felt responsible for validating the token"))
return "", nil, errors.WithStack(ErrRequestUnauthorized.WithDebug("No validator felt responsible for validating the token"))
}

return ar, nil
return foundTokenType, ar, nil
}
10 changes: 5 additions & 5 deletions introspect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,15 @@ func TestIntrospect(t *testing.T) {
scopes: []string{"foo"},
setup: func() {
f.TokenIntrospectionHandlers = TokenIntrospectionHandlers{validator}
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(ErrUnknownRequest)
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenType(""), ErrUnknownRequest)
},
expectErr: ErrRequestUnauthorized,
},
{
description: "should fail",
scopes: []string{"foo"},
setup: func() {
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(ErrInvalidClient)
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenType(""), ErrInvalidClient)
},
expectErr: ErrInvalidClient,
},
Expand All @@ -106,7 +106,7 @@ func TestIntrospect(t *testing.T) {
setup: func() {
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Do(func(ctx context.Context, _ string, _ TokenType, accessRequest AccessRequester, _ []string) {
accessRequest.(*AccessRequest).GrantedScopes = []string{"bar"}
}).Return(nil)
}).Return(TokenType(""), nil)
},
},
{
Expand All @@ -115,13 +115,13 @@ func TestIntrospect(t *testing.T) {
setup: func() {
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Do(func(ctx context.Context, _ string, _ TokenType, accessRequest AccessRequester, _ []string) {
accessRequest.(*AccessRequest).GrantedScopes = []string{"bar"}
}).Return(nil)
}).Return(TokenType(""), nil)
},
},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
c.setup()
_, err := f.IntrospectToken(nil, AccessTokenFromRequest(req), AccessToken, nil, c.scopes...)
_, _, err := f.IntrospectToken(nil, AccessTokenFromRequest(req), AccessToken, nil, c.scopes...)
if c.expectErr != nil {
assert.EqualError(t, err, c.expectErr.Error())
} else {
Expand Down
14 changes: 11 additions & 3 deletions introspection_request_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,10 @@ func (f *Fosite) NewIntrospectionRequest(ctx context.Context, r *http.Request, s
return &IntrospectionResponse{Active: false}, errors.WithStack(ErrRequestUnauthorized.WithDebug("Bearer and introspection token are identical"))
}

if _, err := f.IntrospectToken(ctx, clientToken, AccessToken, session.Clone()); err != nil {
if tt, _, err := f.IntrospectToken(ctx, clientToken, AccessToken, session.Clone()); err != nil {
return &IntrospectionResponse{Active: false}, errors.WithStack(ErrRequestUnauthorized.WithDebug("HTTP Authorization header missing, malformed, or credentials used are invalid"))
} else if tt != "" && tt != AccessToken {
return &IntrospectionResponse{Active: false}, errors.WithStack(ErrRequestUnauthorized.WithDebug("HTTP Authorization header did not provide a valid access token"))
}
} else {
id, secret, ok := r.BasicAuth()
Expand Down Expand Up @@ -156,20 +158,22 @@ func (f *Fosite) NewIntrospectionRequest(ctx context.Context, r *http.Request, s
}
}

ar, err := f.IntrospectToken(ctx, token, TokenType(tokenType), session, strings.Split(scope, " ")...)
tt, ar, err := f.IntrospectToken(ctx, token, TokenType(tokenType), session, strings.Split(scope, " ")...)
if err != nil {
return &IntrospectionResponse{Active: false}, errors.WithStack(ErrInactiveToken.WithDebug(fmt.Sprintf("Validator returned error %s", err.Error())))
}

return &IntrospectionResponse{
Active: true,
AccessRequester: ar,
TokenType: tt,
}, nil
}

type IntrospectionResponse struct {
Active bool `json:"active"`
AccessRequester AccessRequester `json:",extra"`
AccessRequester AccessRequester `json:"extra"`
TokenType TokenType `json:"token_type,omitempty"`
}

func (r *IntrospectionResponse) IsActive() bool {
Expand All @@ -179,3 +183,7 @@ func (r *IntrospectionResponse) IsActive() bool {
func (r *IntrospectionResponse) GetAccessRequester() AccessRequester {
return r.AccessRequester
}

func (r *IntrospectionResponse) GetTokenType() TokenType {
return r.TokenType
}
12 changes: 6 additions & 6 deletions introspection_request_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ func TestNewIntrospectionRequest(t *testing.T) {
"token": []string{"introspect-token"},
},
}
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
validator.EXPECT().IntrospectToken(nil, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(newErr)
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenType(""), nil)
validator.EXPECT().IntrospectToken(nil, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenType(""), newErr)
},
isActive: false,
expectErr: ErrInactiveToken,
Expand All @@ -106,8 +106,8 @@ func TestNewIntrospectionRequest(t *testing.T) {
"token": []string{"introspect-token"},
},
}
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
validator.EXPECT().IntrospectToken(nil, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
validator.EXPECT().IntrospectToken(nil, "some-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenType(""), nil)
validator.EXPECT().IntrospectToken(nil, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenType(""), nil)
},
isActive: true,
},
Expand All @@ -125,7 +125,7 @@ func TestNewIntrospectionRequest(t *testing.T) {
"token": []string{"introspect-token"},
},
}
validator.EXPECT().IntrospectToken(nil, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
validator.EXPECT().IntrospectToken(nil, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenType(""), nil)
},
isActive: true,
},
Expand All @@ -143,7 +143,7 @@ func TestNewIntrospectionRequest(t *testing.T) {
"token": []string{"introspect-token"},
},
}
validator.EXPECT().IntrospectToken(nil, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
validator.EXPECT().IntrospectToken(nil, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenType(""), nil)
},
isActive: true,
},
Expand Down
6 changes: 5 additions & 1 deletion oauth2.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ type OAuth2Provider interface {

// IntrospectToken returns token metadata, if the token is valid. Tokens generated by the authorization endpoint,
// such as the authorization code, can not be introspected.
IntrospectToken(ctx context.Context, token string, tokenType TokenType, session Session, scope ...string) (AccessRequester, error)
IntrospectToken(ctx context.Context, token string, tokenType TokenType, session Session, scope ...string) (TokenType, AccessRequester, error)

// NewIntrospectionRequest initiates token introspection as defined in
// https://tools.ietf.org/search/rfc7662#section-2.1
Expand All @@ -170,6 +170,10 @@ type IntrospectionResponder interface {

// AccessRequester returns nil when IsActive() is false and the original access request object otherwise.
GetAccessRequester() AccessRequester

// GetTokenType optionally returns the type of the token that was introspected. The could be "access_token", "refresh_token",
// or if the type can not be determined an empty string.
GetTokenType() TokenType
}

// Requester is an abstract interface for handling requests in Fosite.
Expand Down