From 38d56542eba00eed160bf12728cb25ef1572d1c3 Mon Sep 17 00:00:00 2001 From: Jacob Herrington Date: Thu, 26 Mar 2020 16:59:24 -0500 Subject: [PATCH] [deploy] Moderator and admin actions log (#6797) * Add audit logging for negative reactions This change saves requests that create negative reactions to the audit log. We need to be monitoring admin and moderator actions throughout the app to increase visiblity on how our tools are being used and identify abuse. Additionally, this will help east concerns about giving elevated users more powerful tools. * Add a basic UI for moderator audit logs * Log experience level ratings * Log tag adjustments * UI tweaks for moderator actions log * Add logging to internal reactions controller * Add logging for admin actions on users and articles * Add searching to the moderator actions table * Move audit instrumentation out of concern We are able to use a PORO here instead of using a Rails concern. Moving this also makes syntax to audit something quite a bit more explicit and obvious to future developers * Change moderator logs pagination page length * Move auditing to after action filters * Add moderator actions to the internal navbar * Add request spec for internal moderator actions * Use request params to populate AuditLog slug --- .../concerns/audit_instrumentation.rb | 16 ---- .../internal/articles_controller.rb | 4 + .../internal/moderator_actions_controller.rb | 12 +++ .../internal/reactions_controller.rb | 4 + app/controllers/internal/tags_controller.rb | 6 +- app/controllers/internal/users_controller.rb | 5 + app/controllers/rating_votes_controller.rb | 3 + app/controllers/reactions_controller.rb | 92 +++++++++++-------- app/controllers/tag_adjustments_controller.rb | 4 + app/models/moderator_action.rb | 7 ++ app/models/reaction.rb | 24 ++++- app/models/user.rb | 4 + app/services/audit/logger.rb | 22 +++++ .../internal/moderator_actions/index.html.erb | 42 +++++++++ app/views/internal/shared/_navbar.html.erb | 2 +- config/routes.rb | 1 + spec/factories/audit_logs.rb | 3 + .../internal/moderator_actions_spec.rb | 40 ++++++++ spec/requests/internal/moderator_logs_spec.rb | 24 +---- 19 files changed, 233 insertions(+), 82 deletions(-) delete mode 100644 app/controllers/concerns/audit_instrumentation.rb create mode 100644 app/controllers/internal/moderator_actions_controller.rb create mode 100644 app/models/moderator_action.rb create mode 100644 app/services/audit/logger.rb create mode 100644 app/views/internal/moderator_actions/index.html.erb create mode 100644 spec/requests/internal/moderator_actions_spec.rb diff --git a/app/controllers/concerns/audit_instrumentation.rb b/app/controllers/concerns/audit_instrumentation.rb deleted file mode 100644 index 1b020e1d3..000000000 --- a/app/controllers/concerns/audit_instrumentation.rb +++ /dev/null @@ -1,16 +0,0 @@ -module AuditInstrumentation - extend ActiveSupport::Concern - - included do - def notify(listener, user, slug) - Audit::Notification.notify(listener) do |payload| - payload.user_id = user.id - payload.roles = user.roles.pluck(:name) - payload.slug = slug - if block_given? - payload.data = yield - end - end - end - end -end diff --git a/app/controllers/internal/articles_controller.rb b/app/controllers/internal/articles_controller.rb index 745753079..7df821dae 100644 --- a/app/controllers/internal/articles_controller.rb +++ b/app/controllers/internal/articles_controller.rb @@ -1,6 +1,10 @@ class Internal::ArticlesController < Internal::ApplicationController layout "internal" + after_action only: [:update] do + Audit::Logger.log(:moderator, current_user, params.dup) + end + def index @pending_buffer_updates = BufferUpdate.where(status: "pending").includes(:article) diff --git a/app/controllers/internal/moderator_actions_controller.rb b/app/controllers/internal/moderator_actions_controller.rb new file mode 100644 index 000000000..4e3e4ef28 --- /dev/null +++ b/app/controllers/internal/moderator_actions_controller.rb @@ -0,0 +1,12 @@ +class Internal::ModeratorActionsController < Internal::ApplicationController + layout "internal" + + def index + @q = AuditLog. + includes(:user). + where(category: "moderator.audit.log"). + order(created_at: :desc). + ransack(params[:q]) + @moderator_actions = @q.result.page(params[:page] || 1).per(25) + end +end diff --git a/app/controllers/internal/reactions_controller.rb b/app/controllers/internal/reactions_controller.rb index f0d63360a..b3edaac5e 100644 --- a/app/controllers/internal/reactions_controller.rb +++ b/app/controllers/internal/reactions_controller.rb @@ -1,4 +1,8 @@ class Internal::ReactionsController < Internal::ApplicationController + after_action only: [:update] do + Audit::Logger.log(:moderator, current_user, params.dup) + end + def update @reaction = Reaction.find(params[:id]) @reaction.update(status: params[:reaction][:status]) diff --git a/app/controllers/internal/tags_controller.rb b/app/controllers/internal/tags_controller.rb index 701094a00..1ed43dcf4 100644 --- a/app/controllers/internal/tags_controller.rb +++ b/app/controllers/internal/tags_controller.rb @@ -1,7 +1,10 @@ class Internal::TagsController < Internal::ApplicationController - include AuditInstrumentation layout "internal" + after_action only: [:update] do + Audit::Logger.log(:moderator, current_user, params.dup) + end + def index @tags = if params[:state] == "supported" Tag.where(supported: true).order("taggings_count DESC").page(params[:page]).per(50) @@ -25,7 +28,6 @@ class Internal::TagsController < Internal::ApplicationController remove_moderator if @remove_user_id @tag.update!(tag_params) - notify(:internal, current_user, __method__) { tag_params.dup } redirect_to "/internal/tags/#{params[:id]}" end diff --git a/app/controllers/internal/users_controller.rb b/app/controllers/internal/users_controller.rb index 2aed5a635..e1f8f6a87 100644 --- a/app/controllers/internal/users_controller.rb +++ b/app/controllers/internal/users_controller.rb @@ -1,6 +1,10 @@ class Internal::UsersController < Internal::ApplicationController layout "internal" + after_action only: %i[update user_status banish full_delete merge] do + Audit::Logger.log(:moderator, current_user, params.dup) + end + def index @users = case params[:state] when /role\-/ @@ -68,6 +72,7 @@ class Internal::UsersController < Internal::ApplicationController rescue StandardError => e flash[:danger] = e.message end + redirect_to "/internal/users/#{@user.id}/edit" end diff --git a/app/controllers/rating_votes_controller.rb b/app/controllers/rating_votes_controller.rb index 259de37e9..b90482de2 100644 --- a/app/controllers/rating_votes_controller.rb +++ b/app/controllers/rating_votes_controller.rb @@ -1,5 +1,8 @@ class RatingVotesController < ApplicationController after_action :verify_authorized + after_action only: [:create] do + Audit::Logger.log(:moderator, current_user, params.dup) + end def create authorize RatingVote diff --git a/app/controllers/reactions_controller.rb b/app/controllers/reactions_controller.rb index 14b9f7735..c9ac88a13 100644 --- a/app/controllers/reactions_controller.rb +++ b/app/controllers/reactions_controller.rb @@ -49,7 +49,9 @@ class ReactionsController < ApplicationController def create authorize Reaction + Rails.cache.delete "count_for_reactable-#{params[:reactable_type]}-#{params[:reactable_id]}" + category = params[:category] || "like" reaction = Reaction.where( user_id: current_user.id, @@ -57,40 +59,38 @@ class ReactionsController < ApplicationController reactable_type: params[:reactable_type], category: category, ).first - result = "" - if reaction - current_user.touch - reaction.destroy - Moderator::SinkArticles.call(reaction.reactable_id) if vomit_reaction_on_user?(reaction) - Notification.send_reaction_notification_without_delay(reaction, reaction_user(reaction)) - Notification.send_reaction_notification_without_delay(reaction, reaction.reactable.organization) if organization_article?(reaction) - result = "destroy" - else - create_params = { - user_id: current_user.id, - reactable_id: params[:reactable_id], - reactable_type: params[:reactable_type], - category: category - } - create_params[:status] = "confirmed" if current_user&.any_admin? - reaction = Reaction.new(create_params) - unless reaction.save + # if the reaction already exists, destroy it + if reaction + result = destroy_reaction(reaction) + + if reaction.negative? && current_user.auditable? + updated_params = params.dup + updated_params[:action] = "destroy" + Audit::Logger.log(:moderator, current_user, updated_params) + end + else + reaction = build_reaction(category) + + if reaction.save + Moderator::SinkArticles.call(reaction.reactable_id) if reaction.vomit_on_user? + + Notification.send_reaction_notification(reaction, reaction.target_user) + Notification.send_reaction_notification(reaction, reaction.reactable.organization) if reaction.reaction_on_organization_article? + + result = "create" + + if category == "readinglist" && current_user.experience_level + rate_article(reaction) + end + + if reaction.negative? && current_user.auditable? + Audit::Logger.log(:moderator, current_user, params.dup) + end + else render json: { error: reaction.errors.full_messages.join(", "), status: 422 }, status: :unprocessable_entity return end - - result = "create" - Moderator::SinkArticles.call(reaction.reactable_id) if vomit_reaction_on_user?(reaction) - Notification.send_reaction_notification(reaction, reaction_user(reaction)) - Notification.send_reaction_notification(reaction, reaction.reactable.organization) if organization_article?(reaction) - if category == "readinglist" && current_user.experience_level - RatingVote.create(article_id: reaction.reactable_id, - group: "experience_level", - user_id: current_user.id, - context: "readinglist_reaction", - rating: current_user.experience_level) - end end render json: { result: result, category: category } end @@ -103,19 +103,31 @@ class ReactionsController < ApplicationController private - def reaction_user(reaction) - if reaction.reactable_type == "User" - reaction.reactable - else - reaction.reactable.user - end + def build_reaction(category) + create_params = { + user_id: current_user.id, + reactable_id: params[:reactable_id], + reactable_type: params[:reactable_type], + category: category + } + create_params[:status] = "confirmed" if current_user&.any_admin? + Reaction.new(create_params) end - def organization_article?(reaction) - reaction.reactable_type == "Article" && reaction.reactable.organization.present? + def destroy_reaction(reaction) + current_user.touch + reaction.destroy + Moderator::SinkArticles.call(reaction.reactable_id) if reaction.vomit_on_user? + Notification.send_reaction_notification_without_delay(reaction, reaction.target_user) + Notification.send_reaction_notification_without_delay(reaction, reaction.reactable.organization) if reaction.reaction_on_organization_article? + "destroy" end - def vomit_reaction_on_user?(reaction) - reaction.reactable_type == "User" && reaction.category == "vomit" + def rate_article(reaction) + RatingVote.create(article_id: reaction.reactable_id, + group: "experience_level", + user_id: current_user.id, + context: "readinglist_reaction", + rating: current_user.experience_level) end end diff --git a/app/controllers/tag_adjustments_controller.rb b/app/controllers/tag_adjustments_controller.rb index e3a7d16bd..436a084b6 100644 --- a/app/controllers/tag_adjustments_controller.rb +++ b/app/controllers/tag_adjustments_controller.rb @@ -1,4 +1,8 @@ class TagAdjustmentsController < ApplicationController + after_action only: %i[create destroy] do + Audit::Logger.log(:moderator, current_user, params.dup) + end + def create authorize(User, :moderation_routes?) service = TagAdjustmentCreationService.new( diff --git a/app/models/moderator_action.rb b/app/models/moderator_action.rb new file mode 100644 index 000000000..4db2b611f --- /dev/null +++ b/app/models/moderator_action.rb @@ -0,0 +1,7 @@ +class ModeratorAction < ApplicationRecord + resourcify + # This class exists to take advantage of Rolify for limiting authorization + # on internal reports. + # NOTE: It is not backed by a database table and should not be expected to + # function like a traditional Rails model +end diff --git a/app/models/reaction.rb b/app/models/reaction.rb index 22bbfa462..dc9fc9fee 100644 --- a/app/models/reaction.rb +++ b/app/models/reaction.rb @@ -79,6 +79,26 @@ class Reaction < ApplicationRecord (receiver.is_a?(User) && reactable.receive_notifications == false) end + def vomit_on_user? + reactable_type == "User" && category == "vomit" + end + + def reaction_on_organization_article? + reactable_type == "Article" && reactable.organization.present? + end + + def target_user + if reactable_type == "User" + reactable + else + reactable.user + end + end + + def negative? + category == "vomit" || category == "thumbsdown" + end + private def touch_user @@ -180,10 +200,6 @@ class Reaction < ApplicationRecord remove_from_index! end - def negative? - category == "vomit" || category == "thumbsdown" - end - def record_field_test_event Users::RecordFieldTestEventWorker.perform_async(user_id, :user_home_feed, "user_creates_reaction") end diff --git a/app/models/user.rb b/app/models/user.rb index 93d4f1f0e..8256e6ac8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -478,6 +478,10 @@ class User < ApplicationRecord MailchimpBot.new(self).unsubscribe_all_newsletters end + def auditable? + trusted || tag_moderator? || any_admin? + end + def tag_moderator? roles.where(name: "tag_moderator").any? end diff --git a/app/services/audit/logger.rb b/app/services/audit/logger.rb new file mode 100644 index 000000000..8b0f3d820 --- /dev/null +++ b/app/services/audit/logger.rb @@ -0,0 +1,22 @@ +module Audit + class Logger + REDACTED_KEYS = %w[authenticity_token utf8 commit].freeze + + def self.log(listener, user, data = nil) + Audit::Notification.notify(listener) do |payload| + payload.user_id = user.id + payload.roles = user.roles.pluck(:name) + payload.data = if block_given? + yield + else + filter_redacted_keys(data) + end + payload.slug = data["action"] + end + end + + def self.filter_redacted_keys(data) + data.except(*REDACTED_KEYS) + end + end +end diff --git a/app/views/internal/moderator_actions/index.html.erb b/app/views/internal/moderator_actions/index.html.erb new file mode 100644 index 000000000..cbb00b0a9 --- /dev/null +++ b/app/views/internal/moderator_actions/index.html.erb @@ -0,0 +1,42 @@ +
+
+
+
+ <%= search_form_for @q, url: internal_moderator_actions_path, class: "form-inline justify-content-end" do |f| %> + + <%= f.label :user_username_cont, "User", class: "sr-only" %> + <%= f.search_field :user_username_cont, placeholder: "User", class: "form-control mx-3" %> + + <%= f.submit "Search", class: "btn btn-secondary" %> + <% end %> +
+
+ +<%= paginate @moderator_actions %> + + + + + + + + + + + + + <% @moderator_actions.each do |action| %> + + + + + + + + <% end %> + +
IDUserActionData + <%= content_tag :span, sort_link(@q, :created_at, "Date"), class: "mx-2" %> +
<%= action.id %><%= link_to action.user.username, internal_user_path(action.user_id) %><%= "#{action.data['action'].humanize} #{action.data['controller'].humanize.singularize.titleize}" if action.data.present? %><%= action.data %><%= action.created_at %>
+ +<%= paginate @moderator_actions %> diff --git a/app/views/internal/shared/_navbar.html.erb b/app/views/internal/shared/_navbar.html.erb index a398ed985..d86b313de 100644 --- a/app/views/internal/shared/_navbar.html.erb +++ b/app/views/internal/shared/_navbar.html.erb @@ -1,4 +1,4 @@ -<% menu_items = %w[comments articles users tags welcome broadcasts reports response_templates pages tools chat_channels permissions growth mods config events badges organizations sponsorships podcasts negative_reactions].sort.each_with_object({}) { |v, h| h[v] = v } %> +<% menu_items = %w[comments articles users tags welcome broadcasts reports response_templates pages tools chat_channels permissions growth mods config events badges organizations sponsorships podcasts negative_reactions moderator_actions].sort.each_with_object({}) { |v, h| h[v] = v } %> <% menu_items[:listings] = "classified_listings" %> <% menu_items[:webhooks] = "webhook_endpoints" %> diff --git a/config/routes.rb b/config/routes.rb index 2f9f1991e..28ccc97a3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -52,6 +52,7 @@ Rails.application.routes.draw do resources :listings, only: %i[index edit update destroy], controller: "classified_listings" resources :pages, only: %i[index new create edit update destroy] resources :mods, only: %i[index update] + resources :moderator_actions, only: %i[index] resources :negative_reactions, only: %i[index] resources :permissions, only: %i[index] resources :podcasts, only: %i[index edit update destroy] do diff --git a/spec/factories/audit_logs.rb b/spec/factories/audit_logs.rb index 3143600ff..cb2e38d5e 100644 --- a/spec/factories/audit_logs.rb +++ b/spec/factories/audit_logs.rb @@ -1,4 +1,7 @@ FactoryBot.define do factory :audit_log do + category { Faker::Name.name } + slug { Faker::Creature::Animal } + data { { action: Faker::ProgrammingLanguage.name, controller: Faker::Lorem.word } } end end diff --git a/spec/requests/internal/moderator_actions_spec.rb b/spec/requests/internal/moderator_actions_spec.rb new file mode 100644 index 000000000..6e91b2b5c --- /dev/null +++ b/spec/requests/internal/moderator_actions_spec.rb @@ -0,0 +1,40 @@ +require "rails_helper" + +RSpec.describe "/internal/moderator_reactions", type: :request do + context "when the user is not an admin" do + let(:user) { create(:user) } + + before do + sign_in user + end + + it "blocks the request" do + expect do + get "/internal/moderator_actions" + end.to raise_error(Pundit::NotAuthorizedError) + end + end + + context "when the user is an admin" do + let(:admin) { create(:user, :admin) } + let!(:audit_log) { create(:audit_log, category: "moderator.audit.log", user_id: admin.id, roles: admin.roles.pluck(:name)) } + + before do + sign_in admin + end + + it "does not block the request" do + expect do + get "/internal/moderator_actions" + end.not_to raise_error + end + + describe "GETS /internal/moderator_actions" do + it "renders to appropriate page" do + get "/internal/moderator_actions" + expect(response.body).to include(admin.username) + expect(response.body).to include(audit_log.id.to_s) + end + end + end +end diff --git a/spec/requests/internal/moderator_logs_spec.rb b/spec/requests/internal/moderator_logs_spec.rb index ffb1acadc..e86a33039 100644 --- a/spec/requests/internal/moderator_logs_spec.rb +++ b/spec/requests/internal/moderator_logs_spec.rb @@ -4,7 +4,7 @@ RSpec.describe "/internal/tags", type: :request do let(:super_admin) { create(:user, :super_admin) } let(:tag_moderator) { create(:user) } let!(:tag) { create(:tag) } - let(:listener) { :internal } + let(:listener) { :moderator } before do sign_in super_admin @@ -15,26 +15,12 @@ RSpec.describe "/internal/tags", type: :request do Audit::Subscribe.forget listener end - def update_params(tag_moderator_id) - { - tag: { - tag_moderator_id: tag_moderator_id - } - } - end - - describe "POST /internal/tag/:id" do + describe "PUT /internal/tag/:id" do it "creates entry for #update action" do - allow(AssignTagModerator).to receive(:add_tag_moderators) + put internal_tag_path(tag.id), params: { id: tag.id, tag: { short_summary: Faker::Hipster.sentence } } - sidekiq_perform_enqueued_jobs do - put "/internal/tags/#{tag.id}", params: update_params(tag_moderator.id.to_s) - log = AuditLog.where(user_id: super_admin.id, slug: :update) - expected = update_params(tag_moderator.id.to_s)[:tag] - - expect(log.first.data.symbolize_keys).to eq expected - expect(log.count).to eq(1) - end + log = AuditLog.where(user_id: super_admin.id, slug: :update) + expect(log.count).to eq(1) end end end