diff --git a/app/controllers/account/applications_controller.rb b/app/controllers/account/applications_controller.rb new file mode 100644 index 000000000..750ed62ff --- /dev/null +++ b/app/controllers/account/applications_controller.rb @@ -0,0 +1,12 @@ +class Account::ApplicationsController < ApplicationController + layout "admin_layout" + + before_action :authenticate_user! + + def index + authorize :account_applications + + @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/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb new file mode 100644 index 000000000..1fcc0fa2f --- /dev/null +++ b/app/controllers/accounts_controller.rb @@ -0,0 +1,13 @@ +class AccountsController < ApplicationController + before_action :authenticate_user! + + def show + authorize :account_page + + if policy(current_user).edit? + redirect_to edit_user_path(current_user) + else + redirect_to edit_email_or_password_user_path(current_user) + end + end +end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 133cc6cb3..10a212870 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -15,15 +15,6 @@ def nav_link(text, link) end end - def user_link_target - # The page the current user's name in the header should link them to - if policy(current_user).edit? - edit_user_path(current_user) - else - edit_email_or_password_user_path(current_user) - end - end - SENSITIVE_QUERY_PARAMETERS = %w[reset_password_token invitation_token].freeze def sensitive_query_parameters? diff --git a/app/helpers/navigation_items_helper.rb b/app/helpers/navigation_items_helper.rb index 90745ab91..8579e11c6 100644 --- a/app/helpers/navigation_items_helper.rb +++ b/app/helpers/navigation_items_helper.rb @@ -24,7 +24,7 @@ def navigation_items end end - items << { text: current_user.name, href: user_link_target } + items << { text: current_user.name, href: account_path } items << { text: "Sign out", href: destroy_user_session_path } items diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index fcebfcebb..cb8754594 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -7,17 +7,28 @@ 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_id" => user.id) - .where("supported_permissions.name" => SupportedPermission::SIGNIN_NAME) - .where(retired: false) + with_signin_permission_for(user) + .not_retired } scope :with_signin_delegatable, lambda { joins(:supported_permissions) - .where(supported_permissions: { name: SupportedPermission::SIGNIN_NAME, delegatable: true }) + .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 @@ -36,7 +47,7 @@ def supported_permission_strings(user = nil) end def signin_permission - supported_permissions.find_by(name: SupportedPermission::SIGNIN_NAME) + supported_permissions.signin.first end def sorted_supported_permissions_grantable_from_ui @@ -78,7 +89,7 @@ def substituted_uri(uri) end def create_signin_supported_permission - supported_permissions.create!(name: SupportedPermission::SIGNIN_NAME, delegatable: true) + supported_permissions.delegatable.signin.create! end def create_user_update_supported_permission diff --git a/app/models/supported_permission.rb b/app/models/supported_permission.rb index 188bbc3d4..be86a9354 100644 --- a/app/models/supported_permission.rb +++ b/app/models/supported_permission.rb @@ -11,6 +11,7 @@ class SupportedPermission < ApplicationRecord scope :delegatable, -> { where(delegatable: true) } scope :grantable_from_ui, -> { where(grantable_from_ui: true) } scope :default, -> { where(default: true) } + scope :signin, -> { where(name: SIGNIN_NAME) } def signin? name.try(:downcase) == SIGNIN_NAME diff --git a/app/policies/account_applications_policy.rb b/app/policies/account_applications_policy.rb new file mode 100644 index 000000000..69902efd2 --- /dev/null +++ b/app/policies/account_applications_policy.rb @@ -0,0 +1,5 @@ +class AccountApplicationsPolicy < BasePolicy + def index? + current_user.govuk_admin? + end +end diff --git a/app/policies/account_page_policy.rb b/app/policies/account_page_policy.rb new file mode 100644 index 000000000..2d3c1d980 --- /dev/null +++ b/app/policies/account_page_policy.rb @@ -0,0 +1,5 @@ +class AccountPagePolicy < BasePolicy + def show? + current_user.present? + end +end diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb new file mode 100644 index 000000000..8f619ade9 --- /dev/null +++ b/app/views/account/applications/index.html.erb @@ -0,0 +1,53 @@ +<% content_for :title, "GOV.UK apps" %> + +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Dashboard", + url: root_path, + }, + { + title: "GOV.UK apps", + } + ] + }) +%> + + + + + + + + + + + <% @applications_with_signin.each do |application| %> + + + + + <% end %> + +
Apps you have access to
NameDescription
<%= application.name %><%= application.description %>
+ +

Apps you don't have access to

