他のユーザーのメール通知設定を変更できないようにした #8149

Ryooo-k · 2024-10-23T11:00:21Z

Issue

#8056 他ユーザーのメール通知をOFFにできてしまう

概要

自分以外のユーザーのメール通知をOFFに設定できないようにしました。

変更前

HTMLでユーザーIDを改変すると、自分以外のユーザーのメール通知をOFFに設定することができる

変更後

URIのuser_IDとtokenが不整合の場合、アクセスできずメール通知をOFFにできない。

変更確認方法

bug/prevent-disabling-other-users-notifications をローカルに取り込む
i. git fetch origin pull/8149/head:bug/prevent-disabling-other-users-notifications
ii. git checkout bug/prevent-disabling-other-users-notifications
foreman start -f Procfile.dev でローカルサーバーを立ち上げる
通知設定変更画面にアクセスする（kimuraさんの通知設定変更の画面）
デベロッパーツールを開く（Mac：opt + cmd + i 、Windows：Ctrl + Shift + i ）
セレクトモードに切り替える。（下図の赤丸部分を選択する）

「オフにする」のボタンを押す

デベロッパーツール上のHTMLで、下図赤枠部が選択されるため、下記手順でtokenを変更する
i. "/users/991528156/mail_notification?token=k6Da-oL3cRi8ApNFO9-Gcg"にカーソルを当て、右クリック
ii. Edit attributeを選択
iii. "/users/991528156/mail_notification?token=k6Da-oL3cRi8ApNFO9-Gcg" を "/users/991528156/mail_notification?token=037i-ef5n7V4EnPv74mtyQ"に変更する
（k6Da-oL3cRi8ApNFO9-Gcgはkimuraさんのtoken、037i-ef5n7V4EnPv74mtyQはkomagataさんのtoken）

8.「オフにする」のボタンを押す

「ユーザーIDもしくはTOKENが違います。」のメッセージが表示されることを確認する。

ログイン済みの場合の画面

未ログインの場合の画面

ユーザー名 kimura 、パスワード testtest でログインする
kimuraさんの登録情報変更ページにアクセスし、メール通知がONになっていることを確認する

ログアウトする
ユーザー名 komagata 、パスワード testtest でログインする
komagataさんの登録情報変更ページにアクセスし、メール通知がONになっていることを確認する

Screenshot

変更前

全てのユーザーIDを受け入れ、他のユーザーのメール通知をOFFにできる。

変更後

URIのユーザーIDとtokenの整合が取れている場合、アクセスできメール通知をOFFにできる。

URIのユーザーIDとtokenが不整合の場合、アクセスできずメール通知をOFFにできない。

Ryooo-k · 2024-10-24T02:19:09Z

@Shrimprin
お疲れ様です！
こちらのレビューをお願いしたいのですが可能でしょうか？
ご確認お願いいたします🙏

Shrimprin · 2024-10-24T11:07:54Z

@Ryooo-k
お疲れ様です！レビュー承知いたしました 👍
今週の土日あたりで対応させていただきます〜

Shrimprin · 2024-10-26T05:58:00Z

@Ryooo-k
お疲れ様です！

動作については問題ないこと確認できました！ 👍

１点、仕様について質問です。
メール通知をOFFにする画面はメール通知をOFFにする対象のユーザー以外からはアクセスされる必要がないという認識です。
なので、画面へのアクセスにログイン必須かつログインユーザーがメール通知をOFFにする対象のユーザーと一致の条件を加えても良いと思ったのですがいかがでしょうか？

Ryooo-k · 2024-10-28T05:10:21Z

@Shrimprin
ご確認ありがとうございます！

画面へのアクセスにログイン必須かつログインユーザーがメール通知をOFFにする対象のユーザーと一致の条件を加えても良いと思ったのですがいかがでしょうか？

確かにメール通知OFF画面に対して直接アクセス制限をかけた方が良いかもしれないですね！
（アクセス制限かけたら現状の条件分岐は一つ減らせそうですし）
ログインユーザーがメール通知をOFFにする対象のユーザーと一致の条件は、結局メール通知OFF画面でHTMLを改変すれば今回のバグと同じ現象になってしまうと思うので、まずはアクセス制限をかけるように変更してみますね👍

Ryooo-k · 2024-10-29T04:42:22Z

@Shrimprin
お疲れ様です！
メール通知OFF画面へのアクセスにはログインを必要要件としました。再度、レビューお願いいたします🙏

Shrimprin

@Ryooo-k
お疲れ様です！
2点コメントしましたのでご確認お願いします 🙏

Shrimprin · 2024-10-29T23:30:14Z

app/controllers/users/mail_notification_controller.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true

 class Users::MailNotificationController < ApplicationController
-  skip_before_action :require_active_user_login, raise: false
  layout 'not_logged_in'


