Skip to content

Commit

Permalink
feat: restrict members that didn't validate their email (#1113)
Browse files Browse the repository at this point in the history
* feat: restrict members that have validated their email

* refactor: add await

---------

Co-authored-by: kim <kim.phanhoang@epfl.ch>
Co-authored-by: Kim Lan Phan Hoang <pyphilia@gmail.com>
  • Loading branch information
3 people authored Jul 22, 2024
1 parent 76b0020 commit 62915ac
Show file tree
Hide file tree
Showing 50 changed files with 508 additions and 158 deletions.
17 changes: 17 additions & 0 deletions src/migrations/1718693920666-add-isvalidated-lastauthenticated.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { MigrationInterface, QueryRunner } from 'typeorm';

export class Migrations1718693920666 implements MigrationInterface {
name = 'Migrations1718693920666';

public async up(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`ALTER TABLE "member" ADD "last_authenticated_at" TIMESTAMP`);
await queryRunner.query(
`ALTER TABLE "member" ADD "is_validated" boolean NOT NULL DEFAULT false`,
);
}

public async down(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`ALTER TABLE "member" DROP COLUMN "last_authenticated_at"`);
await queryRunner.query(`ALTER TABLE "member" DROP COLUMN "is_validated"`);
}
}
23 changes: 16 additions & 7 deletions src/services/auth/plugins/magicLink/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { StatusCodes } from 'http-status-codes';

import fastifyPassport from '@fastify/passport';
import { FastifyPluginAsync, FastifyReply, FastifyRequest, PassportUser } from 'fastify';
import { FastifyPluginAsync, PassportUser } from 'fastify';

import { RecaptchaAction } from '@graasp/sdk';
import { DEFAULT_LANG } from '@graasp/translations';

import { resolveDependency } from '../../../../di/utils';
import { notUndefined } from '../../../../utils/assertions';
import { AUTH_CLIENT_HOST } from '../../../../utils/config';
import { MemberAlreadySignedUp } from '../../../../utils/errors';
import { buildRepositories } from '../../../../utils/repositories';
Expand All @@ -15,6 +16,7 @@ import { getRedirectionUrl } from '../../utils';
import captchaPreHandler from '../captcha';
import { SHORT_TOKEN_PARAM } from '../passport';
import { PassportStrategy } from '../passport/strategies';
import { PassportInfo } from '../passport/types';
import { auth, login, register } from './schemas';
import { MagicLinkService } from './service';

