From 788ca1f11203c3c07b094f7ba4386a1a3373aa8a Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 22 May 2018 09:06:46 +1000 Subject: [PATCH] FIX: stop adding email to unsubscribe url Instead of adding email to unsubscribe url store it in redis for 1 hour rate limit calls to unsubscribe endpoint to ensure there is no risk of bloating redis Also move controller to request specs --- app/controllers/email_controller.rb | 17 ++- spec/controllers/email_controller_spec.rb | 129 --------------------- spec/requests/email_controller_spec.rb | 135 +++++++++++++++++++++- 3 files changed, 145 insertions(+), 136 deletions(-) diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb index cff20ff7c05..644876248f6 100644 --- a/app/controllers/email_controller.rb +++ b/app/controllers/email_controller.rb @@ -41,6 +41,8 @@ class EmailController < ApplicationController end def perform_unsubscribe + RateLimiter.new(nil, "unsubscribe_#{request.ip}", 10, 1.minute).performed! + key = UnsubscribeKey.find_by(key: params[:key]) raise Discourse::NotFound unless key && key.user @@ -99,19 +101,24 @@ class EmailController < ApplicationController unless updated redirect_back fallback_location: path("/") else + + key = "unsub_#{SecureRandom.hex}" + $redis.setex key, 1.hour, user.email + + url = path("/email/unsubscribed?key=#{key}") if topic - redirect_to path("/email/unsubscribed?topic_id=#{topic.id}&email=#{user.email}") - else - redirect_to path("/email/unsubscribed?email=#{user.email}") + url += "&topic_id=#{topic.id}" end + + redirect_to url end end def unsubscribed - @email = params[:email] + @email = $redis.get(params[:key]) @topic_id = params[:topic_id] - user = User.find_by_email(params[:email]) + user = User.find_by_email(@email) raise Discourse::NotFound unless user topic = Topic.find_by(id: params[:topic_id].to_i) if @topic_id @topic = topic if topic && Guardian.new(nil).can_see?(topic) diff --git a/spec/controllers/email_controller_spec.rb b/spec/controllers/email_controller_spec.rb index 8f112c8334f..4fdab4cca6d 100644 --- a/spec/controllers/email_controller_spec.rb +++ b/spec/controllers/email_controller_spec.rb @@ -20,135 +20,6 @@ describe EmailController do end - context '.perform unsubscribe' do - it 'raises not found on invalid key' do - post :perform_unsubscribe, params: { key: "123" }, format: :json - expect(response.status).to eq(404) - end - - it 'can fully unsubscribe' do - user = Fabricate(:user) - key = UnsubscribeKey.create_key_for(user, "all") - - user.user_option.update_columns(email_always: true, - email_digests: true, - email_direct: true, - email_private_messages: true) - - post :perform_unsubscribe, - params: { key: key, unsubscribe_all: "1" }, - format: :json - - expect(response.status).to eq(302) - - user.user_option.reload - - expect(user.user_option.email_always).to eq(false) - expect(user.user_option.email_digests).to eq(false) - expect(user.user_option.email_direct).to eq(false) - expect(user.user_option.email_private_messages).to eq(false) - - end - - it 'can disable mailing list' do - user = Fabricate(:user) - key = UnsubscribeKey.create_key_for(user, "all") - - user.user_option.update_columns(mailing_list_mode: true) - - post :perform_unsubscribe, - params: { key: key, disable_mailing_list: "1" }, - format: :json - - expect(response.status).to eq(302) - - user.user_option.reload - - expect(user.user_option.mailing_list_mode).to eq(false) - end - - it 'can disable digest' do - user = Fabricate(:user) - key = UnsubscribeKey.create_key_for(user, "all") - - user.user_option.update_columns(email_digests: true) - - post :perform_unsubscribe, - params: { key: key, disable_digest_emails: "1" }, - format: :json - - expect(response.status).to eq(302) - - user.user_option.reload - - expect(user.user_option.email_digests).to eq(false) - end - - it 'can unwatch topic' do - p = Fabricate(:post) - key = UnsubscribeKey.create_key_for(p.user, p) - - TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching]) - - post :perform_unsubscribe, - params: { key: key, unwatch_topic: "1" }, - format: :json - - expect(response.status).to eq(302) - - expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:tracking]) - end - - it 'can mute topic' do - p = Fabricate(:post) - key = UnsubscribeKey.create_key_for(p.user, p) - - TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching]) - - post :perform_unsubscribe, - params: { key: key, mute_topic: "1" }, - format: :json - - expect(response.status).to eq(302) - - expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:muted]) - end - - it 'can unwatch category' do - p = Fabricate(:post) - key = UnsubscribeKey.create_key_for(p.user, p) - - cu = CategoryUser.create!(user_id: p.user.id, - category_id: p.topic.category_id, - notification_level: CategoryUser.notification_levels[:watching]) - - post :perform_unsubscribe, - params: { key: key, unwatch_category: "1" }, - format: :json - - expect(response.status).to eq(302) - - expect(CategoryUser.find_by(id: cu.id)).to eq(nil) - end - - it 'can unwatch first post from category' do - p = Fabricate(:post) - key = UnsubscribeKey.create_key_for(p.user, p) - - cu = CategoryUser.create!(user_id: p.user.id, - category_id: p.topic.category_id, - notification_level: CategoryUser.notification_levels[:watching_first_post]) - - post :perform_unsubscribe, - params: { key: key, unwatch_category: "1" }, - format: :json - - expect(response.status).to eq(302) - - expect(CategoryUser.find_by(id: cu.id)).to eq(nil) - end - end - context '.unsubscribe' do render_views diff --git a/spec/requests/email_controller_spec.rb b/spec/requests/email_controller_spec.rb index c51aa276f21..196b3b277a5 100644 --- a/spec/requests/email_controller_spec.rb +++ b/spec/requests/email_controller_spec.rb @@ -5,6 +5,133 @@ RSpec.describe EmailController do let(:topic) { Fabricate(:topic) } let(:private_topic) { Fabricate(:private_message_topic) } + context '.perform unsubscribe' do + it 'raises not found on invalid key' do + post "/email/unsubscribe/123.json" + expect(response.status).to eq(404) + end + + it 'can fully unsubscribe' do + user = Fabricate(:user) + key = UnsubscribeKey.create_key_for(user, "all") + + user.user_option.update_columns(email_always: true, + email_digests: true, + email_direct: true, + email_private_messages: true) + + post "/email/unsubscribe/#{key}.json", + params: { unsubscribe_all: "1" } + + expect(response.status).to eq(302) + + get response.redirect_url + + # cause it worked ... yay + expect(body).to include(user.email) + + user.user_option.reload + + expect(user.user_option.email_always).to eq(false) + expect(user.user_option.email_digests).to eq(false) + expect(user.user_option.email_direct).to eq(false) + expect(user.user_option.email_private_messages).to eq(false) + + end + + it 'can disable mailing list' do + user = Fabricate(:user) + key = UnsubscribeKey.create_key_for(user, "all") + + user.user_option.update_columns(mailing_list_mode: true) + + post "/email/unsubscribe/#{key}.json", + params: { disable_mailing_list: "1" } + + expect(response.status).to eq(302) + + user.user_option.reload + + expect(user.user_option.mailing_list_mode).to eq(false) + end + + it 'can disable digest' do + user = Fabricate(:user) + key = UnsubscribeKey.create_key_for(user, "all") + + user.user_option.update_columns(email_digests: true) + + post "/email/unsubscribe/#{key}.json", + params: { disable_digest_emails: "1" } + + expect(response.status).to eq(302) + + user.user_option.reload + + expect(user.user_option.email_digests).to eq(false) + end + + it 'can unwatch topic' do + p = Fabricate(:post) + key = UnsubscribeKey.create_key_for(p.user, p) + + TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching]) + + post "/email/unsubscribe/#{key}.json", + params: { unwatch_topic: "1" } + + expect(response.status).to eq(302) + + expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:tracking]) + end + + it 'can mute topic' do + p = Fabricate(:post) + key = UnsubscribeKey.create_key_for(p.user, p) + + TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching]) + + post "/email/unsubscribe/#{key}.json", + params: { mute_topic: "1" } + + expect(response.status).to eq(302) + + expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:muted]) + end + + it 'can unwatch category' do + p = Fabricate(:post) + key = UnsubscribeKey.create_key_for(p.user, p) + + cu = CategoryUser.create!(user_id: p.user.id, + category_id: p.topic.category_id, + notification_level: CategoryUser.notification_levels[:watching]) + + post "/email/unsubscribe/#{key}.json", + params: { unwatch_category: "1" } + + expect(response.status).to eq(302) + + expect(CategoryUser.find_by(id: cu.id)).to eq(nil) + end + + it 'can unwatch first post from category' do + p = Fabricate(:post) + key = UnsubscribeKey.create_key_for(p.user, p) + + cu = CategoryUser.create!(user_id: p.user.id, + category_id: p.topic.category_id, + notification_level: CategoryUser.notification_levels[:watching_first_post]) + + post "/email/unsubscribe/#{key}.json", + params: { unwatch_category: "1" } + + expect(response.status).to eq(302) + + expect(CategoryUser.find_by(id: cu.id)).to eq(nil) + end + end + describe '#unsubscribed' do describe 'when email is invalid' do it 'should return the right response' do @@ -15,7 +142,9 @@ RSpec.describe EmailController do describe 'when topic is public' do it 'should return the right response' do - get '/email/unsubscribed', params: { email: user.email, topic_id: topic.id } + key = SecureRandom.hex + $redis.set(key, user.email) + get '/email/unsubscribed', params: { key: key, topic_id: topic.id } expect(response).to be_success expect(response.body).to include(topic.title) end @@ -23,7 +152,9 @@ RSpec.describe EmailController do describe 'when topic is private' do it 'should return the right response' do - get '/email/unsubscribed', params: { email: user.email, topic_id: private_topic.id } + key = SecureRandom.hex + $redis.set(key, user.email) + get '/email/unsubscribed', params: { key: key, topic_id: private_topic.id } expect(response).to be_success expect(response.body).to_not include(private_topic.title) end