Add audit log entries for user roles (#19040)

* Refactor audit log schema

* Add audit log entries for user roles

app/controllers/admin/roles_controller.rb

@@ -23,6 +23,7 @@ module Admin
       @role.current_account = current_account
 
       if @role.save
+        log_action :create, @role
         redirect_to admin_roles_path
       else
         render :new
@@ -39,6 +40,7 @@ module Admin
       @role.current_account = current_account
 
       if @role.update(resource_params)
+        log_action :update, @role
         redirect_to admin_roles_path
       else
         render :edit
@@ -48,6 +50,7 @@ module Admin
     def destroy
       authorize @role, :destroy?
       @role.destroy!
+      log_action :destroy, @role
       redirect_to admin_roles_path
     end
 

app/controllers/admin/users/roles_controller.rb

@@ -14,6 +14,7 @@ module Admin
       @user.current_account = current_account
 
       if @user.update(resource_params)
+        log_action :change_role, @user
         redirect_to admin_account_path(@user.account_id), notice: I18n.t('admin.accounts.change_role.changed_msg')
       else
         render :show

app/controllers/concerns/accountable_concern.rb

@@ -3,7 +3,11 @@
 module AccountableConcern
   extend ActiveSupport::Concern
 
-  def log_action(action, target, options = {})
-    Admin::ActionLog.create(account: current_account, action: action, target: target, recorded_changes: options.stringify_keys)
+  def log_action(action, target)
+    Admin::ActionLog.create(
+      account: current_account,
+      action: action,
+      target: target
+    )
   end
 end

app/helpers/admin/action_logs_helper.rb

@@ -2,64 +2,31 @@
 
 module Admin::ActionLogsHelper
   def log_target(log)
-    if log.target
-      linkable_log_target(log.target)
-    else
-      log_target_from_history(log.target_type, log.recorded_changes)
-    end
-  end
-
-  private
-
-  def linkable_log_target(record)
-    case record.class.name
+    case log.target_type
     when 'Account'
-      link_to record.acct, admin_account_path(record.id)
+      link_to log.human_identifier, admin_account_path(log.target_id)
     when 'User'
-      link_to record.account.acct, admin_account_path(record.account_id)
+      link_to log.human_identifier, admin_account_path(log.route_param)
+    when 'UserRole'
+      link_to log.human_identifier, admin_roles_path(log.target_id)
     when 'CustomEmoji'
-      record.shortcode
+      log.human_identifier
     when 'Report'
-      link_to "##{record.id}", admin_report_path(record)
+      link_to "##{log.human_identifier}", admin_report_path(log.target_id)
     when 'DomainBlock', 'DomainAllow', 'EmailDomainBlock', 'UnavailableDomain'
-      link_to record.domain, "https://#{record.domain}"
+      link_to log.human_identifier, "https://#{log.human_identifier}"
     when 'Status'
-      link_to record.account.acct, ActivityPub::TagManager.instance.url_for(record)
+      link_to log.human_identifier, log.permalink
     when 'AccountWarning'
-      link_to record.target_account.acct, admin_account_path(record.target_account_id)
+      link_to log.human_identifier, admin_account_path(log.target_id)
     when 'Announcement'
-      link_to truncate(record.text), edit_admin_announcement_path(record.id)
+      link_to truncate(log.human_identifier), edit_admin_announcement_path(log.target_id)
     when 'IpBlock'
-      "#{record.ip}/#{record.ip.prefix} (#{I18n.t("simple_form.labels.ip_block.severities.#{record.severity}")})"
+      log.human_identifier
     when 'Instance'
-      record.domain
+      log.human_identifier
     when 'Appeal'
-      link_to record.account.acct, disputes_strike_path(record.strike)
-    end
-  end
-
-  def log_target_from_history(type, attributes)
-    case type
-    when 'User'
-      attributes['username']
-    when 'CustomEmoji'
-      attributes['shortcode']
-    when 'DomainBlock', 'DomainAllow', 'EmailDomainBlock', 'UnavailableDomain'
-      link_to attributes['domain'], "https://#{attributes['domain']}"
-    when 'Status'
-      tmp_status = Status.new(attributes.except('reblogs_count', 'favourites_count'))
-
-      if tmp_status.account
-        link_to tmp_status.account&.acct || "##{tmp_status.account_id}", admin_account_path(tmp_status.account_id)
-      else
-        I18n.t('admin.action_logs.deleted_status')
-      end
-    when 'Announcement'
-      truncate(attributes['text'].is_a?(Array) ? attributes['text'].last : attributes['text'])
-    when 'IpBlock'
-      "#{attributes['ip']}/#{attributes['ip'].prefix} (#{I18n.t("simple_form.labels.ip_block.severities.#{attributes['severity']}")})"
-    when 'Instance'
-      attributes['domain']
+      link_to log.human_identifier, disputes_strike_path(log.route_param)
     end
   end
 end

app/models/account.rb

@@ -362,6 +362,10 @@ class Account < ApplicationRecord
     username
   end
 
+  def to_log_human_identifier
+    acct
+  end
+
   def excluded_from_timeline_account_ids
     Rails.cache.fetch("exclude_account_ids_for:#{id}") { block_relationships.pluck(:target_account_id) + blocked_by_relationships.pluck(:account_id) + mute_relationships.pluck(:target_account_id) }
   end

app/models/account_warning.rb

@@ -43,4 +43,8 @@ class AccountWarning < ApplicationRecord
   def overruled?
     overruled_at.present?
   end
+
+  def to_log_human_identifier
+    target_account.acct
+  end
 end

app/models/admin/action_log.rb

@@ -9,38 +9,42 @@
 #  action           :string           default(""), not null
 #  target_type      :string
 #  target_id        :bigint(8)
-#  recorded_changes :text             default(""), not null
 #  created_at       :datetime         not null
 #  updated_at       :datetime         not null
+#  human_identifier :string
+#  route_param      :string
+#  permalink        :string
 #
 
 class Admin::ActionLog < ApplicationRecord
-  serialize :recorded_changes
+  self.ignored_columns = %w(
+    recorded_changes
+  )
 
   belongs_to :account
   belongs_to :target, polymorphic: true, optional: true
 
   default_scope -> { order('id desc') }
 
+  before_validation :set_human_identifier
+  before_validation :set_route_param
+  before_validation :set_permalink
+
   def action
     super.to_sym
   end
 
-  before_validation :set_changes
-
   private
 
-  def set_changes
-    case action
-    when :destroy, :create
-      self.recorded_changes = target.attributes
-    when :update, :promote, :demote
-      self.recorded_changes = target.previous_changes
-    when :change_email
-      self.recorded_changes = ActiveSupport::HashWithIndifferentAccess.new(
-        email: [target.email, nil],
-        unconfirmed_email: [nil, target.unconfirmed_email]
-      )
+  def set_human_identifier
+    self.human_identifier = target.to_log_human_identifier if target.respond_to?(:to_log_human_identifier)
   end
+
+  def set_route_param
+    self.route_param = target.to_log_route_param if target.respond_to?(:to_log_route_param)
+  end
+
+  def set_permalink
+    self.permalink = target.to_log_permalink if target.respond_to?(:to_log_permalink)
   end
 end

app/models/admin/action_log_filter.rb

@@ -12,6 +12,7 @@ class Admin::ActionLogFilter
     reject_appeal: { target_type: 'Appeal', action: 'reject' }.freeze,
     assigned_to_self_report: { target_type: 'Report', action: 'assigned_to_self' }.freeze,
     change_email_user: { target_type: 'User', action: 'change_email' }.freeze,
+    change_role_user: { target_type: 'User', action: 'change_role' }.freeze,
     confirm_user: { target_type: 'User', action: 'confirm' }.freeze,
     approve_user: { target_type: 'User', action: 'approve' }.freeze,
     reject_user: { target_type: 'User', action: 'reject' }.freeze,
@@ -22,6 +23,7 @@ class Admin::ActionLogFilter
     create_domain_block: { target_type: 'DomainBlock', action: 'create' }.freeze,
     create_email_domain_block: { target_type: 'EmailDomainBlock', action: 'create' }.freeze,
     create_unavailable_domain: { target_type: 'UnavailableDomain', action: 'create' }.freeze,
+    create_user_role: { target_type: 'UserRole', action: 'create' }.freeze,
     demote_user: { target_type: 'User', action: 'demote' }.freeze,
     destroy_announcement: { target_type: 'Announcement', action: 'destroy' }.freeze,
     destroy_custom_emoji: { target_type: 'CustomEmoji', action: 'destroy' }.freeze,
@@ -31,6 +33,7 @@ class Admin::ActionLogFilter
     destroy_instance: { target_type: 'Instance', action: 'destroy' }.freeze,
     destroy_unavailable_domain: { target_type: 'UnavailableDomain', action: 'destroy' }.freeze,
     destroy_status: { target_type: 'Status', action: 'destroy' }.freeze,
+    destroy_user_role: { target_type: 'UserRole', action: 'destroy' }.freeze,
     disable_2fa_user: { target_type: 'User', action: 'disable' }.freeze,
     disable_custom_emoji: { target_type: 'CustomEmoji', action: 'disable' }.freeze,
     disable_user: { target_type: 'User', action: 'disable' }.freeze,
@@ -52,6 +55,7 @@ class Admin::ActionLogFilter
     update_announcement: { target_type: 'Announcement', action: 'update' }.freeze,
     update_custom_emoji: { target_type: 'CustomEmoji', action: 'update' }.freeze,
     update_status: { target_type: 'Status', action: 'update' }.freeze,
+    update_user_role: { target_type: 'UserRole', action: 'update' }.freeze,
     unblock_email_account: { target_type: 'Account', action: 'unblock_email' }.freeze,
   }.freeze
 

app/models/announcement.rb

@@ -34,6 +34,10 @@ class Announcement < ApplicationRecord
   before_validation :set_all_day
   before_validation :set_published, on: :create
 
+  def to_log_human_identifier
+    text
+  end
+
   def publish!
     update!(published: true, published_at: Time.now.utc, scheduled_at: nil)
   end

app/models/appeal.rb

@@ -52,6 +52,14 @@ class Appeal < ApplicationRecord
     update!(rejected_at: Time.now.utc, rejected_by_account: current_account)
   end
 
+  def to_log_human_identifier
+    account.acct
+  end
+
+  def to_log_route_param
+    account_warning_id
+  end
+
   private
 
   def validate_time_frame

app/models/custom_emoji.rb

@@ -46,7 +46,7 @@ class CustomEmoji < ApplicationRecord
   scope :local, -> { where(domain: nil) }
   scope :remote, -> { where.not(domain: nil) }
   scope :alphabetic, -> { order(domain: :asc, shortcode: :asc) }
-  scope :by_domain_and_subdomains, ->(domain) { where(domain: domain).or(where(arel_table[:domain].matches('%.' + domain))) }
+  scope :by_domain_and_subdomains, ->(domain) { where(domain: domain).or(where(arel_table[:domain].matches("%.#{domain}"))) }
   scope :listed, -> { local.where(disabled: false).where(visible_in_picker: true) }
 
   remotable_attachment :image, LIMIT
