Merge pull request #2305 from alphagov/record-background-deletion-of-…

…access-tokens-and-grants-in-event-log

Record background deletion of access tokens and grants in event log

app/models/event_log.rb

@@ -45,6 +45,8 @@ class EventLog < ApplicationRecord
  TWO_STEP_EXEMPTION_UPDATED = LogEntry.new(id: 40, description: "2-step verification exemption updated", require_uid: true, require_initiator: true),
  TWO_STEP_EXEMPTION_REMOVED = LogEntry.new(id: 41, description: "Exemption from 2-step verification removed", require_uid: true, require_initiator: true),
  TWO_STEP_MANDATED = LogEntry.new(id: 42, description: "2-step verification setup mandated at next login", require_uid: true, require_initiator: true),
+ ACCESS_GRANTS_DELETED = LogEntry.new(id: 43, description: "Access grants deleted", require_uid: true),
+ ACCESS_TOKENS_DELETED = LogEntry.new(id: 44, description: "Access tokens deleted", require_uid: true),
 
  # We no longer expire passwords, but we keep this event for history purposes
  PASSWORD_EXPIRED = LogEntry.new(id: 6, description: "Password expired"),

app/models/user.rb

@@ -114,8 +114,11 @@ def exempt_from_2sv?
  reason_for_2sv_exemption.present?
  end
 
- def event_logs
- EventLog.where(uid:).order(created_at: :desc).includes(:user_agent)
+ def event_logs(event: nil)
+ relation = EventLog.where(uid:)
+ relation = relation.merge(EventLog.where(event_id: event.id)) if event.present?
+
+ relation.order(created_at: :desc).includes(:user_agent)
  end
 
  def generate_uid

lib/expired_oauth_access_records_deleter.rb

@@ -0,0 +1,39 @@
+class ExpiredOauthAccessRecordsDeleter
+ CLASSES = {
+ access_grant: Doorkeeper::AccessGrant,
+ access_token: Doorkeeper::AccessToken,
+ }.freeze
+ EVENTS = {
+ access_grant: EventLog::ACCESS_GRANTS_DELETED,
+ access_token: EventLog::ACCESS_TOKENS_DELETED,
+ }.freeze
+
+ HAS_EXPIRED = "expires_in is not null AND DATE_ADD(created_at, INTERVAL expires_in second) < ?".freeze
+
+ def initialize(record_type:)
+ @record_class = CLASSES.fetch(record_type)
+ @event = EVENTS.fetch(record_type)
+ @total_deleted = 0
+ end
+
+ attr_reader :record_class, :total_deleted
+
+ def delete_expired
+ @record_class.where(HAS_EXPIRED, Time.zone.now).in_batches do |relation|
+ records_by_user_id = relation.includes(:application).group_by(&:resource_owner_id)
+ all_users = User.where(id: records_by_user_id.keys)
+
+ all_users.each do |user|
+ application_names = records_by_user_id[user.id].map { |record| record.application.name }
+
+ EventLog.record_event(
+ user,
+ @event,
+ trailing_message: "for #{application_names.to_sentence}",
+ )
+ end
+
+ @total_deleted += relation.delete_all
+ end
+ end
+end

lib/tasks/oauth_access_records.rake

@@ -6,24 +6,12 @@ end
 namespace :oauth_access_records do
  desc "Delete expired OAuth access grants and tokens"
  task delete_expired: :environment do
- klasses = [Doorkeeper::AccessGrant, Doorkeeper::AccessToken]
- klasses.each do |klass|
- ids = [nil]
+ %i[access_grant access_token].each do |record_type|
+ deleter = ExpiredOauthAccessRecordsDeleter.new(record_type:)
 
- count = 0
+ deleter.delete_expired
 
- until ids.empty?
- ids = klass
- .where("expires_in is not null AND DATE_ADD(created_at, INTERVAL expires_in second) < ?", Time.zone.now)
- .limit(1000)
- .pluck(:id)
-
- count += ids.size
-
- klass.where(id: ids).delete_all
- end
-
- puts "Deleted #{count} expired #{klass} records"
+ puts "Deleted #{deleter.total_deleted} expired #{deleter.record_class} records"
  end
  end
 end

test/factories/oauth_access_grants.rb

@@ -0,0 +1,8 @@
+FactoryBot.define do
+ factory :access_grant, class: Doorkeeper::AccessGrant do
+ sequence(:resource_owner_id) { |n| n }
+ application
+ expires_in { 2.hours }
+ redirect_uri { "https://app.com/callback" }
+ end
+end

test/lib/expired_oauth_access_records_deleter_test.rb

