Reset Password Flow - Githubissues

sfbrigade / bats-server

Routed is an app to help ambulances direct non-critical patients to hospital emergency rooms with the most availability.

https://routedapp.org/

GNU Affero General Public License v3.0

18 stars 12 forks source link

Reset Password Flow #295

Closed holliskuang closed 1 year ago

holliskuang commented 1 year ago

Reset Password Flow

React Reset Password Pages
Uses the Node Emailer that was previously built

User clicks on Reset Password button on initial log in page.
User is asked to enter email associated with their account.

Server fetches for email in db. Activate TwoFactorAuth in User Model. User is assigned a two factor code which is sent out to their email
If email does not exist, display error

User enters ToTP code

If totp code matches what is stored in user model, allow passage
Else display error

User sets new password. Passwords must match and follow guidelines in order to proceed
User model is updated with new password, and user is redirected to log in page to let them know that they can log in with new password

Try it out if you get the chance. Would love ideas on how to make a more friendly interface and refactoring to make the code better.

holliskuang commented 1 year ago

Email Templates added. Although, I can't seem to get the logo to appear :(

francisli commented 1 year ago

@holliskuang sorry I missed you on the call... if you check out the diff in the latest commit above, you'll see the change I made to make the logo appear... the emails are going to be viewed not on our site (i.e. in Gmail, in a mail reader, etc), so fully qualified URLs (not relative paths) must be used to reference any assets...

francisli commented 1 year ago

@holliskuang can you also address the test failure?

francisli commented 1 year ago

So, it's interesting how you were able to re-use the same two-factor code to implement the reset password flow, although I'm wondering about a few usability issues...

Is it possible to have two different email templates, so that the language in the email matches what the user is doing (logging in vs resetting password)?
Typically, most reset password flows have you click a link that goes directly to the form, saving the extra step of having to copy/type/paste the code.

What do you think?

holliskuang commented 1 year ago

So, it's interesting how you were able to re-use the same two-factor code to implement the reset password flow, although I'm wondering about a few usability issues...

Is it possible to have two different email templates, so that the language in the email matches what the user is doing (logging in vs resetting password)?

Typically, most reset password flows have you click a link that goes directly to the form, saving the extra step of having to copy/type/paste the code.

What do you think?

@francisli This all makes sense to me! Let me try to implement this.

holliskuang commented 1 year ago

@francisli It looks like for some reason a number of users we use for the tests no longer exist: super.user@example.com, amr.paramedic@example.com, sutter.operational@example.com...etc. As a result, it seems that the API tests are failing as the initial log-in does not occur in the first place. Do you know what I may have done to cause this/any remedies. Dropping and Migrating the test db doesn't change anything.

francisli commented 1 year ago

@holliskuang those users are created from server/test/fixtures/users.json. Those fixtures must be explicitly loaded before the test is run.

holliskuang commented 1 year ago

For all the server tests, the fixtures are loaded before each test. So it seems like it shouldn't be an issue. I am kind of at a loss on what might be happening here.

beforeEach(async () => {
    await helper.loadFixtures(['organizations', 'users']);
  });

francisli commented 1 year ago

@holliskuang I've resolved the merge conflicts in the branch and made some fixes so that tests are now running in CircleCI. The failures also fail when I run locally. Let me know if you have time to work on this, or if I should continue...?

michaelyshih commented 1 year ago

Is it correct for the test to be testing on a user that belongs to an organization that is not mFa enabled? It seems like on the dev branch the test is passing because it's skipping the mfa check on the organization so its always going to the two factor page

francisli commented 1 year ago

@michaelyshih thanks for getting all the tests fixed and passing. I'm going to work on the last formatting/linter errors and clean up the debugging output to get this merged in.

francisli commented 1 year ago

OK, I actually didn't realize that this branch included re-implementing the Login/Reset Password flows in the React SPA.

More work now needs to be done to clean up the redirects/interactions between the server and the SPA for authentication... It's a little messy and confusing right now, because the two-factor implementation is still server-side...