ログインを前提とした画面なので、非ログイン時のレイアウトを使う必要はないかもしれません。
意図があって残しているようであればOKです！

レイアウトが崩れるので'not_logged_in'は残しているんですが、ここ迷ったんですよね〜😕ファイル名変更は、他の複数ファイルで使用されており意味のある命名になっているようなので、ファイル名変更はやりたくはなく、かといってこのファイルのためだけに新しいファイルを作るのも違うかなーと思いました。
なので一旦、このままで進めます！

なるほどです！
ただ、ログインが必要なページにnot_logged_inというレイアウトを使用しているのは混乱を生むため避けた方がいいようにも思えます。
レイアウトが崩れてしまうということですが、CSSを修正して対応するのはいかがでしょうか？
デザインに関わる内容なので @machida さんに相談してみてもいいかと思います！

ご検討よろしくお願いいたします 🙏

Shrimprin · 2024-10-29T23:36:47Z

test/system/mail_notification_test.rb

@@ -3,10 +3,16 @@
 require 'application_system_test_case'

 class MailNotificationsTest < ApplicationSystemTestCase


他のユーザーの通知OFFを設定できないことのテストもあっていいと思います。
ログインユーザーとは別のユーザーのメール通知OFF画面にアクセスすることでテストできると思います。

ご指摘ありがとうございます！追加します👍

Ryooo-k · 2024-10-30T11:18:39Z

@Shrimprin
修正しましたのでご確認お願いします！

Shrimprin

@Ryooo-k
ご対応ありがとうございます！
追加でコメントさせていただきましたのでご確認よろしくお願いいたします 🙏

Shrimprin · 2024-10-30T12:43:14Z

test/system/mail_notification_test.rb

    assert_text 'メール通知をオフにしますか？'
    click_on 'オフにする'
    assert_text 'メール配信を停止しました。'
  end
+
+  test "update user's mail_notification settings without being logged in" do


テスト名はcan not update user's ... などとした方が内容を表していて分かりやすいと思います。

なるほど、can notを付けた方ができないことが強調されてわかりやすいですね。ありがとうございます。

Shrimprin · 2024-10-30T12:43:48Z

test/system/mail_notification_test.rb

+    assert_text 'ログインしてください'
+  end
+
+  test "update another user's mail_notification" do


another usersよりother usersの方が正確かもしれません。
また、こちらも同様にcan not update other user's ...などとした方が良いと思います。

Shrimprin · 2024-10-30T12:49:52Z

app/controllers/users/mail_notification_controller.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true

 class Users::MailNotificationController < ApplicationController
-  skip_before_action :require_active_user_login, raise: false
  layout 'not_logged_in'


なるほどです！
ただ、ログインが必要なページにnot_logged_inというレイアウトを使用しているのは混乱を生むため避けた方がいいようにも思えます。
レイアウトが崩れてしまうということですが、CSSを修正して対応するのはいかがでしょうか？
デザインに関わる内容なので @machida さんに相談してみてもいいかと思います！

ご検討よろしくお願いいたします 🙏

Ryooo-k · 2024-10-31T01:52:47Z

@Shrimprin
ご確認ありがとうございます！
レイアウトの部分は、一度machidaさんに相談してみます！

Ryooo-k · 2024-11-06T06:48:35Z

@machida
先日はお時間いただきありがとうございました！
内容の議事録と方針をまとめておきます。

相談内容

本issueのバグを修正するにあたり、メール通知画面へのアクセスにはログインを必須にしようとしている。layoutには layout 'not_logged_in' を採用しており、ログイン必須の条件とlayout 'not_logged_in'が相違している状態となった。

解決策１

layoutを変更する。この場合はmachidaさんにデザイン変更依頼をする必要がある。

解決策２

ログインを必須とする場合、ユーザビリティが悪くなる。ログインは必須とせず、解読や予測が難しいURIとした方が良い。フィヨルドトーーク！（GitHub）を参考にすると良い。

方針

ログインは不要で進めます。
フィヨルドトーーク！ではidはuuidで生成されるため、解読や予測が難しいURIとなっていました。
本アプリのidはserialで生成されるため、クエリパラメータにSecureRandom.urlsafe_base64メソッドで生成されるUserモデルのカラムのunsubscribe_email_tokenを追加し、そのtokenを用いて判定しようと思います。

Ryooo-k · 2024-11-07T07:07:53Z

@Shrimprin
修正しましたのでご確認お願いいたします！
修正のポイントをまとます。

ログインは不要としました（→そのためlayoutは変更なし）
予測不能なtokenからユーザー情報を取得
クエリパラメータのuser_idとtokenの整合性を確認し、不整合の場合は処理を実行しない

Shrimprin · 2024-11-08T09:35:32Z

