Add additional step for unlocking account#14
Add additional step for unlocking account#14kossy0701 wants to merge 1 commit intogreptile-playgroundfrom
Conversation
|
※このコメントは Greptile による自動コードレビューです。 Greptile OverviewGreptile Summaryこの PR は、メールクライアントの自動リンク訪問によってアカウントのアンロックが意図せず実行されてしまう問題を解決するため、 主な変更内容
発見された問題
良い点
Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant EmailClient
participant Browser
participant UnlocksController
participant Lockable
participant Database
Note over User,Database: アカウントロック時（既存の動作）
User->>UnlocksController: POST /resource/unlock (メール送信リクエスト)
UnlocksController->>Lockable: send_unlock_instructions
Lockable->>Database: unlock_token を保存
Lockable->>User: アンロックメールを送信
Note over User,Database: extra_step が false の場合（既存の動作）
EmailClient->>UnlocksController: GET /resource/unlock (パラメータ付き、自動訪問)
UnlocksController->>UnlocksController: show アクション
UnlocksController->>UnlocksController: attempt_unlock を呼び出し
UnlocksController->>Lockable: unlock_access_by_token
Lockable->>Database: アカウントのロックを解除
UnlocksController->>EmailClient: ログインページへリダイレクト
Note right of EmailClient: メールクライアントの自動訪問で<br/>アンロックされてしまう
Note over User,Database: extra_step が true の場合（新しい動作）
EmailClient->>UnlocksController: GET /resource/unlock (パラメータ付き、自動訪問)
UnlocksController->>UnlocksController: show アクション
UnlocksController->>Browser: 確認ページを表示 (render :show)
Note right of Browser: アカウントはまだロックされたまま<br/>確認リンクを表示
User->>Browser: 確認ページを開く
Browser->>Browser: 「Unlock account」リンクをクリック
Browser->>UnlocksController: GET /resource/unlock/confirm (パラメータ付き)
UnlocksController->>UnlocksController: confirm アクション
UnlocksController->>UnlocksController: attempt_unlock を呼び出し
UnlocksController->>Lockable: unlock_access_by_token
Lockable->>Database: アカウントのロックを解除
UnlocksController->>Browser: ログインページへリダイレクト
Note right of User: ユーザーの明示的な操作で<br/>アンロックが完了
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def show | ||
| if resource_class.extra_step | ||
| @token = params[:unlock_token] | ||
| render :show | ||
| else | ||
| attempt_unlock | ||
| end |
There was a problem hiding this comment.
logic: extra_step が有効な場合の挙動が考慮されていません。
観察: show アクションでは extra_step が有効な場合に HTML ビューをレンダリングしていますが、JSON リクエストの場合の処理が明示的に定義されていません。
理由: render :show は JSON リクエストでは適切なレスポンスを返せない可能性があります。extra_step: true の場合、JSON クライアントは HTML テンプレートまたはエラーを受け取ることになり、API として正しく機能しません。
結論: API/JSON クライアントとの互換性が損なわれる可能性があります。
修正方法: 以下のようにフォーマットに応じた分岐を追加してください。
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/devise/unlocks_controller.rb
Line: 24:30
Comment:
**logic:** ⚠️ **警告**: JSON/API リクエストで `extra_step` が有効な場合の挙動が考慮されていません。
**観察**: `show` アクションでは `extra_step` が有効な場合に HTML ビューをレンダリングしていますが、JSON リクエストの場合の処理が明示的に定義されていません。
**理由**: `render :show` は JSON リクエストでは適切なレスポンスを返せない可能性があります。`extra_step: true` の場合、JSON クライアントは HTML テンプレートまたはエラーを受け取ることになり、API として正しく機能しません。
**結論**: API/JSON クライアントとの互換性が損なわれる可能性があります。
**修正方法**: 以下のようにフォーマットに応じた分岐を追加してください。
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
結論から言うと：
- 全体のサマリと設計理解はだいたい妥当
- HTMLタグの指摘は「本当に
<p>が閉じ忘れてたなら妥当（ただし重大かと言われるとややオーバー） - JSON/API の警告は「言ってることは分かるけど、Devise の設計前提を考えるとちょっと踏み込みすぎ」
という感じかなと思います。
1. 何を言っているコメントか整理
Greptile はざっくりこう言ってますね：
-
概要
-
extra_step設定を追加して、GET /resource/unlock?unlock_token=...は「確認ページを表示するだけ」に変更- 新しい
GET /resource/unlock/confirm?unlock_token=...をクリックしたときに本当に unlock する
-
デフォルトは
extra_step = falseなので後方互換性あり
-
-
問題点
- 「2つの view（
app/views/devise/unlocks/show.html.erbと generator の simple_form テンプレート）で<p>が</p>じゃなくて<p>で閉じられている（HTML 構文エラー）」 - 「
showアクションがextra_step有効時に HTML しか考慮しておらず、JSON/API リクエストの互換性が怪しい」
- 「2つの view（
-
良い点
- メールクライアントの自動リンク踏み問題への対処
- 後方互換性を保っている
- HTML フローについてのテストは十分
大枠としては、今回やりたいこと（2段階 unlock）をちゃんと理解してコメントしているので、
「見当違いなサマリ」ではありません。
2. HTML <p> タグの指摘について
2つのビューファイルで HTML タグの閉じ忘れ（
<p>を</p>じゃなく<p>で閉じてる）
ここは、実際の差分をエディタで見て確認する必要がありますが：
-
本当にそうなっていたなら
→ 「タグ閉じミス」はたしかにバグなので 指摘としては妥当 -
メールテンプレートなので、構文ミスは一部クライアントで表示崩れを起こし得る
- ただし多くのメールクライアントは HTML をかなり寛容に補完してくれるので、
「重大」とまで言うかはちょっと大げさかも
- ただし多くのメールクライアントは HTML をかなり寛容に補完してくれるので、
アクションとしては：
-
show.html.erbと simple_form のunlocks/show.html.erbを実際に開いて、<p>...</p>になっているか- 閉じタグが足りないところがないか
-
あれば素直に直して OK（これは人間も気付きにくい typo なので、Greptile の価値あるキャッチだと思います）
3. JSON/API リクエストに関する警告
該当コードはこんな感じですよね（diff から見える範囲）：
# GET /resource/unlock?unlock_token=abcdef
def show
if resource_class.extra_step
@token = params[:unlock_token]
render :show
else
attempt_unlock
end
end
# GET /resource/unlock/confirm?unlock_token=abcdef
def confirm
attempt_unlock
end
private
def attempt_unlock
self.resource = resource_class.unlock_access_by_token(params[:unlock_token])
yield resource if block_given?
respond_with_navigational(resource.errors, status: :unprocessable_entity) { render :new }
endGreptile はここに対して：
- extra_step: true のとき、
showが HTML のことしか考えていなくて
JSON クライアントにとってはおかしな挙動になるかも- format ごとの分岐を追加すべき
と言っています。
ここが「半分妥当・半分微妙」な理由
-
技術的には：
-
render :showは HTML テンプレート前提 なので、- JSON で
/unlockを叩かれたら「HTML が返ってくる or Missing template になる」可能性はたしかにある
- JSON で
-
なので「API としてちゃんと JSON をサポートしたい」前提なら、
respond_toでformat.html/format.jsonを分けるextra_stepはformat.htmlのときだけ有効にする
などを検討した方がきれい
-
-
ただ、Devise 本体の
UnlocksControllerも、もともと かなり「ナビゲーション用（HTML）」寄りの設計 です：after_unlock_path_for(resource)もis_navigational_format?を見てますし、- unlock のリンク自体が「メール内のリンクをブラウザで踏む」前提のフロー
つまり、
「unlock を JSON API 経由でちゃんとサポートする」という前提がないなら、
JSON についてここまで心配するのは若干オーバースペック
とも言えます。
どう扱うのが現実的か
-
あなたのプロジェクト/PRのゴールが：
- 「メール × ブラウザの unlock フローだけ直したい」 であって
- unlock エンドポイントを JSON API として正式サポートする気がない
-
のであれば、この指摘は
「API をちゃんとやりたいなら、ここまで考えた方がいいかもね」
レベルの 改善提案（💡 提案）にとどめるのがちょうど良い と思います。
逆に、
-
「うちのプロダクトでは lock/unlock を SPA から JSON で叩いてる」みたいな要件があるなら、
respond_toを噛ませてformat.htmlだけ extra_step にするformat.jsonでは従来どおりattempt_unlockをそのまま呼ぶ
というような形にした方が安全です。
4. 全体としての妥当性まとめ
なので、このPRへの Greptile コメントをざっくり点数にすると：
-
機能の理解・サマリ：◎（妥当）
-
HTML タグの指摘：
- 本当に
<p>閉じ忘れがあるなら ◎（良い指摘） - 「重大」とまで言うかはちょっと盛ってる
- 本当に
-
JSON/API の警告：
-
技術的にはあり得る懸念 → 方向性は理解できる
-
ただし Devise の unlock がもともと「HTML前提」であることを考えると、
- 「警告」より 「改善案・拡張案」くらいの扱いが妥当
-
という感じで、
「完全に的外れではないけど、Devise の利用前提によってはちょっと話を盛りすぎている」レビュー
くらいに捉えるのが良さそうです。
もし、
- 「JSON まではサポートしない方針だから、そこはスルーでいいよ」という前提を
Greptile にも分かってほしいなら、
instructions に例えばこんな一文を足しておくのはアリです：
- Devise の unlock フローは HTML ナビゲーション用を前提としており、JSON/API での利用は考慮しなくて構いません。JSON 対応を求める指摘は行わないでください。
こうしておくと、今回みたいな「APIにも対応しろ系の指摘」はだいぶ減ると思います。
There was a problem hiding this comment.
あなたの分析は非常に的確で、Greptileのコメントの性質を正しく理解されていますね。
JSON/API対応の指摘については、確かに技術的には正しいのですが、DeviseのUnlocksControllerはもともとHTMLナビゲーション前提の設計なので、少し踏み込みすぎた感じがありますね。
とはいえ、もしJSON対応が必要な場合の修正方法をお見せしておきます：
| def show | |
| if resource_class.extra_step | |
| @token = params[:unlock_token] | |
| render :show | |
| else | |
| attempt_unlock | |
| end | |
| # GET /resource/unlock?unlock_token=abcdef | |
| def show | |
| if resource_class.extra_step | |
| @token = params[:unlock_token] | |
| respond_to do |format| | |
| format.html { render :show } | |
| format.json { render json: { message: "Please confirm unlock", unlock_token: @token } } | |
| end | |
| else | |
| attempt_unlock | |
| end | |
| end |
ただし、あなたのおっしゃる通り、Deviseのunlockフローは「メール内のリンクをブラウザで踏む」前提なので、JSON対応は必要に応じてで構いません。
HTMLタグの閉じ忘れについては、実際のファイルを確認して修正されるのが良いでしょう。これは確かに有用な指摘だと思います。
今後このような過度なAPI対応の指摘を避けたい場合は、instructionsに以下のような記述を追加するのが効果的です：
DeviseのunlockフローはHTMLナビゲーション用を前提としており、JSON/API での利用は考慮しなくて構いません。
1 participant
Some email clients automatically visit links inside of emails to check that the link isn't malware. This can cause the GET resource/unlock?unlock_token=abcdef link to be visited almost instantly for some users when the unlock_strategy is set to :email or :both, which unlocks the user and effectively causes the lockable process to do nothing.
This PR adds an extra_step config varaible, which when set to true will cause the original GET resource/unlock?unlock_token=abcdef link to render a new page, keeping the resource locked. This new page contains a different link (GET resource/unlock/confirm?unlock_token=abcdef), which when clicked will unlock the resource. When extra_step is false, the app will behave as normal.