Skip to content

Conversation

@AdamFipke
Copy link
Collaborator

@AdamFipke AdamFipke commented Oct 28, 2025

Description

Instigated by one of our users having a "User Not Found" error when they probably shouldn't've.

This PR:

  • adds more detailed error messages to forgot password endpoint
  • improved its robustness a little
  • added several new tests
  • Now made emails case-insensitive for it (@bhunt02 you might want to review this part in particular). Reason being is I could very well see someone try to reset their password but the case is wrong. I used typeorm's ILike() for this

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • This requires a run of yarn install
  • This change requires an addition/change to the production .env variables. These changes are below:
  • This change requires developers to add new .env variables. The file and variables needed are below:
  • This change requires a database query to update old data on production. This query is below:

How Has This Been Tested?

Didn't manually test it, but I added a bunch of new tests. Local tests decided not to work today weeee

Checklist:

  • I have performed a code review of my own code (under the "Files Changed" tab on github) to ensure nothing is committed that shouldn't be (e.g. leftover console.logs, leftover unused logic, or anything else that was accidentally committed)
  • I have commented my code where needed
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that new and existing tests pass locally with my changes
  • Any work that this PR is dependent on has been merged into the main branch
  • Any UI changes have been checked to work on desktop, tablet, and mobile

@AdamFipke AdamFipke requested review from bhunt02 and Copilot October 29, 2025 18:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves the password reset endpoint (/password/reset) by adding better error handling, case-insensitive email lookups, and SSO account validation. The changes include standardized error messages, improved test coverage, and removal of the recaptcha token validation that was previously performed before the Google API call.

Key changes:

  • Implemented case-insensitive email lookup using TypeORM's ILike operator
  • Added specific error handling for SSO accounts (Google and Shibboleth)
  • Standardized error messages using a centralized ERROR_MESSAGES constant
  • Enhanced test coverage with additional edge cases including SSO accounts and case-sensitivity

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
packages/server/src/auth/auth.controller.ts Refactored password reset logic with case-insensitive email search, SSO account validation, and standardized error messages
packages/server/test/auth.integration.ts Expanded test suite to cover SSO accounts, case-insensitive email matching, and improved test organization with shared setup
packages/common/index.ts Added centralized error messages for authentication controller

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@bhunt02 bhunt02 left a comment

Choose a reason for hiding this comment

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

not excited to have to merge these changes into my LTI PR, but looks pretty simple. the edge case where a user needs to reset their password but has accounts with the same email but different case is slightly concerning cuz they might end up locked out of their primary account, but alas. what are your thoughts on that?

@AdamFipke
Copy link
Collaborator Author

AdamFipke commented Nov 2, 2025

the edge case where a user needs to reset their password but has accounts with the same email but different case is slightly concerning cuz they might end up locked out of their primary account, but alas. what are your thoughts on that?

Yeah I had that exact thought. But I guess since other parts of the system assume email + orgID is enough to uniquely identify a user, i would rather they contact me to get their account fixed rather than worry about other parts of the system breaking.

Though come to think of it I should probably capture that error (with the problematic emails) in sentry

@AdamFipke
Copy link
Collaborator Author

@bhunt02 and I guess speaking of, other parts of the system (especially login and register) should probably have case-insensitive email query as well (if it doesn't already i sorta forgor) but I guess that's probably literally what you did with that one PR that we never merged

@bhunt02
Copy link
Collaborator

bhunt02 commented Nov 2, 2025

we didn't merge it for other issues. should probably scrap it and just re-implement the same idea. we might have to delete some user accounts though - but to be fair most of the issue was google sending us weird emails and people with google accounts can't password reset anyway. the one problem would be is if it matched based on priority (they're probably stored internally ordered by ID in whatever file they're in, i assume) and the first match would be the relevant one.

easy way around this: accountType: AccountType.Legacy or whatever

@AdamFipke
Copy link
Collaborator Author

we didn't merge it for other issues. should probably scrap it and just re-implement the same idea. we might have to delete some user accounts though

Right okay yeah, could be saved for another PR, perhaps after the LTI stuff gets merged since that'll probably be a bunch of merge conflicts

but to be fair most of the issue was google sending us weird emails and people with google accounts can't password reset anyway. the one problem would be is if it matched based on priority (they're probably stored internally ordered by ID in whatever file they're in, i assume) and the first match would be the relevant one.

easy way around this: accountType: AccountType.Legacy or whatever

Yeah, I guess that's sorta what I did with this PR. In writing this though I did realise I had a pretty big logic error in my code but I fixed that now (issue was that if there's multiple accounts with the same email, it could either be that the accounts have different account type OR be a case-sensitivity issue, neither of which should be possible)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants