From 30f89cfd9cae1b451ba1810f4191c5ed10094cab Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 12:42:38 +0100
Subject: [PATCH 01/14] Index event logs application_id and event_id

We want to be able to find event logs for particular applications and
particular events. Adding these indexes improves performance a lot.

For example, if we wanted to count the number of successful user
application events, excluding the two smokey user ids, we might do:

    SELECT count(*) FROM `event_logs`
    WHERE `event_logs`.`application_id` = 5
    AND `event_logs`.`event_id` = 47
    AND `event_logs`.`uid` NOT IN (
    'fd016f20-f219-012f-9436-12313c09b582', '4848a260-7471-0130-6823-005056030202'
    )
    ORDER BY `event_logs`.`uid`, `event_logs`.`created_at` DESC

Without the indexes, this explain analyzes as:

    -> Aggregate: count(0)  (cost=349696 rows=1) (actual time=61730..61730 rows=1 loops=1)
        -> Filter: ((event_logs.event_id = 47) and (event_logs.application_id = 5))  (cost=349315 rows=3806) (actual time=18553..61729 rows=8768 loops=1)
            -> Index range scan on event_logs using index_event_logs_on_uid_and_created_at over (NULL < uid < '4848a260-7471-0130-6823-005056030202') OR ('4848a260-7471-0130-6823-005056030202' < uid < 'fd016f20-f219-012f-9436-12313c09b582') OR ('fd016f20-f219-012f-9436-12313c09b582' < uid), with index condition: (event_logs.uid not in ('fd016f20-f219-012f-9436-12313c09b582','4848a260-7471-0130-6823-005056030202'))  (cost=349315 rows=380554) (actual time=18195..61690 rows=221264 loops=1)

It tries to use index_event_logs_on_uid_and_created_at to reduce the
amount of the table it needs to scan, but as we're only excluding a
couple of uids it's going to end up scanning almost the entire table.

With the indexes, it executes as:

    -> Aggregate: count(0)  (cost=17345 rows=1) (actual time=424..424 rows=1 loops=1)
        -> Filter: ((event_logs.event_id = 47) and (event_logs.application_id = 5) and (event_logs.uid not in ('fd016f20-f219-012f-9436-12313c09b582','4848a260-7471-0130-6823-005056030202')))  (cost=17147 rows=1982) (actual time=14.3..422 rows=8768 loops=1)
            -> Intersect rows sorted by row ID  (cost=17147 rows=18790) (actual time=14.3..400 rows=25978 loops=1)
                -> Index range scan on event_logs using index_event_logs_on_application_id over (application_id = 5)  (cost=0.00111..100 rows=90170) (actual time=0.0416..29.6 rows=48566 loops=1)
                -> Index range scan on event_logs using index_event_logs_on_event_id over (event_id = 47)  (cost=0.0011..826 rows=751740) (actual time=0.0211..210 rows=381773 loops=1)

Note the actual time reduction from 61.7 seconds to 0.4 seconds.

It's able to use both indexes to reduce the rows it needs to access just
to those matching the application and event id.
---
 db/migrate/20240809112119_index_event_logs_columns.rb |  8 ++++++++
 db/schema.rb                                          | 10 ++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)
 create mode 100644 db/migrate/20240809112119_index_event_logs_columns.rb

diff --git a/db/migrate/20240809112119_index_event_logs_columns.rb b/db/migrate/20240809112119_index_event_logs_columns.rb
new file mode 100644
index 000000000..815dc9071
--- /dev/null
+++ b/db/migrate/20240809112119_index_event_logs_columns.rb
@@ -0,0 +1,8 @@
+class IndexEventLogsColumns < ActiveRecord::Migration[7.1]
+  def change
+    change_table :event_logs, bulk: true do
+      add_index :event_logs, :application_id
+      add_index :event_logs, :event_id
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index f8c307a13..7b7f06301 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2024_04_29_130154) do
+ActiveRecord::Schema[7.1].define(version: 2024_08_09_112119) do
   create_table "batch_invitation_application_permissions", id: :integer, charset: "utf8mb3", force: :cascade do |t|
     t.integer "batch_invitation_id", null: false
     t.integer "supported_permission_id", null: false
@@ -51,11 +51,13 @@
     t.integer "user_agent_id"
     t.text "user_agent_string"
     t.string "user_email_string"
+    t.index ["application_id"], name: "index_event_logs_on_application_id"
+    t.index ["event_id"], name: "index_event_logs_on_event_id"
     t.index ["uid", "created_at"], name: "index_event_logs_on_uid_and_created_at"
     t.index ["user_agent_id"], name: "event_logs_user_agent_id_fk"
   end
 
-  create_table "oauth_access_grants", id: :integer, charset: "utf8mb3", collation: "utf8_unicode_ci", force: :cascade do |t|
+  create_table "oauth_access_grants", id: :integer, charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t|
     t.integer "resource_owner_id", null: false
     t.integer "application_id", null: false
     t.string "token", null: false
