Merge pull request #2458 from alphagov/hide-access-tokens-for-retired…

…-apps

Hide access tokens for retired apps

app/models/user.rb

@@ -56,7 +56,7 @@ class User < ApplicationRecord
   validate :exemption_from_2sv_data_is_complete
   validate :organisation_has_mandatory_2sv, on: :create
 
-  has_many :authorisations, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id
+  has_many :authorisations, -> { joins(:application) }, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id
   has_many :application_permissions, -> { joins(:application) }, class_name: "UserApplicationPermission", inverse_of: :user, dependent: :destroy
   has_many :supported_permissions, -> { joins(:application) }, through: :application_permissions
   has_many :batch_invitations

app/views/api_users/edit.html.erb

@@ -18,7 +18,7 @@
 <%= form_for @api_user, :html => {:class => 'well add-top-margin'} do |f| %>
   <%= render partial: "form_fields", locals: { f: f } %>
 
-  <% if @api_user.authorisations.present? %>
+  <% if applications_and_permissions(@api_user).any? %>
     <hr />
     <h2 class="add-vertical-margins">Permissions</h2>
     <%= render partial: "shared/user_permissions", locals: { user_object: f.object }%>
@@ -40,7 +40,7 @@
     <%= link_to 'Copy to clipboard', '#', class: 'btn btn-info add-left-margin', data: { 'clipboard-target' => 'access-token' }, id: 'clip-button', title: 'Click to copy access token' %>
   </div>
 <% end %>
-<table class="table table-bordered table-on-white">
+<table id="authorisations" class="table table-bordered table-on-white">
   <thead>
     <tr class="table-header">
       <th>Application</th>

app/views/doorkeeper_applications/edit.html.erb

@@ -115,7 +115,6 @@
           Retiring an application indicates that it is no longer in use.
           Retired applications will not appear on most pages including the dashboard.
           Data associated with retired applications is not deleted.
-          If you're retiring an application you should also disable push updates.
         </p>
       </div>
 

app/views/shared/_user_permissions.html.erb

@@ -18,11 +18,7 @@
        supported_permission_field_prefix = "#{attribute_name}_application_#{application.id}_supported_permission" %>
       <tr>
         <td>
-          <% if application.retired? %>
-            <del><%= application.name %></del>
-          <% else %>
-            <%= application.name %>
-          <% end %>
+          <%= application.name %>
         </td>
         <% if user_object.api_user? %>
           <%

app/views/users/_app_permissions.html.erb

@@ -14,11 +14,7 @@
     %>
       <tr>
         <td>
-          <% if application.retired? %>
-              <del><%= application.name %></del>
-          <% else %>
-              <%= application.name %>
-          <% end %>
+          <%= application.name %>
         </td>
         <% unless user_object.api_user? %>
           <td>

lib/sso_push_credential.rb

@@ -5,6 +5,8 @@ class SSOPushCredential
 
   class << self
     def credentials(application)
+      return if application.retired?
+
       user.grant_application_signin_permission(application)
       user.grant_application_permissions(application, PERMISSIONS)
 

test/controllers/api_users_controller_test.rb

@@ -101,13 +101,17 @@ class ApiUsersControllerTest < ActionController::TestCase
     end
 
     context "GET edit" do
-      should "show the form" do
-        api_user = create(:api_user)
+      setup do
+        @api_user = create(:api_user)
+      end
 
-        get :edit, params: { id: api_user.id }
+      should "show the form for editing an API user" do
+        get :edit, params: { id: @api_user }
 
-        assert_select "input[name='api_user[name]'][value='#{api_user.name}']"
-        assert_select "input[name='api_user[email]'][value='#{api_user.email}']"
+        assert_select "form[action='#{api_user_path(@api_user)}']" do
+          assert_select "input[name='api_user[name]'][value='#{@api_user.name}']"
+          assert_select "input[name='api_user[email]'][value='#{@api_user.email}']"
+        end
       end
 
       should "allow editing permissions for application which user has access to" do
@@ -127,16 +131,23 @@ class ApiUsersControllerTest < ActionController::TestCase
         end
       end
 