@@ -67,6 +67,10 @@ class CustomEmoji < ApplicationRecord
     copy.tap(&:save!)
   end
 
+  def to_log_human_identifier
+    shortcode
+  end
+
   class << self
     def from_text(text, domain = nil)
       return [] if text.blank?

app/models/domain_allow.rb

@@ -19,6 +19,10 @@ class DomainAllow < ApplicationRecord
 
   scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) }
 
+  def to_log_human_identifier
+    domain
+  end
+
   class << self
     def allowed?(domain)
       !rule_for(domain).nil?

app/models/domain_block.rb

@@ -31,6 +31,10 @@ class DomainBlock < ApplicationRecord
   scope :with_user_facing_limitations, -> { where(severity: [:silence, :suspend]).or(where(reject_media: true)) }
   scope :by_severity, -> { order(Arel.sql('(CASE severity WHEN 0 THEN 1 WHEN 1 THEN 2 WHEN 2 THEN 0 END), reject_media, domain')) }
 
+  def to_log_human_identifier
+    domain
+  end
+
   def policies
     if suspend?
       [:suspend]

app/models/email_domain_block.rb

@@ -26,6 +26,10 @@ class EmailDomainBlock < ApplicationRecord
   # Used for adding multiple blocks at once
   attr_accessor :other_domains
 
+  def to_log_human_identifier
+    domain
+  end
+
   def history
     @history ||= Trends::History.new('email_domain_blocks', id)
   end

app/models/form/account_batch.rb

@@ -101,7 +101,7 @@ class Form::AccountBatch
 
   def reject_account(account)
     authorize(account.user, :reject?)
-    log_action(:reject, account.user, username: account.username)
+    log_action(:reject, account.user)
     account.suspend!(origin: :local)
     AccountDeletionWorker.perform_async(account.id, { 'reserve_username' => false })
   end

app/models/instance.rb

