Remove IP tracking columns from users table (#16409)

19 files changed, 141 insertions, 75 deletions

diff --git a/app/controllers/api/v1/admin/accounts_controller.rb b/app/controllers/api/v1/admin/accounts_controller.rb
index 63cc521ed..9b8f2fb05 100644
--- a/app/controllers/api/v1/admin/accounts_controller.rb
+++ b/app/controllers/api/v1/admin/accounts_controller.rb
@@ -94,7 +94,7 @@ class Api::V1::Admin::AccountsController < Api::BaseController
   private
 
   def set_accounts
-    @accounts = filtered_accounts.order(id: :desc).includes(user: [:invite_request, :invite]).to_a_paginated_by_id(limit_param(LIMIT), params_slice(:max_id, :since_id, :min_id))
+    @accounts = filtered_accounts.order(id: :desc).includes(user: [:invite_request, :invite, :ips]).to_a_paginated_by_id(limit_param(LIMIT), params_slice(:max_id, :since_id, :min_id))
   end
 
   def set_account
diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb
index 0184bfb52..3337a43c4 100644
--- a/app/controllers/auth/sessions_controller.rb
+++ b/app/controllers/auth/sessions_controller.rb
@@ -147,7 +147,7 @@ class Auth::SessionsController < Devise::SessionsController
 
     clear_attempt_from_session
 
-    user.update_sign_in!(request, new_sign_in: true)
+    user.update_sign_in!(new_sign_in: true)
     sign_in(user)
     flash.delete(:notice)
 
diff --git a/app/controllers/concerns/user_tracking_concern.rb b/app/controllers/concerns/user_tracking_concern.rb
index efda37fae..45f3aab0d 100644
--- a/app/controllers/concerns/user_tracking_concern.rb
+++ b/app/controllers/concerns/user_tracking_concern.rb
@@ -3,7 +3,7 @@
 module UserTrackingConcern
   extend ActiveSupport::Concern
 
-  UPDATE_SIGN_IN_HOURS = 24
+  UPDATE_SIGN_IN_FREQUENCY = 24.hours.freeze
 
   included do
     before_action :update_user_sign_in
@@ -12,10 +12,10 @@ module UserTrackingConcern
   private
 
   def update_user_sign_in
-    current_user.update_sign_in!(request) if user_needs_sign_in_update?
+    current_user.update_sign_in! if user_needs_sign_in_update?
   end
 
   def user_needs_sign_in_update?
-    user_signed_in? && (current_user.current_sign_in_at.nil? || current_user.current_sign_in_at < UPDATE_SIGN_IN_HOURS.hours.ago)
+    user_signed_in? && (current_user.current_sign_in_at.nil? || current_user.current_sign_in_at < UPDATE_SIGN_IN_FREQUENCY.ago)
   end
 end
diff --git a/app/helpers/admin/dashboard_helper.rb b/app/helpers/admin/dashboard_helper.rb
index 32aaf9f5e..d4a30b97e 100644
--- a/app/helpers/admin/dashboard_helper.rb
+++ b/app/helpers/admin/dashboard_helper.rb
@@ -2,17 +2,17 @@
 
 module Admin::DashboardHelper
   def relevant_account_ip(account, ip_query)
-    default_ip = [account.user_current_sign_in_ip || account.user_sign_up_ip]
+    ips = account.user.ips.to_a
 
     matched_ip = begin
       ip_query_addr = IPAddr.new(ip_query)
-      account.user.recent_ips.find { |(_, ip)| ip_query_addr.include?(ip) } || default_ip
+      ips.find { |ip| ip_query_addr.include?(ip.ip) } || ips.first
     rescue IPAddr::Error
-      default_ip
-    end.last
+      ips.first
+    end
 
     if matched_ip
-      link_to matched_ip, admin_accounts_path(ip: matched_ip)
+      link_to matched_ip.ip, admin_accounts_path(ip: matched_ip.ip)
     else
       '-'
     end
diff --git a/app/models/account.rb b/app/models/account.rb
index 238ea1d65..c459125c7 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -123,7 +123,6 @@ class Account < ApplicationRecord
 
   delegate :email,
            :unconfirmed_email,
-           :current_sign_in_ip,
            :current_sign_in_at,
            :created_at,
            :sign_up_ip,
diff --git a/app/models/account_filter.rb b/app/models/account_filter.rb
index defd531ac..dcb174122 100644
--- a/app/models/account_filter.rb
+++ b/app/models/account_filter.rb
@@ -21,7 +21,7 @@ class AccountFilter
   end
 
   def results
-    scope = Account.includes(:account_stat, user: [:session_activations, :invite_request]).without_instance_actor.reorder(nil)
+    scope = Account.includes(:account_stat, user: [:ips, :invite_request]).without_instance_actor.reorder(nil)
 
     params.each do |key, value|
       scope.merge!(scope_for(key, value.to_s.strip)) if value.present?
diff --git a/app/models/user.rb b/app/models/user.rb
index 374b82d05..49dcb8156 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -14,8 +14,6 @@
 #  sign_in_count             :integer          default(0), not null
 #  current_sign_in_at        :datetime
 #  last_sign_in_at           :datetime
-#  current_sign_in_ip        :inet
-#  last_sign_in_ip           :inet
 #  admin                     :boolean          default(FALSE), not null
 #  confirmation_token        :string
 #  confirmed_at              :datetime
@@ -81,6 +79,7 @@ class User < ApplicationRecord
   has_many :invites, inverse_of: :user
   has_many :markers, inverse_of: :user, dependent: :destroy
   has_many :webauthn_credentials, dependent: :destroy
+  has_many :ips, class_name: 'UserIp', inverse_of: :user
 
   has_one :invite_request, class_name: 'UserInviteRequest', inverse_of: :user, dependent: :destroy
   accepts_nested_attributes_for :invite_request, reject_if: ->(attributes) { attributes['text'].blank? && !Setting.require_invite_text }
@@ -107,7 +106,7 @@ class User < ApplicationRecord
   scope :inactive, -> { where(arel_table[:current_sign_in_at].lt(ACTIVE_DURATION.ago)) }
   scope :active, -> { confirmed.where(arel_table[:current_sign_in_at].gteq(ACTIVE_DURATION.ago)).joins(:account).where(accounts: { suspended_at: nil }) }
   scope :matches_email, ->(value) { where(arel_table[:email].matches("#{value}%")) }
-  scope :matches_ip, ->(value) { where('current_sign_in_ip <<= ?', value).or(where('users.sign_up_ip <<= ?', value)).or(where('users.last_sign_in_ip <<= ?', value)).or(where(id: SessionActivation.select(:user_id).where('ip <<= ?', value))) }
+  scope :matches_ip, ->(value) { left_joins(:ips).where('user_ips.ip <<= ?', value) }
   scope :emailable, -> { confirmed.enabled.joins(:account).merge(Account.searchable) }
 
   before_validation :sanitize_languages
@@ -174,15 +173,11 @@ class User < ApplicationRecord
     prepare_new_user! if new_user && approved?
   end
 
-  def update_sign_in!(request, new_sign_in: false)
+  def update_sign_in!(new_sign_in: false)
     old_current, new_current = current_sign_in_at, Time.now.utc
     self.last_sign_in_at     = old_current || new_current
     self.current_sign_in_at  = new_current
 
-    old_current, new_current = current_sign_in_ip, request.remote_ip
-    self.last_sign_in_ip     = old_current || new_current
-    self.current_sign_in_ip  = new_current
-
     if new_sign_in
       self.sign_in_count ||= 0
       self.sign_in_count  += 1
@@ -201,7 +196,7 @@ class User < ApplicationRecord
   end
 
   def suspicious_sign_in?(ip)
-    !otp_required_for_login? && !skip_sign_in_token? && current_sign_in_at.present? && !recent_ip?(ip)
+    !otp_required_for_login? && !skip_sign_in_token? && current_sign_in_at.present? && !ips.where(ip: ip).exists?
   end
 
   def functional?
@@ -277,31 +272,28 @@ class User < ApplicationRecord
     @shows_application ||= settings.show_application
   end
 
-  # rubocop:disable Naming/MethodParameterName
-  def token_for_app(a)
-    return nil if a.nil? || a.owner != self
-    Doorkeeper::AccessToken.find_or_create_by(application_id: a.id, resource_owner_id: id) do |t|
-      t.scopes = a.scopes
-      t.expires_in = Doorkeeper.configuration.access_token_expires_in
+  def token_for_app(app)
+    return nil if app.nil? || app.owner != self
+
+    Doorkeeper::AccessToken.find_or_create_by(application_id: app.id, resource_owner_id: id) do |t|
+      t.scopes            = app.scopes
+      t.expires_in        = Doorkeeper.configuration.access_token_expires_in
       t.use_refresh_token = Doorkeeper.configuration.refresh_token_enabled?
     end
   end
-  # rubocop:enable Naming/MethodParameterName
 
   def activate_session(request)
-    session_activations.activate(session_id: SecureRandom.hex,
-                                 user_agent: request.user_agent,
-                                 ip: request.remote_ip).session_id
+    session_activations.activate(
+      session_id: SecureRandom.hex,
+      user_agent: request.user_agent,
+      ip: request.remote_ip
+    ).session_id
   end
 
   def clear_other_sessions(id)
     session_activations.exclusive(id)
   end
 
-  def session_active?(id)
-    session_activations.active? id
-  end
-
   def web_push_subscription(session)
     session.web_push_subscription.nil? ? nil : session.web_push_subscription
   end
@@ -364,22 +356,6 @@ class User < ApplicationRecord
     setting_display_media == 'hide_all'
   end
 
-  def recent_ips
-    @recent_ips ||= begin
-      arr = []
-
-      session_activations.each do |session_activation|
-        arr << [session_activation.updated_at, session_activation.ip]
-      end
-
-      arr << [current_sign_in_at, current_sign_in_ip] if current_sign_in_ip.present?
-      arr << [last_sign_in_at, last_sign_in_ip] if last_sign_in_ip.present?
-      arr << [created_at, sign_up_ip] if sign_up_ip.present?
-
-      arr.sort_by { |pair| pair.first || Time.now.utc }.uniq(&:last).reverse!
-    end
-  end
-
   def sign_in_token_expired?
     sign_in_token_sent_at.nil? || sign_in_token_sent_at < 5.minutes.ago
   end
@@ -410,10 +386,6 @@ class User < ApplicationRecord
 
   private
 
-  def recent_ip?(ip)
-    recent_ips.any? { |(_, recent_ip)| recent_ip == ip }
-  end
-
   def send_pending_devise_notifications
     pending_devise_notifications.each do |notification, args, kwargs|
       render_and_send_devise_message(notification, *args, **kwargs)
diff --git a/app/models/user_ip.rb b/app/models/user_ip.rb
new file mode 100644
index 000000000..a8e802e13
--- /dev/null
+++ b/app/models/user_ip.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+# == Schema Information
+#
+# Table name: user_ips
+#
+#  user_id :bigint(8)        primary key
+#  ip      :inet
+#  used_at :datetime
+#
+
+class UserIp < ApplicationRecord
+  self.primary_key = :user_id
+
+  belongs_to :user, foreign_key: :user_id
+
+  def readonly?
+    true
+  end
+end
diff --git a/app/serializers/rest/admin/account_serializer.rb b/app/serializers/rest/admin/account_serializer.rb
index f579d3302..3480e8c5a 100644
--- a/app/serializers/rest/admin/account_serializer.rb
+++ b/app/serializers/rest/admin/account_serializer.rb
@@ -9,6 +9,7 @@ class REST::Admin::AccountSerializer < ActiveModel::Serializer
   attribute :created_by_application_id, if: :created_by_application?
   attribute :invited_by_account_id, if: :invited?
 
+  has_many :ips, serializer: REST::Admin::IpSerializer
   has_one :account, serializer: REST::AccountSerializer
 
   def id
@@ -19,10 +20,6 @@ class REST::Admin::AccountSerializer < ActiveModel::Serializer
     object.user_email
   end
 
-  def ip
-    object.user_current_sign_in_ip.to_s.presence
-  end
-
   def role
     object.user_role
   end
@@ -74,4 +71,12 @@ class REST::Admin::AccountSerializer < ActiveModel::Serializer
   def created_by_application?
     object.user&.created_by_application_id&.present?
   end
+
+  def ips
+    object.user&.ips
+  end
+
+  def ip
+    ips&.first
+  end
 end
diff --git a/app/serializers/rest/admin/ip_serializer.rb b/app/serializers/rest/admin/ip_serializer.rb
new file mode 100644
index 000000000..d11699dc4
--- /dev/null
+++ b/app/serializers/rest/admin/ip_serializer.rb
@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class REST::Admin::IpSerializer < ActiveModel::Serializer
+  attributes :ip, :used_at
+end
diff --git a/app/views/admin/accounts/show.html.haml b/app/views/admin/accounts/show.html.haml
index 64cfc9a77..3867d1b19 100644
--- a/app/views/admin/accounts/show.html.haml
+++ b/app/views/admin/accounts/show.html.haml
@@ -156,12 +156,14 @@
               %time.formatted{ datetime: @account.created_at.iso8601, title: l(@account.created_at) }= l @account.created_at
             %td
 
-          - @account.user.recent_ips.each_with_index do |(_, ip), i|
+          - recent_ips = @account.user.ips.order(used_at: :desc).to_a
+
+          - recent_ips.each_with_index do |recent_ip, i|
             %tr
               - if i.zero?
-                %th{ rowspan: @account.user.recent_ips.size }= t('admin.accounts.most_recent_ip')
-              %td= ip
-              %td= table_link_to 'search', t('admin.accounts.search_same_ip'), admin_accounts_path(ip: ip)
+                %th{ rowspan: recent_ips.size }= t('admin.accounts.most_recent_ip')
+              %td= recent_ip.ip
+              %td= table_link_to 'search', t('admin.accounts.search_same_ip'), admin_accounts_path(ip: recent_ip.ip)
 
           %tr
             %th= t('admin.accounts.most_recent_activity')
diff --git a/app/views/admin_mailer/new_pending_account.text.erb b/app/views/admin_mailer/new_pending_account.text.erb
index bcc251819..a8a2a35fa 100644
--- a/app/views/admin_mailer/new_pending_account.text.erb
+++ b/app/views/admin_mailer/new_pending_account.text.erb
@@ -3,7 +3,7 @@
 <%= raw t('admin_mailer.new_pending_account.body') %>
 
 <%= @account.user_email %> (@<%= @account.username %>)
-<%= @account.user_current_sign_in_ip %>
+<%= @account.user_sign_up_ip %>
 <% if @account.user&.invite_request&.text.present? %>
 
 <%= quote_wrap(@account.user&.invite_request&.text) %>
diff --git a/app/workers/scheduler/ip_cleanup_scheduler.rb b/app/workers/scheduler/ip_cleanup_scheduler.rb
index 918c10ac9..adc99c605 100644
--- a/app/workers/scheduler/ip_cleanup_scheduler.rb
+++ b/app/workers/scheduler/ip_cleanup_scheduler.rb
@@ -16,7 +16,7 @@ class Scheduler::IpCleanupScheduler
 
   def clean_ip_columns!
     SessionActivation.where('updated_at < ?', IP_RETENTION_PERIOD.ago).in_batches.destroy_all
-    User.where('current_sign_in_at < ?', IP_RETENTION_PERIOD.ago).in_batches.update_all(last_sign_in_ip: nil, current_sign_in_ip: nil, sign_up_ip: nil)
+    User.where('current_sign_in_at < ?', IP_RETENTION_PERIOD.ago).in_batches.update_all(sign_up_ip: nil)
     LoginActivity.where('created_at < ?', IP_RETENTION_PERIOD.ago).in_batches.destroy_all
   end
 
diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb
index 5232e6cfd..b434c68fa 100644
--- a/config/initializers/devise.rb
+++ b/config/initializers/devise.rb
@@ -1,11 +1,8 @@
 require 'devise/strategies/authenticatable'
 
 Warden::Manager.after_set_user except: :fetch do |user, warden|
-  if user.session_active?(warden.cookies.signed['_session_id'] || warden.raw_session['auth_id'])
-    session_id = warden.cookies.signed['_session_id'] || warden.raw_session['auth_id']
-  else
-    session_id = user.activate_session(warden.request)
-  end
+  session_id = warden.cookies.signed['_session_id'] || warden.raw_session['auth_id']
+  session_id = user.activate_session(warden.request) unless user.session_activations.active?(session_id)
 
   warden.cookies.signed['_session_id'] = {
     value: session_id,
@@ -17,9 +14,13 @@ Warden::Manager.after_set_user except: :fetch do |user, warden|
 end
 
 Warden::Manager.after_fetch do |user, warden|
-  if user.session_active?(warden.cookies.signed['_session_id'] || warden.raw_session['auth_id'])
+  session_id = warden.cookies.signed['_session_id'] || warden.raw_session['auth_id']
+
+  if session_id && (session = user.session_activations.find_by(session_id: session_id))
+    session.update(ip: warden.request.remote_ip) if session.ip != warden.request.remote_ip
+
     warden.cookies.signed['_session_id'] = {
-      value: warden.cookies.signed['_session_id'] || warden.raw_session['auth_id'],
+      value: session_id,
       expires: 1.year.from_now,
       httponly: true,
       secure: (Rails.env.production? || ENV['LOCAL_HTTPS'] == 'true'),
diff --git a/db/migrate/20210616214526_create_user_ips.rb b/db/migrate/20210616214526_create_user_ips.rb
new file mode 100644
index 000000000..68e81a9d8
--- /dev/null
+++ b/db/migrate/20210616214526_create_user_ips.rb
@@ -0,0 +1,5 @@
+class CreateUserIps < ActiveRecord::Migration[6.1]
+  def change
+    create_view :user_ips
+  end
+end
diff --git a/db/post_migrate/20210616214135_remove_current_sign_in_ip_from_users.rb b/db/post_migrate/20210616214135_remove_current_sign_in_ip_from_users.rb
new file mode 100644
index 000000000..b53b247f2
--- /dev/null
+++ b/db/post_migrate/20210616214135_remove_current_sign_in_ip_from_users.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class RemoveCurrentSignInIpFromUsers < ActiveRecord::Migration[5.2]
+  disable_ddl_transaction!
+
+  def change
+    safety_assured do
+      remove_column :users, :current_sign_in_ip, :inet
+      remove_column :users, :last_sign_in_ip, :inet
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index a1d169b23..d1446c652 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -923,8 +923,6 @@ ActiveRecord::Schema.define(version: 2021_12_13_040746) do
     t.integer "sign_in_count", default: 0, null: false
     t.datetime "current_sign_in_at"
     t.datetime "last_sign_in_at"
-    t.inet "current_sign_in_ip"
-    t.inet "last_sign_in_ip"
     t.boolean "admin", default: false, null: false
     t.string "confirmation_token"
     t.datetime "confirmed_at"
@@ -1120,6 +1118,28 @@ ActiveRecord::Schema.define(version: 2021_12_13_040746) do
   SQL
   add_index "instances", ["domain"], name: "index_instances_on_domain", unique: true
 
+  create_view "user_ips", sql_definition: <<-SQL
+      SELECT t0.user_id,
+      t0.ip,
+      max(t0.used_at) AS used_at
+     FROM ( SELECT users.id AS user_id,
+              users.sign_up_ip AS ip,
+              users.created_at AS used_at
+             FROM users
+            WHERE (users.sign_up_ip IS NOT NULL)
+          UNION ALL
+           SELECT session_activations.user_id,
+              session_activations.ip,
+              session_activations.updated_at
+             FROM session_activations
+          UNION ALL
+           SELECT login_activities.user_id,
+              login_activities.ip,
+              login_activities.created_at
+             FROM login_activities
+            WHERE (login_activities.success = true)) t0
+    GROUP BY t0.user_id, t0.ip;
+  SQL
   create_view "account_summaries", materialized: true, sql_definition: <<-SQL
       SELECT accounts.id AS account_id,
       mode() WITHIN GROUP (ORDER BY t0.language) AS language,
diff --git a/db/views/user_ips_v01.sql b/db/views/user_ips_v01.sql
new file mode 100644
index 000000000..50a8201cd
--- /dev/null
+++ b/db/views/user_ips_v01.sql
@@ -0,0 +1,26 @@
+SELECT
+  user_id,
+  ip,
+  max(used_at) AS used_at
+FROM (
+  SELECT
+    id AS user_id,
+    sign_up_ip AS ip,
+    created_at AS used_at
+  FROM users
+  WHERE sign_up_ip IS NOT NULL
+  UNION ALL
+  SELECT
+    user_id,
+    ip,
+    updated_at
+  FROM session_activations
+  UNION ALL
+  SELECT
+    user_id,
+    ip,
+    created_at
+  FROM login_activities
+  WHERE success = 't'
+) AS t0
+GROUP BY user_id, ip
diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb
index f718f5dd9..2368cc2bf 100644
--- a/spec/controllers/auth/sessions_controller_spec.rb
+++ b/spec/controllers/auth/sessions_controller_spec.rb
@@ -400,7 +400,7 @@ RSpec.describe Auth::SessionsController, type: :controller do
     end
 
     context 'when 2FA is disabled and IP is unfamiliar' do
-      let!(:user) { Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', current_sign_in_at: 3.weeks.ago, current_sign_in_ip: '0.0.0.0') }
+      let!(:user) { Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', current_sign_in_at: 3.weeks.ago) }
 
       before do
         request.remote_ip  = '10.10.10.10'