Merge pull request #2418 from alphagov/rotate-sso-push-access-tokens-…

…before-expiry

Create a new access token for the SSO Push API user *before* expiry not *at* expiry

app/controllers/application_controller.rb

@@ -25,7 +25,7 @@ def current_resource_owner
   end
 
   def application_making_request
-    @_application_making_request ||= ::Doorkeeper::Application.find(doorkeeper_token.application_id) if doorkeeper_token
+    @_application_making_request ||= Doorkeeper::Application.find(doorkeeper_token.application_id) if doorkeeper_token
   end
 
 private

app/controllers/root_controller.rb

@@ -6,13 +6,13 @@ class RootController < ApplicationController
   skip_after_action :verify_authorized
 
   def index
-    applications = ::Doorkeeper::Application.where(show_on_dashboard: true).can_signin(current_user)
+    applications = Doorkeeper::Application.where(show_on_dashboard: true).can_signin(current_user)
 
     @applications_and_permissions = zip_permissions(applications, current_user)
   end
 
   def signin_required
-    @application = ::Doorkeeper::Application.find_by(id: session.delete(:signin_missing_for_application))
+    @application = Doorkeeper::Application.find_by(id: session.delete(:signin_missing_for_application))
   end
 
   def privacy_notice; end

app/jobs/permission_updater.rb

@@ -3,7 +3,7 @@
 class PermissionUpdater < PushUserUpdatesJob
   def perform(uid, application_id)
     user = User.find_by(uid:)
-    application = ::Doorkeeper::Application.find_by(id: application_id)
+    application = Doorkeeper::Application.find_by(id: application_id)
     # It's possible they've been deleted between when the job was scheduled and run.
     return if user.nil? || application.nil?
     return unless application.supports_push_updates?

app/jobs/reauth_enforcer.rb

@@ -2,7 +2,7 @@
 
 class ReauthEnforcer < PushUserUpdatesJob
   def perform(uid, application_id)
-    application = ::Doorkeeper::Application.find_by(id: application_id)
+    application = Doorkeeper::Application.find_by(id: application_id)
     # It's possible the application has been deleted between when the job was scheduled and run.
     return if application.nil?
     return unless application.supports_push_updates?

app/models/doorkeeper/access_grant.rb

@@ -0,0 +1,7 @@
+module Doorkeeper
+  class AccessGrant < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
+    include Models::ExpirationTimeSqlMath
+
+    scope :expired, -> { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} < ?", Time.current) }
+  end
+end

app/models/doorkeeper/access_token.rb

@@ -0,0 +1,9 @@
+module Doorkeeper
+  class AccessToken < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord
+    scope :not_revoked, -> { where(revoked_at: nil) }
+    scope :expires_after, ->(time) { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} > ?", time) }
+    scope :expired, -> { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} < ?", Time.current) }
+    scope :ordered_by_expires_at, -> { order(expiration_time_sql) }
+    scope :ordered_by_application_name, -> { includes(:application).merge(Doorkeeper::Application.ordered_by_name) }
+  end
+end

app/models/doorkeeper/application.rb

@@ -1,11 +1,12 @@
 require "doorkeeper/orm/active_record/application"
 
 # rubocop:disable Rails/ApplicationRecord
-class ::Doorkeeper::Application < ActiveRecord::Base
+class Doorkeeper::Application < ActiveRecord::Base
   # rubocop:enable Rails/ApplicationRecord
   has_many :supported_permissions, dependent: :destroy
 
-  default_scope { order("oauth_applications.name") }
+  default_scope { ordered_by_name }
+  scope :ordered_by_name, -> { order("oauth_applications.name") }
   scope :support_push_updates, -> { where(supports_push_updates: true) }
   scope :not_retired, -> { where(retired: false) }
   scope :can_signin,

app/models/user.rb

@@ -192,7 +192,7 @@ def authorised_applications
   alias_method :applications_used, :authorised_applications
 
   def revoke_all_authorisations
-    authorisations.where(revoked_at: nil).find_each(&:revoke)
+    authorisations.not_revoked.find_each(&:revoke)
   end
 
   def grant_application_signin_permission(application)