@@ -48,6 +48,8 @@ class Instance < ApplicationRecord
     domain
   end
 
+  alias to_log_human_identifier to_param
+
   delegate :exhausted_deliveries_days, to: :delivery_failure_tracker
 
   def availability_over_days(num_days, end_date = Time.now.utc.to_date)

app/models/ip_block.rb

@@ -27,6 +27,10 @@ class IpBlock < ApplicationRecord
 
   after_commit :reset_cache
 
+  def to_log_human_identifier
+    "#{record.ip}/#{record.ip.prefix}"
+  end
+
   class << self
     def blocked?(remote_ip)
       blocked_ips_map = Rails.cache.fetch(CACHE_KEY) { FastIpMap.new(IpBlock.where(severity: :no_access).pluck(:ip)) }

app/models/report.rb

@@ -115,6 +115,10 @@ class Report < ApplicationRecord
     Report.where.not(id: id).where(target_account_id: target_account_id).unresolved.exists?
   end
 
+  def to_log_human_identifier
+    id
+  end
+
   def history
     subquery = [
       Admin::ActionLog.where(
@@ -136,6 +140,8 @@ class Report < ApplicationRecord
     Admin::ActionLog.from(Arel::Nodes::As.new(subquery, Admin::ActionLog.arel_table))
   end
 
+  private
+
   def set_uri
     self.uri = ActivityPub::TagManager.instance.generate_uri_for(self) if uri.nil? && account.local?
   end

app/models/status.rb

@@ -166,6 +166,14 @@ class Status < ApplicationRecord
     ].compact.join("\n\n")
   end
 
+  def to_log_human_identifier
+    account.acct
+  end
+
+  def to_log_permalink
+    ActivityPub::TagManager.instance.uri_for(self)
+  end
+
   def reply?
     !in_reply_to_id.nil? || attributes['reply']
   end

app/models/unavailable_domain.rb

@@ -16,6 +16,10 @@ class UnavailableDomain < ApplicationRecord
 
   after_commit :reset_cache!
 
+  def to_log_human_identifier
+    domain
+  end
+
   private
 
   def reset_cache!

app/models/user.rb

@@ -181,6 +181,14 @@ class User < ApplicationRecord
     update!(disabled: false)
   end
 
+  def to_log_human_identifier
+    account.acct
+  end
+
+  def to_log_route_param
+    account_id
+  end
+
   def confirm
     new_user      = !confirmed?
     self.approved = true if open_registrations? && !sign_up_from_ip_requires_approval?

app/models/user_role.rb

@@ -155,6 +155,10 @@ class UserRole < ApplicationRecord
     end
   end
 
+  def to_log_human_identifier
+    name
+  end
+
   private
 
   def in_permissions?(privilege)

config/locales/en.yml

@@ -235,6 +235,7 @@ en:
         approve_user: Approve User
         assigned_to_self_report: Assign Report
         change_email_user: Change E-mail for User
+        change_role_user: Change Role of User
         confirm_user: Confirm User
         create_account_warning: Create Warning
         create_announcement: Create Announcement
@@ -244,6 +245,7 @@ en:
         create_email_domain_block: Create E-mail Domain Block
         create_ip_block: Create IP rule
         create_unavailable_domain: Create Unavailable Domain
+        create_user_role: Create Role
         demote_user: Demote User
         destroy_announcement: Delete Announcement
         destroy_custom_emoji: Delete Custom Emoji
@@ -254,6 +256,7 @@ en:
         destroy_ip_block: Delete IP rule
         destroy_status: Delete Post
         destroy_unavailable_domain: Delete Unavailable Domain
+        destroy_user_role: Destroy Role
         disable_2fa_user: Disable 2FA
         disable_custom_emoji: Disable Custom Emoji
         disable_sign_in_token_auth_user: Disable E-mail Token Authentication for User
@@ -281,11 +284,13 @@ en:
         update_custom_emoji: Update Custom Emoji
         update_domain_block: Update Domain Block
         update_status: Update Post
+        update_user_role: Update Role
       actions:
         approve_appeal_html: "%{name} approved moderation decision appeal from %{target}"
         approve_user_html: "%{name} approved sign-up from %{target}"
         assigned_to_self_report_html: "%{name} assigned report %{target} to themselves"
         change_email_user_html: "%{name} changed the e-mail address of user %{target}"
+        change_role_user_html: "%{name} changed role of %{target}"
         confirm_user_html: "%{name} confirmed e-mail address of user %{target}"
         create_account_warning_html: "%{name} sent a warning to %{target}"
         create_announcement_html: "%{name} created new announcement %{target}"
@@ -295,9 +300,10 @@ en:
         create_email_domain_block_html: "%{name} blocked e-mail domain %{target}"
         create_ip_block_html: "%{name} created rule for IP %{target}"
         create_unavailable_domain_html: "%{name} stopped delivery to domain %{target}"
+        create_user_role_html: "%{name} created %{target} role"
         demote_user_html: "%{name} demoted user %{target}"
         destroy_announcement_html: "%{name} deleted announcement %{target}"
-        destroy_custom_emoji_html: "%{name} destroyed emoji %{target}"
+        destroy_custom_emoji_html: "%{name} deleted emoji %{target}"
         destroy_domain_allow_html: "%{name} disallowed federation with domain %{target}"
         destroy_domain_block_html: "%{name} unblocked domain %{target}"
         destroy_email_domain_block_html: "%{name} unblocked e-mail domain %{target}"
@@ -305,6 +311,7 @@ en:
         destroy_ip_block_html: "%{name} deleted rule for IP %{target}"
         destroy_status_html: "%{name} removed post by %{target}"
         destroy_unavailable_domain_html: "%{name} resumed delivery to domain %{target}"
+        destroy_user_role_html: "%{name} deleted %{target} role"
         disable_2fa_user_html: "%{name} disabled two factor requirement for user %{target}"
         disable_custom_emoji_html: "%{name} disabled emoji %{target}"
         disable_sign_in_token_auth_user_html: "%{name} disabled e-mail token authentication for %{target}"
@@ -332,7 +339,7 @@ en:
         update_custom_emoji_html: "%{name} updated emoji %{target}"
         update_domain_block_html: "%{name} updated domain block for %{target}"
         update_status_html: "%{name} updated post by %{target}"
-      deleted_status: "(deleted post)"
+        update_user_role_html: "%{name} changed %{target} role"
       empty: No logs found.
       filter_by_action: Filter by action
       filter_by_user: Filter by user

db/migrate/20220824164433_add_human_identifier_to_admin_action_logs.rb

@@ -0,0 +1,7 @@
+class AddHumanIdentifierToAdminActionLogs < ActiveRecord::Migration[6.1]
+  def change
+    add_column :admin_action_logs, :human_identifier, :string
+    add_column :admin_action_logs, :route_param, :string
+    add_column :admin_action_logs, :permalink, :string
+  end
+end

db/post_migrate/20220824164532_remove_recorded_changes_from_admin_action_logs.rb

@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+class RemoveRecordedChangesFromAdminActionLogs < ActiveRecord::Migration[5.2]
+  disable_ddl_transaction!
+
+  def change
+    safety_assured { remove_column :admin_action_logs, :recorded_changes, :text }
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2022_08_08_101323) do
+ActiveRecord::Schema.define(version: 2022_08_24_164532) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -205,9 +205,11 @@ ActiveRecord::Schema.define(version: 2022_08_08_101323) do
     t.string "action", default: "", null: false
     t.string "target_type"
     t.bigint "target_id"
-    t.text "recorded_changes", default: "", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.string "human_identifier"
+    t.string "route_param"
+    t.string "permalink"
     t.index ["account_id"], name: "index_admin_action_logs_on_account_id"
     t.index ["target_type", "target_id"], name: "index_admin_action_logs_on_target_type_and_target_id"
   end

spec/helpers/admin/action_log_helper_spec.rb

@@ -3,32 +3,4 @@
 require 'rails_helper'
 
 RSpec.describe Admin::ActionLogsHelper, type: :helper do
-  klass = Class.new do
-    include ActionView::Helpers
-    include Admin::ActionLogsHelper
-  end
-
-  let(:hoge) { klass.new }
-
-  describe '#log_target' do
-    after do
-      hoge.log_target(log)
-    end
-
-    context 'log.target' do
-      let(:log) { double(target: true) }
-
-      it 'calls linkable_log_target' do
-        expect(hoge).to receive(:linkable_log_target).with(log.target)
-      end
-    end
-
-    context '!log.target' do
-      let(:log) { double(target: false, target_type: :type, recorded_changes: :change) }
-
-      it 'calls log_target_from_history' do
-        expect(hoge).to receive(:log_target_from_history).with(log.target_type, log.recorded_changes)
-      end
-    end
-  end
 end