Investigate stateless pages in AMO app

willdurand commented 4 years ago

The new "blocked" pages are currently "stateful", i.e. authenticated users will remain authenticated on these pages and the HTML will be slightly different between users or between an authenticated user and an anonymous one for instance.

This is because of how the amo app works. When loading the page, we try to fetch the current user profile if we find an auth cookie/token. This is a good strategy for most (all?) of our pages because we usually have user-dependent content (like forms/buttons for authenticated users, menus, etc.).

The amo app returns cache-control: no-store (and no other relevant caching headers AFAIK) because:

pages are different depending on the user (authenticated vs anonymous) as mentioned previously
the redux state is serialized into the HTML and the auth token is available there (because "SSR")

That is why we cannot currently cache the AMO pages at the nginx level (or at any other proxy level).

That being said, the "blocked" pages could be considered "static" pages and it would make sense to aggressively cache them since these pages will likely never change once they are generated.

This issue is about determining whether we could mark specific pages as "stateless", which could then be cached (because the HTML would only contain public data and it would be the same for all users).

We also need to think about the UI because an authenticated user landing on a blocked page will appear as not authenticated depending on what we do (but that problem could be tackled later).

For QA: please check all the pages of the site. All of them should authenticate the user (when performing full page reloads) except for the new "blocked" pages. For these blocked pages, the layout should be a bit different (no authenticate button or menu in the top header). Navigating from a blocked page should result in a full page reload (and so a logged-in user should have the menu on the top header).

I requested extra QA because this patch impacts all the pages of AMO (the HTML disco pane is not affected, in theory).

bqbn commented 4 years ago

The frontend can return a different cache-control header depending on which page the client tries to visit, right? For example, if the page is /blocked-addon/, then even if the addons-server returns cache-control: no-store, the frontend can change it, maybe?

willdurand commented 4 years ago

The frontend can return a different cache-control header depending on which page the client tries to visit, right? For example, if the page is /blocked-addon/, then even if the addons-server returns cache-control: no-store, the frontend can change it, maybe?

The header value isn't the main problem I think. The HTML will still contain the auth token of the first authenticated user who warms up the cache for instance.

willdurand commented 4 years ago

Here is a sketch of patch that would allow us to mark some pages (paths actually) as stateless:

diff --git a/config/default.js b/config/default.js
index 9599f0e5f..11dd0485d 100644
--- a/config/default.js
+++ b/config/default.js
@@ -95,6 +95,11 @@ module.exports = {
   // client caching.
   faviconVersion: 2,

+  // URL patterns of stateless pages.
+  statelessUrlPatterns: [
+    '\/blocked-addon\/',
+  ],
+
   // The keys listed here will be exposed on the client.
   // Since by definition client-side code is public these config keys
   // must not contain sensitive data.
diff --git a/src/core/server/base.js b/src/core/server/base.js
index a41a595f9..0adeb0a1b 100644
--- a/src/core/server/base.js
+++ b/src/core/server/base.js
@@ -288,6 +288,13 @@ function baseServer(
         webpackIsomorphicTools.refresh();
       }

+      const isPageStateless =
+        appName === 'amo' &&
+        config
+          .get('statelessUrlPatterns')
+          .filter((pattern) => new RegExp(pattern).test(req.originalUrl))
+          .length !== 0;
+
       // Make sure the initial page does not get stored. Specifically, we
       // don't want the auth token in Redux state to hang around.
       // See https://github.com/mozilla/addons-frontend/issues/6217
@@ -296,7 +303,7 @@ function baseServer(
       // only affect how the browser loads the page when clicking
       // the back button.
       //
-      const cacheControl = ['no-store'];
+      const cacheControl = isPageStateless ? ['public'] : ['no-store'];

       const cacheAllResponsesFor = config.get('cacheAllResponsesFor');
       if (cacheAllResponsesFor) {
@@ -312,7 +319,9 @@ function baseServer(
         cacheControl.push(`max-age=${cacheAllResponsesFor}`);
       }

-      res.set('Cache-Control', cacheControl.join(', '));
+      if (cacheControl.length) {
+        res.set('Cache-Control', cacheControl.join(', '));
+      }

       // Vary the cache on Do Not Track headers.
       res.vary('DNT');
@@ -341,16 +350,18 @@ function baseServer(
         }
         runningSagas = sagaMiddleware.run(sagas);

-        // TODO: synchronize cookies with Redux store more automatically.
-        // See https://github.com/mozilla/addons-frontend/issues/5617
-        const token = req.universalCookies.get(config.get('cookieName'));
-        if (token) {
-          store.dispatch(setAuthToken(token));
-        } else {
-          // We only need to do this without a token because the user login
-          // saga already sets the site status (the Users API returns site
-          // status in its response).
-          store.dispatch(fetchSiteStatus());
+        if (!isPageStateless) {
+          // TODO: synchronize cookies with Redux store more automatically.
+          // See https://github.com/mozilla/addons-frontend/issues/5617
+          const token = req.universalCookies.get(config.get('cookieName'));
+          if (token) {
+            store.dispatch(setAuthToken(token));
+          } else {
+            // We only need to do this without a token because the user login
+            // saga already sets the site status (the Users API returns site
+            // status in its response).
+            store.dispatch(fetchSiteStatus());
+          }
         }

         if (

Why this patch, it should be possible to aggressively cache the new blocked pages. That being said, the UI is a bit confusing because the page shows the anonymous state, which is expected but still.. In other words, authenticated users would not be authenticated on these pages.

The other problem is that navigating to another page will keep the anonymous state, even for authenticated users. That is because authentication is performed on the (addons-frontend) server. To fix this, we would have to find a way to prevent react-router to perform client side navigation (so that all links will trigger a full page reload).

@mozilla/addons-engineering WDYT?

eviljeff commented 4 years ago

Why this patch, it should be possible to aggressively cache the new blocked pages. That being said, the UI is a bit confusing because the page shows the anonymous state, which is expected but still.. In other words, authenticated users would not be authenticated on these pages.

That's probably fine. It'd be better if we just didn't show the login button rather than indicate they were logged out though.

The other problem is that navigating to another page will keep the anonymous state, even for authenticated users. That is because authentication is performed on the (addons-frontend) server.

That's more the problem imo. It will be like just going to a /blocked-addon page logs them out... except if they force a full reload of the page or manually type a URL in they're suddenly logged in again.

willdurand commented 4 years ago

That's probably fine. It'd be better if we just didn't show the login button rather than indicate they were logged out though.

Yeah, I removed the auth button/menu.

That's more the problem imo. It will be like just going to a /blocked-addon page logs them out... except if they force a full reload of the page or manually type a URL in they're suddenly logged in again.

I fixed this.

AlexandraMoga commented 4 years ago

I've (hopefully) verified all the frontend pages, including navigation between various pages to make sure the authenticated status is set correctly. These are some of the scenarios covered:

unauthenticated user opens a block page -> 'Sign in or Register' button is not present in site header
unauthenticated user navigates to other frontend pages from a block page -> 'Sign in or Register' button is present in site header
unauthenticated user navigates to developer hub from a block page -> DevHub log out landing page is opened
user logs in from a different tab while the blocked addon tab is still opened; user navigates to other AMO pages from the unrefreshed block page -> the user menu is present in site header
authenticated user opens a block page -> the user menu is not present in site header
authenticated user navigates to developer hub from a block page -> DevHub logged in landing page is opened

I'm marking this issue as verified fixed on -dev.

mozilla / addons

Investigate stateless pages in AMO app #13710