app/policies/user_permission_manageable_application_policy.rb

@@ -31,7 +31,7 @@ def resolve
   private
 
     def applications
-      ::Doorkeeper::Application.includes(:supported_permissions)
+      Doorkeeper::Application.includes(:supported_permissions)
     end
   end
 end

app/views/api_users/edit.html.erb

@@ -47,19 +47,27 @@
       <th>Token (hidden)</th>
       <th>Generated</th>
       <th>Expires</th>
+      <th>State</th>
       <th>Action</th>
     </tr>
   </thead>
   <tbody>
-    <% @api_user.authorisations.where(revoked_at: nil).each do |authorisation| %>
+    <% @api_user.authorisations.not_revoked.ordered_by_application_name.ordered_by_expires_at.each do |authorisation| %>
       <tr>
         <td><%= authorisation.application.name %></td>
         <td><code><%= truncate_access_token(authorisation.token) %></code></td>
         <td>
-          <%= Time.at(authorisation.created_at).to_date.to_fs(:govuk_date) %>
+          <%= authorisation.created_at.to_date.to_fs(:govuk_date) %>
         </td>
         <td>
-          <%= Time.at(authorisation.created_at.to_f + authorisation.expires_in).to_date.to_fs(:govuk_date) %>
+          <%= authorisation.expires_at.to_date.to_fs(:govuk_date) %>
+        </td>
+        <td>
+          <% if authorisation.expired? %>
+            <span class="label label-danger">Expired</span>
+          <% else %>
+            <span class="label label-success">Valid</span>
+          <% end %>
         </td>
         <td>
           <div class="btn-group">

config/initializers/doorkeeper.rb

@@ -74,7 +74,7 @@
   # Use a custom class for generating the access token.
   # See https://doorkeeper.gitbook.io/guides/configuration/other-configurations#custom-access-token-generator
   #
-  # access_token_generator '::Doorkeeper::JWT'
+  # access_token_generator 'Doorkeeper::JWT'
 
   # The controller +Doorkeeper::ApplicationController+ inherits from.
   # Defaults to +ActionController::Base+ unless +api_only+ is set, which changes the default to
@@ -119,7 +119,7 @@
   # If you wish to use another hashing implementation, you can override
   # this strategy as follows:
   #
-  # hash_token_secrets using: '::Doorkeeper::Hashing::MyCustomHashImpl'
+  # hash_token_secrets using: 'Doorkeeper::Hashing::MyCustomHashImpl'
   #
   # Keep in mind that changing the hashing function will invalidate all existing
   # secrets, if there are any.
@@ -134,7 +134,7 @@
   # If you wish to use bcrypt for application secret hashing, uncomment
   # this line instead:
   #
-  # hash_application_secrets using: '::Doorkeeper::SecretStoring::BCrypt'
+  # hash_application_secrets using: 'Doorkeeper::SecretStoring::BCrypt'
 
   # When the above option is enabled, and a hashed token or secret is not found,
   # you can allow to fall back to another strategy. For users upgrading

lib/collectors/global_prometheus_collector.rb

@@ -25,8 +25,8 @@ def metrics
     def token_expiry_info
       # Cache metric to prevent needless expensive calls to the database
       Rails.cache.fetch("token_expiry_info", expires_in: 1.hour) do
-        ApiUser.where.not(email: SSOPushCredential::USER_EMAIL).flat_map do |user|
-          user.authorisations.where(revoked_at: nil).map do |token|
+        ApiUser.all.flat_map do |user|
+          user.authorisations.not_revoked.map do |token|
             {
               expires_at: token.expires_at.to_i,
               api_user: user.email,

lib/expired_oauth_access_records_deleter.rb

@@ -8,8 +8,6 @@ class ExpiredOauthAccessRecordsDeleter
     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)
@@ -19,7 +17,7 @@ def initialize(record_type:)
   attr_reader :record_class, :total_deleted
 
   def delete_expired
-    @record_class.where(HAS_EXPIRED, Time.zone.now).in_batches do |relation|
+    @record_class.expired.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)
 

lib/sso_push_credential.rb

