Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#2783 limit number of failed login and reset password requests #2791

Merged
merged 9 commits into from
Aug 16, 2022
Merged

#2783 limit number of failed login and reset password requests #2791

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f8c2ec0
login limiter done single window and brute forcing one user
Aug 2, 2022
0752019
added rate limiters to registration and reset password request
Aug 2, 2022
deedd9a
password strength bar
Aug 3, 2022
59438c5
added request limiters to 3 methods of changing password, password st…
Aug 8, 2022
e752ce3
changes after review, added limiters to 3 types of reseting password
Aug 9, 2022
2f4a4a8
CR fixes
Aug 10, 2022
7ad3b47
test fix
Aug 10, 2022
5a7ad5b
correcting spelling
Aug 11, 2022
8519e57
CR changes
Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions verification/curator-service/api/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions verification/curator-service/api/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"envalid": "^7.2.2",
"express": "^4.17.1",
"express-openapi-validator": "^4.9.0",
"express-rate-limit": "^6.5.1",
"express-session": "^1.17.1",
"express-winston": "^4.2.0",
"i18n-iso-countries": "^7.3.0",
Expand Down
117 changes: 105 additions & 12 deletions verification/curator-service/api/src/controllers/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ import * as crypto from 'crypto';
import EmailClient from '../clients/email-client';
import { ObjectId } from 'mongodb';
import { baseURL, welcomeEmail } from '../util/instance-details';
import {
failed_attempts,
setupFailedAttempts,
handleCheckFailedAttempts,
} from '../model/failed_attempts';
import {
loginLimiter,
registerLimiter,
resetPasswordLimiter,
} from '../util/single-window-rate-limiters';

