fix mozilla#1329: redirect user to FxA sign-in instead of error

smileys2 · Nov 1, 2019 · e349e1c · e349e1c
1 parent d0b3794
commit e349e1c
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 59 deletions.
diff --git a/controllers/user.js b/controllers/user.js
@@ -15,25 +15,9 @@ const sha1 = require("../sha1-utils");
 const FXA_MONITOR_SCOPE = "https://identity.mozilla.com/apps/monitor";
 
 
-async function _getRequestSessionUser(req) {
-  if (req.session && req.session.user) {
-    // make sure the user object has all subscribers and email_addresses properties
-    return DB.getSubscriberById(req.session.user.id);
-  }
-  return null;
-}
-
-async function _requireSessionUser(req,res) {
-  if (!req.session || !req.session.user) {
-    // TODO: can we do a nice redirect to sign in instead of an error?
-    throw new FluentError("error-must-be-signed-in");
-  }
-  return _getRequestSessionUser(req);
-}
-
 async function removeEmail(req, res) {
   const emailId = req.body.emailId;
-  const sessionUser = await _requireSessionUser(req);
+  const sessionUser = req.user;
   const existingEmail = await DB.getEmailById(emailId);
   if (existingEmail.subscriber_id !== sessionUser.id) {
     throw new FluentError("error-not-subscribed");
@@ -45,7 +29,7 @@ async function removeEmail(req, res) {
 
 async function resendEmail(req, res) {
   const emailId = req.body.emailId;
-  const sessionUser = await _requireSessionUser(req);
+  const sessionUser = req.user;
   const existingEmail = await DB.getEmailById(emailId);
 
   if (!existingEmail || !existingEmail.subscriber_id) {
@@ -77,7 +61,7 @@ async function resendEmail(req, res) {
 }
 
 async function updateCommunicationOptions(req, res) {
-  const sessionUser = await _requireSessionUser(req);
+  const sessionUser = req.user;
   // 0 = Send breach alerts to the email address found in brew breach.
   // 1 = Send all breach alerts to user's primary email address.
   const allEmailsToPrimary = (Number(req.body.communicationOption) === 1) ? true : false;
@@ -101,7 +85,7 @@ function _checkForDuplicateEmail(sessionUser, email) {
 
 
 async function add(req, res) {
-  const sessionUser = await _requireSessionUser(req);
+  const sessionUser = await req.user;
   const email = req.body.email;
   if (!email || !isemail.validate(email)) {
     throw new FluentError("user-add-invalid-email");
@@ -188,7 +172,7 @@ function getNewBreachesForEmailEntriesSinceDate(emailEntries, date) {
 
 
 async function getDashboard(req, res) {
-  const user = await _requireSessionUser(req);
+  const user = req.user;
   const allBreaches = req.app.locals.breaches;
   const { verifiedEmails, unverifiedEmails } = await getAllEmailsAndBreaches(user, allBreaches);
   let lastAddedEmail = null;
@@ -250,7 +234,7 @@ async function verify(req, res) {
     throw new FluentError("error-not-subscribed");
   }
 
-  const sessionUser = await _getRequestSessionUser(req);
+  const sessionUser = await req.user;
   if (sessionUser && existingEmail.subscriber_id !== sessionUser.id) {
     // TODO: more specific error message?
     // e.g., "This email verification token is not valid for this account"
@@ -305,7 +289,7 @@ async function getUnsubscribe(req, res) {
 
 
 async function getRemoveFxm(req, res) {
-  const sessionUser = await _requireSessionUser(req);
+  const sessionUser = req.user;
 
   res.render("subpage", {
     title: req.fluentFormat("remove-fxm"),
@@ -317,7 +301,7 @@ async function getRemoveFxm(req, res) {
 
 
 async function postRemoveFxm(req, res) {
-  const sessionUser = await _requireSessionUser(req);
+  const sessionUser = req.user;
   await DB.removeSubscriber(sessionUser);
   await FXA.revokeOAuthTokens(sessionUser);
 
@@ -350,7 +334,7 @@ async function postUnsubscribe(req, res) {
 
 
 async function getPreferences(req, res) {
-  const user = await _requireSessionUser(req);
+  const user = req.user;
   const allBreaches = req.app.locals.breaches;
   const { verifiedEmails, unverifiedEmails } = await getAllEmailsAndBreaches(user, allBreaches);
 

diff --git a/middleware.js b/middleware.js
@@ -9,6 +9,7 @@ Sentry.init({
   environment: AppConstants.NODE_ENV,
 });
 
+const DB = require("./db/DB");
 const { FluentError } = require("./locale-utils");
 const mozlog = require("./log");
 
@@ -92,6 +93,25 @@ function errorHandler (err, req, res, next) {
 }
 
 
+async function _getRequestSessionUser(req, res, next) {
+  if (req.session && req.session.user) {
+    // make sure the user object has all subscribers and email_addresses properties
+    return DB.getSubscriberById(req.session.user.id);
+  }
+  return null;
+}
+
+async function requireSessionUser(req, res, next) {
+  const user = await _getRequestSessionUser(req);
+  if (!user) {
+    return res.redirect("/oauth/init");
+  }
+  req.user = user;
+  next();
+}
+
+
+
 module.exports = {
   addRequestToResponse,
   pickLanguage,
@@ -100,4 +120,5 @@ module.exports = {
   localizeErrorMessages,
   clientErrorHandler,
   errorHandler,
+  requireSessionUser,
 };
diff --git a/routes/user.js b/routes/user.js
@@ -5,7 +5,7 @@ const bearerToken = require("express-bearer-token");
 const bodyParser = require("body-parser");
 const csrf = require("csurf");
 
-const { asyncMiddleware } = require("../middleware");
+const { asyncMiddleware, requireSessionUser } = require("../middleware");
 const {
   add, verify, logout,
   getDashboard, getPreferences, getBreachStats,
@@ -19,20 +19,20 @@ const urlEncodedParser = bodyParser.urlencoded({ extended: false });
 const csrfProtection = csrf();
 
 
-router.get("/dashboard", csrfProtection, asyncMiddleware(getDashboard));
-router.get("/preferences", csrfProtection, asyncMiddleware(getPreferences));
+router.get("/dashboard", csrfProtection, requireSessionUser, asyncMiddleware(getDashboard));
+router.get("/preferences", csrfProtection, requireSessionUser, asyncMiddleware(getPreferences));
 router.use("/breach-stats", bearerToken());
 router.get("/breach-stats", urlEncodedParser, asyncMiddleware(getBreachStats));
 router.get("/logout", logout);
-router.post("/email", urlEncodedParser, csrfProtection, asyncMiddleware(add));
-router.post("/remove-email", urlEncodedParser, csrfProtection, asyncMiddleware(removeEmail));
-router.post("/resend-email", jsonParser, csrfProtection, asyncMiddleware(resendEmail));
-router.post("/update-comm-option", jsonParser, csrfProtection, asyncMiddleware(updateCommunicationOptions));
+router.post("/email", urlEncodedParser, csrfProtection, requireSessionUser, asyncMiddleware(add));
+router.post("/remove-email", urlEncodedParser, csrfProtection, requireSessionUser, asyncMiddleware(removeEmail));
+router.post("/resend-email", jsonParser, csrfProtection, requireSessionUser, asyncMiddleware(resendEmail));
+router.post("/update-comm-option", jsonParser, csrfProtection, requireSessionUser, asyncMiddleware(updateCommunicationOptions));
 router.get("/verify", jsonParser, asyncMiddleware(verify));
 router.use("/unsubscribe", urlEncodedParser);
-router.get("/unsubscribe", asyncMiddleware(getUnsubscribe));
+router.get("/unsubscribe", urlEncodedParser, asyncMiddleware(getUnsubscribe));
 router.post("/unsubscribe", csrfProtection, asyncMiddleware(postUnsubscribe));
-router.get("/remove-fxm", urlEncodedParser, csrfProtection, asyncMiddleware(getRemoveFxm));
-router.post("/remove-fxm", jsonParser, csrfProtection, asyncMiddleware(postRemoveFxm));
+router.get("/remove-fxm", urlEncodedParser, csrfProtection, requireSessionUser, asyncMiddleware(getRemoveFxm));
+router.post("/remove-fxm", jsonParser, csrfProtection, requireSessionUser, asyncMiddleware(postRemoveFxm));
 
 module.exports = router;
diff --git a/tests/controllers/user.test.js b/tests/controllers/user.test.js
@@ -40,6 +40,7 @@ test("user add POST with email adds unverified subscriber and sends verification
     url: "/user/add",
     body: { email: testUserAddEmail },
     session: { user: testSubscriber },
+    user: testSubscriber,
     fluentFormat: jest.fn(),
     headers: {
       referer: "",
@@ -84,6 +85,7 @@ test("user add POST with upperCaseAddress adds email_address record with lowerca
     url: "/user/add",
     body: { email: testUserAddEmail },
     session: { user: testSubscriber },
+    user: testSubscriber,
     fluentFormat: jest.fn(),
     headers: {
       referer: "",
@@ -120,6 +122,7 @@ test("user resendEmail with valid session and email id resets email_address reco
     body: { emailId: testEmailAddressId },
     session: { user: testSubscriber },
     fluentFormat: jest.fn(),
+    user: testSubscriber,
   });
   const resp = httpMocks.createResponse();
   EmailUtils.sendEmail.mockResolvedValue(true);
@@ -142,6 +145,7 @@ test("user updateCommunicationOptions request with valid session updates DB", as
     url: "/user/update-comm-option",
     body: { communicationOption: 0 },
     session: { user: testSubscriber },
+    user: testSubscriber,
   });
   const resp = httpMocks.createResponse();
 
@@ -218,6 +222,7 @@ test("user verify request with valid token verifies user and redirects to dashbo
       session: { user: testSubscriber },
       fluentFormat: jest.fn(),
       app: { locals: { breaches: testBreaches } },
+      user: testSubscriber,
     });
   const resp = httpMocks.createResponse();
 
@@ -241,6 +246,7 @@ test("user verify request with valid token but wrong user session does NOT verif
       session: { user: testSubscriber },
       fluentFormat: jest.fn(),
       app: { locals: { breaches: testBreaches } },
+      user: testSubscriber,
     });
   const resp = httpMocks.createResponse();
 
@@ -262,6 +268,7 @@ test("user verify request for already verified user doesn't send extra email", a
   mockRequest.session = { user: testSubscriber };
   mockRequest.query = { token: alreadyVerifiedToken };
   mockRequest.app = { locals: { breaches: testBreaches } };
+  mockRequest.user = testSubscriber;
   const resp = httpMocks.createResponse();
 
   // Call code-under-test
@@ -365,7 +372,12 @@ test("user unsubscribe POST request with valid session and emailId for email_add
 test("user removeEmail POST request with valid session but wrong emailId for email_address throws error and doesnt remove email", async () => {
   const testEmailAddress = TEST_EMAIL_ADDRESSES.all_emails_to_primary;
   const testEmailId = testEmailAddress.id;
-  const req = { fluentFormat: jest.fn(), body: { emailId: testEmailId }, session: { user: TEST_SUBSCRIBERS.firefox_account }};
+  const req = {
+    fluentFormat: jest.fn(),
+    body: { emailId: testEmailId },
+    session: { user: TEST_SUBSCRIBERS.firefox_account },
+    user: TEST_SUBSCRIBERS.firefox_account,
+  };
   const resp = httpMocks.createResponse();
 
   await expect(user.removeEmail(req, resp)).rejects.toThrow("error-not-subscribed");
@@ -375,18 +387,6 @@ test("user removeEmail POST request with valid session but wrong emailId for ema
 });
 
 
-test("user/remove-fxm GET request with invalid session returns error", async () => {
-  const req = httpMocks.createRequest({
-    method: "GET",
-    url: "/user/remove-fxm",
-    fluentFormat: jest.fn(),
-  });
-  const resp = httpMocks.createResponse();
-
-  await expect(user.getRemoveFxm(req, resp)).rejects.toThrow("error-must-be-signed-in");
-});
-
-
 test("user/remove-fxm GET request with valid session returns 200 and renders remove_fxm", async () => {
   // Set up mocks
   const req = { fluentFormat: jest.fn(), csrfToken: jest.fn(), session: { user: TEST_SUBSCRIBERS.firefox_account }};
@@ -400,18 +400,12 @@ test("user/remove-fxm GET request with valid session returns 200 and renders rem
 });
 
 
-test("user/remove-fxm POST request with invalid session returns error", async () => {
-  // Set up mocks
-  const req = { fluentFormat: jest.fn(), session: {} };
-  const resp = httpMocks.createResponse();
-
-  // Call code-under-test
-  await expect(user.postRemoveFxm(req, resp)).rejects.toThrow("error-must-be-signed-in");
-});
-
-
 test("user remove-fxm POST request with valid session removes from DB and revokes FXA OAuth token", async () => {
-  const req = { fluentFormat: jest.fn(), session: { user: TEST_SUBSCRIBERS.firefox_account, reset: jest.fn() }};
+  const req = {
+    fluentFormat: jest.fn(),
+    session: { user: TEST_SUBSCRIBERS.firefox_account, reset: jest.fn() },
+    user: TEST_SUBSCRIBERS.firefox_account,
+  };
   const resp = httpMocks.createResponse();
   FXA.revokeOAuthTokens = jest.fn();