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: 2FAのバックアップコードの実装 #121

Merged
merged 2 commits into from
Jul 29, 2023

Conversation

u1-liquid
Copy link
Member

What

ioモデレーターチームの要請
2faのセットアップ時にバックアップコードを提供
重要なコードなので言語設定で受け取れないと悲しいのでいったん翻訳にも入れてる

Why

Additional info (optional)

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

Copy link
Collaborator

@riku6460 riku6460 left a comment

Choose a reason for hiding this comment

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

  • 念の為、各翻訳にバックアップコード前の文章を含めたほうがいいかも (英語ならまあ無いよりマシだと思うので)
  • 2FA が有効かつバックアップコードがない場合、設定画面かどこかに警告的なものを出せると良いかもしれない?

Copy link
Collaborator

@Ry0taK Ry0taK left a comment

Choose a reason for hiding this comment

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

(レビューになってなかったので再投稿)
↓みたいな雰囲気でレースコンディションが発生して、バックアップコードが一度のみ使えるという制約が死にそう

  1. 異なるバックアップコードを指定したリクエストAとリクエストBを同時に送信
  2. それぞれのリクエストが↓に到達してプロフィールデータを取得
    https://github.com/MisskeyIO/misskey/blob/e8f527a6d01677d992a7039e6ce1aabb3a1ac85e/packages/backend/src/server/api/SigninApiService.ts#L122
  3. リクエストAが↓に到達、チェックを通過して使用されたバックアップコードを削除
    https://github.com/MisskeyIO/misskey/blob/e8f527a6d01677d992a7039e6ce1aabb3a1ac85e/packages/backend/src/server/api/SigninApiService.ts#L158-L161
  4. リクエストBが↑に到達、チェックを通過して使用されたバックアップコードを削除
  5. この時、4で参照されるバックアップコード一覧は2で取得したプロフィールデータの情報に基づくため、3で削除されたはずのバックアップコードが含まれており、再度使用可能になる

とはいえこの問題が発生するのはリクエストAとリクエストBにそれぞれ正しいコードが含まれていた場合のみなので、この問題を許容するかどうかはおまかせします

OTPAuth.Secret()は内部的にNode.jsのcryptoパッケージを使っているっぽいので生成方法としては問題なさそう

@u1-liquid
Copy link
Member Author

Ry0taKさんに指摘いただいたissueについてはいい感じのsqlが思いつかず・・・

@u1-liquid u1-liquid requested a review from KOBA789 July 28, 2023 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants