Extract PersonalAccessTokenValidator out of User

The token validation logic was quite noisy in the User class, so this
commit pulls it out into its own class.

As part of this we have updated the validation to not return early if
validation fails—now all validation errors will be added to the errors object.

app/models/user.rb

@@ -11,10 +11,6 @@ class User < ApplicationRecord
   has_many :pinned_searches, dependent: :delete_all
 
   ERRORS = {
-    invalid_token: [:personal_access_token, 'is not a valid token for this github user'],
-    missing_notifications_scope: [:personal_access_token, 'does not include the notifications scope'],
-    missing_read_org_scope: [:personal_access_token, 'does not include the read:org scope'],
-    disallowed_tokens: [:personal_access_token, 'is not allowed in this instance'],
     refresh_interval_size: [:refresh_interval, 'must be less than 1 day']
   }.freeze
 
@@ -28,7 +24,7 @@ class User < ApplicationRecord
     less_than_or_equal_to: 86_400_000,
     message: ERRORS[:refresh_interval_size][1]
   }
-  validate :personal_access_token_validator
+  validates_with PersonalAccessTokenValidator
 
   scope :not_recently_synced, -> { where('last_synced_at < ?', 1.minute.ago) }
 
@@ -120,16 +116,6 @@ def effective_access_token
     Octobox.personal_access_tokens_enabled? && personal_access_token.present? ? personal_access_token : access_token
   end
 
-  def personal_access_token_validator
-    return unless personal_access_token.present?
-    return unless validate_tokens_are_enabled
-    return unless validate_token_credentials
-    return unless validate_github_id
-    validate_github_client_notifications_scope
-    validate_github_client_read_scope
-  end
-
-
   def masked_personal_access_token
     personal_access_token.blank? ? '' :
     "#{'*' * 32}#{personal_access_token.slice(-8..-1)}"
@@ -146,36 +132,4 @@ def sync_app_installation_access
     removed_permissions = app_installation_permissions.reject{|ep| app_installation_ids.include?(ep.app_installation_id) }
     removed_permissions.each(&:destroy)
   end
-
-  private
-
-  def validate_tokens_are_enabled
-    return true if Octobox.personal_access_tokens_enabled?
-    errors.add(*ERRORS[:disallowed_tokens])
-    false
-  end
-
-  def validate_token_credentials
-    return true if Octokit::Client.new.validate_credentials(access_token: effective_access_token)
-    errors.add(*ERRORS[:invalid_token])
-    false
-  end
-
-  def validate_github_id
-    return true if github_id == github_client.user.id
-    errors.add(*ERRORS[:invalid_token])
-    false
-  end
-
-  def validate_github_client_notifications_scope
-    return true if github_client.scopes.include? 'notifications'
-    errors.add(*ERRORS[:missing_notifications_scope])
-    false
-  end
-
-  def validate_github_client_read_scope
-    return true if !Octobox.restricted_access_enabled? || github_client.scopes.include?('read:org')
-    errors.add(*ERRORS[:missing_read_org_scope])
-    false
-  end
 end

app/validators/personal_access_token_validator.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+class PersonalAccessTokenValidator < ActiveModel::Validator
+
+  ERRORS = {
+    invalid_token: [:personal_access_token, 'is not a valid token for this github user'],
+    missing_notifications_scope: [:personal_access_token, 'does not include the notifications scope'],
+    missing_read_org_scope: [:personal_access_token, 'does not include the read:org scope'],
+    disallowed_tokens: [:personal_access_token, 'is not allowed in this instance'],
+  }.freeze
+
+  def validate(record)
+    return if record.personal_access_token.blank?
+    validate_tokens_are_enabled(record)
+    validate_token_credentials(record)
+    validate_github_id(record)
+    validate_github_client_notifications_scope(record)
+    validate_github_client_read_scope(record)
+  end
+
+  private
+
+  def validate_tokens_are_enabled(record)
+    unless Octobox.personal_access_tokens_enabled?
+      record.errors.add(*ERRORS[:disallowed_tokens])
+    end
+  end
+
+  def validate_token_credentials(record)
+    valid_credentials = Octokit::Client.new.validate_credentials(
+      access_token: record.effective_access_token)
+    record.errors.add(*ERRORS[:invalid_token]) unless valid_credentials
+  end
+
+  def validate_github_id(record)
+    unless record.github_id == record.github_client.user.id
+      record.errors.add(*ERRORS[:invalid_token])
+    end
+  end
+
+  def validate_github_client_notifications_scope(record)
+    unless record.github_client.scopes.include? 'notifications'
+      record.errors.add(*ERRORS[:missing_notifications_scope])
+    end
+  end
+
+  def validate_github_client_read_scope(record)
+    if Octobox.restricted_access_enabled? && !record.github_client.scopes.include?('read:org')
+      record.errors.add(*ERRORS[:missing_read_org_scope])
+    end
+  end
+end

