From 92189139280cf78540b3894c02946c82982c5fb4 Mon Sep 17 00:00:00 2001
From: Chris Roos <chris@seagul.co.uk>
Date: Thu, 7 Sep 2023 10:49:23 +0100
Subject: [PATCH] List apps the user does not have access to

I've extracted `Application.with_signin_permission_for` and
`Application.not_retired` from `Application.can_signin` so that I can
use it them to list applications that the user doesn't have the signin
permission for, and that are not retired.
---
 .../account/applications_controller.rb        |  3 +-
 app/models/doorkeeper/application.rb          | 18 ++++--
 app/views/account/applications/index.html.erb | 21 ++++++-
 test/integration/account_applications_test.rb | 22 +++++--
 test/models/doorkeeper_application_test.rb    | 62 +++++++++++++++++++
 5 files changed, 116 insertions(+), 10 deletions(-)

diff --git a/app/controllers/account/applications_controller.rb b/app/controllers/account/applications_controller.rb
index 0aa52e0b03..750ed62ff3 100644
--- a/app/controllers/account/applications_controller.rb
+++ b/app/controllers/account/applications_controller.rb
@@ -6,6 +6,7 @@ class Account::ApplicationsController < ApplicationController
   def index
     authorize :account_applications
 
-    @applications = Doorkeeper::Application.can_signin(current_user)
+    @applications_with_signin = Doorkeeper::Application.can_signin(current_user)
+    @applications_without_signin = Doorkeeper::Application.not_retired.without_signin_permission_for(current_user)
   end
 end
diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb
index bca451c94a..cb8754594d 100644
--- a/app/models/doorkeeper/application.rb
+++ b/app/models/doorkeeper/application.rb
@@ -7,12 +7,11 @@ class ::Doorkeeper::Application < ActiveRecord::Base
 
   default_scope { order("oauth_applications.name") }
   scope :support_push_updates, -> { where(supports_push_updates: true) }
+  scope :not_retired, -> { where(retired: false) }
   scope :can_signin,
         lambda { |user|
-          joins(supported_permissions: :user_application_permissions)
-            .where(user_application_permissions: { user: })
-            .merge(SupportedPermission.signin)
-            .where(retired: false)
+          with_signin_permission_for(user)
+            .not_retired
         }
   scope :with_signin_delegatable,
         lambda {
@@ -20,6 +19,17 @@ class ::Doorkeeper::Application < ActiveRecord::Base
             .merge(SupportedPermission.signin)
             .merge(SupportedPermission.delegatable)
         }
+  scope :with_signin_permission_for,
+        lambda { |user|
+          joins(supported_permissions: :user_application_permissions)
+            .where(user_application_permissions: { user: })
+            .merge(SupportedPermission.signin)
+        }
+  scope :without_signin_permission_for,
+        lambda { |user|
+          excluded_app_ids = with_signin_permission_for(user).map(&:id)
+          where.not(id: excluded_app_ids)
+        }
 
   after_create :create_signin_supported_permission
   after_save :create_user_update_supported_permission
diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb
index 373973b80b..8f619ade9b 100644
--- a/app/views/account/applications/index.html.erb
+++ b/app/views/account/applications/index.html.erb
@@ -24,7 +24,26 @@
     </tr>
   </thead>
   <tbody class="govuk-table__body">
-    <% @applications.each do |application| %>
+    <% @applications_with_signin.each do |application| %>
+      <tr class="govuk-table__row">
+        <td class="govuk-table__cell"><%= application.name %></td>
+        <td class="govuk-table__cell"><%= application.description %></td>
+      </tr>
+    <% end %>
+  </tbody>
+</table>
+
+<h2 class="govuk-heading-m" id="other-apps-table-heading">Apps you don't have access to</h2>
+
+<table class="govuk-table" aria-labelledby="other-apps-table-heading">
+  <thead class="govuk-table__head">
+    <tr class="govuk-table__row">
+      <th scope="col" class="govuk-table__header govuk-!-width-one-quarter">Name</th>
+      <th scope="col" class="govuk-table__header govuk-!-width-three-quarters">Description</th>
+    </tr>
+  </thead>
+  <tbody class="govuk-table__body">
+    <% @applications_without_signin.each do |application| %>
       <tr class="govuk-table__row">
         <td class="govuk-table__cell"><%= application.name %></td>
         <td class="govuk-table__cell"><%= application.description %></td>
diff --git a/test/integration/account_applications_test.rb b/test/integration/account_applications_test.rb
index 756ed646c6..c237bbe00b 100644
--- a/test/integration/account_applications_test.rb
+++ b/test/integration/account_applications_test.rb
@@ -22,8 +22,9 @@ class AccountApplicationsTest < ActionDispatch::IntegrationTest
 
       visit account_applications_path
 
-      assert page.has_content?("app-name")
-      assert page.has_content?("app-description")
+      table = find("table caption[text()='Apps you have access to']").ancestor("table")
+      assert table.has_content?("app-name")
+      assert table.has_content?("app-description")
     end
 
     should "not list retired applications the user has access to" do
@@ -37,13 +38,26 @@ class AccountApplicationsTest < ActionDispatch::IntegrationTest
       assert_not page.has_content?("retired-app-name")
     end
 
-    should "not list the applications the user does not have access to" do
+    should "list the applications the user does not have access to" do
       visit new_user_session_path
       signin_with @user
 
       visit account_applications_path
 
-      assert_not page.has_content?("app-name")
+      heading = find("h2", text: "Apps you don't have access to")
+      table = find("table[aria-labelledby='#{heading['id']}']")
+
+      assert table.has_content?("app-name")
+      assert table.has_content?("app-description")
+    end
+
+    should "not list retired applications the user does not have access to" do
+      visit new_user_session_path
+      signin_with @user
+
+      visit account_applications_path
+
+      assert_not page.has_content?("retired-app-name")
     end
   end
 end
diff --git a/test/models/doorkeeper_application_test.rb b/test/models/doorkeeper_application_test.rb
index 01d54f87b4..55a356d6fd 100644
--- a/test/models/doorkeeper_application_test.rb
+++ b/test/models/doorkeeper_application_test.rb
@@ -150,5 +150,67 @@ class ::Doorkeeper::ApplicationTest < ActiveSupport::TestCase
 
       assert_empty Doorkeeper::Application.with_signin_delegatable
     end
+
+    context ".not_retired" do
+      setup do
+        @app = create(:application)
+      end
+
+      should "include apps that have not been retired" do
+        @app.update!(retired: false)
+        assert_equal [@app], Doorkeeper::Application.not_retired
+      end
+
+      should "exclude apps that have been retired" do
+        @app.update!(retired: true)
+        assert_equal [], Doorkeeper::Application.not_retired
+      end
+    end
+
+    context ".with_signin_permission_for" do
+      setup do
+        @user = create(:user)
+        @app = create(:application)
+      end
+
+      should "include applications the user has the signin permission for" do
+        @user.grant_application_signin_permission(@app)
+
+        assert_equal [@app], Doorkeeper::Application.with_signin_permission_for(@user)
+      end
+
+      should "exclude applications the user does not have the signin permission for" do
+        create(:supported_permission, application: @app, name: "not-signin")
+
+        @user.grant_application_permission(@app, %w[not-signin])
+
+        assert_equal [], Doorkeeper::Application.with_signin_permission_for(@user)
+      end
+    end
+
+    context ".without_signin_permission_for" do
+      setup do
+        @user = create(:user)
+        @app = create(:application)
+      end
+
+      should "exclude applications the user has the signin permission for" do
+        @user.grant_application_signin_permission(@app)
+
+        assert_equal [], Doorkeeper::Application.without_signin_permission_for(@user)
+      end
+
+      should "include applications the user does not have the signin permission for" do
+        create(:supported_permission, application: @app, name: "not-signin")
+
+        @user.grant_application_permission(@app, %w[not-signin])
+
+        assert_equal [@app], Doorkeeper::Application.without_signin_permission_for(@user)
+      end
+
+      should "include applications the user doesn't have any permissions for" do
+        assert_equal [@app], Doorkeeper::Application.without_signin_permission_for(@user)
+      end
+    end
   end
 end