Merge pull request #2340 from alphagov/dry-up-signin-permission-logic

Dry up signin permission logic

app/controllers/authorisations_controller.rb

@@ -17,7 +17,7 @@ def create
     authorisation.application_id = params[:doorkeeper_access_token][:application_id]
 
     if authorisation.save
-      @api_user.grant_application_permission(authorisation.application, "signin")
+      @api_user.grant_application_signin_permission(authorisation.application)
       EventLog.record_event(@api_user, EventLog::ACCESS_TOKEN_GENERATED, initiator: current_user, application: authorisation.application, ip_address: user_ip_address)
       flash[:authorisation] = { application_name: authorisation.application.name, token: authorisation.token }
     else

app/models/doorkeeper/application.rb

@@ -11,13 +11,13 @@ class ::Doorkeeper::Application < ActiveRecord::Base
         lambda { |user|
           joins(supported_permissions: :user_application_permissions)
             .where("user_application_permissions.user_id" => user.id)
-            .where("supported_permissions.name" => "signin")
+            .where("supported_permissions.name" => SupportedPermission::SIGNIN_NAME)
             .where(retired: false)
         }
   scope :with_signin_delegatable,
         lambda {
           joins(:supported_permissions)
-            .where(supported_permissions: { name: "signin", delegatable: true })
+            .where(supported_permissions: { name: SupportedPermission::SIGNIN_NAME, delegatable: true })
         }
 
   after_create :create_signin_supported_permission
@@ -36,7 +36,7 @@ def supported_permission_strings(user = nil)
   end
 
   def signin_permission
-    supported_permissions.find_by(name: "signin")
+    supported_permissions.find_by(name: SupportedPermission::SIGNIN_NAME)
   end
 
   def sorted_supported_permissions_grantable_from_ui
@@ -78,7 +78,7 @@ def substituted_uri(uri)
   end
 
   def create_signin_supported_permission
-    supported_permissions.create!(name: "signin", delegatable: true)
+    supported_permissions.create!(name: SupportedPermission::SIGNIN_NAME, delegatable: true)
   end
 
   def create_user_update_supported_permission

app/models/supported_permission.rb

@@ -1,4 +1,6 @@
 class SupportedPermission < ApplicationRecord
+  SIGNIN_NAME = "signin".freeze
+
   belongs_to :application, class_name: "Doorkeeper::Application"
   has_many :user_application_permissions, dependent: :destroy, inverse_of: :supported_permission
 
@@ -11,15 +13,15 @@ class SupportedPermission < ApplicationRecord
   scope :default, -> { where(default: true) }
 
   def signin?
-    name.try(:downcase) == "signin"
+    name.try(:downcase) == SIGNIN_NAME
   end
 
 private
 
   def signin_permission_name_not_changed
     return if new_record? || !name_changed?
 
-    if name_change.first.casecmp("signin").zero?
+    if name_change.first.casecmp(SIGNIN_NAME).zero?
       errors.add(:name, "of permission #{name_change.first} can't be changed")
     end
   end

app/models/user.rb

@@ -167,6 +167,10 @@ def revoke_all_authorisations
     authorisations.where(revoked_at: nil).find_each(&:revoke)
   end
 
+  def grant_application_signin_permission(application)
+    grant_application_permission(application, SupportedPermission::SIGNIN_NAME)
+  end
+
   def grant_application_permission(application, supported_permission_name)
     grant_application_permissions(application, [supported_permission_name]).first
   end

app/views/shared/_user_permissions.html.erb

@@ -31,19 +31,19 @@
             # API Users will always have a "signin" permission for apps for which they have access token.
             # The hidden field ensures it is not lost.
           %>
-          <%= hidden_field_tag supported_permission_field_name, application.signin_permission.id, id: "#{supported_permission_field_prefix}_signin" %>
+          <%= hidden_field_tag supported_permission_field_name, application.signin_permission.id, id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %>
         <% else %>
           <td>
-            <%= label_tag "#{supported_permission_field_prefix}_signin", "Has access to #{application.name}?", class: "rm" %>
+            <%= label_tag "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}", "Has access to #{application.name}?", class: "rm" %>
             <%= check_box_tag supported_permission_field_name, application.signin_permission.id, user_object.has_access_to?(application),
