fix: Improve handling of suspended users signing in with email (#2012)

* chore: Separate signin/auth middleware
fix: Email signin token parsed by JWT middleware
fix: Email signin marked as active when logging in as suspended
fix: Suspended email signin correctly redirected to login screen
closes #1740

* refactor middleware -> lib

* lint

server/auth/providers/email.js

@@ -4,10 +4,10 @@ import Router from "koa-router";
 import { find } from "lodash";
 import { AuthorizationError } from "../../errors";
 import mailer from "../../mailer";
-import auth from "../../middlewares/authentication";
 import methodOverride from "../../middlewares/methodOverride";
 import validation from "../../middlewares/validation";
 import { User, Team } from "../../models";
+import { signIn } from "../../utils/authentication";
 import { getUserForEmailSigninToken } from "../../utils/jwt";
 
 const router = new Router();
@@ -84,25 +84,26 @@ router.post("email", async (ctx) => {
   };
 });
 
-router.get("email.callback", auth({ required: false }), async (ctx) => {
+router.get("email.callback", async (ctx) => {
   const { token } = ctx.request.query;
 
   ctx.assertPresent(token, "token is required");
 
   try {
     const user = await getUserForEmailSigninToken(token);
-
+    if (!user.team.guestSignin) {
-    const team = await Team.findByPk(user.teamId);
+      return ctx.redirect("/?notice=auth-error");
-    if (!team.guestSignin) {
+    }
-      throw new AuthorizationError();
+    if (user.isSuspended) {
+      return ctx.redirect("/?notice=suspended");
     }
 
     await user.update({ lastActiveAt: new Date() });
 
     // set cookies on response and redirect to team subdomain
-    ctx.signIn(user, team, "email", false);
+    signIn(ctx, user, user.team, "email", false);
   } catch (err) {
-    ctx.redirect(`${process.env.URL}?notice=expired-token`);
+    ctx.redirect(`/?notice=expired-token`);
   }
 });
 

server/auth/providers/google.js

@@ -9,7 +9,6 @@ import {
   GoogleWorkspaceRequiredError,
   GoogleWorkspaceInvalidError,
 } from "../../errors";
-import auth from "../../middlewares/authentication";
 import passportMiddleware from "../../middlewares/passport";
 import { getAllowedDomains } from "../../utils/authentication";
 import { StateStore } from "../../utils/passport";
@@ -90,11 +89,7 @@ if (GOOGLE_CLIENT_ID) {
 
   router.get("google", passport.authenticate(providerName));
 
-  router.get(
+  router.get("google.callback", passportMiddleware(providerName));
-    "google.callback",
-    auth({ required: false }),
-    passportMiddleware(providerName)
-  );
 }
 
 export default router;

server/auth/providers/slack.js

@@ -76,11 +76,7 @@ if (SLACK_CLIENT_ID) {
 
   router.get("slack", passport.authenticate(providerName));
 
-  router.get(
+  router.get("slack.callback", passportMiddleware(providerName));
-    "slack.callback",
-    auth({ required: false }),
-    passportMiddleware(providerName)
-  );
 
   router.get("slack.commands", auth({ required: false }), async (ctx) => {
     const { code, state, error } = ctx.request.query;

server/middlewares/authentication.js

@@ -1,10 +1,7 @@
 // @flow
-import addMonths from "date-fns/add_months";
-import JWT from "jsonwebtoken";
 import { AuthenticationError, UserSuspendedError } from "../errors";
-import { User, Event, Team, ApiKey } from "../models";
+import { User, Team, ApiKey } from "../models";
 import type { ContextWithState } from "../types";
-import { getCookieDomain } from "../utils/domains";
 import { getUserForJWT } from "../utils/jwt";
 
 export default function auth(options?: { required?: boolean } = {}) {
@@ -94,78 +91,6 @@ export default function auth(options?: { required?: boolean } = {}) {
       ctx.state.user = user;
     }
 
-    ctx.signIn = (user: User, team: Team, service, isFirstSignin = false) => {
-      if (user.isSuspended) {
-        return ctx.redirect("/?notice=suspended");
-      }
-
-      // update the database when the user last signed in
-      user.updateSignedIn(ctx.request.ip);
-
-      // don't await event creation for a faster sign-in
-      Event.create({
-        name: "users.signin",
-        actorId: user.id,
-        userId: user.id,
-        teamId: team.id,
-        data: {
-          name: user.name,
-          service,
-        },
-        ip: ctx.request.ip,
-      });
-
-      const domain = getCookieDomain(ctx.request.hostname);
-      const expires = addMonths(new Date(), 3);
-
-      // set a cookie for which service we last signed in with. This is
-      // only used to display a UI hint for the user for next time
-      ctx.cookies.set("lastSignedIn", service, {
-        httpOnly: false,
-        expires: new Date("2100"),
-        domain,
-      });
-
-      // set a transfer cookie for the access token itself and redirect
-      // to the teams subdomain if subdomains are enabled
-      if (process.env.SUBDOMAINS_ENABLED === "true" && team.subdomain) {
-        // get any existing sessions (teams signed in) and add this team
-        const existing = JSON.parse(
-          decodeURIComponent(ctx.cookies.get("sessions") || "") || "{}"
-        );
-        const sessions = encodeURIComponent(
-          JSON.stringify({
-            ...existing,
-            [team.id]: {
-              name: team.name,
-              logoUrl: team.logoUrl,
-              url: team.url,
-            },
-          })
-        );
-        ctx.cookies.set("sessions", sessions, {
-          httpOnly: false,
-          expires,
-          domain,
-        });
-
-        ctx.redirect(
-          `${team.url}/auth/redirect?token=${user.getTransferToken()}`
-        );
-      } else {
-        ctx.cookies.set("accessToken", user.getJwtToken(), {
-          httpOnly: false,
-          expires,
-        });
-        ctx.redirect(`${team.url}/home${isFirstSignin ? "?welcome" : ""}`);
-      }
-    };
-
     return next();
   };
 }
-
-// Export JWT methods as a convenience
-export const sign = JWT.sign;
-export const verify = JWT.verify;
-export const decode = JWT.decode;

server/middlewares/passport.js

@@ -1,10 +1,11 @@
 // @flow
 import passport from "@outlinewiki/koa-passport";
+import { type Context } from "koa";
 import type { AccountProvisionerResult } from "../commands/accountProvisioner";
-import type { ContextWithAuthMiddleware } from "../types";
+import { signIn } from "../utils/authentication";
 
 export default function createMiddleware(providerName: string) {
-  return function passportMiddleware(ctx: ContextWithAuthMiddleware) {
+  return function passportMiddleware(ctx: Context) {
     return passport.authorize(
       providerName,
       { session: false },
@@ -27,7 +28,7 @@ export default function createMiddleware(providerName: string) {
           return ctx.redirect("/?notice=suspended");
         }
 
-        ctx.signIn(result.user, result.team, providerName, result.isNewUser);
+        signIn(ctx, result.user, result.team, providerName, result.isNewUser);
       }
     )(ctx);
   };

server/types.js

@@ -1,6 +1,6 @@
 // @flow
 import { type Context } from "koa";
-import { User, Team } from "./models";
+import { User } from "./models";
 
 export type ContextWithState = {|
   ...$Exact<Context>,
@@ -10,13 +10,3 @@ export type ContextWithState = {|
     authType: "app" | "api",
   },
 |};
-
-export type ContextWithAuthMiddleware = {|
-  ...$Exact<ContextWithState>,
-  signIn: (
-    user: User,
-    team: Team,
-    providerName: string,
-    isFirstSignin: boolean
-  ) => void,
-|};

server/utils/authentication.js

@@ -1,7 +1,82 @@
 // @flow
+import addMonths from "date-fns/add_months";
+import { type Context } from "koa";
+import { User, Event, Team } from "../models";
+import { getCookieDomain } from "../utils/domains";
 
 export function getAllowedDomains(): string[] {
   // GOOGLE_ALLOWED_DOMAINS included here for backwards compatability
   const env = process.env.ALLOWED_DOMAINS || process.env.GOOGLE_ALLOWED_DOMAINS;
   return env ? env.split(",") : [];
 }
+
+export function signIn(
+  ctx: Context,
+  user: User,
+  team: Team,
+  service: string,
+  isFirstSignin: boolean = false
+) {
+  if (user.isSuspended) {
+    return ctx.redirect("/?notice=suspended");
+  }
+
+  // update the database when the user last signed in
+  user.updateSignedIn(ctx.request.ip);
+
+  // don't await event creation for a faster sign-in
+  Event.create({
+    name: "users.signin",
+    actorId: user.id,
+    userId: user.id,
+    teamId: team.id,
+    data: {
+      name: user.name,
+      service,
+    },
+    ip: ctx.request.ip,
+  });
+
+  const domain = getCookieDomain(ctx.request.hostname);
+  const expires = addMonths(new Date(), 3);
+
+  // set a cookie for which service we last signed in with. This is
+  // only used to display a UI hint for the user for next time
+  ctx.cookies.set("lastSignedIn", service, {
+    httpOnly: false,
+    expires: new Date("2100"),
+    domain,
+  });
+
+  // set a transfer cookie for the access token itself and redirect
+  // to the teams subdomain if subdomains are enabled
+  if (process.env.SUBDOMAINS_ENABLED === "true" && team.subdomain) {
+    // get any existing sessions (teams signed in) and add this team
+    const existing = JSON.parse(
+      decodeURIComponent(ctx.cookies.get("sessions") || "") || "{}"
+    );
+    const sessions = encodeURIComponent(
+      JSON.stringify({
+        ...existing,
+        [team.id]: {
+          name: team.name,
+          logoUrl: team.logoUrl,
+          url: team.url,
+        },
+      })
+    );
+    ctx.cookies.set("sessions", sessions, {
+      httpOnly: false,
+      expires,
+      domain,
+    });
+
+    ctx.redirect(`${team.url}/auth/redirect?token=${user.getTransferToken()}`);
+  } else {
+    ctx.cookies.set("accessToken", user.getJwtToken(), {
+      httpOnly: false,
+      expires,
+    });
+    ctx.redirect(`${team.url}/home${isFirstSignin ? "?welcome" : ""}`);
+  }
+}

server/utils/jwt.js

@@ -21,6 +21,10 @@ function getJWTPayload(token) {
 export async function getUserForJWT(token: string): Promise<User> {
   const payload = getJWTPayload(token);
 
+  if (payload.type === "email-signin") {
+    throw new AuthenticationError("Invalid token");
+  }
+
   // check the token is within it's expiration time
   if (payload.expiresAt) {
     if (new Date(payload.expiresAt) < new Date()) {