@@ -69,7 +71,7 @@
     t.index ["token"], name: "index_oauth_access_grants_on_token", unique: true
   end
 
-  create_table "oauth_access_tokens", id: :integer, charset: "utf8mb3", collation: "utf8_unicode_ci", force: :cascade do |t|
+  create_table "oauth_access_tokens", id: :integer, charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t|
     t.integer "resource_owner_id", null: false
     t.integer "application_id", null: false
     t.string "token", null: false
@@ -154,7 +156,7 @@
     t.index ["user_id", "application_id", "supported_permission_id"], name: "index_app_permissions_on_user_and_app_and_supported_permission", unique: true
   end
 
-  create_table "users", id: :integer, charset: "utf8mb3", collation: "utf8_unicode_ci", force: :cascade do |t|
+  create_table "users", id: :integer, charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t|
     t.string "name", null: false
     t.string "email", default: "", null: false
     t.string "encrypted_password", default: ""

From 0be00865377873e90aeb9e59f47ac3893abf86a0 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 12:54:10 +0100
Subject: [PATCH 02/14] Add has_many relation from Application to EventLog

Some event logs are specific to a particular application. It's
convenient to be able to do `application.event_logs`.
---
 app/models/doorkeeper/application.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb
index 05625608f..d8dcae546 100644
--- a/app/models/doorkeeper/application.rb
+++ b/app/models/doorkeeper/application.rb
@@ -2,6 +2,7 @@
 
 class Doorkeeper::Application < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
   has_many :supported_permissions, dependent: :destroy
+  has_many :event_logs, class_name: "EventLog"
 
   default_scope { not_retired.ordered_by_name }
 

From ef6047327318285530dca30af0b68c09cb37c2a3 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 12:56:16 +0100
Subject: [PATCH 03/14] Add belongs_to user relation to EventLog

Some event logs belong to a user (via. the uid field). It's convenient
to be able to do `EventLog.include(:user)`, and `event_log.user`.

Note that unfortunately you can't `EventLog.joins(:user)` with this
relation, because the event_logs table and the users table use different
collations.
---
 app/models/event_log.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/models/event_log.rb b/app/models/event_log.rb
index df7ff6f7b..6fe27f202 100644
--- a/app/models/event_log.rb
+++ b/app/models/event_log.rb
@@ -66,6 +66,7 @@ class EventLog < ApplicationRecord
   validates :application_id, presence: { if: proc { |event_log| EVENTS_REQUIRING_APPLICATION.include? event_log.entry } }
 
   belongs_to :initiator, class_name: "User"
+  belongs_to :user, class_name: "User", foreign_key: :uid, primary_key: :uid
   belongs_to :application, class_name: "Doorkeeper::Application"
   belongs_to :user_agent
 

From 4da8e9cf1cf20b7396b5944a6505e53b605edc0e Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 12:59:04 +0100
Subject: [PATCH 04/14] Add an application access logs page

This shows which users have signed in to a particular app at what times.
---
 .../doorkeeper_applications_controller.rb     | 11 +++++
 app/policies/application_policy.rb            |  1 +
 .../access_logs.html.erb                      | 42 +++++++++++++++++++
 .../doorkeeper_applications/edit.html.erb     |  4 ++
 config/routes.rb                              |  1 +
 5 files changed, 59 insertions(+)
 create mode 100644 app/views/doorkeeper_applications/access_logs.html.erb

diff --git a/app/controllers/doorkeeper_applications_controller.rb b/app/controllers/doorkeeper_applications_controller.rb
index 19609b565..2499b892d 100644
--- a/app/controllers/doorkeeper_applications_controller.rb
+++ b/app/controllers/doorkeeper_applications_controller.rb
@@ -24,6 +24,17 @@ def users_with_access
     @users = query.page(params[:page]).per(100)
   end
 
+  def access_logs
+    smokey_uids = User.where("name LIKE 'Smokey%'").pluck(:uid)
+    @logs = @application.event_logs
+      .includes(:user)
+      .where(event_id: 47)
+      .where.not(uid: smokey_uids)
+      .order(created_at: :desc)
+      .page(params[:page])
+      .per(100)
+  end
+
 private
 
   def load_and_authorize_application
diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb
index d35350575..d88e77e73 100644
--- a/app/policies/application_policy.rb
+++ b/app/policies/application_policy.rb
@@ -6,4 +6,5 @@ def index?
   alias_method :update?, :index?
   alias_method :manage_supported_permissions?, :index?
   alias_method :users_with_access?, :index?
+  alias_method :access_logs?, :index?
 end
