Skip to content

Conversation

@dnechay
Copy link
Collaborator

@dnechay dnechay commented Dec 24, 2024

Issue tracking

Part of #2928

Context behind the change

  • refactored error handling of auth module.
  • removed getByCredentials from UserService because in fact it did auth check and shouldn't live there; instead introduced checkPasswordMatchesHash and use it for auth checks
  • in methods where we send emails (e.g. forgotPassword) now we won't throw when user is not found, but just return; previously it was ControlledError with 204 status code, so result is expected to be the same

How has this been tested?

  • unit tests
  • locally trigger auth/signup; make sure it can create user
  • locally trigger auth/signup with same email; make sure it returns duplicated error message and 409 code
  • locally trigger auth/sigin with valid password; make sure it returns jwt
  • locally trigger auth/sigin with invalid password; make sure it returns 400
  • locally trigger user/exchange-oracle-registration with invalid jwt; make sure it returns 401
  • locally trigger user/exchange-oracle-registration with valid jwt; make sure it returns 200

Release plan

Simply merge & deploy

Potential risks; What to monitor; Rollback plan

It might be that there is some client implicitly relying on returned error message/code that changed in this PR. To monitor it we can check logs.

@vercel
Copy link

vercel bot commented Dec 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
human-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 24, 2024 2:01pm
human-dashboard-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 24, 2024 2:01pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
faucet-frontend ⬜️ Ignored (Inspect) Visit Preview Dec 24, 2024 2:01pm
faucet-server ⬜️ Ignored (Inspect) Visit Preview Dec 24, 2024 2:01pm

@dnechay dnechay removed the do-not-merge PR shouldn't be merged until this label is removed label Jan 3, 2025
@dnechay dnechay merged commit 79a6cf4 into develop Jan 3, 2025
12 checks passed
@dnechay dnechay deleted the dnechay/2928-auth branch January 3, 2025 12:26
@dnechay dnechay mentioned this pull request Jan 8, 2025
23 tasks
@dnechay dnechay self-assigned this Mar 12, 2025
@dnechay dnechay moved this to Done in Core-tech - 2025 Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants