-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: master
Are you sure you want to change the base?
Conversation
…nto patient-2fa-backend
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hack4impact/3dp4me/2AkdFRLEHMJY99J9umPpDjC7Pp4t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, this is an amazing amount of progress
@@ -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'] })); |
There was a problem hiding this comment.
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
const sess = { | ||
secret: '3DP4ME', | ||
cookie: { | ||
domain: 'localhost', path: '/', httpOnly: true, secure: false, maxAge: 150000, |
There was a problem hiding this comment.
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
^^
if (err) { return err; } | ||
console.log(req.session); | ||
// do i need this line? | ||
req.session.save(); |
There was a problem hiding this comment.
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
console.log('nah'); | ||
req.logIn(req.user, (err) => { | ||
if (err) { return err; } | ||
console.log(req.session); |
There was a problem hiding this comment.
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
); | ||
} | ||
}), | ||
); | ||
|
||
router.get( | ||
'/patient-portal/:patientId', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@@ -0,0 +1,28 @@ | |||
import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is a duplicate file. You have a file named allRoutes
and one named AllRoutes
and they appear to be the same.
@@ -0,0 +1,148 @@ | |||
import React, { useContext, useEffect, useState } from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you also have duplicate AWSRoutes
}, | ||
}); | ||
|
||
export const send2FAPatientCode = async (_id) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused at what this does. It seems like authenticatePatient
is the one that actually sends the 2FA code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the endpoint that requests a 2FA code?
@@ -42,7 +61,7 @@ const Patient2FALogin = () => { | |||
<button | |||
className="two-factor-authentication-button" | |||
type="submit" | |||
onClick={() => setIsTokenSent(true)} | |||
onClick={() => onTokenSend()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just replace this with
onClick={onTokenSend}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with the onChange and onClick below this
const PatientPortal = () => { | ||
const params = useParams(); | ||
const { patientId } = params; | ||
const [shouldRender, setShouldRender] = useState(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably make the useState default to something, like false
Status:
🚧 In development
Description
Authenticates patients using Passport.js utilizing patient id's as usernames and secure, randomly generated node-2fa tokens as passwords. Also includes some structural changes to the project, such as:
Fixes #457 #442 #465
Todos
Need to Address/Verify Before Merging:
Screenshots
None at the moment, but you can pull and try it out yourself! You will need to:
You should not be able to access anything other than the patient login page and patient portal.
FIXED AS OF 1/2: The patient portal will now redirect if you try to directly access it.