diff --git a/app/views/doorkeeper_applications/access_logs.html.erb b/app/views/doorkeeper_applications/access_logs.html.erb
new file mode 100644
index 000000000..7ea15cca2
--- /dev/null
+++ b/app/views/doorkeeper_applications/access_logs.html.erb
@@ -0,0 +1,42 @@
+<% content_for :title, "#{@application.name} access log" %>
+
+<% content_for :breadcrumbs,
+               render("govuk_publishing_components/components/breadcrumbs", {
+                 collapse_on_mobile: true,
+                 breadcrumbs: [
+                   {
+                     title: "Dashboard",
+                     url: root_path,
+                   },
+                   {
+                     title: "Applications",
+                     url: doorkeeper_applications_path,
+                   },
+                   {
+                     title: @application.name,
+                     url: edit_doorkeeper_application_path(@application),
+                   }
+                 ]
+               })
+%>
+
+<% if @logs.any? %>
+  <%= render "components/table", {
+    caption: pluralize(number_with_delimiter(@logs.total_count), "event"),
+    caption_classes: "govuk-heading-m",
+    head: [
+      { text: "Time" },
+      { text: "Event" },
+    ],
+    rows: @logs.map do |log|
+      [ { text: formatted_date(log), format: "event-log-date" },
+        { text: "#{formatted_message(log)} for <strong>#{log.user&.name}</strong>".html_safe } ] unless log.requires_admin? && !current_user.govuk_admin?
+    end.compact
+  } %>
+
+  <%= paginate(@logs, theme: "gds") %>
+<% else %>
+  <%= render "govuk_publishing_components/components/notice", {
+    title: "No activity logged"
+  } %>
+<% end %>
diff --git a/app/views/doorkeeper_applications/edit.html.erb b/app/views/doorkeeper_applications/edit.html.erb
index 5e12a0cc2..e7aadc099 100644
--- a/app/views/doorkeeper_applications/edit.html.erb
+++ b/app/views/doorkeeper_applications/edit.html.erb
@@ -23,6 +23,10 @@
   doorkeeper_application_supported_permissions_path(@application),
   class: "govuk-link" %>
 
+<%= link_to "View access log",
+            access_logs_doorkeeper_application_path(@application),
+            class: "govuk-link" %>
+
 <div class="govuk-grid-row govuk-!-margin-top-5">
   <div class="govuk-grid-column-two-thirds">
     <%= form_for @application do |f| %>
diff --git a/config/routes.rb b/config/routes.rb
index e35bdbec7..89da1ffc0 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -92,6 +92,7 @@
   resources :doorkeeper_applications, only: %i[index edit update] do
     member do
       get :users_with_access
+      get :access_logs
     end
     resources :supported_permissions, only: %i[index new create edit update destroy] do
       get :confirm_destroy, on: :member

From 3eafa462c7a1d33ee92914dcaae3fbe4f2b6e339 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 13:23:39 +0100
Subject: [PATCH 05/14] Add toggle to include / exclude smokey users

---
 .../doorkeeper_applications_controller.rb        | 12 +++++++++---
 .../doorkeeper_applications/access_logs.html.erb | 16 ++++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/app/controllers/doorkeeper_applications_controller.rb b/app/controllers/doorkeeper_applications_controller.rb
index 2499b892d..9a7cac893 100644
--- a/app/controllers/doorkeeper_applications_controller.rb
+++ b/app/controllers/doorkeeper_applications_controller.rb
@@ -25,12 +25,17 @@ def users_with_access
   end
 
   def access_logs
-    smokey_uids = User.where("name LIKE 'Smokey%'").pluck(:uid)
-    @logs = @application.event_logs
+    relation = @application.event_logs
       .includes(:user)
       .where(event_id: 47)
-      .where.not(uid: smokey_uids)
       .order(created_at: :desc)
+
+    unless params[:include_smokey_users] == "true"
+      smokey_uids = User.where("name LIKE 'Smokey%'").pluck(:uid)
+      relation = relation.where.not(uid: smokey_uids)
+    end
+
+    @logs = relation
       .page(params[:page])
       .per(100)
   end
@@ -55,6 +60,7 @@ def doorkeeper_application_params
       :home_uri,
       :supports_push_updates,
       :api_only,
+      :include_smokey_users,
     )
   end
 end
diff --git a/app/views/doorkeeper_applications/access_logs.html.erb b/app/views/doorkeeper_applications/access_logs.html.erb
index 7ea15cca2..5f6109fef 100644
--- a/app/views/doorkeeper_applications/access_logs.html.erb
+++ b/app/views/doorkeeper_applications/access_logs.html.erb
@@ -1,5 +1,21 @@
 <% content_for :title, "#{@application.name} access log" %>
 