@@ -10,6 +10,7 @@ def credentials(application)
 
       user.authorisations
         .not_expired
+        .expires_after(4.weeks.from_now)
         .create_with(expires_in: 10.years)
         .find_or_create_by!(application_id: application.id).token
     end

lib/tasks/sync_kubernetes_secrets.rake

@@ -10,7 +10,7 @@ namespace :kubernetes do
     missing_users = emails - api_users.map(&:email)
 
     api_users.each do |api_user|
-      api_user.authorisations.where(revoked_at: nil).each do |token|
+      api_user.authorisations.not_revoked.each do |token|
         name = "signon-token-#{api_user.name}-#{token.application.name}".parameterize
         data = { bearer_token: token.token }
 

lib/user_permissions_controller_methods.rb

@@ -3,9 +3,9 @@ module UserPermissionsControllerMethods
 
   def visible_applications(user)
     if user.api_user?
-      applications = ::Doorkeeper::Application.includes(:supported_permissions)
+      applications = Doorkeeper::Application.includes(:supported_permissions)
       if current_user.superadmin?
-        api_user_authorised_apps = user.authorisations.where(revoked_at: nil).pluck(:application_id)
+        api_user_authorised_apps = user.authorisations.not_revoked.pluck(:application_id)
         applications.where(id: api_user_authorised_apps)
       else
         applications.none

script/make_oauth_work_in_dev

@@ -98,7 +98,7 @@ def deverise_uri(uri)
 end
 
 puts "Updating SSO config so that it works in development..."
-applications = ::Doorkeeper::Application.where(retired: false)
+applications = Doorkeeper::Application.where(retired: false)
 applications.each do |application|
   local_config = values_for_app(application)
   if local_config.present?

test/integration/superadmin_application_edit_test.rb

@@ -14,7 +14,7 @@ class SuperAdminApplicationEditTest < ActionDispatch::IntegrationTest
 
       # normal user who's authorised to use app
       @user = create(:user)
-      ::Doorkeeper::AccessToken.create!(resource_owner_id: @user.id, application_id: @application.id, token: "1234")
+      Doorkeeper::AccessToken.create!(resource_owner_id: @user.id, application_id: @application.id, token: "1234")
     end
 
     should "be able to enable push updates to applications" do

test/jobs/push_user_updates_job_test.rb

@@ -12,7 +12,7 @@ class TestJob < PushUserUpdatesJob
       foo_app, _bar_app = *create_list(:application, 2)
 
       # authenticate access
-      ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: foo_app.id, token: "1234")
+      Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: foo_app.id, token: "1234")
 
       assert_enqueued_with(job: TestJob, args: [user.uid, foo_app.id]) do
         TestJob.perform_on(user)

test/lib/sso_push_credential_test.rb

@@ -8,7 +8,7 @@ class SSOPushCredentialTest < ActiveSupport::TestCase
 
   context "given an already authorised application" do
     setup do
-      @authorisation = @user.authorisations.create!(application_id: @application.id)
+      @authorisation = @user.authorisations.create!(application_id: @application.id, expires_in: 5.weeks)
     end
 
     should "return the bearer token for an already-authorized application" do
@@ -37,6 +37,20 @@ class SSOPushCredentialTest < ActiveSupport::TestCase
     end
   end
 
+  context "given an application with an authorisation close to expiry" do
+    setup do
+      @user.authorisations.create!(application_id: @application.id, expires_in: 4.weeks)
+    end
+
+    should "create a new authorisation to replace the expired one" do
+      bearer_token = SSOPushCredential.credentials(@application)
+
+      new_authorisation = @user.authorisations.find_by(token: bearer_token)
+      assert new_authorisation.expires_at > 4.weeks.from_now
+      assert_equal @application.id, new_authorisation.application_id
+    end
+  end
+
   context "given an application with a revoked authorisation" do
     setup do
       @user.authorisations.create!(application_id: @application.id, revoked_at: Time.current)

test/models/doorkeeper/access_grant_test.rb

