Skip to content
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

feat: add phone number validation to user APIs #5882

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

darcyYe
Copy link
Contributor

@darcyYe darcyYe commented May 16, 2024

Summary

Add phone number validation to user APIs.

  1. Put phone number validation utils to @logto/shared
  2. Refactor phone number validation in @logto/experience and @logto/console
  3. Add phone number validation to insertUser and updateUserById methods.

Testing

Covered by CI.

Checklist

  • .changeset
  • unit tests
  • integration tests
  • necessary TSDoc comments

@github-actions github-actions bot added feature Cool stuff size/l labels May 16, 2024
@darcyYe darcyYe force-pushed the yemq-log-8880-add-phone-number-validation-to-user-APIs branch from 4a854dd to 13d885a Compare May 16, 2024 09:13
Copy link

github-actions bot commented May 16, 2024

COMPARE TO master

Total Size Diff 📈 +3.54 KB

Diff by File
Name Diff
packages/console/package.json 📈 +1 Bytes
packages/console/src/pages/UserDetails/UserSettings/index.tsx 📈 +7 Bytes
packages/core/src/libraries/user.test.ts 📈 +158 Bytes
packages/core/src/libraries/user.ts 📈 +956 Bytes
packages/core/src/routes-me/social.ts 📈 +41 Bytes
packages/core/src/routes-me/user.ts 0 Bytes
packages/core/src/routes/admin-user/basics.test.ts 📈 +14 Bytes
packages/core/src/routes/admin-user/basics.ts 📈 +35 Bytes
packages/core/src/routes/admin-user/mfa-verifications.test.ts 📈 +80 Bytes
packages/core/src/routes/admin-user/mfa-verifications.ts 0 Bytes
packages/core/src/routes/admin-user/social.test.ts 📈 +15 Bytes
packages/core/src/routes/admin-user/social.ts 📈 +41 Bytes
packages/core/src/routes/interaction/actions/submit-interaction.mfa.test.ts 0 Bytes
packages/core/src/routes/interaction/actions/submit-interaction.test.ts 0 Bytes
packages/core/src/routes/interaction/actions/submit-interaction.ts 📈 +45 Bytes
packages/core/src/routes/interaction/mfa.test.ts 📈 +42 Bytes
packages/core/src/routes/interaction/mfa.ts 📈 +13 Bytes
packages/core/src/routes/interaction/utils/single-sign-on.test.ts 0 Bytes
packages/core/src/routes/interaction/utils/single-sign-on.ts 📈 +131 Bytes
packages/core/src/routes/interaction/verifications/mfa-payload-verification.test.ts 📈 +58 Bytes
packages/core/src/routes/interaction/verifications/mfa-payload-verification.ts 📈 +4 Bytes
packages/core/src/routes/interaction/verifications/mfa-verification.backup-code.test.ts 📈 +58 Bytes
packages/core/src/routes/interaction/verifications/mfa-verification.test.ts 📈 +47 Bytes
packages/core/src/utils/user.ts 📈 +751 Bytes
packages/experience/package.json 📈 +40 Bytes
packages/experience/src/utils/country-code.ts 📈 +158 Bytes
packages/experience/src/utils/form.ts 📈 +16 Bytes
packages/integration-tests/src/tests/api/admin-user.search.test.ts 📈 +89 Bytes
packages/integration-tests/src/tests/api/admin-user.test.ts 📈 +269 Bytes
packages/integration-tests/src/tests/api/role.user.test.ts 📈 +254 Bytes
packages/shared/package.json 0 Bytes
packages/shared/src/utils/phone.ts 📈 +786 Bytes
pnpm-lock.yaml 📈 +84 Bytes

@darcyYe darcyYe force-pushed the yemq-log-8880-add-phone-number-validation-to-user-APIs branch 4 times, most recently from 8a52112 to af48649 Compare May 17, 2024 03:10
@darcyYe darcyYe marked this pull request as ready for review May 17, 2024 03:36
@darcyYe darcyYe requested a review from a team May 17, 2024 03:36
packages/shared/src/utils/phone.ts Show resolved Hide resolved
packages/shared/src/utils/phone.ts Outdated Show resolved Hide resolved
@@ -106,18 +107,45 @@ export const createUserLibrary = (queries: Queries) => {
{ retries, factor: 0 } // No need for exponential backoff
);

const updateUserById = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

Should put the validPhoneNumber validation logic at the API koaGuard level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering to make this phone number guard at API level. But since the DB insert and update operation could bypass the API-level check since some of them were used in scenarios other than in API handlers.
Make this change on the DB queries/libraries methods IMO should be the safest and the most easy-to-maintain way of doing this.

@charIeszhao
Copy link
Member

Overall looks good to me. Need to resolve Simeng's comments, though.

@darcyYe darcyYe force-pushed the yemq-log-8880-add-phone-number-validation-to-user-APIs branch 2 times, most recently from 8ca8932 to b10179e Compare May 24, 2024 09:03
Copy link

github-actions bot commented Jun 8, 2024

This PR is stale because it has been open 10 for days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Jun 8, 2024
@darcyYe darcyYe force-pushed the yemq-log-8880-add-phone-number-validation-to-user-APIs branch 2 times, most recently from 36b7582 to 182991a Compare June 12, 2024 07:31
@darcyYe darcyYe force-pushed the yemq-log-8880-add-phone-number-validation-to-user-APIs branch from 182991a to c03b7a3 Compare June 12, 2024 10:19
@github-actions github-actions bot removed the stale label Jun 18, 2024
Copy link

github-actions bot commented Jul 2, 2024

This PR is stale because it has been open 10 for days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants