From f234e3707ef3961d4980009a3f0149b3645779f7 Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Tue, 5 Sep 2017 23:50:07 -0700 Subject: [PATCH] server: fix panic caused by deleting refresh token twice through api --- server/api.go | 21 ++++++++++++++++++--- server/api_test.go | 16 +++++++++++++++- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/server/api.go b/server/api.go index 15266994..6aecef59 100644 --- a/server/api.go +++ b/server/api.go @@ -266,12 +266,20 @@ func (d dexAPI) RevokeRefresh(ctx context.Context, req *api.RevokeRefreshReq) (* return nil, err } - var refreshID string + var ( + refreshID string + notFound bool + ) updater := func(old storage.OfflineSessions) (storage.OfflineSessions, error) { - if refreshID = old.Refresh[req.ClientId].ID; refreshID == "" { - return old, fmt.Errorf("user does not have a refresh token for the client = %s", req.ClientId) + refreshRef := old.Refresh[req.ClientId] + if refreshRef == nil || refreshRef.ID == "" { + d.logger.Errorf("api: refresh token issued to client %q for user %q not found for deletion", req.ClientId, id.UserId) + notFound = true + return old, storage.ErrNotFound } + refreshID = refreshRef.ID + // Remove entry from Refresh list of the OfflineSession object. delete(old.Refresh, req.ClientId) @@ -286,7 +294,14 @@ func (d dexAPI) RevokeRefresh(ctx context.Context, req *api.RevokeRefreshReq) (* return nil, err } + if notFound { + return &api.RevokeRefreshResp{NotFound: true}, nil + } + // Delete the refresh token from the storage + // + // TODO(ericchiang): we don't have any good recourse if this call fails. + // Consider garbage collection of refresh tokens with no associated ref. if err := d.s.DeleteRefresh(refreshID); err != nil { d.logger.Errorf("failed to delete refresh token: %v", err) return nil, err diff --git a/server/api_test.go b/server/api_test.go index bea5d225..697d8d76 100644 --- a/server/api_test.go +++ b/server/api_test.go @@ -267,9 +267,23 @@ func TestRefreshToken(t *testing.T) { } resp, err := client.RevokeRefresh(ctx, &revokeReq) - if err != nil || resp.NotFound { + if err != nil { t.Fatalf("Unable to revoke refresh tokens for user: %v", err) } + if resp.NotFound { + t.Errorf("refresh token session wasn't found") + } + + // Try to delete again. + // + // See https://github.com/coreos/dex/issues/1055 + resp, err = client.RevokeRefresh(ctx, &revokeReq) + if err != nil { + t.Fatalf("Unable to revoke refresh tokens for user: %v", err) + } + if !resp.NotFound { + t.Errorf("refresh token session was found") + } if resp, _ := client.ListRefresh(ctx, &listReq); len(resp.RefreshTokens) != 0 { t.Fatalf("Refresh token returned inspite of revoking it.")