+<form class="govuk-form-group">
+<%= render "govuk_publishing_components/components/checkboxes", {
+  name: "include_smokey_users",
+  items: [
+    {
+      label: "Include Smokey Users",
+      value: "true",
+      checked: params["include_smokey_users"] == "true"
+    }
+  ]
+} %>
+<%= render "govuk_publishing_components/components/button", {
+  text: "Submit"
+} %>
+</form>
+
 <% content_for :breadcrumbs,
                render("govuk_publishing_components/components/breadcrumbs", {
                  collapse_on_mobile: true,

From 8bfb9af5367f74d830ae9803cf95710a54a6c097 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 14:56:44 +0100
Subject: [PATCH 06/14] Add month filter to access logs

---
 .../doorkeeper_applications_controller.rb     |  5 +++
 .../access_logs.html.erb                      | 42 ++++++++++++-------
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/app/controllers/doorkeeper_applications_controller.rb b/app/controllers/doorkeeper_applications_controller.rb
index 9a7cac893..899d505ee 100644
--- a/app/controllers/doorkeeper_applications_controller.rb
+++ b/app/controllers/doorkeeper_applications_controller.rb
@@ -35,6 +35,10 @@ def access_logs
       relation = relation.where.not(uid: smokey_uids)
     end
 
+    if params[:month].present?
+      relation = relation.where("DATE_FORMAT(created_at, '%Y-%m')=?", params[:month])
+    end
+
     @logs = relation
       .page(params[:page])
       .per(100)
@@ -61,6 +65,7 @@ def doorkeeper_application_params
       :supports_push_updates,
       :api_only,
       :include_smokey_users,
+      :month,
     )
   end
 end
diff --git a/app/views/doorkeeper_applications/access_logs.html.erb b/app/views/doorkeeper_applications/access_logs.html.erb
index 5f6109fef..e67346374 100644
--- a/app/views/doorkeeper_applications/access_logs.html.erb
+++ b/app/views/doorkeeper_applications/access_logs.html.erb
@@ -1,21 +1,5 @@
 <% content_for :title, "#{@application.name} access log" %>
 
-<form class="govuk-form-group">
-<%= render "govuk_publishing_components/components/checkboxes", {
-  name: "include_smokey_users",
-  items: [
-    {
-      label: "Include Smokey Users",
-      value: "true",
-      checked: params["include_smokey_users"] == "true"
-    }
-  ]
-} %>
-<%= render "govuk_publishing_components/components/button", {
-  text: "Submit"
-} %>
-</form>
-
 <% content_for :breadcrumbs,
                render("govuk_publishing_components/components/breadcrumbs", {
                  collapse_on_mobile: true,
@@ -36,6 +20,32 @@
                })
 %>
 
+<form>
+<%= render "govuk_publishing_components/components/checkboxes", {
+  name: "include_smokey_users",
+  items: [
+    {
+      label: "Include Smokey Users",
+      value: "true",
+      checked: params["include_smokey_users"] == "true"
+    }
+  ]
+} %>
+<%= render "govuk_publishing_components/components/input", {
+  label: {
+    text: "Month"
+  },
+  name: "month",
+  hint: "In YYYY-mm format",
+  value: params["month"]
+} %>
+<div class="govuk-form-group">
+<%= render "govuk_publishing_components/components/button", {
+  text: "Submit"
+} %>
+</div>
+</form>
+
 <% if @logs.any? %>
   <%= render "components/table", {
     caption: pluralize(number_with_delimiter(@logs.total_count), "event"),

From b271e50d7f7b36ac2c79e98bc3f7189fdb62e96b Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 14:57:41 +0100
Subject: [PATCH 07/14] Add monthly access stats

---
 .../doorkeeper_applications_controller.rb     | 19 ++++++
 app/policies/application_policy.rb            |  1 +
 .../doorkeeper_applications/edit.html.erb     |  4 ++
 .../monthly_access_stats.html.erb             | 61 +++++++++++++++++++
 config/routes.rb                              |  1 +
 5 files changed, 86 insertions(+)
 create mode 100644 app/views/doorkeeper_applications/monthly_access_stats.html.erb

diff --git a/app/controllers/doorkeeper_applications_controller.rb b/app/controllers/doorkeeper_applications_controller.rb
index 899d505ee..b70469b25 100644
--- a/app/controllers/doorkeeper_applications_controller.rb
+++ b/app/controllers/doorkeeper_applications_controller.rb
@@ -44,6 +44,25 @@ def access_logs
       .per(100)
   end
 
+  def monthly_access_stats
+    relation = @application.event_logs
+                .where(event_id: 47)
+                .group("DATE_FORMAT(created_at, '%Y-%m')")
+                .order(Arel.sql("DATE_FORMAT(created_at, '%Y-%m') DESC"))
+
+    unless params[:include_smokey_users] == "true"
+      smokey_uids = User.where("name LIKE 'Smokey%'").pluck(:uid)
+      relation = relation.where.not(uid: smokey_uids)
+    end
+
+    @monthly_access_stats = relation
+                .pluck(
+                  Arel.sql("DATE_FORMAT(created_at, '%Y-%m')"),
+                  Arel.sql("COUNT(*)"),
+                  Arel.sql("COUNT(DISTINCT uid)"),
+                )
+  end
+
 private
 
   def load_and_authorize_application
diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb
index d88e77e73..7ef76f899 100644
--- a/app/policies/application_policy.rb
+++ b/app/policies/application_policy.rb
@@ -7,4 +7,5 @@ def index?
   alias_method :manage_supported_permissions?, :index?
   alias_method :users_with_access?, :index?
   alias_method :access_logs?, :index?
+  alias_method :monthly_access_stats?, :index?
 end
diff --git a/app/views/doorkeeper_applications/edit.html.erb b/app/views/doorkeeper_applications/edit.html.erb
index e7aadc099..5ad233244 100644
--- a/app/views/doorkeeper_applications/edit.html.erb
+++ b/app/views/doorkeeper_applications/edit.html.erb
@@ -27,6 +27,10 @@
             access_logs_doorkeeper_application_path(@application),
             class: "govuk-link" %>
 
+<%= link_to "View monthly access stats",
+            monthly_access_stats_doorkeeper_application_path(@application),
+            class: "govuk-link" %>
+
 <div class="govuk-grid-row govuk-!-margin-top-5">
   <div class="govuk-grid-column-two-thirds">
     <%= form_for @application do |f| %>
diff --git a/app/views/doorkeeper_applications/monthly_access_stats.html.erb b/app/views/doorkeeper_applications/monthly_access_stats.html.erb
new file mode 100644
index 000000000..4e0386039
--- /dev/null
+++ b/app/views/doorkeeper_applications/monthly_access_stats.html.erb
@@ -0,0 +1,61 @@
+<% content_for :title, "Monthly access counts to #{@application.name}" %>
+
+<% content_for :breadcrumbs,
+               render("govuk_publishing_components/components/breadcrumbs", {
+                 collapse_on_mobile: true,
+                 breadcrumbs: [
+                   {
+                     title: "Dashboard",
+                     url: root_path,
+                   },
+                   {
+                     title: "Applications",
+                     url: doorkeeper_applications_path,
+                   },
+                   {
+                     title: @application.name,
+                     url: edit_doorkeeper_application_path(@application),
+                   }
+                 ]
+               })
+%>
+
+
+<form class="govuk-form-group">
+  <%= render "govuk_publishing_components/components/checkboxes", {
+    name: "include_smokey_users",
+    items: [
+      {
+        label: "Include Smokey Users",
+        value: "true",
+        checked: params["include_smokey_users"] == "true"
+      }
+    ]
+  } %>
+  <%= render "govuk_publishing_components/components/button", {
+    text: "Submit"
+  } %>
+</form>
+
+<% if @monthly_access_stats.any? %>
+  <%= render "components/table", {
+    head: [
+      { text: "Month" },
+      { text: "Total authorization count" },
+      { text: "Unique users authorization count" },
+      { text: "Access logs" },
+    ],
+    rows: @monthly_access_stats.map do |month, total_count, unique_users_count|
+      [
+        { text: month },
+        { text: total_count },
+        { text: unique_users_count },
+        { text: link_to("#{month} access logs", access_logs_doorkeeper_application_path(@application, month:), class: "govuk-link")},
+      ]
+    end
+  } %>
+<% else %>
+  <%= render "govuk_publishing_components/components/notice", {
+    title: "No activity logged"
+  } %>
+<% end %>
diff --git a/config/routes.rb b/config/routes.rb
index 89da1ffc0..5331e19b7 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -93,6 +93,7 @@
     member do
       get :users_with_access
       get :access_logs
+      get :monthly_access_stats
     end
     resources :supported_permissions, only: %i[index new create edit update destroy] do
       get :confirm_destroy, on: :member

From 231c0e8dd8588b08c974d15d106bbb2c65631f9a Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 15:16:50 +0100
Subject: [PATCH 08/14] Improve formatting of log rows

We can link to the user instead of just bolding their name. I'ts more
elegant to use a next if guard instead of the array unless / compact we
were using.
---
 app/views/doorkeeper_applications/access_logs.html.erb | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/app/views/doorkeeper_applications/access_logs.html.erb b/app/views/doorkeeper_applications/access_logs.html.erb
index e67346374..3c6ec1973 100644
--- a/app/views/doorkeeper_applications/access_logs.html.erb
+++ b/app/views/doorkeeper_applications/access_logs.html.erb
@@ -55,8 +55,12 @@
       { text: "Event" },
     ],
     rows: @logs.map do |log|
-      [ { text: formatted_date(log), format: "event-log-date" },
-        { text: "#{formatted_message(log)} for <strong>#{log.user&.name}</strong>".html_safe } ] unless log.requires_admin? && !current_user.govuk_admin?
+      next if log.requires_admin? && !current_user.govuk_admin?
+
+      [
+        { text: formatted_date(log), format: "event-log-date" },
+        { text: "#{formatted_message(log)} for #{link_to(log.user.name, log.user, class: "govuk-link")}".html_safe },
+      ]
     end.compact
   } %>
 

