diff --git a/server/api/auth.test.js b/server/api/auth.test.js new file mode 100644 index 00000000..a70bb71c --- /dev/null +++ b/server/api/auth.test.js @@ -0,0 +1,43 @@ +/* eslint-disable flowtype/require-valid-file-annotation */ +import TestServer from "fetch-test-server"; +import app from "../app"; +import { buildUser, buildTeam } from "../test/factories"; +import { flushdb } from "../test/support"; + +const server = new TestServer(app.callback()); + +beforeEach(() => flushdb()); +afterAll(() => server.close()); + +describe("#auth.info", () => { + it("should return current authentication", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + + const res = await server.post("/api/auth.info", { + body: { token: user.getJwtToken() }, + }); + const body = await res.json(); + + expect(res.status).toEqual(200); + expect(body.data.user.name).toBe(user.name); + expect(body.data.team.name).toBe(team.name); + }); + + it("should require the team to not be deleted", async () => { + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + + await team.destroy(); + + const res = await server.post("/api/auth.info", { + body: { token: user.getJwtToken() }, + }); + expect(res.status).toEqual(401); + }); + + it("should require authentication", async () => { + const res = await server.post("/api/auth.info"); + expect(res.status).toEqual(401); + }); +}); diff --git a/server/auth/email.js b/server/auth/email.js index defa9459..4159c7dc 100644 --- a/server/auth/email.js +++ b/server/auth/email.js @@ -25,6 +25,10 @@ router.post("email", async (ctx) => { if (user) { const team = await Team.findByPk(user.teamId); + if (!team) { + ctx.redirect(`/?notice=auth-error`); + return; + } // If the user matches an email address associated with an SSO // signin then just forward them directly to that service's diff --git a/server/auth/google.js b/server/auth/google.js index d52a9639..74cd8f50 100644 --- a/server/auth/google.js +++ b/server/auth/google.js @@ -1,6 +1,7 @@ // @flow import crypto from "crypto"; import { OAuth2Client } from "google-auth-library"; +import invariant from "invariant"; import Router from "koa-router"; import { capitalize } from "lodash"; import Sequelize from "sequelize"; @@ -68,15 +69,24 @@ router.get("google.callback", auth({ required: false }), async (ctx) => { const cbResponse = await fetch(cbUrl); const avatarUrl = cbResponse.status === 200 ? cbUrl : tileyUrl; - const [team, isFirstUser] = await Team.findOrCreate({ - where: { - googleId, - }, - defaults: { - name: teamName, - avatarUrl, - }, - }); + let team, isFirstUser; + try { + [team, isFirstUser] = await Team.findOrCreate({ + where: { + googleId, + }, + defaults: { + name: teamName, + avatarUrl, + }, + }); + } catch (err) { + if (err instanceof Sequelize.UniqueConstraintError) { + ctx.redirect(`/?notice=auth-error`); + return; + } + } + invariant(team, "Team must exist"); try { const [user, isFirstSignin] = await User.findOrCreate({ diff --git a/server/auth/slack.js b/server/auth/slack.js index c1915e1a..b8aaca5d 100644 --- a/server/auth/slack.js +++ b/server/auth/slack.js @@ -1,5 +1,6 @@ // @flow import addHours from "date-fns/add_hours"; +import invariant from "invariant"; import Router from "koa-router"; import Sequelize from "sequelize"; import { slackAuth } from "../../shared/utils/routeHelpers"; @@ -40,15 +41,24 @@ router.get("slack.callback", auth({ required: false }), async (ctx) => { const data = await Slack.oauthAccess(code); - const [team, isFirstUser] = await Team.findOrCreate({ - where: { - slackId: data.team.id, - }, - defaults: { - name: data.team.name, - avatarUrl: data.team.image_88, - }, - }); + let team, isFirstUser; + try { + [team, isFirstUser] = await Team.findOrCreate({ + where: { + slackId: data.team.id, + }, + defaults: { + name: data.team.name, + avatarUrl: data.team.image_88, + }, + }); + } catch (err) { + if (err instanceof Sequelize.UniqueConstraintError) { + ctx.redirect(`/?notice=auth-error`); + return; + } + } + invariant(team, "Team must exist"); try { const [user, isFirstSignin] = await User.findOrCreate({ diff --git a/server/middlewares/authentication.test.js b/server/middlewares/authentication.test.js index 96279c75..d934793a 100644 --- a/server/middlewares/authentication.test.js +++ b/server/middlewares/authentication.test.js @@ -1,8 +1,8 @@ /* eslint-disable flowtype/require-valid-file-annotation */ import randomstring from "randomstring"; import { ApiKey } from "../models"; -import { buildUser } from "../test/factories"; -import { flushdb, seed } from "../test/support"; +import { buildUser, buildTeam } from "../test/factories"; +import { flushdb } from "../test/support"; import auth from "./authentication"; beforeEach(() => flushdb()); @@ -11,7 +11,7 @@ describe("Authentication middleware", () => { describe("with JWT", () => { it("should authenticate with correct token", async () => { const state = {}; - const { user } = await seed(); + const user = await buildUser(); const authMiddleware = auth(); await authMiddleware( @@ -29,7 +29,7 @@ describe("Authentication middleware", () => { it("should return error with invalid token", async () => { const state = {}; - const { user } = await seed(); + const user = await buildUser(); const authMiddleware = auth(); try { @@ -52,7 +52,7 @@ describe("Authentication middleware", () => { describe("with API key", () => { it("should authenticate user with valid API key", async () => { const state = {}; - const { user } = await seed(); + const user = await buildUser(); const authMiddleware = auth(); const key = await ApiKey.create({ userId: user.id, @@ -116,7 +116,7 @@ describe("Authentication middleware", () => { it("should allow passing auth token as a GET param", async () => { const state = {}; - const { user } = await seed(); + const user = await buildUser(); const authMiddleware = auth(); await authMiddleware( @@ -138,7 +138,7 @@ describe("Authentication middleware", () => { it("should allow passing auth token in body params", async () => { const state = {}; - const { user } = await seed(); + const user = await buildUser(); const authMiddleware = auth(); await authMiddleware( @@ -159,13 +159,14 @@ describe("Authentication middleware", () => { it("should return an error for suspended users", async () => { const state = {}; - const admin = await buildUser({}); + const admin = await buildUser(); const user = await buildUser({ suspendedAt: new Date(), suspendedById: admin.id, }); const authMiddleware = auth(); + let error; try { await authMiddleware( { @@ -177,11 +178,38 @@ describe("Authentication middleware", () => { }, jest.fn() ); - } catch (e) { - expect(e.message).toEqual( - "Your access has been suspended by the team admin" - ); - expect(e.errorData.adminEmail).toEqual(admin.email); + } catch (err) { + error = err; } + expect(error.message).toEqual( + "Your access has been suspended by the team admin" + ); + expect(error.errorData.adminEmail).toEqual(admin.email); + }); + + it("should return an error for deleted team", async () => { + const state = {}; + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + + await team.destroy(); + + const authMiddleware = auth(); + let error; + try { + await authMiddleware( + { + request: { + get: jest.fn(() => `Bearer ${user.getJwtToken()}`), + }, + state, + cache: {}, + }, + jest.fn() + ); + } catch (err) { + error = err; + } + expect(error.message).toEqual("Invalid token"); }); }); diff --git a/server/models/Team.js b/server/models/Team.js index 9784019e..19fb7d9a 100644 --- a/server/models/Team.js +++ b/server/models/Team.js @@ -69,6 +69,7 @@ const Team = sequelize.define( slackData: DataTypes.JSONB, }, { + paranoid: true, getterMethods: { url() { if (this.domain) { diff --git a/server/utils/jwt.js b/server/utils/jwt.js index c4121832..a71ce4d2 100644 --- a/server/utils/jwt.js +++ b/server/utils/jwt.js @@ -2,7 +2,7 @@ import subMinutes from "date-fns/sub_minutes"; import JWT from "jsonwebtoken"; import { AuthenticationError } from "../errors"; -import { User } from "../models"; +import { Team, User } from "../models"; function getJWTPayload(token) { let payload; @@ -28,7 +28,15 @@ export async function getUserForJWT(token: string): Promise { } } - const user = await User.findByPk(payload.id); + const user = await User.findByPk(payload.id, { + include: [ + { + model: Team, + as: "team", + required: true, + }, + ], + }); if (payload.type === "transfer") { // If the user has made a single API request since the transfer token was