// Global variable for newsletter acceptance
let isNewsletterAccepted: boolean;
Expand Down Expand Up @@ -192,6 +202,7 @@ export class AuthController {

this.router.post(
'/signup',
registerLimiter,
(req: Request, res: Response, next: NextFunction): void => {
passport.authenticate(
'register',
Expand All @@ -214,16 +225,26 @@ export class AuthController {

this.router.post(
'/signin',
loginLimiter,
(req: Request, res: Response, next: NextFunction): void => {
passport.authenticate(
'login',
(error: Error, user: IUser, info: any) => {
(
error: Error,
user: IUser & { timeout: boolean },
info: any,
) => {
if (error) return next(error);
if (!user)
return res
.status(403)
.json({ message: info.message });

if (user.timeout)
return res
.status(429)
.json({ message: info.message });

req.logIn(user, (err) => {
if (err) return next(err);
});
Expand Down Expand Up @@ -422,16 +443,44 @@ export class AuthController {
*/
this.router.post(
'/request-password-reset',
resetPasswordLimiter,
async (req: Request, res: Response): Promise<Response<any>> => {
const email = req.body.email as string;

try {
// Check if user with this email address exists
const user = await users().findOne({ email });
const userPromise = await users()
.find({ email: email })
.collation({ locale: 'en_US', strength: 2 })
.toArray();

const user = userPromise[0] as IUser;
if (!user) {
return res.sendStatus(200);
}

const success = await handleCheckFailedAttempts(
user._id,
'resetPasswordAttempt',
);

if (!success)
return res.status(429).json({
message: 'Too Many Attempts try it one hour later',
});

await failed_attempts().updateOne(
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved
{ userId: user._id },
{
$set: {
resetPasswordAttempt: {
count: 0,
createdAt: new Date(),
},
},
},
);

// Check if user is a Gmail user and send appropriate email message in that case
// 42 googleID was set for non Google accounts in the past just to pass mongoose validation
// so this check has to be made
Expand Down Expand Up @@ -603,6 +652,9 @@ export class AuthController {
const user = (await users().findOne({
_id: result.insertedId,
})) as IUser;

setupFailedAttempts(result.insertedId);

req.login(user, (err: Error) => {
if (!err) {
res.json(user);
Expand Down Expand Up @@ -660,12 +712,13 @@ export class AuthController {
},
async (req, email, password, done) => {
try {
const userPromise = await users().find({ email })
.collation({ locale: 'en_US', strength: 2 })
.toArray();
const userPromise = await users()
.find({ email })
.collation({ locale: 'en_US', strength: 2 })
.toArray();

const user = userPromise[0];

if (user) {
return done(null, false, {
message: 'Email address already exists',
Expand All @@ -688,6 +741,8 @@ export class AuthController {
_id: result.insertedId,
})) as IUser;

setupFailedAttempts(result.insertedId);

// Send welcome email
await this.emailClient.send(
[email],
Expand All @@ -712,9 +767,10 @@ export class AuthController {
},
async (email, password, done) => {
try {
const userPromise = await users().find({ email })
.collation({ locale: 'en_US', strength: 2 })
.toArray();
const userPromise = await users()
.find({ email })
.collation({ locale: 'en_US', strength: 2 })
.toArray();

const user = userPromise[0] as IUser;

Expand All @@ -724,16 +780,42 @@ export class AuthController {
});
}

const success = await handleCheckFailedAttempts(
user._id,
'loginAttempt',
);

if (!success)
return done(
null,
{ timeout: true },
{
message:
'Too Many Attempts try it one hour later',
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved
},
);

const isValidPassword = await isUserPasswordValid(
user,
password,
);
if (!isValidPassword) {

if (!isValidPassword)
return done(null, false, {
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved
message: 'Wrong username or password',
});
}

await failed_attempts().updateOne(
{ userId: user._id },
{
$set: {
loginAttempt: {
count: 0,
createdAt: new Date(),
},
},
},
);
done(null, user);
} catch (error) {
done(error);
Expand Down Expand Up @@ -781,6 +863,8 @@ export class AuthController {
_id: result.insertedId,
})) as IUser;

setupFailedAttempts(result.insertedId);

try {
// Send welcome email
await this.emailClient.send(
Expand Down Expand Up @@ -865,7 +949,13 @@ export class AuthController {
'Supplied bearer token must be scoped for "email"',
);
}
let user = await users().findOne({ email: email });
const userPromise = await users()
.find({ email: email })
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved
.collation({ locale: 'en_US', strength: 2 })
.toArray();

let user = userPromise[0] as IUser | null;

if (!user) {
const result = await users().insertOne({
_id: new ObjectId(),
Expand All @@ -878,7 +968,10 @@ export class AuthController {
user = await users().findOne({
_id: result.insertedId,
});

setupFailedAttempts(result.insertedId);
}

return done(null, user);
} catch (e) {
return done(e);
Expand Down
84 changes: 84 additions & 0 deletions verification/curator-service/api/src/model/failed_attempts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { Collection, ObjectId } from 'mongodb';
import db from './database';

const numberTimeLimiters = {
loginAttempt: {
maxNumberOfFailedLogins: 8,
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved
timeWindowForFailedLogins: 60, //min
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to put the unit in the property name like timeWindowForFailedLoginsMinutes

},
resetPasswordAttempt: {
maxNumberOfFailedLogins: 6,
timeWindowForFailedLogins: 30,
},
};

type attemptName = 'loginAttempt' | 'resetPasswordAttempt';
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved

export type IfailedAttempts = {
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved
_id: ObjectId;
userId: ObjectId;
loginAttempt: {
count: number;
createdAt: Date;
};
resetPasswordAttempt: {
count: number;
createdAt: Date;
};
};

export const failed_attempts = () =>
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved
db().collection('failed_attempts') as Collection<IfailedAttempts>;

export const setupFailedAttempts = async (userId: ObjectId) => {
await failed_attempts().insertOne({
_id: new ObjectId(),
userId: userId,
loginAttempt: {
count: 0,
createdAt: new Date(),
},
resetPasswordAttempt: {
count: 0,
createdAt: new Date(),
},
});
};

export const handleCheckFailedAttempts = async (
userId: ObjectId,
attemptName: attemptName,
) => {
const attempts = (await failed_attempts().findOne({
userId: userId,
})) as IfailedAttempts;

let attemptsNumber = attempts[attemptName].count + 1;

const diffTimeMin =
Math.floor(
Math.abs(Date.now() - attempts[attemptName].createdAt.getTime()) /
1000,
) / 60;

if (
diffTimeMin >= numberTimeLimiters[attemptName].timeWindowForFailedLogins
)
attemptsNumber = 1;

await failed_attempts().updateOne(
{ userId: userId },
{
$set: {
[attemptName]: {
maciej-zarzeczny marked this conversation as resolved.
Show resolved Hide resolved
count: attemptsNumber,
createdAt: new Date(),
},
},
},
);

return (
attemptsNumber < numberTimeLimiters[attemptName].maxNumberOfFailedLogins
);
};