From fec24d307e2e18fd778fd071c413ed9bda3f05d8 Mon Sep 17 00:00:00 2001
From: Matt Moyer <moyerm@vmware.com>
Date: Tue, 6 Apr 2021 11:18:51 -0500
Subject: [PATCH] Fix missing normalization in pkg/oidcclient/filesession.

We have some nice normalization code in this package to remove expired or otherwise malformed cache entries, but we weren't calling it in the appropriate place.

Added calls to normalize the cache data structure before and after each transaction, and added test cases to ensure that it's being called.

Signed-off-by: Matt Moyer <moyerm@vmware.com>
---
 pkg/oidcclient/filesession/filesession.go     |  8 ++-
 .../filesession/filesession_test.go           | 64 ++++++++++++++++++-
 2 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/pkg/oidcclient/filesession/filesession.go b/pkg/oidcclient/filesession/filesession.go
index 151fde71..3417a123 100644
--- a/pkg/oidcclient/filesession/filesession.go
+++ b/pkg/oidcclient/filesession/filesession.go
@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package filesession implements a simple YAML file-based login.sessionCache.
@@ -137,9 +137,15 @@ func (c *Cache) withCache(transact func(*sessionCache)) {
 		cache = emptySessionCache()
 	}
 
+	// Normalize the cache before modifying it, to remove any entries that have already expired.
+	cache = cache.normalized()
+
 	// Process/mutate the session using the provided function.
 	transact(cache)
 
+	// Normalize again to put everything into a known order.
+	cache = cache.normalized()
+
 	// Marshal the session back to YAML and save it to the file.
 	if err := cache.writeTo(c.path); err != nil {
 		c.errReporter(fmt.Errorf("could not write session cache: %w", err))
diff --git a/pkg/oidcclient/filesession/filesession_test.go b/pkg/oidcclient/filesession/filesession_test.go
index 2ba7c55b..4b7f8b0b 100644
--- a/pkg/oidcclient/filesession/filesession_test.go
+++ b/pkg/oidcclient/filesession/filesession_test.go
@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package filesession
@@ -125,6 +125,41 @@ func TestGetToken(t *testing.T) {
 			},
 			wantErrors: []string{},
 		},
+		{
+			name: "valid file but expired cache hit",
+			makeTestFile: func(t *testing.T, tmp string) {
+				validCache := emptySessionCache()
+				validCache.insert(sessionEntry{
+					Key: oidcclient.SessionCacheKey{
+						Issuer:      "test-issuer",
+						ClientID:    "test-client-id",
+						Scopes:      []string{"email", "offline_access", "openid", "profile"},
+						RedirectURI: "http://localhost:0/callback",
+					},
+					CreationTimestamp: metav1.NewTime(now.Add(-2 * time.Hour)),
+					LastUsedTimestamp: metav1.NewTime(now.Add(-1 * time.Hour)),
+					Tokens: oidctypes.Token{
+						AccessToken: &oidctypes.AccessToken{
+							Token:  "test-access-token",
+							Type:   "Bearer",
+							Expiry: metav1.NewTime(now.Add(-1 * time.Hour)),
+						},
+						IDToken: &oidctypes.IDToken{
+							Token:  "test-id-token",
+							Expiry: metav1.NewTime(now.Add(-1 * time.Hour)),
+						},
+					},
+				})
+				require.NoError(t, validCache.writeTo(tmp))
+			},
+			key: oidcclient.SessionCacheKey{
+				Issuer:      "test-issuer",
+				ClientID:    "test-client-id",
+				Scopes:      []string{"email", "offline_access", "openid", "profile"},
+				RedirectURI: "http://localhost:0/callback",
+			},
+			wantErrors: []string{},
+		},
 		{
 			name: "valid file with cache hit",
 			makeTestFile: func(t *testing.T, tmp string) {
@@ -261,6 +296,33 @@ func TestPutToken(t *testing.T) {
 						},
 					},
 				})
+
+				// Insert another entry that hasn't been used for over 90 days.
+				validCache.insert(sessionEntry{
+					Key: oidcclient.SessionCacheKey{
+						Issuer:      "test-issuer-2",
+						ClientID:    "test-client-id-2",
+						Scopes:      []string{"email", "offline_access", "openid", "profile"},
+						RedirectURI: "http://localhost:0/callback",
+					},
+					CreationTimestamp: metav1.NewTime(now.Add(-95 * 24 * time.Hour)),
+					LastUsedTimestamp: metav1.NewTime(now.Add(-91 * 24 * time.Hour)),
+					Tokens: oidctypes.Token{
+						AccessToken: &oidctypes.AccessToken{
+							Token:  "old-access-token2",
+							Type:   "Bearer",
+							Expiry: metav1.NewTime(now.Add(-1 * time.Hour)),
+						},
+						IDToken: &oidctypes.IDToken{
+							Token:  "old-id-token2",
+							Expiry: metav1.NewTime(now.Add(-1 * time.Hour)),
+						},
+						RefreshToken: &oidctypes.RefreshToken{
+							Token: "old-refresh-token2",
+						},
+					},
+				})
+
 				require.NoError(t, os.MkdirAll(filepath.Dir(tmp), 0700))
 				require.NoError(t, validCache.writeTo(tmp))
 			},