From 0e9d467e6b11d8c170acf342663b833bdad5de74 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 16:23:02 +0100
Subject: [PATCH 09/14] Add integration test for application access logs

---
 .../application_access_log_page_test.rb       | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 test/integration/application_access_log_page_test.rb

diff --git a/test/integration/application_access_log_page_test.rb b/test/integration/application_access_log_page_test.rb
new file mode 100644
index 000000000..81c2424a6
--- /dev/null
+++ b/test/integration/application_access_log_page_test.rb
@@ -0,0 +1,53 @@
+require "test_helper"
+
+class ApplicationAccessLogPageIntegrationTest < ActionDispatch::IntegrationTest
+  setup do
+    @application = create(:application, name: "app-name", description: "app-description")
+    @user = create(:user, name: "Normal User")
+  end
+
+  test "users don't have permission to view account access log" do
+    visit root_path
+    signin_with(@user)
+
+    visit access_logs_doorkeeper_application_path(@application)
+    flash = find("div[role='alert']")
+    assert flash.has_content?("You do not have permission to perform this action.")
+  end
+
+  context "logged in as an superadmin" do
+    setup do
+      visit new_user_session_path
+      @superadmin = create(:superadmin_user)
+      signin_with(@superadmin)
+    end
+
+    should "have permission to view account access log" do
+      visit access_logs_doorkeeper_application_path(@application)
+      assert_equal page.title, "app-name access log - GOV.UK Signon"
+    end
+
+    context "when there are no matching events" do
+      should "see a message stating that there is no activity logged" do
+        visit access_logs_doorkeeper_application_path(@application)
+        assert_text "app-name access log"
+        assert_text "No activity logged"
+      end
+    end
+
+    context "when there are matching events" do
+      setup do
+        create(:event_log, event_id: 47, application_id: @application.id, uid: @superadmin.uid)
+        create(:event_log, event_id: 47, application_id: @application.id, uid: @user.uid)
+      end
+
+      should "see a list of events for the application" do
+        visit access_logs_doorkeeper_application_path(@application)
+        assert_text "#{@application.name} access log"
+        assert_text "Successful user application authorization for #{@application.name} for #{@superadmin.name}"
+        assert_text "Successful user application authorization for #{@application.name} for #{@user.name}"
+      end
+    end
+  end
+
+end