Expand Down Expand Up @@ -105,12 +107,7 @@ const plugin: FastifyPluginAsync = async (fastify) => {
schema: auth,
preHandler: fastifyPassport.authenticate(
PassportStrategy.WebMagicLink,
async (
request: FastifyRequest,
reply: FastifyReply,
err: null | Error,
user?: PassportUser,
) => {
async (request, reply, err, user?: PassportUser, info?: PassportInfo) => {
// This function is called after the strategy has been executed.
// It is necessary, so we match the behavior of the original implementation.
if (!user || err) {
Expand All @@ -120,16 +117,28 @@ const plugin: FastifyPluginAsync = async (fastify) => {
reply.redirect(StatusCodes.SEE_OTHER, target.toString());
} else {
request.logIn(user, { session: true });
request.authInfo = info;
}
},
),
},
async (request, reply) => {
const {
user,
authInfo,
query: { url },
log,
} = request;
const member = notUndefined(user?.member);
const redirectionUrl = getRedirectionUrl(log, url ? decodeURIComponent(url) : undefined);
await db.transaction(async (manager) => {
const repositories = buildRepositories(manager);
await memberService.refreshLastAuthenticatedAt(member.id, repositories);
// on auth, if the user used the email sign in, its account gets validated
if (authInfo?.emailValidation && !member.isValidated) {
await memberService.validate(member.id, repositories);
}
});
reply.redirect(StatusCodes.SEE_OTHER, redirectionUrl);
},
);
Expand Down
37 changes: 34 additions & 3 deletions src/services/auth/plugins/magicLink/magicLink.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ describe('Auth routes tests', () => {
const m = await memberRawRepository.findOneBy({ email, name });

expectMember(m, { name, email });
expect(m?.lastAuthenticatedAt).toBeNull();
expect(m?.isValidated).toBeFalsy();

// ensure that the user agreements are set for new registration
expect(m?.userAgreementsDate).toBeDefined();
Expand Down Expand Up @@ -106,6 +108,9 @@ describe('Auth routes tests', () => {
);
const m = await memberRawRepository.findOneBy({ email, name });
expectMember(m, { name, email, extra: { lang } });
expect(m?.lastAuthenticatedAt).toBeNull();
expect(m?.isValidated).toBeFalsy();

// ensure that the user agreements are set for new registration
expect(m?.userAgreementsDate).toBeDefined();
expect(m?.userAgreementsDate).toBeInstanceOf(Date);
Expand Down Expand Up @@ -152,6 +157,9 @@ describe('Auth routes tests', () => {
// ensure that the user agreements are set for new registration
expect(m?.userAgreementsDate).toBeDefined();
expect(m?.userAgreementsDate).toBeInstanceOf(Date);
expect(m?.lastAuthenticatedAt).toBeNull();
expect(m?.isValidated).toBeFalsy();

expect(response.statusCode).toEqual(StatusCodes.NO_CONTENT);
});

Expand Down Expand Up @@ -180,12 +188,15 @@ describe('Auth routes tests', () => {
// ensure that the user agreements are set for new registration
expect(m?.userAgreementsDate).toBeDefined();
expect(m?.userAgreementsDate).toBeInstanceOf(Date);
expect(m?.lastAuthenticatedAt).toBeNull();
expect(m?.isValidated).toBeFalsy();

expect(response.statusCode).toEqual(StatusCodes.NO_CONTENT);
});

it('Sign Up fallback to login for already register member', async () => {
// register already existing member
const member = await saveMember();
const member = await saveMember(MemberFactory({ isValidated: false }));
const mockSendEmail = jest.spyOn(mailerService, 'sendEmail');

const response = await app.inject({
Expand All @@ -205,7 +216,8 @@ describe('Auth routes tests', () => {
const members = await memberRawRepository.findBy({ email: member.email });
expect(members).toHaveLength(1);
expectMember(member, members[0]);

expect(members[0]?.lastAuthenticatedAt).toBeNull();
expect(members[0]?.isValidated).toBeFalsy();
expect(response.statusCode).toEqual(StatusCodes.NO_CONTENT);
});

Expand Down Expand Up @@ -314,14 +326,33 @@ describe('Auth routes tests', () => {

describe('GET /auth', () => {
it('Authenticate successfully', async () => {
const member = await saveMember();
const member = await saveMember(MemberFactory({ isValidated: false }));
const t = sign({ sub: member.id }, JWT_SECRET);
const response = await app.inject({
method: HttpMethod.Get,
url: `/auth?t=${t}`,
});
expect(response.statusCode).toEqual(StatusCodes.SEE_OTHER);
expect(response.headers.location).not.toContain('error');

const m = await memberRawRepository.findOneBy({ email: member.email });
expect(m?.lastAuthenticatedAt).toBeDefined();
expect(m?.isValidated).toBeFalsy();
});

it('Authenticate successfully with email validation', async () => {
const member = await saveMember(MemberFactory({ isValidated: false }));
const t = sign({ sub: member.id, emailValidation: true }, JWT_SECRET);
const response = await app.inject({
method: HttpMethod.Get,
url: `/auth?t=${t}`,
});
expect(response.statusCode).toEqual(StatusCodes.SEE_OTHER);
expect(response.headers.location).not.toContain('error');

const m = await memberRawRepository.findOneBy({ email: member.email });
expect(m?.lastAuthenticatedAt).toBeDefined();
expect(m?.isValidated).toBeTruthy();
});

it('Fail if token contains undefined memberId', async () => {
Expand Down
12 changes: 11 additions & 1 deletion src/services/auth/plugins/mobile/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
MOBILE_DEEP_LINK_PROTOCOL,
} from '../../../../utils/config';
import { buildRepositories } from '../../../../utils/repositories';
import { MemberService } from '../../../member/service';
import { generateAuthTokensPair, getRedirectionUrl } from '../../utils';
import captchaPreHandler from '../captcha';
import {
Expand All @@ -32,6 +33,7 @@ const plugin: FastifyPluginAsync = async (fastify) => {

const mobileService = resolveDependency(MobileService);
const memberPasswordService = resolveDependency(MemberPasswordService);
const memberService = resolveDependency(MemberService);

// no need to add CORS support here - only used by mobile app

Expand Down Expand Up @@ -124,8 +126,16 @@ const plugin: FastifyPluginAsync = async (fastify) => {
schema: mauth,
preHandler: authenticateJWTChallengeVerifier,
},
async ({ user }) => {
async ({ user, authInfo }) => {
const member = notUndefined(user?.member);
await db.transaction(async (manager) => {
const repositories = buildRepositories(manager);
await memberService.refreshLastAuthenticatedAt(member.id, repositories);
// on auth, if the user used the email sign in, its account gets validated
if (authInfo?.emailValidation && !member.isValidated) {
await memberService.validate(member.id, repositories);
}
});
return generateAuthTokensPair(member.id);
},
);
Expand Down
24 changes: 21 additions & 3 deletions src/services/auth/plugins/mobile/mobile.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -581,11 +581,14 @@ describe('Mobile Endpoints', () => {
url: '/m/register',
payload: {
name: username,
email: email,
email,
challenge,
captcha: MOCK_CAPTCHA,
},
});
let m = await memberRawRepository.findOneBy({ email });
expect(m?.lastAuthenticatedAt).toBeNull();
expect(m?.isValidated).toBeFalsy();

expect(responseRegister.statusCode).toBe(StatusCodes.NO_CONTENT);
expect(mockSendEmail).toHaveBeenCalledTimes(1);
Expand All @@ -599,8 +602,12 @@ describe('Mobile Endpoints', () => {
verifier,
},
});

expect(responseAuth.statusCode).toBe(StatusCodes.OK);

m = await memberRawRepository.findOneBy({ email });
expect(m?.lastAuthenticatedAt).toBeDefined();
expect(m?.isValidated).toBeTruthy();

const responseAuthBody = responseAuth.json();
expect(responseAuthBody).toHaveProperty('refreshToken');
expect(responseAuthBody).toHaveProperty('authToken');
Expand All @@ -622,7 +629,10 @@ describe('Mobile Endpoints', () => {
it('Password', async () => {
mockCaptchaValidation(RecaptchaAction.SignInWithPasswordMobile);

const member = await saveMemberAndPassword(undefined, MOCK_PASSWORD);
const member = await saveMemberAndPassword(
MemberFactory({ isValidated: false }),
MOCK_PASSWORD,
);

const responseLogin = await app.inject({
method: HttpMethod.Post,
Expand All @@ -638,6 +648,9 @@ describe('Mobile Endpoints', () => {
const responseLoginBody = responseLogin.json();
const authUrl = new URL(responseLoginBody.resource);

let m = await memberRawRepository.findOneBy({ email: member.email });
expect(m?.lastAuthenticatedAt).toBeNull();
expect(m?.isValidated).toBeFalsy();
const responseAuth = await app.inject({
method: HttpMethod.Post,
url: `/m/auth`,
Expand All @@ -648,6 +661,11 @@ describe('Mobile Endpoints', () => {
});

expect(responseAuth.statusCode).toBe(StatusCodes.OK);

m = await memberRawRepository.findOneBy({ email: member.email });
expect(m?.lastAuthenticatedAt).toBeDefined();
expect(m?.isValidated).toBeFalsy();

const responseAuthBody = responseAuth.json();
expect(responseAuthBody).toHaveProperty('refreshToken');
expect(responseAuthBody).toHaveProperty('authToken');
Expand Down
12 changes: 10 additions & 2 deletions src/services/auth/plugins/passport/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { HttpMethod } from '@graasp/sdk';

import build, { clearDatabase } from '../../../../../test/app';
import { resolveDependency } from '../../../../di/utils';
import { AppDataSource } from '../../../../plugins/datasource';
import {
APPS_JWT_SECRET,
AUTH_TOKEN_JWT_SECRET,
Expand Down Expand Up @@ -42,6 +43,7 @@ import {
// mock datasource
jest.mock('../../../../plugins/datasource');
const MOCKED_ROUTE = '/mock-route';
const memberRawRepository = AppDataSource.getRepository(Member);

const expectUserApp = (
user: PassportUser,
Expand Down Expand Up @@ -144,7 +146,10 @@ describe('Passport Plugin', () => {
});
it('Valid Session Member', async () => {
const cookie = await logIn(app, member);
handler.mockImplementation(({ user }) => expectMember(user.member, member));
handler.mockImplementation(async ({ user }) => {
const rawMember = await memberRawRepository.findOneBy({ id: member.id });
expectMember(rawMember, user.member);
});
const response = await app.inject({
path: MOCKED_ROUTE,
headers: { cookie },
Expand Down Expand Up @@ -206,7 +211,10 @@ describe('Passport Plugin', () => {
});
it('Valid Session Member', async () => {
const cookie = await logIn(app, member);
handler.mockImplementation(({ user }) => expectMember(user.member, member));
handler.mockImplementation(async ({ user }) => {
const rawMember = await memberRawRepository.findOneBy({ id: member.id });
expectMember(rawMember, user.member);
});
const response = await app.inject({
path: MOCKED_ROUTE,
headers: { cookie },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ export default (
secretOrKey: JWT_SECRET,
passReqToCallback: true,
},
async ({ body: { verifier } }, { sub, challenge }, done: StrictVerifiedCallback) => {
async (
{ body: { verifier } },
{ sub, challenge, emailValidation },
done: StrictVerifiedCallback,
) => {
const spreadException: boolean = options?.propagateError ?? false;
//-- Verify Challenge --//
try {
Expand All @@ -43,7 +47,7 @@ export default (
const member = await memberRepository.get(sub);
if (member) {
// Token has been validated
return done(null, { member });
return done(null, { member }, { emailValidation });
} else {
// Authentication refused
return done(
Expand Down
4 changes: 2 additions & 2 deletions src/services/auth/plugins/passport/strategies/magicLink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ export default (
jwtFromRequest: ExtractJwt.fromUrlQueryParameter(tokenQueryParameter),
secretOrKey: jwtSecret,
},
async ({ sub }, done: StrictVerifiedCallback) => {
async ({ sub, emailValidation }, done: StrictVerifiedCallback) => {
try {
const member = await memberRepository.get(sub);
if (member) {
// Token has been validated
return done(null, { member });
return done(null, { member }, { emailValidation });
} else {
// Authentication refused
return done(
Expand Down
4 changes: 2 additions & 2 deletions src/services/auth/plugins/passport/strategies/password.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { UnauthorizedMember } from '../../../../../utils/errors';
import { Repositories } from '../../../../../utils/repositories';
import { MemberPasswordService } from '../../password/service';
import { PassportStrategy } from '../strategies';
import { CustomStrategyOptions, StrictVerifiedCallback } from '../types';
import { CustomStrategyOptions } from '../types';

export default (
passport: Authenticator,
Expand All @@ -20,7 +20,7 @@ export default (
{
usernameField: 'email',
},
async (email, password, done: StrictVerifiedCallback) => {
async (email, password, done) => {
try {
const member = await memberPasswordService.authenticate(repositories, email, password);
if (member) {
Expand Down
5 changes: 4 additions & 1 deletion src/services/auth/plugins/passport/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@ export type StrictVerifiedCallback = (
// If false, the authentication will fail with a 401 Unauthorize if no error is provided.
user: PassportUser | false,

info?: never, // Currently not used.
info?: PassportInfo, // Data passed to `req.authInfo`
) => void;

export type CustomStrategyOptions = {
// If true, the client will receive a more detailed error message, instead of a generic 401 Unauthorized.
// We recommend setting this to true in development, and false in production.
propagateError?: boolean;
};
export type PassportInfo = {
emailValidation?: boolean; // True if the user logged from an email link.
};
2 changes: 1 addition & 1 deletion src/services/auth/plugins/password/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { FastifyInstance, LightMyRequestResponse } from 'fastify';
import { MemberFactory, RecaptchaAction } from '@graasp/sdk';

import build, { clearDatabase } from '../../../../../test/app';
import seed from '../../../../../test/mock';
import seed from '../../../../../test/mocks';
import { mockCaptchaValidation } from '../../../../../test/utils';
import { resolveDependency } from '../../../../di/utils';
import { MailerService } from '../../../../plugins/mailer/service';
Expand Down
Loading

0 comments on commit 62915ac

Please sign in to comment.