/signin の設計がおかしい可能性がある #14699
Comments
syuilo
added
packages/frontend
|
成功時も次の入力項目を指示するときも200にして、レスポンスをこんな感じにする？ 成功時{
"success": true,
"id": "xxxxxxxx",
"i": "xxxxxxxx"
}次の入力項目を指示{
"success": false,
"next": "password"
} |
|
そうね |
|
これでいくか |
|
successというよりfinishとかの方がよさそう |
|
私はおかしくないと思います。 資格情報が足りない状態でのレスポンスに200は強い違和感を持ちます 403が違うってのはある程度同意しますし、401は不適当ですし400あたりがよいと私は感じます |
|
API側の実装がこれで正しいとしたら、クライアント側をエラーが出ないようにリクエストするように修正するべきだわね |
200を返さずにエラーを出さずにそれをしようとしたら結局前のログイン画面に戻ってしまう（新ログイン画面はエラーが返ってくる or 次の操作が示されることを前提に組まれている） |
|
そう、だからどちらかの設計がおかしいということになる |
|
エラーとして返ってくるのってそんなにまずいかしら |
|
そもそもsigninという名のAPIがsignin以外の処理(次のステップを指定するなど)を担当している時点で若干おかしい |
とてもまずい |
|
4xxを期待してリクエストすることも私はおかしくないと考えます。 私はHTTPのステータスコード上のエラーとプログラム上のthrowされるようなエラーとは別物と考えてます |
|
ステータスコードじゃなく、実際にプログラム上もエラーやfailedとして表現されている |
|
バックエンド的にはエラーではあると思う。 フロント的にはthrowしない方が適切かも |
|
バックエンド的にエラーなのだとしたら、フロントエンドはバックエンドがエラーになってしまわないように正しく実装するべきという話になる |
これも資格情報不足でチャレンジ種別を返すはそこまでおかしくないと思います(401もそうなってるし) |
これはそうしてあるはず |
|
そこまでおかしくはないけど、ちょっとはおかしい(直感的ではない) |
|
別のエンドポイントとして必要資格情報を返すエンドポイントは追加した方が良いかも けどsigninは足りない資格情報がある時は4xxを返してあげるべきだと思う。(ここに必要資格情報を含めるかはどっちでもいいけど今あるならついてて良さそう) |
これすると二要素認証が有効化されているかどうかなどが簡単に取れてしまう（UserDetailedに二要素認証関連のプロパティが公開されているのと何ら変わらなくなる）ので目的を達成できなくなる（工夫次第かも？） |
|
エンドポイント分けると場合によってはリクエストが二度手間というか無駄になる場合がありそう |
これサインイン試行でも同じこと起きるものだと私は認識してるのですが違う感じですか(現状の理解不足があるかも) |
そういうシステムって単純なHTTPリクエストのエラーもログ取るの？（throwされたときやUnhandled Exceptionに動くものだと思っていたけど） |
二要素認証（ワンタイムパスワード）が必要かどうかはパスワードを入力して送信するまでわからない（次に必要な資格情報を一回ごとに返す仕様になっているので） |
一般にエラーは問題だとして通知・報告される傾向にある |
|
確かに想定されたリクエストがエラーとして表示されるのは気持ち悪いかも vs 一般のWebアプリとかのAPI見たらちゃんと対応するステータスコード返しているので問題ないのかも
|
なるほど エラーを集める仕組みにかんしては後処理でどうにかするべきな気がする 別言語の例になっちゃうけどjavaのClassLoaderではとあるローダで見つからなかったのを例外とするんだけど、複数のローダを組み合わせて使う場合はそのエラーは別のローダへのフォールバックのトリガーになるので、ある意味意図しているもので、そういうのはthrowされたタイミングでログが残されるべきではないし残されないようにログの収集側がするべきだと私は思ってる |
|
本当に対応したステータスコードなのだろうか？ |
|
API側もこのフローを想定して実装されているのだから、エラー系のステータスコードを返すのはおかしいように思う |
|
エラートラッキングサービスなどがあることからも分かるように、エラーは
といった観念が含まれているけど、今回はどれにも当てはまらない(仕様通りでこれを前提としたフローなのだから報告される必要もないし起こしてはならないものでもないし修正されるべきものでもない) |
|
HTTPのステータスコードには起こさないべきものという程の意味はないと思うけどなぁ...(そういう意味付けをしてるツールがあるかもしれないけど) 400はリクエストのどっかが期待したもの(今回は2faトークン等)ではなかったということで、実際サインインするという操作については足りないっていうエラーを返すべきだと思う |
|
実態にあまりそぐわない /signin という名前だから「情報が足りなかったらエラーにする」のが正しいように思えてしまうのだと思う |
|
MisskeyはRESTじゃないからべつに #14699 (comment) のプラクティスに絶対に則っておくべきということはない（これが公開APIなら仕様が一般的ではなく受け入れ難いかもしれないけど、内部API(外部アプリが使用しないエンドポイント)なのでそのへんのハンドリングはある程度やりやすいようにしても良いのかも） |
あーこれなら200でもいいと感じる |
|
このAPIはサインインするにあたっての情報が足りない状態でリクエストされることを期待しているのだから何もエラー要素がない |
仮に |
|
消す |
|
|
Summary
仕様通り想定して飛んできているリクエストに対してエラーを返すのはおかしい
Purpose
あ
Do you want to implement this feature yourself?
The text was updated successfully, but these errors were encountered: