Implement @DouweM's feedback.

- Extract a duplicated `redirect_to`
- Fix a typo: "token", not "certificate"
- Have the "Expires at" datepicker be attached to a text field, not inline
- Have both private tokens and personal access tokens verified in a
  single "authenticate_from_private_token" method, both in the
  application and API. Move relevant logic to
  `User#find_by_personal_access_token`
- Remove unnecessary constants relating to API auth. We don't need a
  separate constant for personal access tokens since the param is the
  same as for private tokens.

7 files changed, 22 insertions, 42 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index a5b358f10f1..72d1b97bf56 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -8,7 +8,7 @@ class ApplicationController < ActionController::Base
   include PageLayoutHelper
   include WorkhorseHelper
 
-  before_action :authenticate_user_from_token!
+  before_action :authenticate_user_from_private_token!
   before_action :authenticate_user!
   before_action :validate_user_service_ticket!
   before_action :reject_blocked!
@@ -64,8 +64,11 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def authenticate_user_from_token!
-    user = get_user_from_private_token || get_user_from_personal_access_token
+  # This filter handles both private tokens and personal access tokens
+  def authenticate_user_from_private_token!
+    token_string = params[:private_token].presence || request.headers['PRIVATE-TOKEN'].presence
+    user = User.find_by_authentication_token(token_string) || User.find_by_personal_access_token(token_string)
+
     if user
       # Notice we are passing store false, so the user is not
       # actually stored in the session and a token is needed
@@ -376,17 +379,4 @@ class ApplicationController < ActionController::Base
     (controller_name == 'groups' && action_name == page_type) ||
     (controller_name == 'dashboard' && action_name == page_type)
   end
-
-  # From https://github.com/plataformatec/devise/wiki/How-To:-Simple-Token-Authentication-Example
-  # https://gist.github.com/josevalim/fb706b1e933ef01e4fb6
-  def get_user_from_private_token
-    user_token = params[:private_token].presence || request.headers['PRIVATE-TOKEN'].presence
-    User.find_by_authentication_token(user_token.to_s) if user_token
-  end
-
-  def get_user_from_personal_access_token
-    token_string = params[:private_token].presence || request.headers['PRIVATE-TOKEN'].presence
-    personal_access_token = PersonalAccessToken.active.find_by_token(token_string) if token_string
-    personal_access_token.user if personal_access_token
-  end
 end
diff --git a/app/controllers/profiles/personal_access_tokens_controller.rb b/app/controllers/profiles/personal_access_tokens_controller.rb
index 86e08fed8e2..508b82a9a6c 100644
--- a/app/controllers/profiles/personal_access_tokens_controller.rb
+++ b/app/controllers/profiles/personal_access_tokens_controller.rb
@@ -21,10 +21,12 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
     @personal_access_token = current_user.personal_access_tokens.find(params[:id])
 
     if @personal_access_token.revoke!
-      redirect_to profile_personal_access_tokens_path, notice: "Revoked personal access token #{@personal_access_token.name}!"
+      flash[:notice] = "Revoked personal access token #{@personal_access_token.name}!"
     else
-      redirect_to profile_personal_access_tokens_path, alert: "Could not revoke personal access token #{@personal_access_token.name}."
+      flash[:alert] = "Could not revoke personal access token #{@personal_access_token.name}."
     end
+
+    redirect_to profile_personal_access_tokens_path
   end
 
   private
diff --git a/app/models/user.rb b/app/models/user.rb
index 65fe88e0287..388e2652ba9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -269,6 +269,11 @@ class User < ActiveRecord::Base
       find_by!('lower(username) = ?', username.downcase)
     end
 
+    def find_by_personal_access_token(token_string)
+      personal_access_token = PersonalAccessToken.active.find_by_token(token_string) if token_string
+      personal_access_token.user if personal_access_token
+    end
+
     def by_username_or_id(name_or_id)
       find_by('users.username = ? OR users.id = ?', name_or_id.to_s, name_or_id.to_i)
     end