test/models/user_test.rb

@@ -10,7 +10,7 @@ class UserTest < ActiveSupport::TestCase
 
   def assert_error_present(model_object, error)
     refute model_object.valid?
-    model_object.errors[error[0]].include? error[1]
+    assert model_object.errors[error[0]].include? error[1]
   end
 
   test 'must have a github id' do
@@ -76,34 +76,6 @@ def assert_error_present(model_object, error)
     assert_equal 'abcdefg', @user.access_token
   end
 
-  test 'does not allow a personal_access_token for another user' do
-    stub_personal_access_tokens_enabled
-    @user.personal_access_token = '1234'
-    stub_user_request(body: '{"id": 98}')
-    assert_error_present(@user, User::ERRORS[:invalid_token])
-  end
-
-  test 'does not allow a personal_access_token without the notifications scope' do
-    stub_personal_access_tokens_enabled
-    @user.personal_access_token = '1234'
-    stub_user_request(oauth_scopes: 'user, repo')
-    assert_error_present(@user, User::ERRORS[:missing_notifications_scope])
-  end
-
-  test 'does not allow a personal_access_token without the read:org scope if restricted_access enabled' do
-    stub_personal_access_tokens_enabled
-    stub_restricted_access_enabled
-    @user.personal_access_token = '1234'
-    stub_user_request(oauth_scopes: 'user, repo')
-    assert_error_present(@user, User::ERRORS[:missing_read_org_scope])
-  end
-
-  test 'does not allow setting personal_access_token without being enabled' do
-    stub_personal_access_tokens_enabled(value: false)
-    @user.personal_access_token = '1234'
-    assert_error_present(@user, User::ERRORS[:disallowed_tokens])
-  end
-
   test '#github_client returns an Octokit::Client with the correct access_token' do
     assert_equal @user.github_client.class, Octokit::Client
     assert_equal @user.github_client.access_token, @user.access_token

test/validators/personal_access_token_validator_test.rb

@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+require 'test_helper'
+
+class PersonalAccessTokenValidatorTest < ActiveSupport::TestCase
+  setup do
+    @user = build(:user, personal_access_token: '1234')
+    stub_user_request(user: @user)
+    stub_personal_access_tokens_enabled
+    stub_notifications_request
+  end
+
+  def assert_error_present(model_object, error)
+    refute model_object.valid?
+    assert model_object.errors[error[0]].include? error[1]
+  end
+
+  test 'it allows a blank personal_access_token' do
+    @user.personal_access_token = nil
+    assert @user.valid?
+  end
+
+  test 'does not allow setting personal_access_token without being enabled' do
+    stub_personal_access_tokens_enabled(value: false)
+    assert_error_present(@user, PersonalAccessTokenValidator::ERRORS[:disallowed_tokens])
+  end
+
+  test 'does not allow invalid credentials' do
+    Octokit::Client.any_instance.stubs(:validate_credentials).returns(false)
+    stub_user_request(user: @user)
+    assert_error_present(@user, PersonalAccessTokenValidator::ERRORS[:invalid_token])
+  end
+
+  test 'does not allow a personal_access_token for another user' do
+    stub_user_request(body: '{"id": 98}')
+    assert_error_present(@user, PersonalAccessTokenValidator::ERRORS[:invalid_token])
+  end
+
+  test 'does not allow a personal_access_token without the notifications scope' do
+    stub_user_request(user: @user, oauth_scopes: 'user, repo')
+    assert_error_present(@user, PersonalAccessTokenValidator::ERRORS[:missing_notifications_scope])
+  end
+
+  test 'does not allow a personal_access_token without the read:org scope if restricted_access enabled' do
+    stub_restricted_access_enabled
+    stub_user_request(oauth_scopes: 'user, repo')
+    assert_error_present(@user, PersonalAccessTokenValidator::ERRORS[:missing_read_org_scope])
+  end
+end