From 64b88af049507f305011210a7f271927eeaf6bb9 Mon Sep 17 00:00:00 2001 From: Stephen Paul Weber Date: Wed, 25 Aug 2021 12:47:53 -0500 Subject: [PATCH 1/2] Do not lose WebRegisterManager on retart Store web registrations in redis. Set an expiry so they don't grow in RAM forever as they previously would have without a restart. --- lib/registration.rb | 2 +- lib/web_register_manager.rb | 14 +++++++++----- sgx_jmp.rb | 4 ++-- test/test_helper.rb | 11 ++++++++++- test/test_registration.rb | 8 ++++---- test/test_web_register_manager.rb | 13 +++++++------ 6 files changed, 33 insertions(+), 19 deletions(-) diff --git a/lib/registration.rb b/lib/registration.rb index fe02828419c7f51a7976a7cf3fe8ed2c45eb2206..bf41946fffaff3d5291df525c8e36c12815a8f2e 100644 --- a/lib/registration.rb +++ b/lib/registration.rb @@ -17,7 +17,7 @@ class Registration if registered Registered.new(registered.phone) else - web_register_manager[customer.jid].choose_tel.then do |tel| + web_register_manager[customer.jid].then(&:choose_tel).then do |tel| Activation.for(customer, tel) end end diff --git a/lib/web_register_manager.rb b/lib/web_register_manager.rb index 0a332c3f1afc2ba2b6a171e577397f178185ebf1..905a96c57d70ead892677974ac77a1c535a86b57 100644 --- a/lib/web_register_manager.rb +++ b/lib/web_register_manager.rb @@ -1,16 +1,20 @@ # frozen_string_literal: true class WebRegisterManager - def initialize - @tel_map = Hash.new { ChooseTel.new } + THIRTY_DAYS = 60 * 60 * 24 * 30 + + def initialize(redis: REDIS) + @redis = redis end - def []=(jid, tel) - @tel_map[jid.to_s] = HaveTel.new(tel) + def set(jid, tel) + @redis.setex("pending_tel_for-#{jid}", tel, THIRTY_DAYS) end def [](jid) - @tel_map[jid.to_s] + @redis.get("pending_tel_for-#{jid}").then do |tel| + tel ? HaveTel.new(tel) : ChooseTel.new + end end class HaveTel diff --git a/sgx_jmp.rb b/sgx_jmp.rb index 56dc740a39373be0b4b64572ee66c847bf66ce42..2db7f18b3ddffb06b22c41da5d39eab01d3fbb3b 100644 --- a/sgx_jmp.rb +++ b/sgx_jmp.rb @@ -547,8 +547,8 @@ command :execute?, node: "web-register", sessionid: nil do |iq| cmd.form.fields = [var: "to", value: jid] cmd.form.type = "submit" }).then { |result| - final_jid = result.form.field("from")&.value.to_s.strip - web_register_manager[final_jid] = tel + web_register_manager.set(result.form.field("from")&.value.to_s.strip, tel) + }.then { BLATHER << iq.reply.tap { |reply| reply.status = :completed } }.catch { |e| panic(e, sentry_hub) } end diff --git a/test/test_helper.rb b/test/test_helper.rb index a61cb3219c15cfe584d05ebeff404c5fd03abccc..4faa24aeb3892c2c893a5b26f5ae9060075497db 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -133,10 +133,19 @@ class PromiseMock < Minitest::Mock end class FakeRedis - def initialize(values) + def initialize(values={}) @values = values end + def set(key, value) + @values[key] = value + EMPromise.resolve("OK") + end + + def setex(key, value, _expiry) + set(key, value) + end + def get(key) EMPromise.resolve(@values[key]) end diff --git a/test/test_registration.rb b/test/test_registration.rb index 7093b12dca898d7a5474bcb6f03680b19a4c0783..06dd58ffcb8950365d6f6305843ce421641935d2 100644 --- a/test/test_registration.rb +++ b/test/test_registration.rb @@ -35,8 +35,8 @@ class RegistrationTest < Minitest::Test em :test_for_registered def test_for_activated - web_manager = WebRegisterManager.new - web_manager["test@example.net"] = "+15555550000" + web_manager = WebRegisterManager.new(redis: FakeRedis.new) + web_manager.set("test@example.net", "+15555550000") result = execute_command do sgx = OpenStruct.new(registered?: EMPromise.resolve(nil)) Registration.for( @@ -54,8 +54,8 @@ class RegistrationTest < Minitest::Test def test_for_not_activated_with_customer_id sgx = OpenStruct.new(registered?: EMPromise.resolve(nil)) - web_manager = WebRegisterManager.new - web_manager["test@example.net"] = "+15555550000" + web_manager = WebRegisterManager.new(redis: FakeRedis.new) + web_manager.set("test@example.net", "+15555550000") iq = Blather::Stanza::Iq::Command.new iq.from = "test@example.com" result = execute_command(iq) do diff --git a/test/test_web_register_manager.rb b/test/test_web_register_manager.rb index 675b676ae77281ef29da3ad86a42cda5bcc77b49..3abac88834dafe85ca5790b52e08d3bdaf009040 100644 --- a/test/test_web_register_manager.rb +++ b/test/test_web_register_manager.rb @@ -5,19 +5,20 @@ require "web_register_manager" class WebRegisterManagerTest < Minitest::Test def setup - @manager = WebRegisterManager.new + @manager = WebRegisterManager.new(redis: FakeRedis.new) end def test_set_get - assert_kind_of WebRegisterManager::ChooseTel, @manager["jid@example.com"] - @manager["jid@example.com"] = "+15555550000" - assert_kind_of WebRegisterManager::HaveTel, @manager["jid@example.com"] + assert_kind_of WebRegisterManager::ChooseTel, @manager["jid@example.com"].sync + @manager.set("jid@example.com", "+15555550000").sync + assert_kind_of WebRegisterManager::HaveTel, @manager["jid@example.com"].sync end + em :test_set_get def test_choose_tel_have_tel jid = "jid@example.com" - @manager[jid] = "+15555550000" - assert_equal "+15555550000", @manager[jid].choose_tel.sync + @manager.set(jid, "+15555550000").sync + assert_equal "+15555550000", @manager[jid].then(&:choose_tel).sync end em :test_choose_tel_have_tel end From c49b2c6ed2fabce87d87ca0e03821fca12d27be8 Mon Sep 17 00:00:00 2001 From: Stephen Paul Weber Date: Wed, 25 Aug 2021 12:48:57 -0500 Subject: [PATCH 2/2] No more legacy session for BTC Set the same key as web register manager, so that on next register jmp.chat the tel we were using in this flow will be used. Not needed if they came from web register, but will still extend expiry in that case and no harm. Clean up pending tel key on Finish. --- lib/registration.rb | 20 ++++++++------------ test/test_registration.rb | 5 +++++ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/registration.rb b/lib/registration.rb index bf41946fffaff3d5291df525c8e36c12815a8f2e..a34bdec776f1dd4bf1db1d3705ee50ee6c0dee48 100644 --- a/lib/registration.rb +++ b/lib/registration.rb @@ -157,6 +157,8 @@ class Registration class Bitcoin Payment.kinds[:bitcoin] = method(:new) + THIRTY_DAYS = 60 * 60 * 24 * 30 + def initialize(customer, tel) @customer = customer @customer_id = customer.customer_id @@ -165,20 +167,13 @@ class Registration attr_reader :customer_id, :tel - def legacy_session_save - sid = SecureRandom.hex - REDIS.mset( - "reg-sid_for-#{customer_id}", sid, - "reg-session_tel-#{sid}", tel - ) - end - def save EMPromise.all([ - legacy_session_save, - REDIS.mset( - "pending_tel_for-#{customer_id}", tel, - "pending_plan_for-#{customer_id}", @customer.plan_name + REDIS.setex("pending_tel_for-#{@customer.jid}", tel, THIRTY_DAYS), + REDIS.setex( + "pending_plan_for-#{customer_id}", + @customer.plan_name, + THIRTY_DAYS ) ]) end @@ -458,6 +453,7 @@ class Registration def customer_active_tel_purchased @customer.register!(@tel).catch(&method(:raise_setup_error)).then { EMPromise.all([ + REDIS.del("pending_tel_for-#{@customer.jid}"), REDIS.set("catapult_fwd-#{@tel}", cheogram_sip_addr), @customer.fwd_timeout = 25 # ~5 seconds / ring, 5 rings ]) diff --git a/test/test_registration.rb b/test/test_registration.rb index 06dd58ffcb8950365d6f6305843ce421641935d2..f82c9eb2e966ab3f1d6cc055b0a6de8a77fa3d3a 100644 --- a/test/test_registration.rb +++ b/test/test_registration.rb @@ -569,6 +569,11 @@ class RegistrationTest < Minitest::Test "sip:test%40example.net@sip.cheogram.com" ] ) + Registration::Finish::REDIS.expect( + :del, + nil, + ["pending_tel_for-test@example.net"] + ) BackendSgx::REDIS.expect( :set, nil,