+      should "not allow editing permissions for application which user does not have access to" do
+        application = create(:application, name: "app-name", with_supported_permissions: %w[edit])
+        api_user = create(:api_user, with_permissions: { application => [SupportedPermission::SIGNIN_NAME] })
+
+        get :edit, params: { id: api_user.id }
+
+        assert_select "table#editable-permissions", count: 0
+      end
+
       should "not allow editing permissions for retired application" do
         application = create(:application, name: "retired-app-name", retired: true)
         api_user = create(:api_user, with_permissions: { application => [SupportedPermission::SIGNIN_NAME] })
         create(:access_token, resource_owner_id: api_user.id, application:)
 
         get :edit, params: { id: api_user.id }
 
-        assert_select "table#editable-permissions tr" do
-          assert_select "td", text: "retired-app-name", count: 0
-        end
+        assert_select "table#editable-permissions", count: 0
       end
 
       should "allow editing permissions for API-only application" do
@@ -150,6 +161,65 @@ class ApiUsersControllerTest < ActionController::TestCase
           assert_select "td", text: "api-only-app-name"
         end
       end
+
+      should "show API user's access tokens" do
+        application = create(:application)
+        token = create(:access_token, resource_owner_id: @api_user.id, application:)
+
+        get :edit, params: { id: @api_user }
+
+        assert_select "table#authorisations tbody td", text: application.name do |td|
+          assert_select td.first.parent, "code", text: /^#{token[0..7]}/
+        end
+      end
+
+      should "show button for regenerating API user's access token for an application" do
+        application = create(:application)
+        token = create(:access_token, resource_owner_id: @api_user.id, application:)
+
+        get :edit, params: { id: @api_user }
+
+        regenerate_token_path = revoke_api_user_authorisation_path(@api_user, token, regenerate: true)
+
+        assert_select "table#authorisations tbody td", text: application.name do |td|
+          assert_select td.first.parent, "form[action='#{regenerate_token_path}']" do
+            assert_select "input[type='submit']", value: "Re-generate"
+          end
+        end
+      end
+
+      should "show button for revoking API user's access token for an application" do
+        application = create(:application)
+        token = create(:access_token, resource_owner_id: @api_user.id, application:)
+
+        get :edit, params: { id: @api_user }
+
+        revoke_token_path = revoke_api_user_authorisation_path(@api_user, token)
+
+        assert_select "table#authorisations tbody td", text: application.name do |td|
+          assert_select td.first.parent, "form[action='#{revoke_token_path}']" do
+            assert_select "input[type='submit']", value: "Revoke"
+          end
+        end
+      end
+
+      should "not show API user's revoked access tokens" do
+        application = create(:application)
+        create(:access_token, resource_owner_id: @api_user.id, application:, revoked_at: Time.current)
+
+        get :edit, params: { id: @api_user }
+
+        assert_select "table#authorisations tbody td", text: application.name, count: 0
+      end
+
+      should "not show API user's access tokens for retired applications" do
+        application = create(:application, retired: true)
+        create(:access_token, resource_owner_id: @api_user.id, application:)
+
+        get :edit, params: { id: @api_user }
+
+        assert_select "table#authorisations tbody td", text: application.name, count: 0
+      end
     end
 
     context "PUT update" do

test/jobs/permission_updater_test.rb

@@ -61,6 +61,14 @@ def users_url(application)
         PermissionUpdater.new.perform(@user.uid, @application.id + 42)
       end
 
+      should "do nothing if the application is retired" do
+        @application = create(:application, retired: true)
+
+        SSOPushClient.expects(:new).never
+
+        PermissionUpdater.new.perform(@user.uid, @application.id)
+      end
+
       should "do nothing if the application doesn't support push updates" do
         @application = create(:application, supports_push_updates: false)
 

test/jobs/push_user_updates_job_test.rb

@@ -11,12 +11,22 @@ class TestJob < PushUserUpdatesJob
       user = create(:user)
       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")
+      create(:access_token, resource_owner_id: user.id, application: foo_app)
 
       assert_enqueued_with(job: TestJob, args: [user.uid, foo_app.id]) do
         TestJob.perform_on(user)
       end
     end
