/signin の設計がおかしい可能性がある

[syuilo]

Summary

仕様通り想定して飛んできているリクエストに対してエラーを返すのはおかしい

Purpose

あ

Do you want to implement this feature yourself?

• [ ] Yes, I will implement this by myself and send a pull request

[kakkokari-gtyih]

成功時も次の入力項目を指示するときも200にして、レスポンスをこんな感じにする？

成功時

{
    "success": true,
    "id": "xxxxxxxx",
    "i": "xxxxxxxx"
}

次の入力項目を指示

{
    "success": false,
    "next": "password"
}

[syuilo]

そうね

[kakkokari-gtyih]

これでいくか

[syuilo]

successというよりfinishとかの方がよさそう

[kakkokari-gtyih]

うーん資格情報が足りない状態でのレスポンスに200は強い違和感を持ちますね

403が違うってのは多分そうなのですが、401は不適当ですし400あたりがよいと私は感じます (https://github.com/misskey-dev/misskey/pull/14700#issuecomment-2393353797)

@syuilo

[anatawa12]

仕様通り想定して飛んできているリクエストに対してエラーを返すのはおかしい

私はおかしくないと思います。

資格情報が足りない状態でのレスポンスに200は強い違和感を持ちます

403が違うってのはある程度同意しますし、401は不適当ですし400あたりがよいと私は感じます

[syuilo]

API側の実装がこれで正しいとしたら、クライアント側をエラーが出ないようにリクエストするように修正するべきだわね

[kakkokari-gtyih]

API側の実装がこれで正しいとしたら、クライアント側をエラーが出ないようにリクエストするように修正するべきだわね

200を返さずにエラーを出さずにそれをしようとしたら結局前のログイン画面に戻ってしまう（新ログイン画面はエラーが返ってくる or 次の操作が示されることを前提に組まれている）

[syuilo]

そう、だからどちらかの設計がおかしいということになる

[kakkokari-gtyih]

エラーとして返ってくるのってそんなにまずいかしら

[syuilo]

そもそもsigninという名のAPIがsignin以外の処理(次のステップを指定するなど)を担当している時点で若干おかしい

[syuilo]

エラーとして返ってくるのってそんなにまずいかしら

とてもまずい 発生したエラーを報告するような仕組みが入っていたとしたら、(正常な手順で)ログインするたびにエラーが報告されることになる

[anatawa12]

4xxを期待してリクエストすることも私はおかしくないと考えます。

私はHTTPのステータスコード上のエラーとプログラム上のthrowされるようなエラーとは別物と考えてます

[syuilo]

ステータスコードじゃなく、実際にプログラム上もエラーやfailedとして表現されている

[anatawa12]

バックエンド的にはエラーではあると思う。

フロント的にはthrowしない方が適切かも

[syuilo]

バックエンド的にエラーなのだとしたら、フロントエンドはバックエンドがエラーになってしまわないように正しく実装するべきという話になる でも現在のフロントエンドの実装が問題ないのだとしたら間違っているのはバックエンドということになりそう

[anatawa12]

そもそもsigninという名のAPIがsignin以外の処理(次のステップを指定するなど)を担当している時点で若干おかしい

これも資格情報不足でチャレンジ種別を返すはそこまでおかしくないと思います(401もそうなってるし)

[kakkokari-gtyih]

フロント的にはthrowしない方が適切かも

これはそうしてあるはず

[syuilo]

そこまでおかしくはないけど、ちょっとはおかしい(直感的ではない) ちょっとでもおかしいのであれば直した方が良い

[anatawa12]

別のエンドポイントとして必要資格情報を返すエンドポイントは追加した方が良いかも

けどsigninは足りない資格情報がある時は4xxを返してあげるべきだと思う。(ここに必要資格情報を含めるかはどっちでもいいけど今あるならついてて良さそう)

[kakkokari-gtyih]

別のエンドポイントとして必要資格情報を返すエンドポイントは追加した方が良いかも

これすると二要素認証が有効化されているかどうかなどが簡単に取れてしまう（UserDetailedに二要素認証関連のプロパティが公開されているのと何ら変わらなくなる）ので目的を達成できなくなる（工夫次第かも？）

[syuilo]

エンドポイント分けると場合によってはリクエストが二度手間というか無駄になる場合がありそう

[anatawa12]

これすると二要素認証が有効化されているかどうかなどが簡単に取れてしまう（UserDetailedに二要素認証関連のプロパティが公開されているのと何ら変わらなくなる）ので目的を達成できなくなる（工夫次第かも？）

これサインイン試行でも同じこと起きるものだと私は認識してるのですが違う感じですか(現状の理解不足があるかも)

[kakkokari-gtyih]

発生したエラーを報告するような仕組みが入っていたとしたら、(正常な手順で)ログインするたびにエラーが報告されることになる

そういうシステムって単純なHTTPリクエストのエラーもログ取るの？（throwされたときやUnhandled Exceptionに動くものだと思っていたけど）

[kakkokari-gtyih]

これすると二要素認証が有効化されているかどうかなどが簡単に取れてしまう（UserDetailedに二要素認証関連のプロパティが公開されているのと何ら変わらなくなる）ので目的を達成できなくなる（工夫次第かも？）

これサインイン試行でも同じこと起きるものだと私は認識してるのですが違う感じですか(現状の理解不足があるかも)

二要素認証（ワンタイムパスワード）が必要かどうかはパスワードを入力して送信するまでわからない（次に必要な資格情報を一回ごとに返す仕様になっているので）

[syuilo]

エラーとして返ってくるのってそんなにまずいかしら

とてもまずい 発生したエラーを報告するような仕組みが入っていたとしたら、(正常な手順で)ログインするたびにエラーが報告されることになる

一般にエラーは問題だとして通知・報告される傾向にある 例えばブラウザの開発者ツールでもリクエストがエラーになったら目立つようにされている けど今回のケースはこれら一連のフローは事前に想定されたもので、すべて仕様通りに動いてるわけだから問題であるとして通知される必要はない

[kakkokari-gtyih]

確かに想定されたリクエストがエラーとして表示されるのは気持ち悪いかも vs 一般のWebアプリとかのAPI見たらちゃんと対応するステータスコード返しているので問題ないのかも

[anatawa12]

二要素認証（ワンタイムパスワード）が必要かどうかはパスワードを入力して送信するまでわからない（次に必要な資格情報を一回ごとに返す仕様になっているので）

なるほど

エラーを集める仕組みにかんしては後処理でどうにかするべきな気がする

別言語の例になっちゃうけどjavaのClassLoaderではとあるローダで見つからなかったのを例外とするんだけど、複数のローダを組み合わせて使う場合はそのエラーは別のローダへのフォールバックのトリガーになるので、ある意味意図しているもので、そういうのはthrowされたタイミングでログが残されるべきではないし残されないようにログの収集側がするべきだと私は思ってる

[syuilo]

本当に対応したステータスコードなのだろうか？

[syuilo]

API側もこのフローを想定して実装されているのだから、エラー系のステータスコードを返すのはおかしいように思う

[syuilo]

エラートラッキングサービスなどがあることからも分かるように、エラーは

• 報告されるべきもの
• 起こしてはならないもの
• 修正されるべきもの

といった観念が含まれているけど、今回はどれにも当てはまらない(仕様通りでこれを前提としたフローなのだから報告される必要もないし起こしてはならないものでもないし修正されるべきものでもない)

[anatawa12]

HTTPのステータスコードには起こさないべきものという程の意味はないと思うけどなぁ...(そういう意味付けをしてるツールがあるかもしれないけど)

400はリクエストのどっかが期待したもの(今回は2faトークン等)ではなかったということで、実際サインインするという操作については足りないっていうエラーを返すべきだと思う

[syuilo]

実態にあまりそぐわない /signin という名前だから「情報が足りなかったらエラーにする」のが正しいように思えてしまうのだと思う これが /process-signin-flow みたいな名前だとしたらどうだろうか

[kakkokari-gtyih]

MisskeyはRESTじゃないからべつに https://github.com/misskey-dev/misskey/issues/14699#issuecomment-2393416190 のプラクティスに絶対に則っておくべきということはない（これが公開APIなら仕様が一般的ではなく受け入れ難いかもしれないけど、内部API(外部アプリが使用しないエンドポイント)なのでそのへんのハンドリングはある程度やりやすいようにしても良いのかも）

[anatawa12]

これが /process-signin-flow みたいな名前だとしたらどうだろうか

あーこれなら200でもいいと感じる

[syuilo]

このAPIはサインインするにあたっての情報が足りない状態でリクエストされることを期待しているのだから何もエラー要素がない

[kakkokari-gtyih]

これが /process-signin-flow みたいな名前だとしたらどうだろうか

仮に /process-signin-flow 的な名称にするとして、/signin は消す？（Misskey Web上にあるのは対話式のサインインのみなのでおそらく使う機会はなくなる）

[syuilo]

消す

[kakkokari-gtyih]

/signin-flow とかでも良さそう