applies relevant changes to the code and code structure

24 files changed, 317 insertions, 279 deletions

diff --git a/app/assets/stylesheets/pages/settings.scss b/app/assets/stylesheets/pages/settings.scss
index 905ecbff57c..4a8e4344851 100644
--- a/app/assets/stylesheets/pages/settings.scss
+++ b/app/assets/stylesheets/pages/settings.scss
@@ -25,8 +25,8 @@
   padding-top: 0;
 }
 
-.personal-access-token-token-container {
-  #personal-access-token-token {
+.impersonation-token-token-container {
+  #impersonation-token-token {
     width: 80%;
     display: inline;
   }
diff --git a/app/controllers/admin/impersonation_tokens_controller.rb b/app/controllers/admin/impersonation_tokens_controller.rb
new file mode 100644
index 00000000000..43e718bcc97
--- /dev/null
+++ b/app/controllers/admin/impersonation_tokens_controller.rb
@@ -0,0 +1,49 @@
+class Admin::ImpersonationTokensController < Admin::ApplicationController
+  before_action :user
+
+  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))
+
+    if @impersonation_token.save
+      flash[:impersonation_token] = @impersonation_token.token
+      redirect_to admin_user_impersonation_tokens_path, notice: "A new impersonation token has been created."
+    else
+      set_index_vars
+      render :index
+    end
+  end
+
+  def revoke
+    @impersonation_token = user.personal_access_tokens.impersonation.find(params[:id])
+
+    if @impersonation_token.revoke!
+      flash[:notice] = "Revoked impersonation token #{@impersonation_token.name}!"
+    else
+      flash[:alert] = "Could not revoke impersonation token #{@impersonation_token.name}."
+    end
+
+    redirect_to admin_user_impersonation_tokens_path
+  end
+
+  private
+
+  def user
+    @user ||= User.find_by!(username: params[:user_id])
+  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
+    @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
+  end
+end
diff --git a/app/controllers/admin/personal_access_tokens_controller.rb b/app/controllers/admin/personal_access_tokens_controller.rb
deleted file mode 100644
index f32a4160433..00000000000
--- a/app/controllers/admin/personal_access_tokens_controller.rb
+++ /dev/null
@@ -1,49 +0,0 @@
-class Admin::PersonalAccessTokensController < Admin::ApplicationController
-  before_action :user
-
-  def index
-    set_index_vars
-  end
-
-  def create
-    # We never want to non-impersonate a user
-    @personal_access_token = user.personal_access_tokens.generate(personal_access_token_params.merge(impersonation: true))
-
-    if @personal_access_token.save
-      flash[:personal_access_token] = @personal_access_token.token
-      redirect_to admin_user_personal_access_tokens_path, notice: "A new personal access token has been created."
-    else
-      set_index_vars
-      render :index
-    end
-  end
-
-  def revoke
-    @personal_access_token = user.personal_access_tokens.find(params[:id])
-
-    if @personal_access_token.revoke!
-      flash[:notice] = "Revoked personal access token #{@personal_access_token.name}!"
-    else
-      flash[:alert] = "Could not revoke personal access token #{@personal_access_token.name}."
-    end
-
-    redirect_to admin_user_personal_access_tokens_path
-  end
-
-  private
-
-  def user
-    @user ||= User.find_by!(username: params[:user_id])
-  end
-
-  def personal_access_token_params
-    params.require(:personal_access_token).permit(:name, :expires_at, :impersonation, scopes: [])
-  end
-
-  def set_index_vars
-    @personal_access_token ||= user.personal_access_tokens.build
-    @scopes = Gitlab::Auth::SCOPES
-    @active_personal_access_tokens = PersonalAccessToken.and_impersonation_tokens.where(user_id: user.id).active.order(:expires_at)
-    @inactive_personal_access_tokens = PersonalAccessToken.and_impersonation_tokens.where(user_id: user.id).inactive
-  end
-end
diff --git a/app/controllers/profiles/personal_access_tokens_controller.rb b/app/controllers/profiles/personal_access_tokens_controller.rb
index 6e007f17913..a7eca876c2f 100644
--- a/app/controllers/profiles/personal_access_tokens_controller.rb
+++ b/app/controllers/profiles/personal_access_tokens_controller.rb
@@ -4,7 +4,7 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
   end
 
   def create
-    @personal_access_token = current_user.personal_access_tokens.generate(personal_access_token_params)
+    @personal_access_token = current_user.personal_access_tokens.build(personal_access_token_params)
 
     if @personal_access_token.save
       flash[:personal_access_token] = @personal_access_token.token
diff --git a/app/finders/personal_access_tokens_finder.rb b/app/finders/personal_access_tokens_finder.rb
new file mode 100644
index 00000000000..ad3f7f4a437
--- /dev/null
+++ b/app/finders/personal_access_tokens_finder.rb
@@ -0,0 +1,41 @@
+class PersonalAccessTokensFinder
+  def initialize(user, params = {})
+    @user = user
+    @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
+  end
+
+  private
+
+  def state?
+    @params[:state].presence
+  end
+
+  def impersonation?
+    @params[:impersonation].presence
+  end
+
+  def token_id?
+    @params[:personal_access_token_id].presence
+  end
+
+  def find_token_by_id(personal_access_tokens, pat_id)
+    personal_access_tokens.find_by(id: pat_id)
+  end
+end
diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb
index cf94b01c33e..676e0832d54 100644
--- a/app/models/personal_access_token.rb
+++ b/app/models/personal_access_token.rb
@@ -7,20 +7,13 @@ class PersonalAccessToken < ActiveRecord::Base
 
   belongs_to :user
 
+  before_save :ensure_token
+
   default_scope { where(impersonation: false) }
   scope :active, -> { where(revoked: false).where("expires_at >= NOW() OR expires_at IS NULL") }
   scope :inactive, -> { where("revoked = true OR expires_at < NOW()") }
-  scope :impersonation, -> { where(impersonation: true) }
-
-  class << self
-    alias_method :and_impersonation_tokens, :unscoped
-
-    def generate(params)
-      personal_access_token = self.new(params)
-      personal_access_token.ensure_token
-      personal_access_token
-    end
-  end
+  scope :impersonation, -> { unscoped.where(impersonation: true) }
+  scope :with_impersonation_tokens, ->  { unscoped }
 
   def revoke!
     self.revoked = true
diff --git a/app/views/admin/impersonation_tokens/index.html.haml b/app/views/admin/impersonation_tokens/index.html.haml
new file mode 100644
index 00000000000..9116384e322
--- /dev/null
+++ b/app/views/admin/impersonation_tokens/index.html.haml
@@ -0,0 +1,65 @@
+- page_title "Impersonation Tokens", @user.name, "Users"
+= render 'admin/users/head'
+
+.row.prepend-top-default
+  .col-lg-12
+    %h5.prepend-top-0
+      Add a Impersonation Token
+    %p.profile-settings-content
+      Pick a name for the application, and we'll give the respective user a unique token.
+    = render "shared/personal_access_tokens_form", path: admin_user_impersonation_tokens_path, impersonation: true, personal_access_token: @impersonation_token, scopes: @scopes
+
+    %hr
+
+    %h5 Active Impersonation Tokens (#{@active_impersonation_tokens.length})
+    %p.profile-settings-content
+      To see all the user's personal access tokens you must impersonate first
+    - if @active_impersonation_tokens.present?
+      .table-responsive
+        %table.table.active-impersonation-tokens
+          %thead
+            %tr
+              %th Name
+              %th Created
+              %th Expires
+              %th Scopes
+              %th Token
+              %th
+          %tbody
+            - @active_impersonation_tokens.each do |impersonation_token|
+              %tr
+                %td= impersonation_token.name
+                %td= impersonation_token.created_at.to_date.to_s(:medium)
+                %td
+                  - if impersonation_token.expires?
+                    %span{ class: ('text-warning' if impersonation_token.expires_soon?) }
+                      In #{distance_of_time_in_words_to_now(impersonation_token.expires_at)}
+                  - else
+                    %span.impersonation_tokens-never-expires-label Never
+                %td= impersonation_token.scopes.present? ? impersonation_token.scopes.join(", ") : "<no scopes selected>"
+                %td.impersonation-token-token-container
+                  = text_field_tag 'impersonation-token-token', impersonation_token.token, readonly: true, class: "form-control"
+                  = clipboard_button(clipboard_text: impersonation_token.token)
+                %td= link_to "Revoke", revoke_admin_user_impersonation_token_path(id: impersonation_token.id, user_id: impersonation_token.user.username), method: :put, class: "btn btn-danger pull-right", data: { confirm: "Are you sure you want to revoke this impersonation token? This action cannot be undone." }
+    - else
+      .settings-message.text-center
+        This user has no active impersonation tokens.
+
+    %hr
+
+    %h5 Inactive Impersonation Tokens (#{@inactive_impersonation_tokens.length})
+    - if @inactive_impersonation_tokens.present?
+      .table-responsive
+        %table.table.inactive-impersonation-tokens
+          %thead
+            %tr
+              %th Name
+              %th Created
+          %tbody
+            - @inactive_impersonation_tokens.each do |token|
+              %tr
+                %td= token.name
+                %td= token.created_at.to_date.to_s(:medium)
+    - else
+      .settings-message.text-center
+        This user has no inactive impersonation tokens.
diff --git a/app/views/admin/personal_access_tokens/index.html.haml b/app/views/admin/personal_access_tokens/index.html.haml
deleted file mode 100644
index c4646afcee3..00000000000
--- a/app/views/admin/personal_access_tokens/index.html.haml
+++ /dev/null
@@ -1,79 +0,0 @@
-- page_title "Personal Access Tokens"
-= render 'admin/users/head'
-
-.row.prepend-top-default
-  .col-lg-12
-    %h5.prepend-top-0
-      Add a Personal Access Token
-    %p.profile-settings-content
-      Pick a name for the application, and we'll give you a unique token.
-    = render "profiles/personal_access_tokens/form", user: :admin_user, personal_access_token: @personal_access_token, scopes: @scopes
-
-    %hr
-
-    %h5 Active Personal Access Tokens (#{@active_personal_access_tokens.length})
-    - if @active_personal_access_tokens.present?
-      .table-responsive
-        %table.table.active-personal-access-tokens
-          %thead
-            %tr
-              %th Name
-              %th Created
-              %th Expires
-              %th Scopes
-              %th Token
-              %th Impersonation
-              %th
-          %tbody
-            - @active_personal_access_tokens.each do |personal_access_token|
-              %tr
-                %td= personal_access_token.name
-                %td= personal_access_token.created_at.to_date.to_s(:medium)
-                %td
-                  - if personal_access_token.expires?
-                    %span{ class: ('text-warning' if personal_access_token.expires_soon?) }
-                      In #{distance_of_time_in_words_to_now(personal_access_token.expires_at)}
-                  - else
-                    %span.personal-access-personal_access_tokens-never-expires-label Never
-                %td= personal_access_token.scopes.present? ? personal_access_token.scopes.join(", ") : "<no scopes selected>"
-                %td.personal-access-token-token-container
-                  = text_field_tag 'personal-access-token-token', personal_access_token.token, readonly: true, class: "form-control"
-                  = clipboard_button(clipboard_text: personal_access_token.token)
-                %td= personal_access_token.impersonation
-                %td= link_to "Revoke", revoke_admin_user_personal_access_token_path(id: personal_access_token.id, user_id: personal_access_token.user.username), method: :put, class: "btn btn-danger pull-right", data: { confirm: "Are you sure you want to revoke this token? This action cannot be undone." }
-    - else
-      .settings-message.text-center
-        This user has no active tokens.
-
-    %hr
-
-    %h5 Inactive Personal Access Tokens (#{@inactive_personal_access_tokens.length})
-    - if @inactive_personal_access_tokens.present?
-      .table-responsive
-        %table.table.inactive-personal-access-tokens
-          %thead
-            %tr
-              %th Name
-              %th Created
-          %tbody
-            - @inactive_personal_access_tokens.each do |token|
-              %tr
-                %td= token.name
-                %td= token.created_at.to_date.to_s(:medium)
-    - else
-      .settings-message.text-center
-        This user has no inactive tokens.
-
-: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'));
-    }
-  });
diff --git a/app/views/admin/users/_head.html.haml b/app/views/admin/users/_head.html.haml
index c95ae93b710..1ded8fa6086 100644
--- a/app/views/admin/users/_head.html.haml
+++ b/app/views/admin/users/_head.html.haml
@@ -21,6 +21,6 @@
     = link_to "SSH keys", keys_admin_user_path(@user)
   = nav_link(controller: :identities) do
     = link_to "Identities", admin_user_identities_path(@user)
-  = nav_link(controller: :personal_access_tokens) do
-    = link_to "Access Tokens", admin_user_personal_access_tokens_path(@user)
+  = nav_link(controller: :impersonation_tokens) do
+    = link_to "Access Tokens", admin_user_impersonation_tokens_path(@user)
 .append-bottom-default
diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml
index c74cc1b6906..8d5008642c9 100644
--- a/app/views/profiles/personal_access_tokens/index.html.haml
+++ b/app/views/profiles/personal_access_tokens/index.html.haml
@@ -29,7 +29,7 @@
     %p.profile-settings-content
       Pick a name for the application, and we'll give you a unique token.
 
-    = render "form", user: :profile, personal_access_token: @personal_access_token, scopes: @scopes
+    = render "shared/personal_access_tokens_form", path: profile_personal_access_tokens_path, personal_access_token: @personal_access_token, scopes: @scopes
 
     %hr
 
diff --git a/app/views/profiles/personal_access_tokens/_form.html.haml b/app/views/shared/_personal_access_tokens_form.html.haml
index 286d35d1f3b..546e90a1d62 100644
--- a/app/views/profiles/personal_access_tokens/_form.html.haml
+++ b/app/views/shared/_personal_access_tokens_form.html.haml
@@ -1,7 +1,8 @@
+- impersonation = impersonation || false
 - personal_access_token = local_assigns.fetch(:personal_access_token)
 - scopes = local_assigns.fetch(:scopes)
 
-= form_for [user, personal_access_token], method: :post, html: { class: 'js-requires-input' } do |f|
+= form_for personal_access_token, url: path, method: :post, html: { class: 'js-requires-input' } do |f|
 
   = form_errors(personal_access_token)
 
@@ -18,4 +19,19 @@
     = render 'shared/tokens/scopes_form', prefix: 'personal_access_token', token: personal_access_token, scopes: scopes
 
   .prepend-top-default
-    = f.submit 'Create Personal Access Token', class: "btn btn-create"
+    - type = impersonation ? "Impersonation" : "Personal Access"
+    = f.submit "Create #{type} Token", class: "btn btn-create"
+
+:javascript
+  var $dateField = $('.datepicker');
+  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'));
+    }
+  });
diff --git a/changelogs/unreleased/25367-add-impersonation-token.yml b/changelogs/unreleased/25367-add-impersonation-token.yml
index efd5c0488e1..4a30f960036 100644
--- a/changelogs/unreleased/25367-add-impersonation-token.yml
+++ b/changelogs/unreleased/25367-add-impersonation-token.yml
@@ -1,4 +1,4 @@
 ---
 title: Manage user personal access tokens through api and add impersonation tokens
-merge_request: 8350
-author: Simon Vocella @voxsim
+merge_request: 9099
+author: Simon Vocella
diff --git a/config/routes/admin.rb b/config/routes/admin.rb
index 6d2748df386..486ce3c5c87 100644
--- a/config/routes/admin.rb
+++ b/config/routes/admin.rb
@@ -2,7 +2,7 @@ namespace :admin do
   resources :users, constraints: { id: /[a-zA-Z.\/0-9_\-]+/ } do
     resources :keys, only: [:show, :destroy]
     resources :identities, except: [:show]
-    resources :personal_access_tokens, only: [:index, :create] do
+    resources :impersonation_tokens, only: [:index, :create] do
       member do
         put :revoke
       end
diff --git a/db/migrate/20161228135550_add_impersonation_to_personal_access_tokens.rb b/db/migrate/20161228135550_add_impersonation_to_personal_access_tokens.rb
index c16d508e141..48648043dbb 100644
--- a/db/migrate/20161228135550_add_impersonation_to_personal_access_tokens.rb
+++ b/db/migrate/20161228135550_add_impersonation_to_personal_access_tokens.rb
@@ -9,7 +9,7 @@ class AddImpersonationToPersonalAccessTokens < ActiveRecord::Migration
   DOWNTIME = false
 
   def up
-    add_column_with_default :personal_access_tokens, :impersonation, :boolean, default: false
+    add_column_with_default :personal_access_tokens, :impersonation, :boolean, default: false, null: false
   end
 
   def down
diff --git a/doc/api/personal_access_tokens.md b/doc/api/personal_access_tokens.md
index 0fd04a0033d..81e6f10f0c6 100644
--- a/doc/api/personal_access_tokens.md
+++ b/doc/api/personal_access_tokens.md
@@ -52,6 +52,8 @@ Parameters:
 POST /personal_access_tokens
 ```
 
+It responds with the new personal access token for the current user.
+
 Parameters:
 
 | Attribute | Type | Required | Description |
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 4e8d2410496..54bcca25834 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -697,7 +697,7 @@ module API
       expose :active?, as: :active
     end
 
-    class BasicPersonalAccessToken < Grape::Entity
+    class PersonalAccessToken < Grape::Entity
       expose :id, :name, :revoked, :created_at, :scopes
       expose :active?, as: :active
       expose :expires_at do |personal_access_token|
@@ -705,9 +705,12 @@ module API
       end
     end
 
-    class PersonalAccessToken < BasicPersonalAccessToken
-      expose :impersonation
+    class PersonalAccessTokenWithToken < PersonalAccessToken
       expose :token
     end
+
+    class ImpersonationToken < PersonalAccessTokenWithToken
+      expose :impersonation
+    end
   end
 end
diff --git a/lib/api/personal_access_tokens.rb b/lib/api/personal_access_tokens.rb
index 7afb8eec14c..763888bb57e 100644
--- a/lib/api/personal_access_tokens.rb
+++ b/lib/api/personal_access_tokens.rb
@@ -5,41 +5,30 @@ module API
     resource :personal_access_tokens do
       desc 'Retrieve personal access tokens' do
         detail 'This feature was introduced in GitLab 9.0'
-        success Entities::BasicPersonalAccessToken
+        success Entities::PersonalAccessToken
       end
       params do
         optional :state, type: String, default: 'all', values: %w[all active inactive], desc: 'Filters (all|active|inactive) personal_access_tokens'
       end
-      get do
-        personal_access_tokens = current_user.personal_access_tokens
-
-        case params[:state]
-        when "active"
-          personal_access_tokens = personal_access_tokens.active
-        when "inactive"
-          personal_access_tokens = personal_access_tokens.inactive
-        end
-
-        present personal_access_tokens, with: Entities::BasicPersonalAccessToken
-      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'
-        success Entities::BasicPersonalAccessToken
+        success Entities::PersonalAccessToken
       end
       params do
         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 = PersonalAccessToken.find_by(id: params[:personal_access_token_id], user_id: current_user.id)
-        not_found!('PersonalAccessToken') unless personal_access_token
+        personal_access_token = PersonalAccessTokensFinder.new(current_user, declared_params(include_missing: false)).execute
+        not_found!('Personal Access Token') unless personal_access_token
 
-        present personal_access_token, with: Entities::BasicPersonalAccessToken
+        present personal_access_token, with: Entities::PersonalAccessToken
       end
 
       desc 'Create a personal access token' do
         detail 'This feature was introduced in GitLab 9.0'
-        success Entities::BasicPersonalAccessToken
+        success Entities::PersonalAccessTokenWithToken
       end
       params do
         requires :name, type: String, desc: 'The name of the personal access token'
@@ -47,13 +36,10 @@ module API
         optional :scopes, type: Array, desc: 'The array of scopes of the personal access token'
       end
       post do
-        parameters = declared_params(include_missing: false)
-        parameters[:user_id] = current_user.id
-
-        personal_access_token = PersonalAccessToken.generate(parameters)
+        personal_access_token = current_user.personal_access_tokens.build(declared_params(include_missing: false))
 
         if personal_access_token.save
-          present personal_access_token, with: Entities::PersonalAccessToken
+          present personal_access_token, with: Entities::PersonalAccessTokenWithToken
         else
           render_validation_error!(personal_access_token)
         end
@@ -61,14 +47,13 @@ module API
 
       desc 'Revoke a personal access token' do
         detail 'This feature was introduced in GitLab 9.0'
-        success Entities::BasicPersonalAccessToken
       end
       params do
         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 = PersonalAccessToken.find_by(id: params[:personal_access_token_id], user_id: current_user.id)
-        not_found!('PersonalAccessToken') unless personal_access_token
+        personal_access_token = PersonalAccessTokensFinder.new(current_user, declared_params(include_missing: false)).execute
+        not_found!('Personal Access Token') unless personal_access_token
 
         personal_access_token.revoke!
 
diff --git a/lib/api/users.rb b/lib/api/users.rb
index c302a6dd690..d29f6dde210 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -9,6 +9,11 @@ module API
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
       helpers do
+        def find_user(params)
+          user = User.find_by(id: params[:id])
+          user ? user : not_found!('User')
+        end
+
         params :optional_attributes do
           optional :skype, type: String, desc: 'The Skype username'
           optional :linkedin, type: String, desc: 'The LinkedIn username'
@@ -364,40 +369,28 @@ module API
       end
 
       params do
-        requires :user_id, type: Integer, desc: 'The ID of the user'
+        requires :id, type: Integer, desc: 'The ID of the user'
       end
-      segment ':user_id' do
+      segment ':id' do
         resource :personal_access_tokens do
           before { authenticated_as_admin! }
 
           desc 'Retrieve personal access tokens. Available only for admins.' do
             detail 'This feature was introduced in GitLab 9.0'
-            success Entities::PersonalAccessToken
+            success Entities::ImpersonationToken
           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'
           end
           get do
-            user = User.find_by(id: params[:user_id])
-            not_found!('User') unless user
-
-            personal_access_tokens = PersonalAccessToken.and_impersonation_tokens.where(user_id: user.id)
-            personal_access_tokens = personal_access_tokens.impersonation if params[:impersonation]
-
-            case params[:state]
-            when "active"
-              personal_access_tokens = personal_access_tokens.active
-            when "inactive"
-              personal_access_tokens = personal_access_tokens.inactive
-            end
-
-            present personal_access_tokens, with: Entities::PersonalAccessToken
+            user = find_user(params)
+            present PersonalAccessTokensFinder.new(user, params).execute, with: Entities::ImpersonationToken
           end
 
           desc 'Create a personal access token. Available only for admins.' do
             detail 'This feature was introduced in GitLab 9.0'
-            success Entities::PersonalAccessToken
+            success Entities::ImpersonationToken
           end
           params do
             requires :name, type: String, desc: 'The name of the personal access token'
@@ -406,13 +399,11 @@ module API
             optional :impersonation, type: Boolean, default: false, desc: 'The impersonation flag of the personal access token'
           end
           post do
-            user = User.find_by(id: params[:user_id])
-            not_found!('User') unless user
-
-            personal_access_token = PersonalAccessToken.generate(declared_params(include_missing: false, include_parent_namespaces: true))
+            user = find_user(params)
+            personal_access_token = PersonalAccessTokensFinder.new(user).execute.build(declared_params(include_missing: false))
 
             if personal_access_token.save
-              present personal_access_token, with: Entities::PersonalAccessToken
+              present personal_access_token, with: Entities::ImpersonationToken
             else
               render_validation_error!(personal_access_token)
             end
@@ -420,34 +411,33 @@ module API
 
           desc 'Retrieve personal access token. Available only for admins.' do
             detail 'This feature was introduced in GitLab 9.0'
-            success Entities::PersonalAccessToken
+            success Entities::ImpersonationToken
           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 = User.find_by(id: params[:user_id])
-            not_found!('User') unless user
+          get ':personal_access_token_id' do
+            user = find_user(params)
 
-            personal_access_token = PersonalAccessToken.and_impersonation_tokens.find_by(user_id: user.id, id: params[:personal_access_token_id])
-            not_found!('PersonalAccessToken') unless personal_access_token
+            personal_access_token = PersonalAccessTokensFinder.new(user, declared_params(include_missing: false)).execute
+            not_found!('Personal Access Token') unless personal_access_token
 
-            present personal_access_token, with: Entities::PersonalAccessToken
+            present personal_access_token, with: Entities::ImpersonationToken
           end
 
           desc 'Revoke a personal access token. Available only for admins.' do
             detail 'This feature was introduced in GitLab 9.0'
-            success Entities::PersonalAccessToken
           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 = User.find_by(id: params[:user_id])
-            not_found!('User') unless user
+          delete ':personal_access_token_id' do
+            user = find_user(params)
 
-            personal_access_token = PersonalAccessToken.and_impersonation_tokens.find_by(user_id: user.id, id: params[:personal_access_token_id])
-            not_found!('PersonalAccessToken') unless personal_access_token
+            personal_access_token = PersonalAccessTokensFinder.new(user, declared_params(include_missing: false)).execute
+            not_found!('Personal Access Token') unless personal_access_token
 
             personal_access_token.revoke!
 
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index e48462a4bd6..ef261d08b1d 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -105,9 +105,9 @@ module Gitlab
       def personal_access_token_check(password)
         return unless password.present?
 
-        token = PersonalAccessToken.and_impersonation_tokens.active.find_by_token(password)
+        token = PersonalAccessToken.with_impersonation_tokens.active.find_by_token(password)
 
-        if token && (valid_api_token?(token) || token.impersonation)
+        if token && valid_api_token?(token)
           Gitlab::Auth::Result.new(token.user, nil, :personal_token, full_authentication_abilities)
         end
       end
diff --git a/spec/features/admin/admin_users_personal_access_tokens_spec.rb b/spec/features/admin/admin_users_impersonation_tokens_spec.rb
index 772aeebf43f..c37cf1178df 100644
--- a/spec/features/admin/admin_users_personal_access_tokens_spec.rb
+++ b/spec/features/admin/admin_users_impersonation_tokens_spec.rb
@@ -1,19 +1,19 @@
 require 'spec_helper'
 
-describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
+describe 'Admin > Users > Impersonation Tokens', feature: true, js: true do
   let(:admin) { create(:admin) }
   let!(:user) { create(:user) }
 
   def active_personal_access_tokens
-    find(".table.active-personal-access-tokens")
+    find(".table.active-impersonation-tokens")
   end
 
   def inactive_personal_access_tokens
-    find(".table.inactive-personal-access-tokens")
+    find(".table.inactive-impersonation-tokens")
   end
 
   def created_personal_access_token
-    find("#created-personal-access-token").value
+    find("#created-impersonation-token").value
   end
 
   def disallow_personal_access_token_saves!
@@ -28,7 +28,7 @@ describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
     it "allows creation of a token" do
       name = FFaker::Product.brand
 
-      visit admin_user_personal_access_tokens_path(user_id: user.username)
+      visit admin_user_impersonation_tokens_path(user_id: user.username)
       fill_in "Name", with: name
 
       # Set date to 1st of next month
@@ -40,31 +40,20 @@ describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
       check "api"
       check "read_user"
 
-      click_on "Create Personal Access Token"
+      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(active_personal_access_tokens).to have_text('true')
-    end
-
-    context "when creation fails" do
-      it "displays an error message" do
-        disallow_personal_access_token_saves!
-        visit admin_user_personal_access_tokens_path(user_id: user.username)
-        fill_in "Name", with: FFaker::Product.brand
-
-        expect { click_on "Create Personal Access Token" }.not_to change { PersonalAccessToken.count }
-        expect(page).to have_content("Name cannot be nil")
-      end
     end
   end
 
   describe "inactive tokens" do
-    let!(:personal_access_token) { create(:personal_access_token, user: user) }
+    let!(:personal_access_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)
 
-    it "allows revocation of an active token" do
-      visit admin_user_personal_access_tokens_path(user_id: user.username)
       click_on "Revoke"
 
       expect(inactive_personal_access_tokens).to have_text(personal_access_token.name)
@@ -72,17 +61,20 @@ describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
 
     it "moves expired tokens to the 'inactive' section" do
       personal_access_token.update(expires_at: 5.days.ago)
-      visit admin_user_personal_access_tokens_path(user_id: user.username)
+
+      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
-        disallow_personal_access_token_saves!
-        visit admin_user_personal_access_tokens_path(user_id: user.username)
+        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
diff --git a/spec/features/profiles/personal_access_tokens_spec.rb b/spec/features/profiles/personal_access_tokens_spec.rb
index bce4f7c9f3d..74e4e157dc5 100644
--- a/spec/features/profiles/personal_access_tokens_spec.rb
+++ b/spec/features/profiles/personal_access_tokens_spec.rb
@@ -26,7 +26,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do
   end
 
   describe "token creation" do
-    it "allows creation of a token" do
+    it "allows creation of a non impersonation token" do
       name = FFaker::Product.brand
 
       visit profile_personal_access_tokens_path
@@ -60,6 +60,18 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do
     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 non impersonated tokens' do
+      visit profile_personal_access_tokens_path
+
+      expect(active_personal_access_tokens).to have_text(personal_access_token.name)
+      expect(active_personal_access_tokens).not_to have_text(impersonation_token.name)
+    end
+  end
+
   describe "inactive tokens" do
     let!(:personal_access_token) { create(:personal_access_token, user: user) }
 
diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb
index c10c3bc3f31..b98a4d7fd1c 100644
--- a/spec/models/personal_access_token_spec.rb
+++ b/spec/models/personal_access_token_spec.rb
@@ -1,18 +1,21 @@
 require 'spec_helper'
 
 describe PersonalAccessToken, models: true do
-  describe ".generate" do
-    it "generates a random token" do
-      personal_access_token = PersonalAccessToken.generate({})
-      expect(personal_access_token.token).to be_present
+  describe '.build' do
+    let(:personal_access_token) { build(:personal_access_token) }
+    let(:invalid_personal_access_token) { build(:personal_access_token, token: nil) }
+
+    it 'is a valid personal access token' do
+      expect(personal_access_token).to be_valid
     end
 
-    it "doesn't save the record" do
-      personal_access_token = PersonalAccessToken.generate({})
-      expect(personal_access_token).not_to be_persisted
+    it 'ensures that the token is generated' do
+      invalid_personal_access_token.save!
+
+      expect(invalid_personal_access_token).to be_valid
+      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) }
diff --git a/spec/requests/api/personal_access_tokens_spec.rb b/spec/requests/api/personal_access_tokens_spec.rb
index f7a89a6539c..98c8794efa4 100644
--- a/spec/requests/api/personal_access_tokens_spec.rb
+++ b/spec/requests/api/personal_access_tokens_spec.rb
@@ -16,7 +16,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(3)
+      expect(json_response.size).to eq(user.personal_access_tokens.count)
 
       json_personal_access_token = json_response.detect do |personal_access_token|
         personal_access_token['id'] == active_personal_access_token.id
@@ -73,7 +73,7 @@ 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 eq(nil)
+      expect(PersonalAccessToken.find(personal_access_token_id)).not_to be_nil
     end
   end
 
@@ -85,14 +85,14 @@ describe API::PersonalAccessTokens, api: true  do
       get api("/personal_access_tokens/#{not_found_token}", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a 404 error if personal access token exists but it is a personal access tokens of another user' do
       get api("/personal_access_tokens/#{personal_access_token_of_another_user.id}", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a personal access token and does not expose token in the json response' do
@@ -111,14 +111,14 @@ describe API::PersonalAccessTokens, api: true  do
       delete api("/personal_access_tokens/#{not_found_token}", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a 404 error if personal access token exists but it is a personal access tokens of another user' do
       delete api("/personal_access_tokens/#{personal_access_token_of_another_user.id}", user)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'revokes a personal access token and does not expose token in the json response' do
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 0ebd5eb872e..f5b6d30b9f6 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -1158,7 +1158,7 @@ describe API::Users, api: true  do
     end
   end
 
-  describe 'GET /users/:user_id/personal_access_tokens' do
+  describe 'GET /users/:id/personal_access_tokens' do
     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) }
@@ -1178,12 +1178,12 @@ describe API::Users, api: true  do
       expect(json_response['message']).to eq('403 Forbidden')
     end
 
-    it 'returns an array of personal access tokens' do
+    it 'returns an array of non impersonated personal access tokens' do
       get api("/users/#{user.id}/personal_access_tokens", admin)
 
       expect(response).to have_http_status(200)
       expect(json_response).to be_an Array
-      expect(json_response.size).to eq(4)
+      expect(json_response.size).to eq(user.personal_access_tokens.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)
@@ -1194,6 +1194,7 @@ describe API::Users, 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.active.count)
       expect(json_response).to all(include('active' => true))
     end
 
@@ -1202,6 +1203,7 @@ describe API::Users, 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.inactive.count)
       expect(json_response).to all(include('active' => false))
     end
 
@@ -1210,17 +1212,18 @@ describe API::Users, 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.impersonation.count)
       expect(json_response).to all(include('impersonation' => true))
     end
   end
 
-  describe 'POST /users/:user_id/personal_access_tokens' 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(:impersonation) { true }
 
-    it 'returns validation error if personal access token miss some attributes' do
+    it 'returns validation error if personal access token misses some attributes' do
       post api("/users/#{user.id}/personal_access_tokens", admin)
 
       expect(response).to have_http_status(400)
@@ -1253,23 +1256,20 @@ describe API::Users, api: true  do
         impersonation: impersonation
 
       expect(response).to have_http_status(201)
-
-      personal_access_token_id = json_response['id']
-
       expect(json_response['name']).to eq(name)
       expect(json_response['scopes']).to eq(scopes)
       expect(json_response['expires_at']).to eq(expires_at)
       expect(json_response['id']).to be_present
       expect(json_response['created_at']).to be_present
-      expect(json_response['active']).to eq(false)
-      expect(json_response['revoked']).to eq(false)
+      expect(json_response['active']).to be_falsey
+      expect(json_response['revoked']).to be_falsey
       expect(json_response['token']).to be_present
       expect(json_response['impersonation']).to eq(impersonation)
-      expect(PersonalAccessToken.and_impersonation_tokens.find(personal_access_token_id)).not_to eq(nil)
+      expect(PersonalAccessToken.with_impersonation_tokens.find(json_response['id'])).not_to be_nil
     end
   end
 
-  describe 'GET /users/:user_id/personal_access_tokens/:personal_access_token_id' do
+  describe 'GET /users/:id/personal_access_tokens/:personal_access_token_id' do
     let!(:personal_access_token) { create(:personal_access_token, user: user, revoked: false) }
     let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user, revoked: false) }
 
@@ -1284,7 +1284,7 @@ describe API::Users, api: true  do
       get api("/users/#{user.id}/personal_access_tokens/#{not_existing_pat_id}", admin)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a 403 error when authenticated as normal user' do
@@ -1299,17 +1299,24 @@ describe API::Users, api: true  do
 
       expect(response).to have_http_status(200)
       expect(json_response['token']).to be_present
+      expect(json_response['impersonation']).to be_falsey
     end
 
-    it 'returns an impersonation token' do
+    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 eq(true)
+      expect(json_response['impersonation']).to be_truthy
     end
   end
 
-  describe 'DELETE /users/:user_id/personal_access_tokens/:personal_access_token_id' do
+  describe 'DELETE /users/:id/personal_access_tokens/:personal_access_token_id' do
     let!(:personal_access_token) { create(:personal_access_token, user: user, revoked: false) }
     let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user, revoked: false) }
 
@@ -1324,7 +1331,7 @@ describe API::Users, api: true  do
       delete api("/users/#{user.id}/personal_access_tokens/#{not_existing_pat_id}", admin)
 
       expect(response).to have_http_status(404)
-      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+      expect(json_response['message']).to eq('404 Personal Access Token Not Found')
     end
 
     it 'returns a 403 error when authenticated as normal user' do
@@ -1338,16 +1345,24 @@ describe API::Users, api: true  do
       delete api("/users/#{user.id}/personal_access_tokens/#{personal_access_token.id}", admin)
 
       expect(response).to have_http_status(204)
-      expect(personal_access_token.revoked).to eq(false)
-      expect(personal_access_token.reload.revoked).to eq(true)
+      expect(personal_access_token.revoked).to be_falsey
+      expect(personal_access_token.reload.revoked).to be_truthy
     end
 
-    it 'revokes an impersonation token' do
+    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 eq(false)
-      expect(impersonation_token.reload.revoked).to eq(true)
+      expect(impersonation_token.revoked).to be_falsey
+      expect(impersonation_token.reload.revoked).to be_truthy
     end
   end
 end