From 2e41f7bb0b277d4264aab6565c978b7f83b5d5ed Mon Sep 17 00:00:00 2001
From: Matchu <matchu1993@gmail.com>
Date: Tue, 23 Nov 2021 13:00:56 -0800
Subject: [PATCH] Send `Vary: Authorization` cache header

I don't think this is actually relevant in-app right now, but I figured sending it is More Correct, and is likely to prevent future bugs if anything (and prevent future question about why we're _not_ sending it).

I also removed the `maxAge: 0` on `currentUser`, now that I've updated Fastly to no longer default to 5-minute caching when no cache time is specified. I can see why that's a reasonable default for Fastly, but we've been pretty careful about specifying Cache-Control headers when relevant, so the extra caching is mostly incorrect.
---
 src/server/index.js      | 16 ++++++++++++++++
 src/server/types/User.js | 14 ++++----------
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/src/server/index.js b/src/server/index.js
index 0174a38..32e9b0e 100644
--- a/src/server/index.js
+++ b/src/server/index.js
@@ -74,6 +74,22 @@ const config = {
       ...buildLoaders(db),
     };
   },
+  formatResponse: (res, context) => {
+    // The Authorization header can affect the response, so we signal that here
+    // for caching user data! That way, login/logout will refresh user data,
+    // even if it was briefly cached.
+    //
+    // NOTE: Our frontend JS only sends the Authorization header for user data
+    //       queries. For public data, the header will be absent, and different
+    //       users will still be able to share the same public cache data.
+    //
+    // NOTE: At time of writing, I'm not sure we use this in app? I think all
+    //       current user data queries request fields with `maxAge: 0`. But I'm
+    //       adding it just to remove a potential surprise gotcha later!
+    context.response.http.headers.set("Vary", "Authorization");
+
+    return res;
+  },
 
   plugins,
 
diff --git a/src/server/types/User.js b/src/server/types/User.js
index 9d40f1e..b34d19c 100644
--- a/src/server/types/User.js
+++ b/src/server/types/User.js
@@ -51,16 +51,10 @@ const typeDefs = gql`
     """
     The currently logged-in user.
     """
-    # Don't allow caching of *anything* nested inside currentUser, because we
-    # want logins/logouts always reset user data properly.
-    #
-    # TODO: If we wanted to privately cache a currentUser field, we could
-    #       remove the maxAge condition here, and attach user ID to the GraphQL
-    #       request URL when sending auth headers. That way, changing user
-    #       would send different requests and avoid the old cache hits. (But we
-    #       should leave the scope, to emphasize that the CDN cache shouldn't
-    #       cache it.)
-    currentUser: User @cacheControl(maxAge: 0, scope: PRIVATE)
+    # NOTE: The client might privately cache some of the data in here, which is
+    #       okay, because we set the header "Vary: Authorization", so
+    #       login/logout will change the local cache key!
+    currentUser: User @cacheControl(scope: PRIVATE)
   }
 `;