Merge pull request #2452 from alphagov/hide-api-only-apps-on-most-pages

Hide API-only apps on most pages

app/controllers/account/applications_controller.rb

@@ -12,7 +12,7 @@ def show
   def index
     authorize [:account, Doorkeeper::Application]
 
-    @applications_with_signin = Doorkeeper::Application.can_signin(current_user)
-    @applications_without_signin = Doorkeeper::Application.without_signin_permission_for(current_user)
+    @applications_with_signin = Doorkeeper::Application.not_api_only.can_signin(current_user)
+    @applications_without_signin = Doorkeeper::Application.not_api_only.without_signin_permission_for(current_user)
   end
 end

app/controllers/account/permissions_controller.rb

@@ -4,7 +4,7 @@ class Account::PermissionsController < ApplicationController
   before_action :authenticate_user!
 
   def index
-    @application = Doorkeeper::Application.find(params[:application_id])
+    @application = Doorkeeper::Application.not_api_only.find(params[:application_id])
 
     authorize [:account, @application], :view_permissions?
 

app/controllers/doorkeeper_applications_controller.rb

@@ -45,7 +45,7 @@ def doorkeeper_application_params
       :retired,
       :home_uri,
       :supports_push_updates,
-      :show_on_dashboard,
+      :api_only,
     )
   end
 end

app/controllers/root_controller.rb

@@ -6,7 +6,7 @@ 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.not_api_only.can_signin(current_user)
 
     @applications_and_permissions = zip_permissions(applications, current_user)
   end

app/models/doorkeeper/application.rb

@@ -9,6 +9,8 @@ class Doorkeeper::Application < ActiveRecord::Base # rubocop:disable Rails/Appli
   scope :support_push_updates, -> { where(supports_push_updates: true) }
   scope :retired, -> { where(retired: true) }
   scope :not_retired, -> { where(retired: false) }
+  scope :api_only, -> { where(api_only: true) }
+  scope :not_api_only, -> { where(api_only: false) }
   scope :can_signin, ->(user) { with_signin_permission_for(user) }
   scope :with_signin_delegatable,
         lambda {

app/policies/user_permission_manageable_application_policy.rb

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

app/views/doorkeeper_applications/edit.html.erb

@@ -97,8 +97,13 @@
 
       <div class="govuk-body">
         <label>
-          <%= f.check_box :show_on_dashboard %> Show on dashboard
+          <%= f.check_box :api_only %> Is API-only
         </label>
+
+        <p>
+          An API-only application is one that doesn't have a user interface and is only used by API users.
+          API-only applications will not appear on most pages including the dashboard.
+        </p>
       </div>
 
       <div class="govuk-body">

app/views/doorkeeper_applications/index.html.erb

@@ -5,12 +5,17 @@
     {
       id: "active",
       label: "Active applications",
-      content: render("application_list", applications: @applications)
+      content: render("application_list", applications: @applications.not_api_only)
+    },
+    {
+      id: "api-only",
+      label: "API-only applications",
+      content: render("application_list", applications: @applications.api_only)
     },
     {
       id: "retired",
       label: "Retired applications",
-      content: render("application_list", applications: @applications.unscoped.retired)
+      content: render("application_list", applications: @applications.unscoped.retired.ordered_by_name)
     }
   ]
 } %>

db/migrate/20231019080657_replace_oauth_applications_show_on_dashboard_with_api_only.rb

@@ -0,0 +1,17 @@
+class ReplaceOauthApplicationsShowOnDashboardWithApiOnly < ActiveRecord::Migration[7.0]
+  def up
+    add_column :oauth_applications, :api_only, :boolean, default: false, null: false
+
+    update "UPDATE oauth_applications SET api_only = !show_on_dashboard"
+
+    remove_column :oauth_applications, :show_on_dashboard
+  end
+
+  def down
+    add_column :oauth_applications, :show_on_dashboard, :boolean, default: true, null: false
+
+    update "UPDATE oauth_applications SET show_on_dashboard = !api_only"
+
+    remove_column :oauth_applications, :api_only
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2023_10_18_141956) do
+ActiveRecord::Schema[7.0].define(version: 2023_10_19_080657) 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
@@ -95,8 +95,8 @@
     t.string "description"
     t.boolean "supports_push_updates", default: true
     t.boolean "retired", default: false, null: false
-    t.boolean "show_on_dashboard", default: true, null: false
     t.boolean "confidential", default: true, null: false
+    t.boolean "api_only", default: false, null: false
     t.index ["name"], name: "unique_application_name", unique: true
     t.index ["uid"], name: "index_oauth_applications_on_uid", unique: true
   end

lib/user_permissions_controller_methods.rb

@@ -26,6 +26,10 @@ def zip_permissions(applications, user)
   end
 
   def all_applications_and_permissions_for(user)
-    user.supported_permissions.includes(:application).group_by(&:application)
+    user
+      .supported_permissions
+      .merge(Doorkeeper::Application.not_api_only)
+      .includes(:application)
+      .group_by(&:application)
   end
 end

test/controllers/account/applications_controller_test.rb

@@ -24,6 +24,49 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
   end
 
   context "#index" do
+    context "logged in as a GOV.UK admin" do
+      should "display the button to grant access to an application" do
+        application = create(:application, name: "app-name")
+        sign_in create(:admin_user)
+
+        get :index
+
+        assert_select "tr td", text: "app-name"
+        assert_select "form[action='#{account_application_signin_permission_path(application)}']"
+      end
+
+      should "display the button to remove access to an application" do
+        application = create(:application, name: "app-name")
+        application.signin_permission.update!(delegatable: false)
+        user = create(:admin_user, with_signin_permissions_for: [application])
+
+        sign_in user
+
+        get :index
+
+        assert_select "tr td", text: "app-name"
+        assert_select "a[href='#{delete_account_application_signin_permission_path(application)}']"
+      end
+
+      should "not display a retired application" do
+        create(:application, name: "retired-app-name", retired: true)
+        sign_in create(:admin_user)
+
+        get :index
+
+        assert_select "tr td", text: "retired-app-name", count: 0
+      end
+
+      should "not display an API-only application" do
+        create(:application, name: "api-only-app-name", api_only: true)
+        sign_in create(:admin_user)
+
+        get :index
+
+        assert_select "tr td", text: "api-only-app-name", count: 0
+      end
+    end
+
     context "logged in as a publishing manager" do
       should "not display the button to grant access to an application" do
         application = create(:application, name: "app-name")

test/controllers/account/manage_permissions_controller_test.rb

@@ -30,6 +30,34 @@ class Account::ManagePermissionsControllerTest < ActionController::TestCase
           assert_select "td", count: 1, text: non_delegatable_no_access_to_app.name
         end
       end
+
+      should "not list retired applications" do
+        user = create(:admin_user, email: "admin@gov.uk")
+        sign_in user
+
+        retired_app = create(:application, retired: true)
+        user.grant_application_signin_permission(retired_app)
+
+        get :show
+
+        assert_select ".container" do
+          assert_select "td", count: 0, text: retired_app.name
+        end
+      end
+
+      should "not list API-only applications" do
+        user = create(:admin_user, email: "admin@gov.uk")
+        sign_in user
+
+        api_only_app = create(:application, api_only: true)
+        user.grant_application_signin_permission(api_only_app)
+
+        get :show
+
+        assert_select ".container" do
+          assert_select "td", count: 0, text: api_only_app.name
+        end
+      end
     end
 
     context "organisation admin" do
@@ -78,6 +106,20 @@ class Account::ManagePermissionsControllerTest < ActionController::TestCase
           assert_select "td", count: 1, text: "GDS Admin"
         end
       end
+
+      should "not list API-only applications" do
+        user = create(:organisation_admin_user)
+        sign_in user
+
+        api_only_app = create(:application, api_only: true)
+        user.grant_application_signin_permission(api_only_app)
+
+        get :show
+
+        assert_select "#all-permissions" do
+          assert_select "td", count: 0, text: api_only_app.name
+        end
+      end
     end
 
     context "super organisation admin" do

test/controllers/account/permissions_controller_test.rb

@@ -34,6 +34,16 @@ class Account::PermissionsControllerTest < ActionController::TestCase
       end
     end
 