From 1db3ef43cfd20b316b5a28a5996aa12f5fc9a139 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 16:35:09 +0100
Subject: [PATCH 10/14] Add integration test for monthly access stats

---
 ...lication_monthly_access_stats_page_test.rb | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 test/integration/application_monthly_access_stats_page_test.rb

diff --git a/test/integration/application_monthly_access_stats_page_test.rb b/test/integration/application_monthly_access_stats_page_test.rb
new file mode 100644
index 000000000..794e51416
--- /dev/null
+++ b/test/integration/application_monthly_access_stats_page_test.rb
@@ -0,0 +1,61 @@
+require "test_helper"
+
+class ApplicationMonthlyAccessStatsPageIntegrationTest < ActionDispatch::IntegrationTest
+  setup do
+    @application = create(:application, name: "app-name", description: "app-description")
+    @user = create(:user, name: "Normal User")
+  end
+
+  test "users don't have permission to view account access log" do
+    visit root_path
+    signin_with(@user)
+
+    visit monthly_access_stats_doorkeeper_application_path(@application)
+    flash = find("div[role='alert']")
+    assert flash.has_content?("You do not have permission to perform this action.")
+  end
+
+  context "logged in as an superadmin" do
+    setup do
+      visit new_user_session_path
+      @superadmin = create(:superadmin_user)
+      signin_with(@superadmin)
+    end
+
+    should "have permission to view account access log" do
+      visit monthly_access_stats_doorkeeper_application_path(@application)
+      assert_equal page.title, "Monthly access counts to app-name - GOV.UK Signon"
+    end
+
+    context "when there are no matching events" do
+      should "see a message stating that there is no activity logged" do
+        visit monthly_access_stats_doorkeeper_application_path(@application)
+        assert_text "Monthly access counts to app-name"
+        assert_text "No activity logged"
+      end
+    end
+
+    context "when there are matching events" do
+      setup do
+        create(:event_log, created_at: Date.new(2020, 1, 1), event_id: 47, application_id: @application.id, uid: @superadmin.uid)
+        create(:event_log, created_at: Date.new(2020, 1, 1), event_id: 47, application_id: @application.id, uid: @superadmin.uid)
+        create(:event_log, created_at: Date.new(2020, 1, 1), event_id: 47, application_id: @application.id, uid: @user.uid)
+        create(:event_log, created_at: Date.new(2020, 2, 1), event_id: 47, application_id: @application.id, uid: @superadmin.uid)
+        create(:event_log, created_at: Date.new(2020, 2, 1), event_id: 47, application_id: @application.id, uid: @user.uid)
+      end
+
+      should "see a list of events for the application" do
+        visit monthly_access_stats_doorkeeper_application_path(@application)
+        assert_text "Monthly access counts to #{@application.name}"
+
+        assert_text "Month Total authorization count Unique users authorization count Access logs"
+        # Test data has two months - these should be sorted in descending order
+        # 2020-02 has two events for two different users, so we should get 2 2
+        assert_text "2020-02 2 2 2020-02 access logs"
+        # 2020-01 has three events for two different users, so we should get 3 2
+        assert_text "2020-01 3 2 2020-01 access logs"
+      end
+    end
+  end
+
+end

From 28b6df7cb4e9650e5c715da0416451fdaa0c1f22 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Fri, 9 Aug 2024 16:59:18 +0100
Subject: [PATCH 11/14] Appease rubocop

