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

@@ -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))

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))
 			},