+    should "exclude API-only applications" do
+      sign_in create(:admin_user)
+
+      application = create(:application, api_only: true)
+
+      assert_raises(ActiveRecord::RecordNotFound) do
+        get :index, params: { application_id: application.id }
+      end
+    end
+
     should "order permissions by whether the user has access and then alphabetically" do
       application = create(:application,
                            with_supported_permissions: %w[aaa bbb ttt uuu])

test/controllers/api_users_controller_test.rb

@@ -38,6 +38,45 @@ class ApiUsersControllerTest < ActionController::TestCase
         get :index
         assert_select "td", count: 0, text: /web_user@email.com/
       end
+
+      should "list applications for api user" do
+        application = create(:application, name: "app-name")
+        api_user = create(:api_user, with_permissions: { application => [SupportedPermission::SIGNIN_NAME] })
+        create(:access_token, resource_owner_id: api_user.id, application:)
+
+        get :index
+
+        assert_select "td>span" do |spans|
+          apps_span = spans.find { |s| s.text.strip == "Apps" }
+          assert_select apps_span.parent, "ul>li", text: "app-name"
+        end
+      end
+
+      should "not list retired applications for api user" do
+        application = create(:application, name: "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 :index
+
+        assert_select "td>span" do |spans|
+          apps_span = spans.find { |s| s.text.strip == "Apps" }
+          assert_select apps_span.parent, "ul>li", text: "app-name", count: 0
+        end
+      end
+
+      should "list API-only applications for api user" do
+        application = create(:application, name: "app-name", api_only: true)
+        api_user = create(:api_user, with_permissions: { application => [SupportedPermission::SIGNIN_NAME] })
+        create(:access_token, resource_owner_id: api_user.id, application:)
+
+        get :index
+
+        assert_select "td>span" do |spans|
+          apps_span = spans.find { |s| s.text.strip == "Apps" }
+          assert_select apps_span.parent, "ul>li", text: "app-name"
+        end
+      end
     end
 
     context "POST create" do
@@ -70,6 +109,47 @@ class ApiUsersControllerTest < ActionController::TestCase
         assert_select "input[name='api_user[name]'][value='#{api_user.name}']"
         assert_select "input[name='api_user[email]'][value='#{api_user.email}']"
       end
+
+      should "allow editing permissions for application which user has 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] })
+        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: "app-name"
+          assert_select "td" do
+            assert_select "select[name='api_user[supported_permission_ids][]']" do
+              assert_select "option", text: "edit"
+            end
+          end
+        end
+      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
+      end
+
+      should "allow editing permissions for API-only application" do
+        application = create(:application, name: "api-only-app-name", api_only: 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: "api-only-app-name"
+        end
+      end
     end
 
     context "PUT update" do
@@ -83,6 +163,34 @@ class ApiUsersControllerTest < ActionController::TestCase
         assert_equal "Updated API user #{api_user.email} successfully", flash[:notice]
       end
 
+      should "update the user's permissions" do
+        application = create(:application, name: "app-name", with_supported_permissions: %w[edit])
+        api_user = create(:api_user, with_permissions: { application => [SupportedPermission::SIGNIN_NAME] })
+        create(:access_token, resource_owner_id: api_user.id, application:)
+
+        signin_permission = application.supported_permissions.find_by(name: SupportedPermission::SIGNIN_NAME)
+        edit_permission = application.supported_permissions.find_by(name: "edit")
+        permissions = [signin_permission, edit_permission]
+
+        put :update, params: { id: api_user.id, api_user: { supported_permission_ids: permissions } }
+
+        assert_same_elements permissions, api_user.reload.supported_permissions
+      end
+
+      should "update the user's permissions for API-only app" do
+        application = create(:application, name: "app-name", with_supported_permissions: %w[edit], api_only: true)
+        api_user = create(:api_user, with_permissions: { application => [SupportedPermission::SIGNIN_NAME] })
+        create(:access_token, resource_owner_id: api_user.id, application:)
+
+        signin_permission = application.supported_permissions.find_by(name: SupportedPermission::SIGNIN_NAME)
+        edit_permission = application.supported_permissions.find_by(name: "edit")
+        permissions = [signin_permission, edit_permission]
+
+        put :update, params: { id: api_user.id, api_user: { supported_permission_ids: permissions } }
+
+        assert_same_elements permissions, api_user.reload.supported_permissions
+      end
+
       should "redisplay the form with errors if save fails" do
         api_user = create(:api_user)