Merge pull request #1056 from ericchiang/fix-api-panic

server: fix panic caused by deleting refresh token twice through api

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 == "" {
+		refreshRef := old.Refresh[req.ClientId]
-			return old, fmt.Errorf("user does not have a refresh token for the client = %s", 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

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.")