---
 test/integration/application_access_log_page_test.rb           | 1 -
 test/integration/application_monthly_access_stats_page_test.rb | 1 -
 2 files changed, 2 deletions(-)

diff --git a/test/integration/application_access_log_page_test.rb b/test/integration/application_access_log_page_test.rb
index 81c2424a6..270d22575 100644
--- a/test/integration/application_access_log_page_test.rb
+++ b/test/integration/application_access_log_page_test.rb
@@ -49,5 +49,4 @@ class ApplicationAccessLogPageIntegrationTest < ActionDispatch::IntegrationTest
       end
     end
   end
-
 end
diff --git a/test/integration/application_monthly_access_stats_page_test.rb b/test/integration/application_monthly_access_stats_page_test.rb
index 794e51416..92fd18e86 100644
--- a/test/integration/application_monthly_access_stats_page_test.rb
+++ b/test/integration/application_monthly_access_stats_page_test.rb
@@ -57,5 +57,4 @@ class ApplicationMonthlyAccessStatsPageIntegrationTest < ActionDispatch::Integra
       end
     end
   end
-
 end

From 4168b37ef0053f6e41f08eb708bb845f0f8d9ef6 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Wed, 14 Aug 2024 09:16:10 +0100
Subject: [PATCH 12/14] Use human readable event_id

---
 app/controllers/doorkeeper_applications_controller.rb |  4 ++--
 test/integration/application_access_log_page_test.rb  |  5 +++--
 .../application_monthly_access_stats_page_test.rb     | 11 ++++++-----
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/app/controllers/doorkeeper_applications_controller.rb b/app/controllers/doorkeeper_applications_controller.rb
index b70469b25..2108374d0 100644
--- a/app/controllers/doorkeeper_applications_controller.rb
+++ b/app/controllers/doorkeeper_applications_controller.rb
@@ -27,7 +27,7 @@ def users_with_access
   def access_logs
     relation = @application.event_logs
       .includes(:user)
-      .where(event_id: 47)
+      .where(event_id: EventLog::SUCCESSFUL_USER_APPLICATION_AUTHORIZATION.id)
       .order(created_at: :desc)
 
     unless params[:include_smokey_users] == "true"
@@ -46,7 +46,7 @@ def access_logs
 
   def monthly_access_stats
     relation = @application.event_logs
-                .where(event_id: 47)
+                .where(event_id: EventLog::SUCCESSFUL_USER_APPLICATION_AUTHORIZATION.id)
                 .group("DATE_FORMAT(created_at, '%Y-%m')")
                 .order(Arel.sql("DATE_FORMAT(created_at, '%Y-%m') DESC"))
 
diff --git a/test/integration/application_access_log_page_test.rb b/test/integration/application_access_log_page_test.rb
index 270d22575..65c4b9b0d 100644
--- a/test/integration/application_access_log_page_test.rb
+++ b/test/integration/application_access_log_page_test.rb
@@ -37,8 +37,9 @@ class ApplicationAccessLogPageIntegrationTest < ActionDispatch::IntegrationTest
 
     context "when there are matching events" do
       setup do
-        create(:event_log, event_id: 47, application_id: @application.id, uid: @superadmin.uid)
-        create(:event_log, event_id: 47, application_id: @application.id, uid: @user.uid)
+        event_id = 47
+        create(:event_log, event_id:, application_id: @application.id, uid: @superadmin.uid)
+        create(:event_log, event_id:, application_id: @application.id, uid: @user.uid)
       end
 
       should "see a list of events for the application" do
diff --git a/test/integration/application_monthly_access_stats_page_test.rb b/test/integration/application_monthly_access_stats_page_test.rb
index 92fd18e86..897685502 100644
--- a/test/integration/application_monthly_access_stats_page_test.rb
+++ b/test/integration/application_monthly_access_stats_page_test.rb
@@ -37,11 +37,12 @@ class ApplicationMonthlyAccessStatsPageIntegrationTest < ActionDispatch::Integra
 
     context "when there are matching events" do
       setup do
-        create(:event_log, created_at: Date.new(2020, 1, 1), event_id: 47, application_id: @application.id, uid: @superadmin.uid)
-        create(:event_log, created_at: Date.new(2020, 1, 1), event_id: 47, application_id: @application.id, uid: @superadmin.uid)
-        create(:event_log, created_at: Date.new(2020, 1, 1), event_id: 47, application_id: @application.id, uid: @user.uid)
-        create(:event_log, created_at: Date.new(2020, 2, 1), event_id: 47, application_id: @application.id, uid: @superadmin.uid)
-        create(:event_log, created_at: Date.new(2020, 2, 1), event_id: 47, application_id: @application.id, uid: @user.uid)
+        event_id = EventLog::SUCCESSFUL_USER_APPLICATION_AUTHORIZATION.id
+        create(:event_log, created_at: Date.new(2020, 1, 1), event_id:, application_id: @application.id, uid: @superadmin.uid)
+        create(:event_log, created_at: Date.new(2020, 1, 1), event_id:, application_id: @application.id, uid: @superadmin.uid)
+        create(:event_log, created_at: Date.new(2020, 1, 1), event_id:, application_id: @application.id, uid: @user.uid)
+        create(:event_log, created_at: Date.new(2020, 2, 1), event_id:, application_id: @application.id, uid: @superadmin.uid)
+        create(:event_log, created_at: Date.new(2020, 2, 1), event_id:, application_id: @application.id, uid: @user.uid)
       end
 
       should "see a list of events for the application" do