-                              id: "#{supported_permission_field_prefix}_signin" %>
+                              id: "#{supported_permission_field_prefix}_#{SupportedPermission::SIGNIN_NAME}" %>
           </td>
         <% end %>
         <td>
           <%= label_tag "#{supported_permission_field_prefix}_ids", "Permissions for #{application.name}", class: "rm" %>
           <% supported_permissions_options = application.supported_permissions.grantable_from_ui
                                                .inject({}) {|h, per| h.merge(per.name => per.id) }
-             supported_permissions_options.delete('signin') %>
+             supported_permissions_options.delete(SupportedPermission::SIGNIN_NAME) %>
           <%= select_tag supported_permission_field_name,
                           options_for_select(supported_permissions_options,
                             user_object.permission_ids_for(application) - [application.signin_permission.id]),

app/views/users/_app_permissions.html.erb

@@ -22,11 +22,11 @@
         </td>
         <% unless user_object.api_user? %>
           <td>
-            <%= permission_names.include?('signin') ? 'Yes' : 'No' %>
+            <%= permission_names.include?(SupportedPermission::SIGNIN_NAME) ? 'Yes' : 'No' %>
           </td>
         <% end %>
         <td>
-          <%= permission_names.reject{|p| p == 'signin'}.to_sentence %>
+          <%= permission_names.reject{|p| p == SupportedPermission::SIGNIN_NAME}.to_sentence %>
         </td>
       </tr>
     <% end %>

lib/numbers/metrics.rb

@@ -65,7 +65,7 @@ def to_a
   private
 
     def has_signin_permissions?(permission)
-      permission.permissions.include?("signin")
+      permission.permissions.include?(SupportedPermission::SIGNIN_NAME)
     end
 
     def count_values(map)

lib/signin_permission_granter.rb

@@ -5,7 +5,7 @@ def self.call(users:, application:)
       next if user.has_access_to?(application)
 
       puts "-- Adding signin permission for #{application.name}"
-      user.grant_application_permission(application, "signin")
+      user.grant_application_signin_permission(application)
 
       if application.supports_push_updates?
         PermissionUpdater.perform_later(user.uid, application.id)

lib/sso_push_credential.rb

@@ -1,10 +1,11 @@
 class SSOPushCredential
-  PERMISSIONS = %w[signin user_update_permission].freeze
+  PERMISSIONS = %w[user_update_permission].freeze
   USER_NAME = "Signon API Client (permission and suspension updater)".freeze
   USER_EMAIL = "signon+permissions@alphagov.co.uk".freeze
 
   class << self
     def credentials(application)
+      user.grant_application_signin_permission(application)
       user.grant_application_permissions(application, PERMISSIONS)
 
       user.authorisations

lib/user_creator.rb

@@ -35,7 +35,7 @@ def extract_applications_from_names
 
   def grant_requested_signin_permissions
     applications.each do |application|
-      user.grant_application_permission(application, "signin")
+      user.grant_application_signin_permission(application)
     end
   end
 

test/controllers/authorisations_controller_test.rb

@@ -58,11 +58,11 @@ class AuthorisationsControllerTest < ActionController::TestCase
       end
 
       should "not duplicate 'signin' permission for the authorised application if it already exists" do
-        @api_user.grant_application_permission(@application, "signin")
+        @api_user.grant_application_signin_permission(@application)
 
         post :create, params: { api_user_id: @api_user.id, doorkeeper_access_token: { application_id: @application.id } }
 
-        assert_equal %w[signin], @api_user.permissions_for(@application)
+        assert_equal [SupportedPermission::SIGNIN_NAME], @api_user.permissions_for(@application)
       end
     end
   end

test/controllers/root_controller_test.rb

@@ -26,7 +26,7 @@ def setup
   test "Your applications should include apps you have permission to signin to" do
     exclusive_app = create(:application, name: "Exclusive app")
     everybody_app = create(:application, name: "Everybody app")
-    user = create(:user, with_permissions: { exclusive_app => [], everybody_app => %w[signin] })
+    user = create(:user, with_permissions: { exclusive_app => [], everybody_app => [SupportedPermission::SIGNIN_NAME] })
 
     sign_in user