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

fix: 登録メール送信時に重複確認を行う #10231

Merged

Conversation

Ry0taK
Copy link
Contributor

@Ry0taK Ry0taK commented Mar 6, 2023

What

  • signupエンドポイントにおいて、メールアドレスの検証が走る場合にユーザー名の重複確認が行われていなかった問題を修正

Why

  • 既に登録が完了しているユーザー名で登録メールを送れてしまい、ユーザーの混乱を招くため

Additional info (optional)

  • partially fixes 新規登録にメール認証が必須の時、ユーザー名が被っていてもメール送信まではできてしまう #10076
  • 既に登録プロセスが完了している場合に登録が出来ないようになりますが、同時に同じユーザー名で登録プロセスを開始した場合、上記Issueで説明されている問題が依然として発生します。
    • Pendingになっているユーザーのリポジトリにおいても重複確認を行うべきか検討しましたが、メールアドレスを間違えた場合などに再度登録したいというケースがあることを想定して一旦はそのままにしています。
    • そのため、メールアドレス認証ページにおいて別途エラーハンドリングを行い、適切なメッセージを表示するべきかもしれません
  • メール登録を必須にしているインスタンスにおいて、以下の状況において動作確認済み
    1. ユーザー名が重複している場合、400が帰ることを確認
    2. ユーザー名が重複していない場合、204が帰りメールが飛んでくることを確認

@github-actions github-actions bot added the packages/backend Server side specific issue/PR label Mar 6, 2023
@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #10231 (3f403e2) into develop (f5b63d8) will decrease coverage by 0.01%.
The diff coverage is 30.00%.

@@             Coverage Diff             @@
##           develop   #10231      +/-   ##
===========================================
- Coverage    69.23%   69.22%   -0.01%     
===========================================
  Files          707      707              
  Lines        65407    65420      +13     
  Branches      5213     5215       +2     
===========================================
+ Hits         45283    45290       +7     
- Misses       20124    20130       +6     
Impacted Files Coverage Δ
...ackages/backend/src/server/api/SignupApiService.ts 50.23% <26.31%> (-1.26%) ⬇️
packages/backend/src/misc/correct-filename.ts 100.00% <100.00%> (ø)
packages/backend/src/core/NotificationService.ts 100.00% <0.00%> (+4.28%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Ry0taK Ry0taK marked this pull request as ready for review March 6, 2023 10:38
@github-actions github-actions bot requested review from acid-chicken and syuilo March 6, 2023 10:38
@rinsuki
Copy link
Contributor

rinsuki commented Mar 6, 2023

これクライアント側の表示ってどうなります?

@Ry0taK
Copy link
Contributor Author

Ry0taK commented Mar 6, 2023

これクライアント側の表示ってどうなります?

ボタンは押せますが、押しても何も反応しない状態になります
(ボタンを押せなくするのは #10238 で対応しています)

@syuilo syuilo merged commit a4ca127 into misskey-dev:develop Mar 9, 2023
@syuilo
Copy link
Member

syuilo commented Mar 9, 2023

🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR
Projects
None yet
3 participants