diff --git a/app/views/layouts/nav/_profile.html.haml b/app/views/layouts/nav/_profile.html.haml
index d9f1e1c0acb..bb6f14a6225 100644
--- a/app/views/layouts/nav/_profile.html.haml
+++ b/app/views/layouts/nav/_profile.html.haml
@@ -15,7 +15,6 @@
           Applications
   = nav_link(controller: :personal_access_tokens) do
     = link_to profile_personal_access_tokens_path, title: 'Personal Access Tokens' do
-      = icon('ticket fw')
       %span
         Personal Access Tokens
   = nav_link(controller: :emails) do
diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml
index 456e770dc80..1b45548bd02 100644
--- a/app/views/profiles/personal_access_tokens/index.html.haml
+++ b/app/views/profiles/personal_access_tokens/index.html.haml
@@ -34,8 +34,7 @@
 
       .form-group
         = f.label :expires_at, class: 'label-light'
-        = f.hidden_field :expires_at, class: "form-control", required: false
-        .datepicker.personal-access-tokens-expires-at
+        = f.text_field :expires_at, class: "datepicker form-control", required: false
 
       .prepend-top-default
         = f.submit 'Create Personal Access Token', class: "btn btn-create"
@@ -63,7 +62,7 @@
                     = token.expires_at.to_date.to_s(:medium)
                   - else
                     %span.personal-access-tokens-never-expires-label Never
-                %td= link_to "Revoke", revoke_profile_personal_access_token_path(token), method: :put, class: "btn btn-danger pull-right", data: { confirm: "Are you sure you want to revoke this certificate? This action cannot be undone." }
+                %td= link_to "Revoke", revoke_profile_personal_access_token_path(token), 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
@@ -95,18 +94,10 @@
   var date = $('#personal_access_token_expires_at').val();
 
   var datepicker = $(".datepicker").datepicker({
-    altFormat: "yy-mm-dd",
-    altField: "#personal_access_token_expires_at",
+    dateFormat: "yy-mm-dd",
     minDate: 0
   });
 
-  if (date) {
-    datepicker.datepicker("setDate", $.datepicker.parseDate('yy-mm-dd', date));
-  }
-  else {
-    datepicker.datepicker("setDate", null);
-  }
-
   $("#created-personal-access-token").click(function() {
     this.select();
   });
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 8c4a707e7ee..77e407b54c5 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -4,26 +4,18 @@ module API
     PRIVATE_TOKEN_PARAM = :private_token
     SUDO_HEADER = "HTTP_SUDO"
     SUDO_PARAM = :sudo
-    PERSONAL_ACCESS_TOKEN_PARAM = PRIVATE_TOKEN_PARAM
-    PERSONAL_ACCESS_TOKEN_HEADER = PRIVATE_TOKEN_HEADER
 
     def parse_boolean(value)
       [ true, 1, '1', 't', 'T', 'true', 'TRUE', 'on', 'ON' ].include?(value)
     end
 
     def find_user_by_private_token
-      private_token = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s
-      User.find_by_authentication_token(private_token)
-    end
-
-    def find_user_by_personal_access_token
-      personal_access_token_string = (params[PERSONAL_ACCESS_TOKEN_PARAM] || env[PERSONAL_ACCESS_TOKEN_HEADER]).to_s
-      personal_access_token = PersonalAccessToken.active.find_by_token(personal_access_token_string)
-      personal_access_token.user if personal_access_token
+      token_string = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s
+      User.find_by_authentication_token(token_string) || User.find_by_personal_access_token(token_string)
     end
 
     def current_user
-      @current_user ||= (find_user_by_private_token || find_user_by_personal_access_token || doorkeeper_guard)
+      @current_user ||= (find_user_by_private_token || doorkeeper_guard)
 
       unless @current_user && Gitlab::UserAccess.allowed?(@current_user)
         return nil
diff --git a/spec/features/profiles/personal_access_tokens_spec.rb b/spec/features/profiles/personal_access_tokens_spec.rb
index d824e1d288d..a85930c7543 100644
--- a/spec/features/profiles/personal_access_tokens_spec.rb
+++ b/spec/features/profiles/personal_access_tokens_spec.rb
@@ -41,6 +41,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do
       fill_in "Name", with: FFaker::Product.brand
 
       # Set date to 1st of next month
+      find_field("Expires at").trigger('focus')
       find("a[title='Next']").click
       click_on "1"