From 8e84ebf0cb211c1d94145399b05c9f2ad0e4d4b0 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 16 Jan 2022 13:23:50 +0100 Subject: [PATCH] Remove IP tracking columns from users table (#16409) --- .../api/v1/admin/accounts_controller.rb | 2 +- app/controllers/auth/sessions_controller.rb | 2 +- .../concerns/user_tracking_concern.rb | 6 +- app/helpers/admin/dashboard_helper.rb | 10 ++-- app/models/account.rb | 1 - app/models/account_filter.rb | 2 +- app/models/user.rb | 58 +++++-------------- app/models/user_ip.rb | 19 ++++++ .../rest/admin/account_serializer.rb | 13 +++-- app/serializers/rest/admin/ip_serializer.rb | 5 ++ app/views/admin/accounts/show.html.haml | 10 ++-- .../admin_mailer/new_pending_account.text.erb | 2 +- app/workers/scheduler/ip_cleanup_scheduler.rb | 2 +- config/initializers/devise.rb | 15 ++--- db/migrate/20210616214526_create_user_ips.rb | 5 ++ ...35_remove_current_sign_in_ip_from_users.rb | 12 ++++ db/schema.rb | 24 +++++++- db/views/user_ips_v01.sql | 26 +++++++++ .../auth/sessions_controller_spec.rb | 2 +- 19 files changed, 141 insertions(+), 75 deletions(-) create mode 100644 app/models/user_ip.rb create mode 100644 app/serializers/rest/admin/ip_serializer.rb create mode 100644 db/migrate/20210616214526_create_user_ips.rb create mode 100644 db/post_migrate/20210616214135_remove_current_sign_in_ip_from_users.rb create mode 100644 db/views/user_ips_v01.sql diff --git a/app/controllers/api/v1/admin/accounts_controller.rb b/app/controllers/api/v1/admin/accounts_controller.rb index 63cc521ed0..9b8f2fb059 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 0184bfb52e..3337a43c45 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 efda37fae7..45f3aab0d0 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 32aaf9f5e7..d4a30b97eb 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 238ea1d655..c459125c7b 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 defd531acb..dcb1741227 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 374b82d05d..49dcb81560 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 0000000000..a8e802e136 --- /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 f579d33028..3480e8c5a1 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 0000000000..d11699dc4e --- /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 64cfc9a77b..3867d1b19e 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 bcc2518190..a8a2a35fa5 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 918c10ac99..adc99c6054 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 5232e6cfda..b434c68fac 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 0000000000..68e81a9d81 --- /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 0000000000..b53b247f22 --- /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 a1d169b23c..d1446c6523 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 0000000000..50a8201cdf --- /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 f718f5dd92..2368cc2bfb 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'