-
Notifications
You must be signed in to change notification settings - Fork 2
Forgot Password Back-End TLC #420
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
base: main
Are you sure you want to change the base?
Conversation
…proved its robustness. Also added several new tests for 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.
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
ILikeoperator - Added specific error handling for SSO accounts (Google and Shibboleth)
- Standardized error messages using a centralized
ERROR_MESSAGESconstant - 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.
bhunt02
left a comment
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.
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?
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 |
|
@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 |
|
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: |
…can be either have different account types or be a case-sensitivity issue
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
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) |
Description
Instigated by one of our users having a "User Not Found" error when they probably shouldn't've.
This PR:
ILike()for thisType of change
yarn installHow 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:
console.logs, leftover unused logic, or anything else that was accidentally committed)