oauth2: Resolves several issues related to revokation (#281)

This patch resolves several issues related to token revokation as well as duplicate authorize code usage:

* oauth2: Revoking access or refresh tokens should revoke past and future tokens too
* oauth2: Revoke access and refresh tokens when authorize code is used twice

Additionally, this patch resolves an issue where refreshing a token would not revoke previous tokens.

Closes #278
Closes #280

HISTORY.md

@@ -43,6 +43,55 @@ bumps (`0.1.0` -> `0.2.0`).
 
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->
 
+## 0.20.0
+
+This release implements an OAuth 2.0 Best Practice with regards to revoking already issued access and refresh tokens
+if an authorization code is used more than one time.
+
+## Breaking Changes
+
+### JWT Claims
+
+- `github.com/ory/fosite/token/jwt.JWTClaims.Audience` is no longer a `string`, but a string slice `[]string`.
+- `github.com/ory/fosite/handler/openid.IDTokenClaims` is no longer a `string`, but a string slice `[]string`.
+
+### `AuthorizeCodeStorage`
+
+This improves security as, in the event of an authorization code being leaked, all associated tokens are revoked. To
+implement this feature, a breaking change had to be introduced. The `github.com/ory/fosite/handler/oauth2.AuthorizeCodeStorage`
+interface changed as follows:
+
+- `DeleteAuthorizeCodeSession(ctx context.Context, code string) (err error)` has been removed from the interface and
+is no longer used by this library.
+- `InvalidateAuthorizeCodeSession(ctx context.Context, code string) (err error)` has been introduced.
+- The error `github.com/ory/fosite/handler/oauth2.ErrInvalidatedAuthorizeCode` has been added.
+
+The following documentation sheds light on how you should update your storage adapter:
+
+```
+// ErrInvalidatedAuthorizeCode is an error indicating that an authorization code has been
+// used previously.
+var ErrInvalidatedAuthorizeCode = errors.New("Authorization code has ben invalidated")
+
+// AuthorizeCodeStorage handles storage requests related to authorization codes.
+type AuthorizeCodeStorage interface {
+	// GetAuthorizeCodeSession stores the authorization request for a given authorization code.
+	CreateAuthorizeCodeSession(ctx context.Context, code string, request fosite.Requester) (err error)
+
+	// GetAuthorizeCodeSession hydrates the session based on the given code and returns the authorization request.
+	// If the authorization code has been invalidated with `InvalidateAuthorizeCodeSession`, this
+	// method should return the ErrInvalidatedAuthorizeCode error.
+	//
+	// Make sure to also return the fosite.Requester value when returning the ErrInvalidatedAuthorizeCode error!
+	GetAuthorizeCodeSession(ctx context.Context, code string, session fosite.Session) (request fosite.Requester, err error)
+
+	// InvalidateAuthorizeCodeSession is called when an authorize code is being used. The state of the authorization
+	// code should be set to invalid and consecutive requests to GetAuthorizeCodeSession should return the
+	// ErrInvalidatedAuthorizeCode error.
+	InvalidateAuthorizeCodeSession(ctx context.Context, code string) (err error)
+}
+```
+
 ## 0.19.0
 
 This release improves the OpenID Connect vaildation strategy which now properly handles `prompt`, `max_age`, and `id_token_hint`

compose/compose_oauth2.go

@@ -29,14 +29,14 @@ import (
 // an access token, refresh token and authorize code validator.
 func OAuth2AuthorizeExplicitFactory(config *Config, storage interface{}, strategy interface{}) interface{} {
 	return &oauth2.AuthorizeExplicitGrantHandler{
-		AccessTokenStrategy:   strategy.(oauth2.AccessTokenStrategy),
-		RefreshTokenStrategy:  strategy.(oauth2.RefreshTokenStrategy),
-		AuthorizeCodeStrategy: strategy.(oauth2.AuthorizeCodeStrategy),
-		CoreStorage:           storage.(oauth2.CoreStorage),
-		AuthCodeLifespan:      config.GetAuthorizeCodeLifespan(),
-		AccessTokenLifespan:   config.GetAccessTokenLifespan(),
-		ScopeStrategy:         config.GetScopeStrategy(),
-		//TokenRevocationStorage: storage.(oauth2.TokenRevocationStorage),
+		AccessTokenStrategy:    strategy.(oauth2.AccessTokenStrategy),
+		RefreshTokenStrategy:   strategy.(oauth2.RefreshTokenStrategy),
+		AuthorizeCodeStrategy:  strategy.(oauth2.AuthorizeCodeStrategy),
+		CoreStorage:            storage.(oauth2.CoreStorage),
+		AuthCodeLifespan:       config.GetAuthorizeCodeLifespan(),
+		AccessTokenLifespan:    config.GetAccessTokenLifespan(),
+		ScopeStrategy:          config.GetScopeStrategy(),
+		TokenRevocationStorage: storage.(oauth2.TokenRevocationStorage),
 	}
 }
 

errors.go

@@ -29,7 +29,10 @@ import (
 )
 
 var (
-	ErrUnknownRequest = &RFC6749Error{
+	// ErrInvalidatedAuthorizeCode is an error indicating that an authorization code has been
+	// used previously.
+	ErrInvalidatedAuthorizeCode = errors.New("Authorization code has ben invalidated")
+	ErrUnknownRequest           = &RFC6749Error{
 		Name:        errUnknownErrorName,
 		Description: "The handler is not responsible for this request",
 		Code:        http.StatusBadRequest,

handler/oauth2/flow_authorize_code_auth.go

@@ -53,6 +53,8 @@ type AuthorizeExplicitGrantHandler struct {
 	// SanitationWhiteList is a whitelist of form values that are required by the token endpoint. These values
 	// are safe for storage in a database (cleartext).
 	SanitationWhiteList []string
+
+	TokenRevocationStorage TokenRevocationStorage
 }
 
 func (c *AuthorizeExplicitGrantHandler) HandleAuthorizeEndpointRequest(ctx context.Context, ar fosite.AuthorizeRequester, resp fosite.AuthorizeResponder) error {

handler/oauth2/flow_authorize_code_token.go

@@ -45,20 +45,25 @@ func (c *AuthorizeExplicitGrantHandler) HandleTokenEndpointRequest(ctx context.C
 	code := request.GetRequestForm().Get("code")
 	signature := c.AuthorizeCodeStrategy.AuthorizeCodeSignature(code)
 	authorizeRequest, err := c.CoreStorage.GetAuthorizeCodeSession(ctx, signature, request.GetSession())
-	if err != nil && errors.Cause(err).Error() == fosite.ErrNotFound.Error() {
-		// If an authorize code is used twice (which is likely the case here), we should try and invalidate any previously
-		// issued access and refresh tokens.
-		// reqID := authorizeRequest.GetID()
-		//
-		var debug string
-		// if revErr := c.TokenRevocationStorage.RevokeAccessToken(ctx, reqID); revErr != nil {
-		// 	debug += revErr.Error() + "\n"
-		// }
-		// if revErr := c.TokenRevocationStorage.RevokeRefreshToken(ctx, reqID); revErr != nil {
-		//	debug += revErr.Error() + "\n"
-		// }
+	if errors.Cause(err) == fosite.ErrInvalidatedAuthorizeCode {
+		if authorizeRequest == nil {
+			return fosite.ErrServerError.
+				WithDescription("Misconfigured code lead to an error that prohibited the OAuth 2.0 Framework from processing this request.").
+				WithDebug("GetAuthorizeCodeSession must return a value for fosite.Requester when returning ErrInvalidatedAuthorizeCode.")
+		}
 
+		//If an authorize code is used twice, we revoke all refresh and access tokens associated with this request.
+		reqID := authorizeRequest.GetID()
+		debug := "The authorization code has already been used."
+		if revErr := c.TokenRevocationStorage.RevokeAccessToken(ctx, reqID); revErr != nil {
+			debug += "Additionally, an error occurred during processing the access token revocation: " + revErr.Error() + "."
+		}
+		if revErr := c.TokenRevocationStorage.RevokeRefreshToken(ctx, reqID); revErr != nil {
+			debug += "Additionally, an error occurred during processing the refresh token revocation: " + revErr.Error() + "."
+		}
 		return errors.WithStack(fosite.ErrInvalidGrant.WithDebug(debug))
+	} else if err != nil && errors.Cause(err).Error() == fosite.ErrNotFound.Error() {
+		return errors.WithStack(fosite.ErrInvalidGrant.WithDebug(err.Error()))
 	} else if err != nil {
 		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
 	}
@@ -133,7 +138,7 @@ func (c *AuthorizeExplicitGrantHandler) PopulateTokenEndpointResponse(ctx contex
 		}
 	}
 
-	if err := c.CoreStorage.DeleteAuthorizeCodeSession(ctx, signature); err != nil {
+	if err := c.CoreStorage.InvalidateAuthorizeCodeSession(ctx, signature); err != nil {
 		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
 	} else if err := c.CoreStorage.CreateAccessTokenSession(ctx, accessSignature, requester.Sanitize([]string{})); err != nil {
 		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))

handler/oauth2/flow_authorize_code_token_test.go

@@ -149,9 +149,9 @@ func TestAuthorizeCode_PopulateTokenEndpointResponse(t *testing.T) {
 					err := h.PopulateTokenEndpointResponse(nil, c.areq, aresp)
 
 					if c.expectErr != nil {
-						require.EqualError(t, errors.Cause(err), c.expectErr.Error(), "%v", err)
+						require.EqualError(t, errors.Cause(err), c.expectErr.Error(), "%+v", err)
 					} else {
-						require.NoError(t, err, "%v", err)
+						require.NoError(t, err, "%+v", err)
 					}
 
 					if c.check != nil {
@@ -171,11 +171,11 @@ func TestAuthorizeCode_HandleTokenEndpointRequest(t *testing.T) {
 			store := storage.NewMemoryStore()
 
 			h := AuthorizeExplicitGrantHandler{
-				CoreStorage:           store,
-				AuthorizeCodeStrategy: hmacshaStrategy,
-				ScopeStrategy:         fosite.HierarchicScopeStrategy,
-				//TokenRevocationStorage: store,
-				AuthCodeLifespan: time.Minute,
+				CoreStorage:            store,
+				AuthorizeCodeStrategy:  hmacshaStrategy,
+				ScopeStrategy:          fosite.HierarchicScopeStrategy,
+				TokenRevocationStorage: store,
+				AuthCodeLifespan:       time.Minute,
 			}
 			for i, c := range []struct {
 				areq        *fosite.AccessRequest
@@ -311,6 +311,30 @@ func TestAuthorizeCode_HandleTokenEndpointRequest(t *testing.T) {
 						require.NoError(t, store.CreateAuthorizeCodeSession(nil, signature, authreq))
 					},
 				},
+				{
+					areq: &fosite.AccessRequest{
+						GrantTypes: fosite.Arguments{"authorization_code"},
+						Request: fosite.Request{
+							Form: url.Values{},
+							Client: &fosite.DefaultClient{
+								GrantTypes: fosite.Arguments{"authorization_code"},
+							},
+							GrantedScopes: fosite.Arguments{"foo", "offline"},
+							Session:       &fosite.DefaultSession{},
+							RequestedAt:   time.Now().UTC(),
+						},
+					},
+					setup: func(t *testing.T, areq *fosite.AccessRequest, authreq *fosite.AuthorizeRequest) {
+						code, sig, err := strategy.GenerateAuthorizeCode(nil, nil)
+						require.NoError(t, err)
+						areq.Form.Add("code", code)
+
+						require.NoError(t, store.CreateAuthorizeCodeSession(nil, sig, areq))
+						require.NoError(t, store.InvalidateAuthorizeCodeSession(nil, sig))
+					},
+					description: "should fail because code has been used already",
+					expectErr:   fosite.ErrInvalidGrant,
+				},
 			} {
 				t.Run(fmt.Sprintf("case=%d/description=%s", i, c.description), func(t *testing.T) {
 					if c.setup != nil {
@@ -321,9 +345,9 @@ func TestAuthorizeCode_HandleTokenEndpointRequest(t *testing.T) {
 
 					err := h.HandleTokenEndpointRequest(context.Background(), c.areq)
 					if c.expectErr != nil {
-						require.EqualError(t, errors.Cause(err), c.expectErr.Error())
+						require.EqualError(t, errors.Cause(err), c.expectErr.Error(), "%+v", err)
 					} else {
-						require.NoError(t, err)
+						require.NoError(t, err, "%+v", err)
 					}
 				})
 			}

handler/oauth2/flow_refresh.go

@@ -101,15 +101,20 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con
 	}
 
 	signature := c.RefreshTokenStrategy.RefreshTokenSignature(requester.GetRequestForm().Get("refresh_token"))
-	if ts, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, nil); err != nil {
+	ts, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, nil)
+	if err != nil {
 		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
 	} else if err := c.TokenRevocationStorage.RevokeAccessToken(ctx, ts.GetID()); err != nil {
 		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
 	} else if err := c.TokenRevocationStorage.RevokeRefreshToken(ctx, ts.GetID()); err != nil {
 		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
-	} else if err := c.TokenRevocationStorage.CreateAccessTokenSession(ctx, accessSignature, requester.Sanitize([]string{})); err != nil {
+	}
+
+	storeReq := requester.Sanitize([]string{})
+	storeReq.SetID(ts.GetID())
+	if err := c.TokenRevocationStorage.CreateAccessTokenSession(ctx, accessSignature, storeReq); err != nil {
 		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
-	} else if err := c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, requester.Sanitize([]string{})); err != nil {
+	} else if err := c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
 		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
 	}
 

handler/oauth2/flow_refresh_test.go

@@ -190,6 +190,7 @@ func TestRefreshFlow_PopulateTokenEndpointResponse(t *testing.T) {
 				{
 					description: "should pass",
 					setup: func() {
+						areq.ID = "req-id"
 						areq.GrantTypes = fosite.Arguments{"refresh_token"}
 						areq.Scopes = fosite.Arguments{"foo", "bar"}
 						areq.GrantedScopes = fosite.Arguments{"foo", "bar"}
@@ -206,6 +207,7 @@ func TestRefreshFlow_PopulateTokenEndpointResponse(t *testing.T) {
 						_, err := store.GetRefreshTokenSession(nil, signature, nil)
 						require.Error(t, err)
 
+						assert.Equal(t, "req-id", areq.ID)
 						require.NoError(t, strategy.ValidateAccessToken(nil, areq, aresp.GetAccessToken()))
 						require.NoError(t, strategy.ValidateRefreshToken(nil, areq, aresp.ToMap()["refresh_token"].(string)))
 						assert.Equal(t, "bearer", aresp.GetTokenType())

handler/oauth2/storage.go

@@ -33,12 +33,22 @@ type CoreStorage interface {
 	RefreshTokenStorage
 }
 
+// AuthorizeCodeStorage handles storage requests related to authorization codes.
 type AuthorizeCodeStorage interface {
+	// GetAuthorizeCodeSession stores the authorization request for a given authorization code.
 	CreateAuthorizeCodeSession(ctx context.Context, code string, request fosite.Requester) (err error)
 
+	// GetAuthorizeCodeSession hydrates the session based on the given code and returns the authorization request.
+	// If the authorization code has been invalidated with `InvalidateAuthorizeCodeSession`, this
+	// method should return the ErrInvalidatedAuthorizeCode error.
+	//
+	// Make sure to also return the fosite.Requester value when returning the fosite.ErrInvalidatedAuthorizeCode error!
 	GetAuthorizeCodeSession(ctx context.Context, code string, session fosite.Session) (request fosite.Requester, err error)
 
-	DeleteAuthorizeCodeSession(ctx context.Context, code string) (err error)
+	// InvalidateAuthorizeCodeSession is called when an authorize code is being used. The state of the authorization
+	// code should be set to invalid and consecutive requests to GetAuthorizeCodeSession should return the
+	// ErrInvalidatedAuthorizeCode error.
+	InvalidateAuthorizeCodeSession(ctx context.Context, code string) (err error)
 }
 
 type AccessTokenStorage interface {

handler/openid/flow_refresh_token.go

@@ -57,6 +57,7 @@ func (c *OpenIDConnectRefreshHandler) HandleTokenEndpointRequest(ctx context.Con
 
 	// We need to reset the expires at value
 	sess.IDTokenClaims().ExpiresAt = time.Time{}
+	sess.IDTokenClaims().Nonce = ""
 	return nil
 }
 

integration/authorize_code_grant_test.go

@@ -43,6 +43,13 @@ func TestAuthorizeCodeFlow(t *testing.T) {
 		runAuthorizeCodeGrantTest(t, strategy)
 	}
 }
+func TestAuthorizeCodeFlowDupeCode(t *testing.T) {
+	for _, strategy := range []oauth2.AccessTokenStrategy{
+		hmacStrategy,
+	} {
+		runAuthorizeCodeGrantDupeCodeTest(t, strategy)
+	}
+}
 
 func runAuthorizeCodeGrantTest(t *testing.T, strategy interface{}) {
 	f := compose.Compose(new(compose.Config), fositeStore, strategy, nil, compose.OAuth2AuthorizeExplicitFactory, compose.OAuth2TokenIntrospectionFactory)
@@ -87,3 +94,38 @@ func runAuthorizeCodeGrantTest(t *testing.T, strategy interface{}) {
 		})
 	}
 }
+
+func runAuthorizeCodeGrantDupeCodeTest(t *testing.T, strategy interface{}) {
+	f := compose.Compose(new(compose.Config), fositeStore, strategy, nil, compose.OAuth2AuthorizeExplicitFactory, compose.OAuth2TokenIntrospectionFactory)
+	ts := mockServer(t, f, &fosite.DefaultSession{})
+	defer ts.Close()
+
+	oauthClient := newOAuth2Client(ts)
+	fositeStore.Clients["my-client"].RedirectURIs[0] = ts.URL + "/callback"
+
+	oauthClient = newOAuth2Client(ts)
+	state := "12345678901234567890"
+
+	resp, err := http.Get(oauthClient.AuthCodeURL(state))
+	require.NoError(t, err)
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+
+	token, err := oauthClient.Exchange(goauth.NoContext, resp.Request.URL.Query().Get("code"))
+	require.NoError(t, err)
+	require.NotEmpty(t, token.AccessToken)
+
+	req, err := http.NewRequest("GET", ts.URL+"/info", nil)
+	require.NoError(t, err)
+	req.Header.Set("Authorization", "Bearer "+token.AccessToken)
+
+	resp, err = http.DefaultClient.Do(req)
+	require.NoError(t, err)
+	require.Equal(t, http.StatusNoContent, resp.StatusCode)
+
+	_, err = oauthClient.Exchange(goauth.NoContext, resp.Request.URL.Query().Get("code"))
+	require.Error(t, err)
+
+	resp, err = http.DefaultClient.Get(ts.URL + "/info")
+	require.NoError(t, err)
+	require.Equal(t, http.StatusUnauthorized, resp.StatusCode)
+}