+
+    should "not perform_async updates on user's retired applications" do
+      user = create(:user)
+      retired_app = create(:application, retired: true)
+
+      create(:access_token, resource_owner_id: user.id, application: retired_app)
+
+      assert_no_enqueued_jobs do
+        TestJob.perform_on(user)
+      end
+    end
   end
 end

test/jobs/reauth_enforcer_test.rb

@@ -28,5 +28,15 @@ class ReauthEnforcerTest < ActiveSupport::TestCase
 
       ReauthEnforcer.new.perform("a-uid", app.id)
     end
+
+    should "do nothing if the application is retired" do
+      app = create(:application, retired: true)
+
+      mock_client = mock("sso_push_client")
+      SSOPushClient.stubs(:new).returns(mock_client)
+      mock_client.expects(:reauth_user).never
+
+      ReauthEnforcer.new.perform("a-uid", app.id)
+    end
   end
 end

test/lib/collectors/global_prometheus_collector_test.rb

@@ -3,12 +3,13 @@
 class GlobalPrometheusCollectorTest < ActiveSupport::TestCase
   def setup
     @collector = Collectors::GlobalPrometheusCollector.new
-    @api_user = api_user_with_token("user1", token_count: 3)
+    @api_user = api_user_with_token("user1", token_count: 4)
   end
 
   context "#metrics" do
-    should "list all non-revoked token expiry timestamps" do
+    should "list all non-revoked token expiry timestamps for non-retired aps" do
       @api_user.authorisations[2].revoke
+      @api_user.authorisations[3].application.update!(retired: true)
 
       metrics = @collector.metrics
 

test/lib/sso_push_credential_test.rb

@@ -81,6 +81,22 @@ class SSOPushCredentialTest < ActiveSupport::TestCase
     end
   end
 
+  context "given a retired application" do
+    setup do
+      @application.update!(retired: true)
+    end
+
+    should "not return a token" do
+      assert_nil SSOPushCredential.credentials(@application)
+    end
+
+    should "not create a new authorisation" do
+      SSOPushCredential.credentials(@application)
+
+      assert_empty @user.authorisations
+    end
+  end
+
   should "create an authorisation if one does not already exist" do
     assert_equal 0, @user.authorisations.count
 

test/lib/tasks/sync_kubernetes_secrets_test.rb

@@ -73,7 +73,7 @@ class KubernetesTaskTest < ActiveSupport::TestCase
       })
     end
 
-    should "raise an exception about missing user, but not skip other existing users" do
+    should "raise an exception about missing app, but not skip other existing apps" do
       apps = [create(:application), create(:application)]
       names = [apps[0].name, "Do Not Exist", apps[1].name]
 
@@ -88,6 +88,21 @@ class KubernetesTaskTest < ActiveSupport::TestCase
 
       assert_match(/Do Not Exist/, err.message)
     end
+
+    should "raise an exception about retired app" do
+      app = create(:application, retired: true)
+
+      stub_config_map(@client, [app.name], [])
+      expect_secrets_created_for_only_apps(@client, [])
+
+      err = assert_raises StandardError do
+        Rake::Task["kubernetes:sync_app_secrets"].execute({
+          config_map_name: "config_map_name",
+        })
+      end
+
+      assert_match(/#{app.name}/, err.message)
+    end
   end
 
   def expect_secret_tokens_created_for_only_users(client, users)

test/models/user_test.rb

@@ -10,6 +10,24 @@ def setup
     @user = create(:user)
   end
 
+  context "#authorisations" do
+    should "return access tokens for user" do
+      token = create(:access_token, resource_owner_id: @user.id)
+      another_user = create(:user)
+      token_for_another_user = create(:access_token, resource_owner_id: another_user.id)
+
+      assert_includes @user.authorisations, token
+      assert_not_includes @user.authorisations, token_for_another_user
+    end
+
+    should "not include access tokens for retired applications" do
+      application = create(:application, retired: true)
+      token = create(:access_token, resource_owner_id: @user.id, application:)
+
+      assert_not_includes @user.authorisations, token
+    end
+  end
+
   context "#application_permissions" do
     should "return user application permissions for user" do
       application = create(:application)