enhance(frontend): サインイン画面の改善

kakkokari-gtyih commented 1 week ago

What

14655 を取り込み
MkSignin周りを全面的に改修
一部足りてなさそうなエラーメッセージを追加
二要素認証系のプロパティを外部に露出しないように

Why

Close #14655

Additional info (optional)

Checklist

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

github-actions[bot] commented 1 week ago

このPRによるapi.jsonの差分

差分はこちら

```diff --- base +++ head @@ -77005,18 +77005,6 @@ "private" ] }, - "twoFactorEnabled": { - "type": "boolean", - "default": false - }, - "usePasswordLessLogin": { - "type": "boolean", - "default": false - }, - "securityKeys": { - "type": "boolean", - "default": false - }, "roles": { "type": "array", "items": { @@ -77039,6 +77027,15 @@ "moderationNote": { "type": "string" }, + "twoFactorEnabled": { + "type": "boolean" + }, + "usePasswordLessLogin": { + "type": "boolean" + }, + "securityKeys": { + "type": "boolean" + }, "isFollowing": { "type": "boolean" }, @@ -77103,9 +77100,6 @@ "publicReactions", "followingVisibility", "followersVisibility", - "twoFactorEnabled", - "usePasswordLessLogin", - "securityKeys", "roles", "memo" ] @@ -77882,6 +77876,18 @@ "type": "object", "$ref": "#/components/schemas/RolePolicies" }, + "twoFactorEnabled": { + "type": "boolean", + "default": false + }, + "usePasswordLessLogin": { + "type": "boolean", + "default": false + }, + "securityKeys": { + "type": "boolean", + "default": false + }, "email": { "type": [ "string", @@ -77954,7 +77960,10 @@ "emailNotificationTypes", "achievements", "loggedInDays", - "policies" + "policies", + "twoFactorEnabled", + "usePasswordLessLogin", + "securityKeys" ] }, "UserDetailedNotMe": { ``` [Get diff files from Workflow Page](https://github.com/misskey-dev/misskey/actions/runs/11174636574)

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 5.11364% with 1002 lines in your changes missing coverage. Please review.

Project coverage is 41.29%. Comparing base (e344650) to head (e962140). Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
packages/frontend/src/components/MkSignin.vue	0.00%	329 Missing :warning:
...ackages/frontend/src/components/MkSignin.input.vue	0.00%	205 Missing and 1 partial :warning:
...ages/frontend/src/components/MkSignin.password.vue	0.00%	180 Missing and 1 partial :warning:
...kages/frontend/src/components/MkSignin.passkey.vue	0.00%	91 Missing and 1 partial :warning:
packages/frontend/src/components/MkSignin.totp.vue	0.00%	73 Missing and 1 partial :warning:
...ackages/frontend/src/components/MkSigninDialog.vue	0.00%	65 Missing :warning:
...ackages/backend/src/server/api/SigninApiService.ts	27.02%	54 Missing :warning:
...ges/backend/src/core/entities/UserEntityService.ts	87.50%	0 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #14658 +/- ## =========================================== + Coverage 39.69% 41.29% +1.59% =========================================== Files 1544 1552 +8 Lines 193859 200240 +6381 Branches 3592 3615 +23 =========================================== + Hits 76950 82682 +5732 - Misses 116313 116957 +644 - Partials 596 601 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

syuilo commented 1 week ago

パスワード入力しても「続ける」が活性化しないかも

syuilo commented 1 week ago

2faのトークン入力とパスワード入力は一緒の画面で良い気もした

syuilo commented 1 week ago

🎨

kakkokari-gtyih commented 1 week ago

パスワード入力しても「続ける」が活性化しないかも

Captcha周りのチェックがなにかおかしいかも

syuilo commented 1 week ago

2faのトークン入力とパスワード入力は一緒の画面で良い気もした

YOSASOUであれば実装お願いしたい

kakkokari-gtyih commented 1 week ago

2faのトークン入力とパスワード入力は一緒の画面で良い気もした

YOSASOUであれば実装お願いしたい

パスキーが使えるかどうかの出し分け（パスキーの登録がある場合はパスキーが優先される）とか、パスキーが無理な場合にワンタイムパスワードも使えるような挙動をできるようになっているんだけど、それをするには画面が分かれていたほうがやりやすそうというのがあった

syuilo commented 1 week ago

ほむん

syuilo commented 1 week ago

CAPTHCAの設定をミスった状態で管理者アカウントからログアウトするとサーバーから締め出される問題があるわね

syuilo commented 1 week ago

実際にログインが行われるときはボタンを続けるじゃなくてログインにしたいわね

syuilo commented 1 week ago

考えられるパターンを整理したい

syuilo commented 1 week ago

2fa未設定でのログイン(ユーザー名とパスワード入力)
2fa設定済でのログイン(ユーザー名とパスワードとトークン入力)
2faとパスキー設定済みでのログイン(パスキー認証)

etc?

syuilo commented 1 week ago

パスキーが使えるかどうかの出し分け（パスキーの登録がある場合はパスキーが優先される）

ユーザー名入力した後で、パスキー登録があればパスキー使えるようになっているということかしら

syuilo commented 1 week ago

パスキー登録している人は最初にパスキーボタン押すだろうからその処理は無くても良いのではと思った

syuilo commented 1 week ago

あーパスキー登録はあるけどもパスワードレスログインを無効にしているケースってパスワードとパスキー認証させないといけないのか

syuilo commented 1 week ago

2fa設定されているかどうか
パスキー登録されているかどうか
パスワードレスログインが有効化されているかどうか

この3つの変数がある

syuilo commented 1 week ago

ヌァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァァンンンンオオオオンンオンオンオンオンンンンンンンンン゛ン゛！！！！！！！！！！！！！！！！

syuilo commented 1 week ago

passkey = true, passwordLess = false の場合ってどうなる？

パスワード+パスキー
パスワード+TOTP
パスワード+(TOTP or パスキー)
パスワード+TOTP+パスキー
パスワード or パスキー
パスワード or TOTP
パスワード or TOTP or パスキー
その他

どれかしら

syuilo commented 1 week ago

そもそもどういう時にパスワードレスログインを有効にして、どういう時にパスワードレスログインを無効にすれば良いのかしらオンオフ切り替えられるということはオンにするとデメリットがあるということだと思うけど何だったかしら

syuilo commented 1 week ago

あとパスワードレスログイン有効時の挙動は

有効にすると、絶対にパスワードを使ったログインは行えなくなり、パスキーだけのログインのみ行えるようになる
有効にすると、パスワードを使ったログインのほか、パスキーだけのログインも行えるようになる

どっちが正しいかしら(パスワードレス「しかできないよ」なのかパスワードレス「もできるよ」なのか)

syuilo commented 1 week ago

前者だとする場合、パスワードレスが無効の場合(つまり「『パスワードレスしかできない』ということはない」ということになる)、「パスワードレスログインもできるし、パスワード有りログインもできる」ということになりそうだけど合ってるのかしら

syuilo commented 1 week ago

デシジョンテーブルみたいなの欲しい

syuilo commented 1 week ago

MPが0になった

syuilo commented 1 week ago

TASUKETE

kakkokari-gtyih commented 1 week ago

バックエンドの改修も併せてやっているから、ついでに、クレデンシャルの要素が一つ追加されるごとに/signinにリクエストを飛ばして次の指示（ログイン成功なのか、キャプチャに進むのか、2FAを要求するのか）をAPIから返却させるのでもいいかもしれないフロントエンド側で次の操作の判定をしなくて良くなるので、より確実に処理ができそう

syuilo commented 1 week ago

フロントエンド側で次の操作の判定が無いとなると、 https://github.com/misskey-dev/misskey/pull/14658#issuecomment-2384841860 が達成できないかも

kakkokari-gtyih commented 1 week ago

フロントエンド側で次の操作の判定が無いとなると、 #14658 (comment) が達成できないかも

「この操作が完了すればログインする」という情報も含めるようにすれば…？（別に「次へ」を押していってそのうちログインでも問題なさそうではある、たぶんGoogleアカウントとかもそうだった気がする）

syuilo commented 1 week ago

ほむん

syuilo commented 1 week ago

https://github.com/misskey-dev/misskey/pull/14655 のすべてが取り込まれているわけではないっぽい？

syuilo commented 1 week ago

パスワードが間違っていた時にCAPTHCAがリセットされないかも

syuilo commented 1 week ago

先に https://github.com/misskey-dev/misskey/pull/14655 入れるか

kakkokari-gtyih commented 1 week ago

パスワードが間違っていた時にCAPTHCAがリセットされないかも

なおした

kakkokari-gtyih commented 1 week ago

バックエンドの改修も併せてやっているから、ついでに、クレデンシャルの要素が一つ追加されるごとに/signinにリクエストを飛ばして次の指示（ログイン成功なのか、キャプチャに進むのか、2FAを要求するのか）をAPIから返却させるのでもいいかもしれないフロントエンド側で次の操作の判定をしなくて良くなるので、より確実に処理ができそう

これやった（ついでに自分とモデレータ以外から二要素認証系のプロパティを取れないようにした）

kakkokari-gtyih commented 1 week ago

14655 のすべてが取り込まれているわけではないっぽい？

develop取り込んだので実質全部取り込んでるはず

syuilo commented 1 week ago

ヘッダーの区切り要るかしら

kakkokari-gtyih commented 1 week ago

スクロールが発生するときにちょっと境界線が見えにくい気はする（ただし好みの問題なので全然消しても問題ない）

syuilo commented 1 week ago

スクロールした時だけ表示されるのが理想ではあるわね

kakkokari-gtyih commented 1 week ago

(Off-topic) Chromaticが落ちているのはMkAbuseReportのStoryを確認するなどの対応が必要そう

kakkokari-gtyih commented 1 week ago

ボーダー消した

syuilo commented 1 week ago

headerはbodyと一体化させたいわね

syuilo commented 1 week ago

見た目上

kakkokari-gtyih commented 1 week ago

細かい調整はお願いしてもいいかしら

syuilo commented 1 week ago

👍🏿

misskey-dev / misskey