+ + + + + + + + + + <% @applications_without_signin.each do |application| %> + + + + + <% end %> + +
NameDescription
<%= application.name %><%= application.description %>
diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 62e107957..74bbdd6eb 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -31,7 +31,7 @@ <% end %> <% content_for :navbar_right do %> - <%= link_to current_user.name, user_link_target %> + <%= link_to current_user.name, account_path %> • <%= link_to 'Sign out', destroy_user_session_path %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index d6549285a..6ee78fad9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -49,6 +49,11 @@ end resource :user, only: [:show] + resource :account, only: [:show] + namespace :account do + resources :applications, only: [:index] + end + resources :batch_invitations, only: %i[new create show] resources :bulk_grant_permission_sets, only: %i[new create show] resources :organisations, only: %i[index edit update] diff --git a/test/factories/users.rb b/test/factories/users.rb index 3f9f2a5ea..c78a3d7df 100644 --- a/test/factories/users.rb +++ b/test/factories/users.rb @@ -1,5 +1,5 @@ FactoryBot.define do - factory :user do + factory :user, aliases: [:normal_user] do transient do with_permissions { {} } with_signin_permissions_for { [] } diff --git a/test/integration/account_applications_test.rb b/test/integration/account_applications_test.rb new file mode 100644 index 000000000..c237bbe00 --- /dev/null +++ b/test/integration/account_applications_test.rb @@ -0,0 +1,63 @@ +require "test_helper" + +class AccountApplicationsTest < ActionDispatch::IntegrationTest + context "#index" do + setup do + @application = create(:application, name: "app-name", description: "app-description") + @retired_application = create(:application, retired: true, name: "retired-app-name") + @user = FactoryBot.create(:admin_user) + end + + should "not be accessible to signed out users" do + visit account_applications_path + + assert_current_url new_user_session_path + end + + should "list the applications the user has access to" do + @user.grant_application_signin_permission(@application) + + visit new_user_session_path + signin_with @user + + visit account_applications_path + + 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 + @user.grant_application_signin_permission(@retired_application) + + visit new_user_session_path + signin_with @user + + visit account_applications_path + + assert_not page.has_content?("retired-app-name") + end + + should "list the applications the user does not have access to" do + visit new_user_session_path + signin_with @user + + visit account_applications_path + + 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/integration/account_test.rb b/test/integration/account_test.rb new file mode 100644 index 000000000..23f320ecb --- /dev/null +++ b/test/integration/account_test.rb @@ -0,0 +1,33 @@ +require "test_helper" + +class AccountTest < ActionDispatch::IntegrationTest + context "#show" do + should "not be accessible to signed out users" do + visit account_path + + assert_current_url new_user_session_path + end + + should "redirect to user's edit page for admin users" do + user = FactoryBot.create(:admin_user) + + visit new_user_session_path + signin_with user + + visit account_path + + assert_current_url edit_user_path(user) + end + + should "redirect to edit email or password page for normal users" do + user = FactoryBot.create(:user) + + visit new_user_session_path + signin_with user + + visit account_path + + assert_current_url edit_email_or_password_user_path(user) + end + end +end diff --git a/test/models/doorkeeper_application_test.rb b/test/models/doorkeeper_application_test.rb index 01d54f87b..55a356d6f 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 diff --git a/test/models/supported_permission_test.rb b/test/models/supported_permission_test.rb index 3766e1a65..0f9e2cd0c 100644 --- a/test/models/supported_permission_test.rb +++ b/test/models/supported_permission_test.rb @@ -56,4 +56,11 @@ class SupportedPermissionTest < ActiveSupport::TestCase assert_not default_permissions.include? permission_three assert_not default_permissions.include? application_one.signin_permission end + + test ".signin returns all signin permissions" do + app1 = create(:application, with_supported_permissions: %w[app1-permission]) + app2 = create(:application, with_supported_permissions: %w[app2-permission]) + + assert_same_elements [app1.signin_permission, app2.signin_permission], SupportedPermission.signin + end end diff --git a/test/policies/account_applications_policy_test.rb b/test/policies/account_applications_policy_test.rb new file mode 100644 index 000000000..c3004343f --- /dev/null +++ b/test/policies/account_applications_policy_test.rb @@ -0,0 +1,32 @@ +require "test_helper" +require "support/policy_helpers" + +class AccountApplicationsPolicyTest < ActiveSupport::TestCase + include PolicyHelpers + + context "accessing index?" do + %i[superadmin admin].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = FactoryBot.build(:"#{user_role}_user") + end + + should "be permitted" do + assert permit?(@current_user, nil, :index) + end + end + end + + %i[super_organisation_admin organisation_admin normal].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = FactoryBot.build(:"#{user_role}_user") + end + + should "be forbidden" do + assert forbid?(@current_user, nil, :index) + end + end + end + end +end diff --git a/test/policies/account_page_policy_test.rb b/test/policies/account_page_policy_test.rb new file mode 100644 index 000000000..465c4f0a3 --- /dev/null +++ b/test/policies/account_page_policy_test.rb @@ -0,0 +1,14 @@ +require "test_helper" +require "support/policy_helpers" + +class AccountPagePolicyTest < ActiveSupport::TestCase + include PolicyHelpers + + should "allow logged in users to see show irrespective of their role" do + assert permit?(build(:user), nil, :show) + end + + should "not allow anonymous visitors to see show" do + assert forbid?(nil, nil, :show) + end +end