diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb index 01d07426cd2..6c47bc406f0 100644 --- a/app/controllers/email_controller.rb +++ b/app/controllers/email_controller.rb @@ -10,7 +10,7 @@ class EmailController < ApplicationController end def unsubscribe - @user = User.find_by_temporary_key(params[:key]) + @user = DigestUnsubscribeKey.user_for_key(params[:key]) # Don't allow the use of a key while logged in as a different user if current_user.present? && (@user != current_user) @@ -28,7 +28,7 @@ class EmailController < ApplicationController end def resubscribe - @user = User.find_by_temporary_key(params[:key]) + @user = DigestUnsubscribeKey.user_for_key(params[:key]) raise Discourse::NotFound unless @user.present? @user.update_column(:email_digests, true) end diff --git a/app/jobs/scheduled/clean_up_digest_keys.rb b/app/jobs/scheduled/clean_up_digest_keys.rb new file mode 100644 index 00000000000..a74eee87eb3 --- /dev/null +++ b/app/jobs/scheduled/clean_up_digest_keys.rb @@ -0,0 +1,13 @@ +module Jobs + + class CleanUpDigestKeys < Jobs::Scheduled + every 1.day + + def execute(args) + DigestUnsubscribeKey.where('created_at < ?', 2.months.ago).delete_all + end + + end + +end + diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 310f8cd8fa5..4c249adcacf 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -69,6 +69,7 @@ class UserNotifications < ActionMailer::Base @featured_topics, @new_topics = @featured_topics[0..4], @featured_topics[5..-1] @markdown_linker = MarkdownLinker.new(Discourse.base_url) + @unsubscribe_key = DigestUnsubscribeKey.create_key_for(@user) build_email user.email, from_alias: I18n.t('user_notifications.digest.from', site_name: SiteSetting.title), diff --git a/app/models/digest_unsubscribe_key.rb b/app/models/digest_unsubscribe_key.rb new file mode 100644 index 00000000000..5f48e181e75 --- /dev/null +++ b/app/models/digest_unsubscribe_key.rb @@ -0,0 +1,19 @@ +class DigestUnsubscribeKey < ActiveRecord::Base + belongs_to :user + + before_create :generate_random_key + + def self.create_key_for(user) + DigestUnsubscribeKey.create(user_id: user.id).key + end + + def self.user_for_key(key) + where(key: key).first.try(:user) + end + + private + + def generate_random_key + self.key = SecureRandom.hex(32) + end +end diff --git a/app/models/user.rb b/app/models/user.rb index c0de2fd439e..20f159e86f3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -164,14 +164,6 @@ class User < ActiveRecord::Base name.titleize end - # Find a user by temporary key, nil if not found or key is invalid - def self.find_by_temporary_key(key) - user_id = $redis.get("temporary_key:#{key}") - if user_id.present? - find_by(id: user_id.to_i) - end - end - def self.find_by_username_or_email(username_or_email) if username_or_email.include?('@') find_by_email(username_or_email) @@ -203,13 +195,6 @@ class User < ActiveRecord::Base save end - # Use a temporary key to find this user, store it in redis with an expiry - def temporary_key - key = SecureRandom.hex(32) - $redis.setex "temporary_key:#{key}", 2.months, id.to_s - key - end - def created_topic_count stat = user_stat || create_user_stat stat.topic_count diff --git a/app/views/user_notifications/digest.html.erb b/app/views/user_notifications/digest.html.erb index 70fb28aad10..fda5eb5720c 100644 --- a/app/views/user_notifications/digest.html.erb +++ b/app/views/user_notifications/digest.html.erb @@ -73,6 +73,6 @@ diff --git a/app/views/user_notifications/digest.text.erb b/app/views/user_notifications/digest.text.erb index 8c86148aafc..f4e8a78252f 100644 --- a/app/views/user_notifications/digest.text.erb +++ b/app/views/user_notifications/digest.text.erb @@ -33,6 +33,6 @@ <%=raw(t :'user_notifications.digest.unsubscribe', site_link: site_link, - unsubscribe_link: raw(@markdown_linker.create(t('user_notifications.digest.click_here'), email_unsubscribe_url(key: @user.temporary_key, only_path: true)))) %> + unsubscribe_link: raw(@markdown_linker.create(t('user_notifications.digest.click_here'), email_unsubscribe_url(key: @unsubscribe_key, only_path: true)))) %> <%= raw(@markdown_linker.references) %> diff --git a/db/migrate/20150213174159_create_digest_unsubscribe_keys.rb b/db/migrate/20150213174159_create_digest_unsubscribe_keys.rb new file mode 100644 index 00000000000..102146b6aa0 --- /dev/null +++ b/db/migrate/20150213174159_create_digest_unsubscribe_keys.rb @@ -0,0 +1,46 @@ +class CreateDigestUnsubscribeKeys < ActiveRecord::Migration + def up + create_table :digest_unsubscribe_keys, id: false do |t| + t.string :key, limit: 64, null: false + t.integer :user_id, null: false + t.timestamps + end + execute "ALTER TABLE digest_unsubscribe_keys ADD PRIMARY KEY (key)" + add_index :digest_unsubscribe_keys, :created_at + + migrate_redis_keys + end + + # It is slightly odd to migrate from redis to postgres; I imagine a lot + # could fail, so if anything does we just rescue + def migrate_redis_keys + return if Rails.env.test? + + temp_keys = $redis.keys('temporary_key:*') + if temp_keys.present? + temp_keys.map! do |key| + user_id = $redis.get(key).to_i + ttl = $redis.ttl(key).to_i + + if ttl > 0 + ttl = "'#{ttl.seconds.ago.strftime('%Y-%m-%d %H:%M:%S')}'" + else + ttl = "CURRENT_TIMESTAMP" + end + $redis.del(key) + key.gsub!('temporary_key:', '') + user_id ? "('#{key}', #{user_id}, #{ttl}, #{ttl})" : nil + end + temp_keys.compact! + if temp_keys.present? + execute "INSERT INTO digest_unsubscribe_keys (key, user_id, created_at, updated_at) VALUES #{temp_keys.join(', ')}" + end + end + rescue + # If anything goes wrong, continue with other migrations + end + + def down + drop_table :digest_unsubscribe_keys + end +end diff --git a/spec/controllers/email_controller_spec.rb b/spec/controllers/email_controller_spec.rb index 65dc32b5b75..c1635c53130 100644 --- a/spec/controllers/email_controller_spec.rb +++ b/spec/controllers/email_controller_spec.rb @@ -22,10 +22,11 @@ describe EmailController do context '.resubscribe' do let(:user) { Fabricate(:user, email_digests: false) } + let(:key) { DigestUnsubscribeKey.create_key_for(user) } context 'with a valid key' do before do - get :resubscribe, key: user.temporary_key + get :resubscribe, key: key user.reload end @@ -39,10 +40,11 @@ describe EmailController do context '.unsubscribe' do let(:user) { Fabricate(:user) } + let(:key) { DigestUnsubscribeKey.create_key_for(user) } context 'with a valid key' do before do - get :unsubscribe, key: user.temporary_key + get :unsubscribe, key: key user.reload end @@ -69,7 +71,7 @@ describe EmailController do let!(:logged_in_user) { log_in(:coding_horror) } before do - get :unsubscribe, key: user.temporary_key + get :unsubscribe, key: key user.reload end @@ -87,7 +89,7 @@ describe EmailController do before do log_in_user(user) - get :unsubscribe, key: user.temporary_key + get :unsubscribe, key: DigestUnsubscribeKey.create_key_for(user) user.reload end diff --git a/spec/jobs/clean_up_digest_keys.rb b/spec/jobs/clean_up_digest_keys.rb new file mode 100644 index 00000000000..d3cbcff40e4 --- /dev/null +++ b/spec/jobs/clean_up_digest_keys.rb @@ -0,0 +1,13 @@ +module Jobs + + class CleanUpDigestKeys < Jobs::Scheduled + every 1.day + + def execute(args) + # Remove unused digest keys + DigestUnsubscribeKey.where('created_at < ?', 2.months.ago).delete_all + end + end + +end + diff --git a/spec/models/digest_unsubscribe_key_spec.rb b/spec/models/digest_unsubscribe_key_spec.rb new file mode 100644 index 00000000000..f658a402191 --- /dev/null +++ b/spec/models/digest_unsubscribe_key_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' +require_dependency 'digest_unsubscribe_key' + +describe DigestUnsubscribeKey do + + it { is_expected.to belong_to :user } + + describe 'key' do + + let(:user) { Fabricate(:user) } + let!(:key) { DigestUnsubscribeKey.create_key_for(user) } + + it 'has a temporary key' do + expect(key).to be_present + end + + describe '#user_for_key' do + + it 'can be used to find the user' do + expect(DigestUnsubscribeKey.user_for_key(key)).to eq(user) + end + + it 'returns nil with an invalid key' do + expect(DigestUnsubscribeKey.user_for_key('asdfasdf')).to be_blank + end + + end + + end + +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 69bd8cb3a6c..8ecb4c55e93 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -384,29 +384,6 @@ describe User do end end - describe 'temporary_key' do - - let(:user) { Fabricate(:user) } - let!(:temporary_key) { user.temporary_key} - - it 'has a temporary key' do - expect(temporary_key).to be_present - end - - describe 'User#find_by_temporary_key' do - - it 'can be used to find the user' do - expect(User.find_by_temporary_key(temporary_key)).to eq(user) - end - - it 'returns nil with an invalid key' do - expect(User.find_by_temporary_key('asdfasdf')).to be_blank - end - - end - - end - describe 'email_hash' do before do @user = Fabricate(:user)