@@ -0,0 +1,15 @@
+require "test_helper"
+
+class Doorkeeper::AccessGrantTest < ActiveSupport::TestCase
+  context ".expired" do
+    should "return grants that have expired" do
+      grant_expiring_1_day_ago = create(:access_grant, expires_in: -1.day)
+      grant_expiring_in_1_day = create(:access_grant, expires_in: 1.day)
+
+      grants = Doorkeeper::AccessGrant.expired
+
+      assert_includes grants, grant_expiring_1_day_ago
+      assert_not_includes grants, grant_expiring_in_1_day
+    end
+  end
+end

test/models/doorkeeper/access_token_test.rb

@@ -0,0 +1,64 @@
+require "test_helper"
+
+class Doorkeeper::AccessTokenTest < ActiveSupport::TestCase
+  context ".not_revoked" do
+    should "return tokens that have not been revoked" do
+      revoked_token = create(:access_token, revoked_at: Time.current)
+      non_revoked_token = create(:access_token, revoked_at: nil)
+
+      tokens = Doorkeeper::AccessToken.not_revoked
+
+      assert_not_includes tokens, revoked_token
+      assert_includes tokens, non_revoked_token
+    end
+  end
+
+  context ".expires_after" do
+    should "return tokens expiring after specified time" do
+      token_expiring_in_1_week = create(:access_token, expires_in: 1.week)
+      token_expiring_in_3_weeks = create(:access_token, expires_in: 3.weeks)
+
+      tokens = Doorkeeper::AccessToken.expires_after(2.weeks.from_now)
+
+      assert_not_includes tokens, token_expiring_in_1_week
+      assert_includes tokens, token_expiring_in_3_weeks
+    end
+  end
+
+  context ".expired" do
+    should "return tokens that have expired" do
+      token_expiring_1_day_ago = create(:access_token, expires_in: -1.day)
+      token_expiring_in_1_day = create(:access_token, expires_in: 1.day)
+
+      tokens = Doorkeeper::AccessToken.expired
+
+      assert_includes tokens, token_expiring_1_day_ago
+      assert_not_includes tokens, token_expiring_in_1_day
+    end
+  end
+
+  context ".ordered_by_expires_at" do
+    should "return tokens ordered by expiry time" do
+      token_expiring_in_2_weeks = create(:access_token, expires_in: 2.weeks)
+      token_expiring_in_1_week = create(:access_token, expires_in: 1.week)
+
+      tokens = Doorkeeper::AccessToken.ordered_by_expires_at
+
+      assert_equal [token_expiring_in_1_week, token_expiring_in_2_weeks], tokens
+    end
+  end
+
+  context ".ordered_by_application_name" do
+    should "return tokens ordered by application name" do
+      application_named_foo = create(:application, name: "Foo")
+      application_named_bar = create(:application, name: "Bar")
+
+      token_for_foo = create(:access_token, application: application_named_foo)
+      token_for_bar = create(:access_token, application: application_named_bar)
+
+      tokens = Doorkeeper::AccessToken.ordered_by_application_name
+
+      assert_equal [token_for_bar, token_for_foo], tokens
+    end
+  end
+end

test/models/doorkeeper_application_test.rb → test/models/doorkeeper/application_test.rb

@@ -1,6 +1,6 @@
 require "test_helper"
 
-class ::Doorkeeper::ApplicationTest < ActiveSupport::TestCase
+class Doorkeeper::ApplicationTest < ActiveSupport::TestCase
   should "have a signin supported permission on create" do
     assert_not_nil create(:application).signin_permission
   end
@@ -242,4 +242,15 @@ class ::Doorkeeper::ApplicationTest < ActiveSupport::TestCase
       end
     end
   end
+
+  context ".ordered_by_name" do
+    should "return applications ordered by name" do
+      application_named_foo = create(:application, name: "Foo")
+      application_named_bar = create(:application, name: "Bar")
+
+      applications = Doorkeeper::Application.ordered_by_name
+
+      assert_equal [application_named_bar, application_named_foo], applications
+    end
+  end
 end

test/models/user_test.rb

@@ -1110,7 +1110,7 @@ def setup
   end
 
   def authenticate_access(user, app)
-    ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id)
+    Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id)
   end
 
   def assert_user_has_permissions(expected_permissions, application, user)