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.

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

Patient Authentication Backend #547

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions backend/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@ require('dotenv').config({ path: `${process.env.NODE_ENV}.env` });
require('./utils/aws/awsSetup');
const path = require('path');

const passport = require('passport');
const log = require('loglevel');
const express = require('express');
const fileUpload = require('express-fileupload');
const cors = require('cors');
const bodyParser = require('body-parser');
const session = require('express-session');
const MongoStore = require('connect-mongo');
const cookieParser = require('cookie-parser');

const { errorHandler } = require('./utils');
const { requireAuthentication } = require('./middleware/authentication');
const { initDB } = require('./utils/initDb');
const {
setResponseHeaders,
Expand All @@ -24,7 +27,7 @@ const app = express();
app.use(configureHelment());
app.use(setResponseHeaders);
app.use(express.static(path.join(__dirname, '../frontend/build')));
app.use(cors());
app.use(cors({ credentials: true, origin: 'http://localhost:3000', methods: ['GET', 'POST'] }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need the origin property for this to work? If so, then we need to make origin part of the env vars so that the production environment doesn't have localhost as origin

app.use(
fileUpload({
createParentPath: true,
Expand All @@ -48,7 +51,40 @@ app.get('/*', (req, res, next) => {
}
});

app.use(requireAuthentication);
/**
* This is the secret used to sign the session ID cookie.
* This can be either a string for a single secret, or an array of multiple secrets.
* If an array of secrets is provided, only the first element will be used to sign the session
* ID cookie, while all the elements will be considered when verifying the signature in requests.
* The secret itself should be not easily parsed by a human and would best be a random set of
* characters.
*
* Patients will be logged in a session for 5 minutes, unless they refresh to extend this period.
* maxAge can also be set to null, which keeps a user logged in until the BROWSER is closed.
*/
const sess = {
secret: '3DP4ME',
cookie: {
domain: 'localhost', path: '/', httpOnly: true, secure: false, maxAge: 150000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue with domain as with origin ^^

},
resave: false,
saveUninitialized: false,
store: MongoStore.create({ mongoUrl: process.env.DB_URI }),
};

if (app.get('env') === 'production') {
app.set('trust proxy', 1); // trust first proxy
sess.cookie.secure = true; // serve secure cookies
}

app.use(cookieParser());

app.use(session(sess));

app.use(bodyParser.urlencoded({ extended: false }));
app.use(passport.initialize());
app.use(passport.session());

app.use(logRequest);
app.use(require('./routes'));

Expand Down
23 changes: 23 additions & 0 deletions backend/middleware/conditionalAuthentication.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const log = require('loglevel');

const {
ERR_AUTH_FAILED,
} = require('../utils/constants');
const { sendResponse } = require('../utils/response');

const { requireAuthentication } = require('./authentication');
const { requirePatientAuthentication } = require('./verifyPatient');

module.exports.requireConditionalAuthentication = async (req, res, next) => {
try {
const user = req?.session?.passport?.user;
if (!user) {
requireAuthentication(req, res, next);
} else {
requirePatientAuthentication(req, res);
}
} catch (error) {
log.error(error);
sendResponse(res, 401, ERR_AUTH_FAILED);
}
};
41 changes: 41 additions & 0 deletions backend/middleware/patientAuthentication.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
const passport = require('passport');
const LocalStrategy = require('passport-local').Strategy;
const twofactor = require('node-2fa');

const { models } = require('../models');
const { TWO_FACTOR_WINDOW_MINS } = require('../utils/constants');

const verifyPatientToken = (patient, token) => {
const patientSecret = patient.secret;

if (patient.secret) {
return twofactor.verifyToken(patientSecret, token, TWO_FACTOR_WINDOW_MINS);
}

return false;
};

passport.use('passport-local', new LocalStrategy(
(_id, token, done) => {
models.Patient.findById(_id, (err, user) => {
if (err) { return done(err); }
if (!user) {
return done(null, false, { message: 'No such user exists.' });
}
if (!(verifyPatientToken(user, token))) {
return done(null, false, { message: 'Incorrect token.' });
}
return done(null, user);
});
},
));

passport.serializeUser((user, done) => {
done(null, user._id);
});

passport.deserializeUser((_id, done) => {
models.Patient.findById(_id, (err, user) => {
done(err, user);
});
});
26 changes: 26 additions & 0 deletions backend/middleware/verifyPatient.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const log = require('loglevel');

const {
ERR_AUTH_FAILED,
ERR_NOT_APPROVED,
} = require('../utils/constants');
const { sendResponse } = require('../utils/response');

/**
* Middleware requires the incoming request to be authenticated. If not authenticated, a response
* is sent back to the client, and the middleware chain is stopped. Authentication is done by
* checking the request for a user, which is automatically attached when Passport logs a user in.
*/
module.exports.requirePatientAuthentication = async (req, res) => {
try {
const user = req?.session?.passport?.user;
if (!user) {
sendResponse(res, 401, ERR_NOT_APPROVED);
} else {
sendResponse(res, 200);
}
} catch (error) {
log.error(error);
sendResponse(res, 401, ERR_AUTH_FAILED);
}
};
6 changes: 6 additions & 0 deletions backend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
"axios": "^0.21.2",
"babel-eslint": "^10.1.0",
"body-parser": "^1.19.0",
"connect-mongo": "^4.6.0",
"cookie-parser": "^1.4.6",
"cors": "^2.8.5",
"cross-env": "^7.0.3",
"del": "^6.0.0",
Expand All @@ -23,6 +25,7 @@
"express": "^4.17.1",
"express-async-errors": "^3.1.1",
"express-fileupload": "^1.2.0",
"express-session": "^1.17.2",
"faker": "^5.5.3",
"fs": "^0.0.1-security",
"gulp": "^4.0.2",
Expand All @@ -40,6 +43,9 @@
"node": "^16.9.1",
"node-2fa": "^2.0.2",
"omit-deep-lodash": "^1.1.5",
"passport": "^0.5.0",
"passport-local": "^1.0.0",
"passport-session": "^1.0.2",
"prettier": "^2.4.1",
"superagent-defaults": "^0.1.14",
"supertest": "^6.1.3",
Expand Down
91 changes: 82 additions & 9 deletions backend/routes/api/authentication.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,52 @@
const express = require('express');
const twofactor = require('node-2fa');
const passport = require('passport');

const accountSid = process.env.ACCOUNT_SID;
const authToken = process.env.TWILIO_AUTH_TOKEN;

const client = require('twilio')(accountSid, authToken);

const {
TWILIO_SENDING_NUMBER,
TWILIO_WHATSAPP_PREFIX,
} = require('../../utils/constants');
const { errorWrap } = require('../../utils');
const { models } = require('../../models');
const { sendResponse } = require('../../utils/response');
const { TWO_FACTOR_WINDOW_MINS } = require('../../utils/constants');

const router = express.Router();

require('../../middleware/patientAuthentication');

router.post('/authenticated/:patientId', passport.authenticate('passport-local'), async (req, res) => {
const { patientId } = req.params;
console.log('ya');
if (!req.user) { return res.redirect(`/${patientId}`); }
console.log('nah');
req.logIn(req.user, (err) => {
if (err) { return err; }
console.log(req.session);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Many console.logs that could be removed

// do i need this line?
req.session.save();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably don't need this line anymore. I'd try without it

return sendResponse(
res,
200,
'Successfully authenticated patient',
);
});
});

/**
* Get secret, generate the token, then return the token
* If a patient's secret does not already exist, generate a new secret, then also return the token
* Send the patient token through Twilio to Whatsapp
*/
router.get(
'/:patientId',
errorWrap(async (req, res) => {
const { patientId } = req.params;

const patient = await models.Patient.findById(patientId);

if (!patient) {
Expand All @@ -39,12 +69,14 @@ router.get(

const newToken = twofactor.generateToken(patient.secret);

await sendResponse(
res,
200,
'New authentication key generated',
newToken,
);
client.messages
.create({
body: `Your one time token is ${newToken.token}`,
to: `${TWILIO_WHATSAPP_PREFIX}${patient.phoneNumber}`,
from: TWILIO_SENDING_NUMBER,
})
.then((message) => console.log(message.sid))
.catch((err) => console.log(err));
}),
);

Expand Down Expand Up @@ -76,17 +108,58 @@ router.post(
res,
200,
'Patient verified',
isAuthenticated,
);
} else {
await sendResponse(
res,
404,
'Invalid token entered',
isAuthenticated,
);
}
}),
);

router.get(
'/patient-portal/:patientId',
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have a route for getting a patient by id. It seems like this route would be redundant then. The only difference would be the authentication... I.e. If a volunteer is authenticated, they can view any patient. If a patient is authenticated, they can only view their own data.

So I would just reuse the existing patient endpoint but change the auth middleware on that endpoint so that it allows either type of user to access it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But now that I'm looking at this more, it seems like you were just using this endpoint for testing right?

errorWrap(async (req, res) => {
const { patientId } = req.params;
let patient;

try {
patient = await models.Patient.findById(patientId);
} catch {
await sendResponse(
res,
404,
'An error occurred while checking for patient authentication',
);
return;
}

if (!patient) {
await sendResponse(
res,
404,
'Invalid patient ID',
);
return;
}

if (!req.user) {
await sendResponse(
res,
404,
'Session expired',
);
return;
}

await sendResponse(
res,
200,
'Patient verified',
);
}),
);

module.exports = router;
2 changes: 1 addition & 1 deletion backend/routes/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ router.use('/metadata', require('./metadata'));
router.use('/users', require('./users'));
router.use('/roles', require('./roles'));
router.use('/messages', require('./messages'));
router.use('/authentication', require('./authentication'));
router.use('/patient-2fa', require('./authentication'));

module.exports = router;
3 changes: 3 additions & 0 deletions backend/routes/api/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ const {
TWILIO_RECEIVING_NUMBER,
TWILIO_SENDING_NUMBER,
} = require('../../utils/constants');
const { requireAuthentication } = require('../../middleware/authentication');
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest, we can delete this file. It doesn't contain any routes that we're using.


router.use(requireAuthentication);

router.post('/sms', async (req, res) => {
const phone = req?.body?.WaId;
Expand Down
9 changes: 6 additions & 3 deletions backend/routes/api/metadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,16 @@ const {
updateStepsInTransaction,
getReadableSteps,
} = require('../../utils/stepUtils');
const { requireAuthentication } = require('../../middleware/authentication');
const { requireConditionalAuthentication } = require('../../middleware/conditionalAuthentication');

/**
* Gets the metadata for a step. This describes the fields contained in the steps.
* If a user isn't allowed to view step, it isn't returned to them.
*/
router.get(
'/steps',
requireConditionalAuthentication,
errorWrap(async (req, res) => {
const metaData = await getReadableSteps(req);

Expand All @@ -36,7 +39,7 @@ router.get(
*/
router.post(
'/steps',
requireAdmin,
requireAuthentication, requireAdmin,
errorWrap(async (req, res) => {
try {
const stepToCreate = req.body;
Expand All @@ -62,7 +65,7 @@ router.post(
*/
router.put(
'/steps/',
requireAdmin,
requireAuthentication, requireAdmin,
errorWrap(async (req, res) => {
try {
let stepData = [];
Expand Down Expand Up @@ -101,7 +104,7 @@ router.put(
*/
router.delete(
'/steps/:stepkey',
requireAdmin,
requireAuthentication, requireAdmin,
errorWrap(async (req, res) => {
const { stepkey } = req.params;
const step = await models.Step.deleteOne({ key: stepkey });
Expand Down
Loading