@@ -0,0 +1,108 @@
+require "test_helper"
+
+class ExpiredOauthAccessRecordsDeleterTest < ActiveSupport::TestCase
+ setup do
+ $stdout.stubs(:write)
+ end
+
+ context "deleting grant records" do
+ should "delete expired `Doorkeeper::AccessGrant`s" do
+ user = create(:user)
+ create(:access_grant, resource_owner_id: user.id, expires_in: 0)
+ one_hour_grant = create(:access_grant, resource_owner_id: user.id, expires_in: 1.hour)
+
+ Timecop.travel(5.minutes.from_now)
+
+ deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_grant)
+ deleter.delete_expired
+
+ assert_equal [one_hour_grant], Doorkeeper::AccessGrant.where(resource_owner_id: user.id)
+ end
+
+ should "provide a count of the total number of records deleted" do
+ user = create(:user)
+ create(:access_grant, resource_owner_id: user.id, expires_in: 0)
+ create(:access_grant, resource_owner_id: user.id, expires_in: 0)
+
+ Timecop.travel(5.minutes.from_now)
+
+ deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_grant)
+ deleter.delete_expired
+
+ assert_equal 2, deleter.total_deleted
+ end
+
+ should "create a single account activity log entry per User" do
+ user = create(:user)
+ create(:access_grant, resource_owner_id: user.id, expires_in: 0)
+ create(:access_grant, resource_owner_id: user.id, expires_in: 0)
+
+ Timecop.travel(5.minutes.from_now)
+
+ deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_grant)
+ deleter.delete_expired
+
+ assert_equal 1, user.event_logs(event: EventLog::ACCESS_GRANTS_DELETED).count
+ end
+ end
+
+ context "deleting token records" do
+ should "delete expired `Doorkeeper::AccessToken`s" do
+ user = create(:user)
+ create(:access_token, resource_owner_id: user.id, expires_in: 0)
+ one_hour_token = create(:access_token, resource_owner_id: user.id, expires_in: 1.hour)
+
+ Timecop.travel(5.minutes.from_now)
+
+ deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_token)
+ deleter.delete_expired
+
+ assert_equal [one_hour_token], user.authorisations
+ end
+
+ should "provide a count of the total number of records deleted" do
+ user = create(:user)
+ create(:access_token, resource_owner_id: user.id, expires_in: 0)
+ create(:access_token, resource_owner_id: user.id, expires_in: 0)
+
+ Timecop.travel(5.minutes.from_now)
+
+ deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_token)
+ deleter.delete_expired
+
+ assert_equal 2, deleter.total_deleted
+ end
+
+ should "create a single account activity log entry per User" do
+ user = create(:user)
+ create(:access_token, resource_owner_id: user.id, expires_in: 0)
+ create(:access_token, resource_owner_id: user.id, expires_in: 0)
+ other_user = create(:user)
+ create(:access_token, resource_owner_id: other_user.id, expires_in: 0)
+ create(:access_token, resource_owner_id: other_user.id, expires_in: 0)
+
+ Timecop.travel(5.minutes.from_now)
+
+ deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_token)
+ deleter.delete_expired
+
+ assert_equal(1, user.event_logs(event: EventLog::ACCESS_TOKENS_DELETED).count)
+ assert_equal(1, other_user.event_logs(event: EventLog::ACCESS_TOKENS_DELETED).count)
+ end
+
+ should "include the names of the affected applications in the log entry" do
+ user = create(:user)
+ create(:access_token, application: create(:application, name: "Sweet Publisher"), resource_owner_id: user.id, expires_in: 0)
+ create(:access_token, application: create(:application, name: "Sour Publisher"), resource_owner_id: user.id, expires_in: 0)
+
+ Timecop.travel(5.minutes.from_now)
+
+ deleter = ExpiredOauthAccessRecordsDeleter.new(record_type: :access_token)
+ deleter.delete_expired
+
+ user_event_log = user.event_logs(event: EventLog::ACCESS_TOKENS_DELETED).first
+ assert_match(/Sweet/, user_event_log.trailing_message)
+ assert_match(/Sour/, user_event_log.trailing_message)
+ end
+ end
+end

test/models/user_test.rb

@@ -687,6 +687,24 @@ def setup
  end
  end
 
+ context "#event_logs" do
+ should "return all of user's EventLogs" do
+ user = create(:user)
+ create(:event_log, uid: user.uid, event_id: EventLog::UNSUCCESSFUL_LOGIN.id)
+ create(:event_log, uid: user.uid, event_id: EventLog::SUCCESSFUL_LOGIN.id)
+
+ assert_equal 2, user.event_logs.count
+ end
+
+ should "filter user's EventLogs by event type when an argument is given" do
+ user = create(:user)
+ create(:event_log, uid: user.uid, event_id: EventLog::UNSUCCESSFUL_LOGIN.id)
+ create(:event_log, uid: user.uid, event_id: EventLog::SUCCESSFUL_LOGIN.id)
+
+ assert_equal 1, user.event_logs(event: EventLog::SUCCESSFUL_LOGIN).count
+ end
+ end
+
  def authenticate_access(user, app)
  ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id)
  end