From 66e5a103d6e20517bbde615977b805dd57274e0e Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Wed, 14 Aug 2024 09:33:23 +0100
Subject: [PATCH 13/14] Make "month" label more descriptive

---
 app/views/doorkeeper_applications/access_logs.html.erb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/doorkeeper_applications/access_logs.html.erb b/app/views/doorkeeper_applications/access_logs.html.erb
index 3c6ec1973..b9d875af1 100644
--- a/app/views/doorkeeper_applications/access_logs.html.erb
+++ b/app/views/doorkeeper_applications/access_logs.html.erb
@@ -33,7 +33,7 @@
 } %>
 <%= render "govuk_publishing_components/components/input", {
   label: {
-    text: "Month"
+    text: "Year and month"
   },
   name: "month",
   hint: "In YYYY-mm format",

From 168dd3ffe959cd2d8c9537a0fae4814fca071247 Mon Sep 17 00:00:00 2001
From: Richard Towers <richard.towers@digital.cabinet-office.gov.uk>
Date: Wed, 14 Aug 2024 14:47:56 +0100
Subject: [PATCH 14/14] Add notes about acces log data

---
 .../access_logs.html.erb                      | 22 ++++++++++++++
 .../monthly_access_stats.html.erb             | 29 +++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/app/views/doorkeeper_applications/access_logs.html.erb b/app/views/doorkeeper_applications/access_logs.html.erb
index b9d875af1..1cab66296 100644
--- a/app/views/doorkeeper_applications/access_logs.html.erb
+++ b/app/views/doorkeeper_applications/access_logs.html.erb
@@ -46,6 +46,28 @@
 </div>
 </form>
 
+<%= render "govuk_publishing_components/components/details", {
+  title: "About this data"
+} do %>
+<p class="govuk-body">
+  Signon records a "successful authorization" event whenever a user uses Signon to access one of the publishing
+  applications. This is a record of all of these events for <%= @application.name %>.
+</p>
+<p class="govuk-body">
+  Applications cache authentications for around 20 hours, so if a user clicks an application multiple
+  times a day, they may only appear in the event log once.
+</p>
+<p class="govuk-body">
+  <% if DateTime.current.before? DateTime.new(2025, 11, 1)  # This branch can be removed after November 2025 %>
+  Note that authorization data has only been recorded in the Signon event log since November 2023, so it is not
+  possible to view events before that date.
+  <% else %>
+  Note that data in the event log in Signon is only retained for 2 years, so it is not possible to view events
+  before that date.
+  <% end %>
+</p>
+<% end %>
+
 <% if @logs.any? %>
   <%= render "components/table", {
     caption: pluralize(number_with_delimiter(@logs.total_count), "event"),
diff --git a/app/views/doorkeeper_applications/monthly_access_stats.html.erb b/app/views/doorkeeper_applications/monthly_access_stats.html.erb
index 4e0386039..e43cba8ca 100644
--- a/app/views/doorkeeper_applications/monthly_access_stats.html.erb
+++ b/app/views/doorkeeper_applications/monthly_access_stats.html.erb
@@ -37,6 +37,35 @@
   } %>
 </form>
 
+<%= render "govuk_publishing_components/components/details", {
+  title: "About this data"
+} do %>
+<p class="govuk-body">
+  Signon records a "successful authorization" event whenever a user uses Signon to access one of the publishing
+  applications. This is a monthly count of all of these events for <%= @application.name %>.
+</p>
+<p class="govuk-body">
+  Applications cache authentications for around 20 hours, so if a user clicks an application multiple
+  times a day, they may only appear in the event log once.
+</p>
+<p class="govuk-body">
+  The total authorization count is the total number of events recorded in the log for the month (if a
+  user accesses the same app multiple times, this number will increase).
+</p>
+<p class="govuk-body">
+  The unique users authorization count is the number of distinct users who recorded events in the log for the month
+  (if a user accesses the same app multiple times in a month, this will only count as one unique user).
+</p>
+<p class="govuk-body">
+  <% if DateTime.current.before? DateTime.new(2025, 11, 1)  # This branch can be removed after November 2025 %>
+  Note that authorization data has only been recorded in the Signon event log since November 2023, so it is not
+  possible to view events before that date.
+  <% else %>
+  Note that data in the event log in Signon is only retained for 2 years, so it is not possible to view events
+  before that date.
+  <% end %>
+</p>
+<% end %>
 <% if @monthly_access_stats.any? %>
   <%= render "components/table", {
     head: [