refactors finder and correlated code

app/controllers/admin/impersonation_tokens_controller.rb

 class Admin::ImpersonationTokensController < Admin::ApplicationController
-  before_action :user
+  before_action :user, :finder
   def index
     set_index_vars
   end
   def create
-    # We never want to non-impersonate a user
-    @impersonation_token = user.personal_access_tokens.build(impersonation_token_params.merge(impersonation: true))
+    @impersonation_token = finder.execute.build(impersonation_token_params)
     if @impersonation_token.save
       flash[:impersonation_token] = @impersonation_token.token
@@ -19,7 +18,7 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController
   end
   def revoke
-    @impersonation_token = user.personal_access_tokens.impersonation.find(params[:id])
+    @impersonation_token = finder.execute(id: params[:id])
     if @impersonation_token.revoke!
       flash[:notice] = "Revoked impersonation token #{@impersonation_token.name}!"
@@ -36,14 +35,21 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController
     @user ||= User.find_by!(username: params[:user_id])
   end
+  def finder
+    @finder ||= PersonalAccessTokensFinder.new(user: user, impersonation: true)
+  end
+
   def impersonation_token_params
     params.require(:personal_access_token).permit(:name, :expires_at, :impersonation, scopes: [])
   end
   def set_index_vars
-    @impersonation_token ||= user.personal_access_tokens.build
+    finder.params[:state] = 'active'
+    @impersonation_token ||= finder.execute.build
     @scopes = Gitlab::Auth::SCOPES
-    @active_impersonation_tokens = user.personal_access_tokens.impersonation.active.order(:expires_at)
-    @inactive_impersonation_tokens = user.personal_access_tokens.impersonation.inactive
+    finder.params[:order] = :expires_at
+    @active_impersonation_tokens = finder.execute
+    finder.params[:state] = 'inactive'
+    @inactive_impersonation_tokens = finder.execute
   end
 end

app/controllers/profiles/personal_access_tokens_controller.rb

 class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
+  before_action :finder
+
   def index
     set_index_vars
   end
   def create
-    @personal_access_token = current_user.personal_access_tokens.build(personal_access_token_params)
+    @personal_access_token = finder.execute.build(personal_access_token_params)
     if @personal_access_token.save
       flash[:personal_access_token] = @personal_access_token.token
@@ -16,7 +18,7 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
   end
   def revoke
-    @personal_access_token = current_user.personal_access_tokens.find(params[:id])
+    @personal_access_token = finder.execute(id: params[:id])
     if @personal_access_token.revoke!
       flash[:notice] = "Revoked personal access token #{@personal_access_token.name}!"
@@ -29,14 +31,21 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
   private
+  def finder
+    @finder ||= PersonalAccessTokensFinder.new(user: current_user, impersonation: false)
+  end
+
   def personal_access_token_params
     params.require(:personal_access_token).permit(:name, :expires_at, scopes: [])
   end
   def set_index_vars
-    @personal_access_token ||= current_user.personal_access_tokens.build
+    finder.params[:state] = 'active'
+    @personal_access_token ||= finder.execute.build
     @scopes = Gitlab::Auth::SCOPES
-    @active_personal_access_tokens = current_user.personal_access_tokens.active.order(:expires_at)
-    @inactive_personal_access_tokens = current_user.personal_access_tokens.inactive
+    finder.params[:order] = :expires_at
+    @active_personal_access_tokens = finder.execute
+    finder.params[:state] = 'inactive'
+    @inactive_personal_access_tokens = finder.execute
   end
 end

app/finders/personal_access_tokens_finder.rb

 class PersonalAccessTokensFinder
-  def initialize(user, params = {})
-    @user = user
+  attr_accessor :params
+
+  def initialize(params = {})
     @params = params
   end
-  def execute
-    pat_id = token_id?
-    personal_access_tokens = @user.personal_access_tokens
-    personal_access_tokens = personal_access_tokens.impersonation if impersonation?
-    return find_token_by_id(personal_access_tokens, pat_id) if pat_id
-    case state?
-    when 'active'
-      personal_access_tokens.active
-    when 'inactive'
-      personal_access_tokens.inactive
-    else
-      personal_access_tokens
-    end
+  def execute(token: nil, id: nil)
+    tokens = by_impersonation
+    return tokens.find_by_token(token) if token
+    return tokens.find_by_id(id) if id
+    tokens = by_state(tokens)
+    tokens.order(@params[:order]) if @params[:order]
+
+    tokens
   end
   private
-  def state?
-    @params[:state].presence
-  end
-
-  def impersonation?
-    @params[:impersonation].presence
+  def personal_access_tokens
+    @params[:user] ? @params[:user].personal_access_tokens : PersonalAccessToken.all
   end
-  def token_id?
-    @params[:personal_access_token_id].presence
+  def by_impersonation
+    case @params[:impersonation]
+    when true
+      personal_access_tokens.with_impersonation
+    when false
+      personal_access_tokens.without_impersonation
+    else
+      personal_access_tokens
+    end
   end
-  def find_token_by_id(personal_access_tokens, pat_id)
-    personal_access_tokens.find_by(id: pat_id)
+  def by_state(tokens)
+    case @params[:state]
+    when 'active'
+      tokens.active
+    when 'inactive'
+      tokens.inactive
+    else
+      tokens
+    end
   end
 end

app/models/personal_access_token.rb

@@ -9,11 +9,10 @@ class PersonalAccessToken < ActiveRecord::Base
   before_save :ensure_token
-  default_scope { where(impersonation: false) }
-  scope :active, -> { where(revoked: false).where("expires_at >= NOW() OR expires_at IS NULL") }
+  scope :active, -> { where("revoked = false AND (expires_at >= NOW() OR expires_at IS NULL)") }
   scope :inactive, -> { where("revoked = true OR expires_at < NOW()") }
-  scope :impersonation, -> { unscoped.where(impersonation: true) }
-  scope :with_impersonation_tokens, ->  { unscoped }
+  scope :with_impersonation, -> { where(impersonation: true) }
+  scope :without_impersonation, -> { where(impersonation: false) }
   def revoke!
     self.revoked = true

app/views/profiles/personal_access_tokens/index.html.haml

@@ -86,19 +86,6 @@
 :javascript
-  var $dateField = $('#personal_access_token_expires_at');
-  var date = $dateField.val();
-
-  new Pikaday({
-    field: $dateField.get(0),
-    theme: 'gitlab-theme',
-    format: 'yyyy-mm-dd',
-    minDate: new Date(),
-    onSelect: function(dateText) {
-      $dateField.val(dateFormat(new Date(dateText), 'yyyy-mm-dd'));
-    }
-  });
-
   $("#created-personal-access-token").click(function() {
     this.select();
   });

app/views/shared/_personal_access_tokens_form.html.haml

@@ -29,7 +29,7 @@
   new Pikaday({
     field: $dateField.get(0),
     theme: 'gitlab-theme',
-    format: 'YYYY-MM-DD',
+    format: 'yyyy-mm-dd',
     minDate: new Date(),
     onSelect: function(dateText) {
       $dateField.val(dateFormat(new Date(dateText), 'yyyy-mm-dd'));

doc/api/personal_access_tokens.md

@@ -2,11 +2,19 @@
 ## List
+This function takes pagination parameters `page` and `per_page` to restrict the list of personal access tokens.
+
 ```
 GET /personal_access_tokens
 ```
-An example:
+Parameters:
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `state`   | string | no | filter tokens based on state (all, active, inactive) |
+
+Example response:
 ```json
 [
   {
@@ -20,20 +28,6 @@ An example:
 ]
 ```
-In addition, you can filter tokens based on state: `all`, `active` and `inactive`
-
-```
-GET /personal_access_tokens?state=all
-```
-
-```
-GET /personal_access_tokens?state=active
-```
-
-```
-GET /personal_access_tokens?state=inactive
-```
-
 ## Show
 ```

doc/api/users.md

@@ -831,18 +831,21 @@ Example response:
 ## Retrieve user personal access tokens
 It retrieves every personal access token of the user. Note that only administrators can do this.
+This function takes pagination parameters `page` and `per_page` to restrict the list of personal access tokens.
 ```
-GET /users/:user_id/personal_access_tokens
+GET /users/:id/personal_access_tokens
 ```
 Parameters:
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
-| `user_id` | integer | yes | The ID of the user |
-An example:
+| `id` | integer | yes | The ID of the user |
+| `state`   | string | no | filter tokens based on state (all, active, inactive) |
+| `impersonation` | boolean | no | The impersonation flag of the personal access token |
+Example response:
 ```json
 [
   {
@@ -852,45 +855,25 @@ An example:
     "expires_at": "2017-01-04",
     "scopes": ['api'],
     "active": true,
-    "impersonation": false,
+    "impersonation": true,
     "token": "9koXpg98eAheJpvBs5tK"
   }
 ]
 ```
-In addition, you can filter tokens based on state: `all`, `active` and `inactive`
-
-```
-GET /users/:user_id/personal_access_tokens?state=all
-```
-
-```
-GET /users/:user_id/personal_access_tokens?state=active
-```
-
-```
-GET /users/:user_id/personal_access_tokens?state=inactive
-```
-
-Finally, you can filter based on impersonation: `true` or `false`.
-
-```
-GET /users/:user_id/personal_access_tokens?impersonation=true
-```
-
 ## Show a user personal access token
 It shows a user's personal access token. Note that only administrators can do this.
 ```
-GET /users/:user_id/personal_access_tokens/:personal_access_token_id
+GET /users/:id/personal_access_tokens/:personal_access_token_id
 ```
 Parameters:
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
-| `user_id` | integer | yes | The ID of the user |
+| `id` | integer | yes | The ID of the user |
 | `personal_access_token_id` | integer | yes | The ID of the personal access token |
 ## Create a personal access token
@@ -901,14 +884,14 @@ both API calls and Git reads and writes. The user will not see these tokens in h
 settings page.
 ```
-POST /users/:user_id/personal_access_tokens
+POST /users/:id/personal_access_tokens
 ```
 Parameters:
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
-| `user_id` | integer | yes | The ID of the user |
+| `id` | integer | yes | The ID of the user |
 | `name` | string | yes | The name of the personal access token |
 | `expires_at` | date | no | The expiration date of the personal access token |
 | `scopes` | array | no | The array of scopes of the personal access token |
@@ -919,12 +902,12 @@ Parameters:
 It revokes a personal access token. Note that only administrators can revoke impersonation tokens.
 ```
-DELETE /users/:user_id/personal_access_tokens/:personal_access_token_id
+DELETE /users/:id/personal_access_tokens/:personal_access_token_id
 ```
 Parameters:
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
-| `user_id` | integer | yes | The ID of the user |
+| `id` | integer | yes | The ID of the user |
 | `personal_access_token_id` | integer | yes | The ID of the personal access token |

lib/api/personal_access_tokens.rb

 module API
   class PersonalAccessTokens < Grape::API
-    before { authenticate! }
+    include PaginationParams
+
+    before do
+      authenticate!
+      @finder = PersonalAccessTokensFinder.new(user: current_user, impersonation: false)
+    end
     resource :personal_access_tokens do
       desc 'Retrieve personal access tokens' do
@@ -9,8 +14,12 @@ module API
       end
       params do
         optional :state, type: String, default: 'all', values: %w[all active inactive], desc: 'Filters (all|active|inactive) personal_access_tokens'
+        use :pagination
+      end
+      get do
+        @finder.params.merge!(declared_params(include_missing: false))
+        present paginate(@finder.execute), with: Entities::PersonalAccessToken
       end
-      get { present PersonalAccessTokensFinder.new(current_user, params).execute, with: Entities::PersonalAccessToken }
       desc 'Retrieve personal access token' do
         detail 'This feature was introduced in GitLab 9.0'
@@ -20,7 +29,7 @@ module API
         requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
       end
       get ':personal_access_token_id' do
-        personal_access_token = PersonalAccessTokensFinder.new(current_user, declared_params(include_missing: false)).execute
+        personal_access_token = @finder.execute(id: declared_params[:personal_access_token_id])
         not_found!('Personal Access Token') unless personal_access_token
         present personal_access_token, with: Entities::PersonalAccessToken
@@ -36,7 +45,7 @@ module API
         optional :scopes, type: Array, desc: 'The array of scopes of the personal access token'
       end
       post do
-        personal_access_token = current_user.personal_access_tokens.build(declared_params(include_missing: false))
+        personal_access_token = @finder.execute.build(declared_params(include_missing: false))
         if personal_access_token.save
           present personal_access_token, with: Entities::PersonalAccessTokenWithToken
@@ -52,12 +61,10 @@ module API
         requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
       end
       delete ':personal_access_token_id' do
-        personal_access_token = PersonalAccessTokensFinder.new(current_user, declared_params(include_missing: false)).execute
+        personal_access_token = @finder.execute(id: declared_params[:personal_access_token_id])
         not_found!('Personal Access Token') unless personal_access_token
         personal_access_token.revoke!
-
-        no_content!
       end
     end
   end

lib/api/users.rb

@@ -373,7 +373,11 @@ module API
       end
       segment ':id' do
         resource :personal_access_tokens do
-          before { authenticated_as_admin! }
+          before do
+            authenticated_as_admin!
+            user = find_user(params)
+            @finder = PersonalAccessTokensFinder.new(user: user)
+          end
           desc 'Retrieve personal access tokens. Available only for admins.' do
             detail 'This feature was introduced in GitLab 9.0'
@@ -381,11 +385,12 @@ module API
           end
           params do
             optional :state, type: String, default: 'all', values: %w[all active inactive], desc: 'Filters (all|active|inactive) personal_access_tokens'
-            optional :impersonation, type: Boolean, default: false, desc: 'Filters only impersonation personal_access_tokens'
+            optional :impersonation, type: Boolean, desc: 'Filters only impersonation personal_access_tokens'
+            use :pagination
           end
           get do
-            user = find_user(params)
-            present PersonalAccessTokensFinder.new(user, params).execute, with: Entities::ImpersonationToken
+            @finder.params.merge!(declared_params(include_missing: false))
+            present paginate(@finder.execute), with: Entities::ImpersonationToken
           end
           desc 'Create a personal access token. Available only for admins.' do
@@ -396,11 +401,10 @@ module API
             requires :name, type: String, desc: 'The name of the personal access token'
             optional :expires_at, type: Date, desc: 'The expiration date in the format YEAR-MONTH-DAY of the personal access token'
             optional :scopes, type: Array, desc: 'The array of scopes of the personal access token'
-            optional :impersonation, type: Boolean, default: false, desc: 'The impersonation flag of the personal access token'
+            optional :impersonation, type: Boolean, desc: 'The impersonation flag of the personal access token'
           end
           post do
-            user = find_user(params)
-            personal_access_token = PersonalAccessTokensFinder.new(user).execute.build(declared_params(include_missing: false))
+            personal_access_token = @finder.execute.build(declared_params(include_missing: false))
             if personal_access_token.save
               present personal_access_token, with: Entities::ImpersonationToken
@@ -415,12 +419,9 @@ module API
           end
           params do
             requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
-            optional :impersonation, type: Boolean, default: false, desc: 'The impersonation flag of the personal access token'
           end
           get ':personal_access_token_id' do
-            user = find_user(params)
-
-            personal_access_token = PersonalAccessTokensFinder.new(user, declared_params(include_missing: false)).execute
+            personal_access_token = @finder.execute(id: declared_params[:personal_access_token_id])
             not_found!('Personal Access Token') unless personal_access_token
             present personal_access_token, with: Entities::ImpersonationToken
@@ -431,17 +432,12 @@ module API
           end
           params do
             requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
-            optional :impersonation, type: Boolean, default: false, desc: 'The impersonation flag of the personal access token'
           end
           delete ':personal_access_token_id' do
-            user = find_user(params)
-
-            personal_access_token = PersonalAccessTokensFinder.new(user, declared_params(include_missing: false)).execute
+            personal_access_token = @finder.execute(id: declared_params[:personal_access_token_id])
             not_found!('Personal Access Token') unless personal_access_token
             personal_access_token.revoke!
-
-            no_content!
           end
         end
       end

lib/gitlab/auth.rb

@@ -105,7 +105,7 @@ module Gitlab
       def personal_access_token_check(password)
         return unless password.present?
-        token = PersonalAccessToken.with_impersonation_tokens.active.find_by_token(password)
+        token = PersonalAccessTokensFinder.new(state: 'active').execute(token: password)
         if token && valid_api_token?(token)
           Gitlab::Auth::Result.new(token.user, nil, :personal_token, full_authentication_abilities)

spec/features/admin/admin_users_impersonation_tokens_spec.rb

@@ -4,24 +4,14 @@ describe 'Admin > Users > Impersonation Tokens', feature: true, js: true do
   let(:admin) { create(:admin) }
   let!(:user) { create(:user) }
-  def active_personal_access_tokens
+  def active_impersonation_tokens
     find(".table.active-impersonation-tokens")
   end
-  def inactive_personal_access_tokens
+  def inactive_impersonation_tokens
     find(".table.inactive-impersonation-tokens")
   end
-  def created_personal_access_token
-    find("#created-impersonation-token").value
-  end
-
-  def disallow_personal_access_token_saves!
-    allow_any_instance_of(PersonalAccessToken).to receive(:save).and_return(false)
-    errors = ActiveModel::Errors.new(PersonalAccessToken.new).tap { |e| e.add(:name, "cannot be nil") }
-    allow_any_instance_of(PersonalAccessToken).to receive(:errors).and_return(errors)
-  end
-
   before { login_as(admin) }
   describe "token creation" do
@@ -40,44 +30,43 @@ describe 'Admin > Users > Impersonation Tokens', feature: true, js: true do
       check "api"
       check "read_user"
-      expect { click_on "Create Impersonation Token" }.to change { PersonalAccessToken.impersonation.count }
-      expect(active_personal_access_tokens).to have_text(name)
-      expect(active_personal_access_tokens).to have_text('In')
-      expect(active_personal_access_tokens).to have_text('api')
-      expect(active_personal_access_tokens).to have_text('read_user')
+      expect { click_on "Create Impersonation Token" }.to change { PersonalAccessToken.with_impersonation.count }
+      expect(active_impersonation_tokens).to have_text(name)
+      expect(active_impersonation_tokens).to have_text('In')
+      expect(active_impersonation_tokens).to have_text('api')
+      expect(active_impersonation_tokens).to have_text('read_user')
+    end
+  end
+
+  describe 'active tokens' do
+    let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user) }
+    let!(:personal_access_token) { create(:personal_access_token, user: user) }
+
+    it 'only shows impersonation tokens' do
+      visit admin_user_impersonation_tokens_path(user_id: user.username)
+
+      expect(active_impersonation_tokens).to have_text(impersonation_token.name)
+      expect(active_impersonation_tokens).not_to have_text(personal_access_token.name)
     end
   end
   describe "inactive tokens" do
-    let!(:personal_access_token) { create(:impersonation_personal_access_token, user: user) }
+    let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user) }
     it "allows revocation of an active impersonation token" do
       visit admin_user_impersonation_tokens_path(user_id: user.username)
       click_on "Revoke"
-      expect(inactive_personal_access_tokens).to have_text(personal_access_token.name)
+      expect(inactive_impersonation_tokens).to have_text(impersonation_token.name)
     end
     it "moves expired tokens to the 'inactive' section" do
-      personal_access_token.update(expires_at: 5.days.ago)
+      impersonation_token.update(expires_at: 5.days.ago)
       visit admin_user_impersonation_tokens_path(user_id: user.username)
-      expect(inactive_personal_access_tokens).to have_text(personal_access_token.name)
-    end
-
-    context "when revocation fails" do
-      before { disallow_personal_access_token_saves! }
-
-      it "displays an error message" do
-        visit admin_user_impersonation_tokens_path(user_id: user.username)
-
-        click_on "Revoke"
-
-        expect(active_personal_access_tokens).to have_text(personal_access_token.name)
-        expect(page).to have_content("Could not revoke")
-      end
+      expect(inactive_impersonation_tokens).to have_text(impersonation_token.name)
     end
   end
 end

spec/finders/personal_access_tokens_finder_spec.rb

+require 'spec_helper'
+
+describe PersonalAccessTokensFinder do
+  describe '#execute' do
+    let(:user) { create(:user) }
+    let!(:active_personal_access_token) { create(:personal_access_token, user: user) }
+    let!(:expired_personal_access_token) { create(:expired_personal_access_token, user: user) }
+    let!(:revoked_personal_access_token) { create(:revoked_personal_access_token, user: user) }
+    let!(:active_impersonation_token) { create(:impersonation_personal_access_token, user: user, impersonation: true) }
+    let!(:expired_impersonation_token) { create(:expired_personal_access_token, user: user, impersonation: true) }
+    let!(:revoked_impersonation_token) { create(:revoked_personal_access_token, user: user, impersonation: true) }
+
+    subject { finder.execute }
+
+    describe 'without user' do
+      let(:finder) { described_class.new }
+
+      it do
+        is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token,
+          revoked_personal_access_token, expired_personal_access_token,
+          revoked_impersonation_token, expired_impersonation_token)
+      end
+
+      describe 'without impersonation' do
+        before { finder.params.merge!(impersonation: false) }
+
+        it { is_expected.to contain_exactly(active_personal_access_token, revoked_personal_access_token, expired_personal_access_token) }
+
+        describe 'with active state' do
+          before { finder.params.merge!(state: 'active') }
+
+          it { is_expected.to contain_exactly(active_personal_access_token) }
+        end
+
+        describe 'with inactive state' do
+          before { finder.params.merge!(state: 'inactive') }
+
+          it { is_expected.to contain_exactly(revoked_personal_access_token, expired_personal_access_token) }
+        end
+      end
+
+      describe 'with impersonation' do
+        before { finder.params.merge!(impersonation: true) }
+
+        it { is_expected.to contain_exactly(active_impersonation_token, revoked_impersonation_token, expired_impersonation_token) }
+
+        describe 'with active state' do
+          before { finder.params.merge!(state: 'active') }
+
+          it { is_expected.to contain_exactly(active_impersonation_token) }
+        end
+
+        describe 'with inactive state' do
+          before { finder.params.merge!(state: 'inactive') }
+
+          it { is_expected.to contain_exactly(revoked_impersonation_token, expired_impersonation_token) }
+        end
+      end
+
+      describe 'with active state' do
+        before { finder.params.merge!(state: 'active') }
+
+        it { is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token) }
+      end
+
+      describe 'with inactive state' do
+        before { finder.params.merge!(state: 'inactive') }
+
+        it do
+          is_expected.to contain_exactly(expired_personal_access_token, revoked_personal_access_token,
+            expired_impersonation_token, revoked_impersonation_token)
+        end
+      end
+
+      describe 'with id' do
+        subject { finder.execute(id: active_personal_access_token.id) }
+
+        it { is_expected.to eq(active_personal_access_token) }
+
+        describe 'with impersonation' do
+          before { finder.params.merge!(impersonation: true) }
+
+          it { is_expected.to be_nil }
+        end
+      end
+
+      describe 'with token' do
+        subject { finder.execute(token: active_personal_access_token.token) }
+
+        it { is_expected.to eq(active_personal_access_token) }
+
+        describe 'with impersonation' do
+          before { finder.params.merge!(impersonation: true) }
+
+          it { is_expected.to be_nil }
+        end
+      end
+    end
+
+    describe 'with user' do
+      let(:user2) { create(:user) }
+      let(:finder) { described_class.new(user: user) }
+      let!(:other_user_active_personal_access_token) { create(:personal_access_token, user: user2) }
+      let!(:other_user_expired_personal_access_token) { create(:expired_personal_access_token, user: user2) }
+      let!(:other_user_revoked_personal_access_token) { create(:revoked_personal_access_token, user: user2) }
+      let!(:other_user_active_impersonation_token) { create(:impersonation_personal_access_token, user: user2, impersonation: true) }
+      let!(:other_user_expired_impersonation_token) { create(:expired_personal_access_token, user: user2, impersonation: true) }
+      let!(:other_user_revoked_impersonation_token) { create(:revoked_personal_access_token, user: user2, impersonation: true) }
+
+      it do
+        is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token,
+          revoked_personal_access_token, expired_personal_access_token,
+          revoked_impersonation_token, expired_impersonation_token)
+      end
+
+      describe 'without impersonation' do
+        before { finder.params.merge!(impersonation: false) }
+
+        it { is_expected.to contain_exactly(active_personal_access_token, revoked_personal_access_token, expired_personal_access_token) }
+
+        describe 'with active state' do
+          before { finder.params.merge!(state: 'active') }
+
+          it { is_expected.to contain_exactly(active_personal_access_token) }
+        end
+
+        describe 'with inactive state' do
+          before { finder.params.merge!(state: 'inactive') }
+
+          it { is_expected.to contain_exactly(revoked_personal_access_token, expired_personal_access_token) }
+        end
+      end
+
+      describe 'with impersonation' do
+        before { finder.params.merge!(impersonation: true) }
+
+        it { is_expected.to contain_exactly(active_impersonation_token, revoked_impersonation_token, expired_impersonation_token) }
+
+        describe 'with active state' do
+          before { finder.params.merge!(state: 'active') }
+
+          it { is_expected.to contain_exactly(active_impersonation_token) }
+        end
+
+        describe 'with inactive state' do
+          before { finder.params.merge!(state: 'inactive') }
+
+          it { is_expected.to contain_exactly(revoked_impersonation_token, expired_impersonation_token) }
+        end
+      end
+
+      describe 'with active state' do
+        before { finder.params.merge!(state: 'active') }
+
+        it { is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token) }
+      end
+
+      describe 'with inactive state' do
+        before { finder.params.merge!(state: 'inactive') }
+
+        it do
+          is_expected.to contain_exactly(expired_personal_access_token, revoked_personal_access_token,
+            expired_impersonation_token, revoked_impersonation_token)
+        end
+      end
+
+      describe 'with id' do
+        subject { finder.execute(id: active_personal_access_token.id) }
+
+        it { is_expected.to eq(active_personal_access_token) }
+
+        describe 'with impersonation' do
+          before { finder.params.merge!(impersonation: true) }
+
+          it { is_expected.to be_nil }
+        end
+      end
+
+      describe 'with token' do
+        subject { finder.execute(token: active_personal_access_token.token) }
+
+        it { is_expected.to eq(active_personal_access_token) }
+
+        describe 'with impersonation' do
+          before { finder.params.merge!(impersonation: true) }
+
+          it { is_expected.to be_nil }
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/auth_spec.rb

@@ -118,10 +118,10 @@ describe Gitlab::Auth, lib: true do
       end
       it 'succeeds if it is an impersonation token' do
-        personal_access_token = create(:personal_access_token, impersonation: true, scopes: [])
+        impersonation_token = create(:personal_access_token, impersonation: true, scopes: ['api'])
         expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '')
-        expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(personal_access_token.user, nil, :personal_token, full_authentication_abilities))
+        expect(gl_auth.find_for_git_client('', impersonation_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(impersonation_token.user, nil, :personal_token, full_authentication_abilities))
       end
       it 'fails for personal access tokens with other scopes' do
@@ -131,6 +131,13 @@ describe Gitlab::Auth, lib: true do
         expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil))
       end
+      it 'fails for impersonation token with other scopes' do
+        impersonation_token = create(:personal_access_token, scopes: ['read_user'])
+
+        expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
+        expect(gl_auth.find_for_git_client('', impersonation_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil))
+      end
+
       it 'fails if password is nil' do
         expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '')
         expect(gl_auth.find_for_git_client('', nil, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil))

spec/models/personal_access_token_spec.rb

@@ -16,6 +16,7 @@ describe PersonalAccessToken, models: true do
       expect(invalid_personal_access_token.token).not_to be_nil
     end
   end
+
   describe ".active?" do
     let(:active_personal_access_token) { build(:personal_access_token) }
     let(:revoked_personal_access_token) { build(:revoked_personal_access_token) }

spec/requests/api/personal_access_tokens_spec.rb

@@ -5,8 +5,10 @@ describe API::PersonalAccessTokens, api: true  do
   let(:user)  { create(:user) }
   let(:not_found_token) { (PersonalAccessToken.maximum('id') || 0) + 10 }
+  let(:finder) { PersonalAccessTokensFinder.new(user: user, impersonation: false) }
   describe "GET /personal_access_tokens" do
+    let!(:active_impersonation_token) { create(:impersonation_personal_access_token, user: user) }
     let!(:active_personal_access_token) { create(:personal_access_token, user: user) }
     let!(:revoked_personal_access_token) { create(:revoked_personal_access_token, user: user) }
     let!(:expired_personal_access_token) { create(:expired_personal_access_token, user: user) }
@@ -16,7 +18,7 @@ describe API::PersonalAccessTokens, api: true  do
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
-      expect(json_response.size).to eq(user.personal_access_tokens.count)
+      expect(json_response.size).to eq(finder.execute.count)
       json_personal_access_token = json_response.detect do |personal_access_token|
         personal_access_token['id'] == active_personal_access_token.id
@@ -27,18 +29,24 @@ describe API::PersonalAccessTokens, api: true  do
     end
     it 'returns an array of active personal access tokens if active is set to true' do
+      finder.params[:state] = 'active'
+
       get api("/personal_access_tokens?state=active", user)
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
+      expect(json_response.size).to eq(finder.execute.count)
       expect(json_response).to all(include('active' => true))
     end
     it 'returns an array of inactive personal access tokens if active is set to false' do
+      finder.params[:state] = 'inactive'
+
       get api("/personal_access_tokens?state=inactive", user)
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
+      expect(json_response.size).to eq(finder.execute.count)
       expect(json_response).to all(include('active' => false))
     end
   end
@@ -46,7 +54,7 @@ describe API::PersonalAccessTokens, api: true  do
   describe 'POST /personal_access_tokens' do
     let(:name) { 'my new pat' }
     let(:expires_at) { '2016-12-28' }
-    let(:scopes) { ['api', 'read_user'] }
+    let(:scopes) { %w(api read_user) }
     it 'returns validation error if personal access token miss some attributes' do
       post api("/personal_access_tokens", user)
@@ -73,7 +81,8 @@ describe API::PersonalAccessTokens, api: true  do
       expect(json_response['active']).to eq(false)
       expect(json_response['revoked']).to eq(false)
       expect(json_response['token']).to be_present
-      expect(PersonalAccessToken.find(personal_access_token_id)).not_to be_nil
+      expect(json_response['impersonation']).not_to be_present
+      expect(finder.execute(id: personal_access_token_id)).not_to be_nil
     end
   end

spec/requests/api/users_spec.rb

@@ -12,6 +12,7 @@ describe API::Users, api: true  do
   let(:ldap_blocked_user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') }
   let(:not_existing_user_id) { (User.maximum('id') || 0 ) + 10 }
   let(:not_existing_pat_id) { (PersonalAccessToken.maximum('id') || 0 ) + 10 }
+  let(:finder) { PersonalAccessTokensFinder.new(user: user) }
   describe "GET /users" do
     context "when unauthenticated" do
@@ -1178,41 +1179,60 @@ describe API::Users, api: true  do
       expect(json_response['message']).to eq('403 Forbidden')
     end
-    it 'returns an array of non impersonated personal access tokens' do
+    it 'returns an array of all impersonated and non-impersonated tokens' do
       get api("/users/#{user.id}/personal_access_tokens", admin)
       expect(response).to have_http_status(200)
+      expect(response).to include_pagination_headers
       expect(json_response).to be_an Array
-      expect(json_response.size).to eq(user.personal_access_tokens.count)
+      expect(json_response.size).to eq(finder.execute.count)
+    end
+
+    it 'returns an array of non impersonated personal access tokens' do
+      finder.params[:impersonation] = false
+
+      get api("/users/#{user.id}/personal_access_tokens?impersonation=false", admin)
+
+      expect(response).to have_http_status(200)
+      expect(response).to include_pagination_headers
+      expect(json_response).to be_an Array
+      expect(json_response.size).to eq(finder.execute.count)
       expect(json_response.detect do |personal_access_token|
         personal_access_token['id'] == active_personal_access_token.id
       end['token']).to eq(active_personal_access_token.token)
     end
     it 'returns an array of active personal access tokens if active is set to true' do
-      get api("/users/#{user.id}/personal_access_tokens?state=active", admin)
+      finder.params.merge!(state: 'active', impersonation: false)
+
+      get api("/users/#{user.id}/personal_access_tokens?state=active&impersonation=false", admin)
       expect(response).to have_http_status(200)
+      expect(response).to include_pagination_headers
       expect(json_response).to be_an Array
-      expect(json_response.size).to eq(user.personal_access_tokens.active.count)
+      expect(json_response.size).to eq(finder.execute.count)
       expect(json_response).to all(include('active' => true))
     end
     it 'returns an array of inactive personal access tokens if active is set to false' do
-      get api("/users/#{user.id}/personal_access_tokens?state=inactive", admin)
+      finder.params.merge!(state: 'inactive', impersonation: false)
+      get api("/users/#{user.id}/personal_access_tokens?impersonation=false&state=inactive", admin)
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
-      expect(json_response.size).to eq(user.personal_access_tokens.inactive.count)
+      expect(json_response.size).to eq(finder.execute.count)
       expect(json_response).to all(include('active' => false))
     end
     it 'returns an array of impersonation personal access tokens if impersonation is set to true' do
+      finder.params[:impersonation] = true
+
       get api("/users/#{user.id}/personal_access_tokens?impersonation=true", admin)
       expect(response).to have_http_status(200)
+      expect(response).to include_pagination_headers
       expect(json_response).to be_an Array
-      expect(json_response.size).to eq(user.personal_access_tokens.impersonation.count)
+      expect(json_response.size).to eq(finder.execute.count)
       expect(json_response).to all(include('impersonation' => true))
     end
   end
@@ -1220,7 +1240,7 @@ describe API::Users, api: true  do
   describe 'POST /users/:id/personal_access_tokens' do
     let(:name) { 'my new pat' }
     let(:expires_at) { '2016-12-28' }
-    let(:scopes) { ['api', 'read_user'] }
+    let(:scopes) { %w(api read_user) }
     let(:impersonation) { true }
     it 'returns validation error if personal access token misses some attributes' do
@@ -1265,7 +1285,7 @@ describe API::Users, api: true  do
       expect(json_response['revoked']).to be_falsey
       expect(json_response['token']).to be_present
       expect(json_response['impersonation']).to eq(impersonation)
-      expect(PersonalAccessToken.with_impersonation_tokens.find(json_response['id'])).not_to be_nil
+      expect(finder.execute(id: json_response['id'])).not_to be_nil
     end
   end
@@ -1301,19 +1321,6 @@ describe API::Users, api: true  do
       expect(json_response['token']).to be_present
       expect(json_response['impersonation']).to be_falsey
     end
-
-    it 'does not return an impersonation token without the specified field' do
-      get api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}", admin)
-
-      expect(response).to have_http_status(404)
-    end
-
-    it 'returns an impersonation token' do
-      get api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}?impersonation=true", admin)
-
-      expect(response).to have_http_status(200)
-      expect(json_response['impersonation']).to be_truthy
-    end
   end
   describe 'DELETE /users/:id/personal_access_tokens/:personal_access_token_id' do
@@ -1348,21 +1355,5 @@ describe API::Users, api: true  do
       expect(personal_access_token.revoked).to be_falsey
       expect(personal_access_token.reload.revoked).to be_truthy
     end
-
-    it 'does not find impersonated token without specified field' do
-      delete api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}", admin)
-
-      expect(response).to have_http_status(404)
-      expect(impersonation_token.revoked).to be_falsey
-      expect(impersonation_token.reload.revoked).to be_falsey
-    end
-
-    it 'revokes an impersonation token' do
-      delete api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}?impersonation=true", admin)
-
-      expect(response).to have_http_status(204)
-      expect(impersonation_token.revoked).to be_falsey
-      expect(impersonation_token.reload.revoked).to be_truthy
-    end
   end
 end