@Ryooo-k
承知しました！いろいろと仕様をご検討いただきありがとうございます 🙏
土日の間に確認させていただきます～ 👍

Shrimprin · 2024-11-10T04:46:07Z

@Ryooo-k
確認しました！OKだと思います 👍

他のユーザーは知り得ないtokenを使い、メールを受け取った本人しか変更できなくしたのですね！
こういう方法もあるのだな〜勉強になりました 👏

Ryooo-k · 2024-11-11T00:04:45Z

@Shrimprin
ご確認ありがとうございます！
自分も勉強になりました！色々ご指摘ありがとうございました✨

@komagata
メンバーレビューが完了しましたので、メンターレビューをお願いいたします🙇

komagata

確認させて頂きました。OKです〜🙆‍♂️

Ryooo-k added 2 commits October 23, 2024 19:56

他ユーザーでメール通知をOFFにできないようにした

f3de490

ログイン時と未ログイン時のテストを追加

e5063bb

Ryooo-k marked this pull request as ready for review October 24, 2024 02:16

Ryooo-k changed the title ~~Bug/prevent disabling other users notifications~~ 他のユーザーのメール通知設定を変更できないようにした Oct 24, 2024

Ryooo-k self-assigned this Oct 24, 2024

Ryooo-k requested a review from Shrimprin October 24, 2024 02:18

メール通知OFF画面にアクセスする際はログインを必須とした

e710ae3

Shrimprin reviewed Oct 29, 2024

View reviewed changes

無効な操作のテストパターンを追加した

99dce22

Shrimprin reviewed Oct 30, 2024

View reviewed changes

Ryooo-k added 2 commits November 7, 2024 15:05

ログイン不要とし、tokenとuser_idでアクセス制御するようにした

c05a690

不適切なテスト名を修正

46940d4

Shrimprin approved these changes Nov 10, 2024

View reviewed changes

komagata approved these changes Nov 15, 2024

View reviewed changes

komagata merged commit 71589d6 into main Nov 15, 2024
4 checks passed

komagata deleted the bug/prevent-disabling-other-users-notifications branch November 15, 2024 04:46

github-actions bot mentioned this pull request Nov 15, 2024

Release 2024-11-18 04:22:29 +0000 #8180

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

他のユーザーのメール通知設定を変更できないようにした #8149

他のユーザーのメール通知設定を変更できないようにした #8149

Ryooo-k commented Oct 23, 2024 •

edited

Loading

Ryooo-k commented Oct 24, 2024

Shrimprin commented Oct 24, 2024

Shrimprin commented Oct 26, 2024

Ryooo-k commented Oct 28, 2024

Ryooo-k commented Oct 29, 2024

Shrimprin left a comment

Shrimprin Oct 29, 2024

Ryooo-k Oct 30, 2024 •

edited

Loading

Shrimprin Oct 30, 2024

Shrimprin Oct 29, 2024

Ryooo-k Oct 30, 2024

Ryooo-k commented Oct 30, 2024

Shrimprin left a comment

Shrimprin Oct 30, 2024

Ryooo-k Oct 31, 2024

Shrimprin Oct 30, 2024

Shrimprin Oct 30, 2024

Ryooo-k commented Oct 31, 2024

Ryooo-k commented Nov 6, 2024 •

edited

Loading

Ryooo-k commented Nov 7, 2024

Shrimprin commented Nov 8, 2024

Shrimprin commented Nov 10, 2024

Ryooo-k commented Nov 11, 2024

komagata left a comment

		@@ -3,10 +3,16 @@
		require 'application_system_test_case'

		class MailNotificationsTest < ApplicationSystemTestCase

他のユーザーのメール通知設定を変更できないようにした #8149

他のユーザーのメール通知設定を変更できないようにした #8149

Conversation

Ryooo-k commented Oct 23, 2024 • edited Loading

Issue

概要

変更前

変更後

変更確認方法

Screenshot

変更前

変更後

Ryooo-k commented Oct 24, 2024

Shrimprin commented Oct 24, 2024

Shrimprin commented Oct 26, 2024

Ryooo-k commented Oct 28, 2024

Ryooo-k commented Oct 29, 2024

Shrimprin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryooo-k Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryooo-k commented Oct 30, 2024

Shrimprin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryooo-k commented Oct 31, 2024

Ryooo-k commented Nov 6, 2024 • edited Loading

相談内容

解決策１

解決策２

方針

Ryooo-k commented Nov 7, 2024

Shrimprin commented Nov 8, 2024

Shrimprin commented Nov 10, 2024

Ryooo-k commented Nov 11, 2024

komagata left a comment

Choose a reason for hiding this comment

Ryooo-k commented Oct 23, 2024 •

edited

Loading

Ryooo-k Oct 30, 2024 •

edited

Loading

Ryooo-k commented Nov 6, 2024 •

edited

Loading