Commit 7ee0898a authored by Timothy Andrew's avatar Timothy Andrew

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.
parent faa0e3f7
...@@ -8,7 +8,7 @@ class ApplicationController < ActionController::Base ...@@ -8,7 +8,7 @@ class ApplicationController < ActionController::Base
include PageLayoutHelper include PageLayoutHelper
include WorkhorseHelper include WorkhorseHelper
before_action :authenticate_user_from_token! before_action :authenticate_user_from_private_token!
before_action :authenticate_user! before_action :authenticate_user!
before_action :validate_user_service_ticket! before_action :validate_user_service_ticket!
before_action :reject_blocked! before_action :reject_blocked!
...@@ -64,8 +64,11 @@ class ApplicationController < ActionController::Base ...@@ -64,8 +64,11 @@ class ApplicationController < ActionController::Base
end end
end end
def authenticate_user_from_token! # This filter handles both private tokens and personal access tokens
user = get_user_from_private_token || get_user_from_personal_access_token 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 if user
# Notice we are passing store false, so the user is not # Notice we are passing store false, so the user is not
# actually stored in the session and a token is needed # actually stored in the session and a token is needed
...@@ -376,17 +379,4 @@ class ApplicationController < ActionController::Base ...@@ -376,17 +379,4 @@ class ApplicationController < ActionController::Base
(controller_name == 'groups' && action_name == page_type) || (controller_name == 'groups' && action_name == page_type) ||
(controller_name == 'dashboard' && action_name == page_type) (controller_name == 'dashboard' && action_name == page_type)
end 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 end
...@@ -21,10 +21,12 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController ...@@ -21,10 +21,12 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController
@personal_access_token = current_user.personal_access_tokens.find(params[:id]) @personal_access_token = current_user.personal_access_tokens.find(params[:id])
if @personal_access_token.revoke! 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 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 end
redirect_to profile_personal_access_tokens_path
end end
private private
......
...@@ -269,6 +269,11 @@ class User < ActiveRecord::Base ...@@ -269,6 +269,11 @@ class User < ActiveRecord::Base
find_by!('lower(username) = ?', username.downcase) find_by!('lower(username) = ?', username.downcase)
end 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) 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) find_by('users.username = ? OR users.id = ?', name_or_id.to_s, name_or_id.to_i)
end end
......
...@@ -15,7 +15,6 @@ ...@@ -15,7 +15,6 @@
Applications Applications
= nav_link(controller: :personal_access_tokens) do = nav_link(controller: :personal_access_tokens) do
= link_to profile_personal_access_tokens_path, title: 'Personal Access Tokens' do = link_to profile_personal_access_tokens_path, title: 'Personal Access Tokens' do
= icon('ticket fw')
%span %span
Personal Access Tokens Personal Access Tokens
= nav_link(controller: :emails) do = nav_link(controller: :emails) do
......
...@@ -34,8 +34,7 @@ ...@@ -34,8 +34,7 @@
.form-group .form-group
= f.label :expires_at, class: 'label-light' = f.label :expires_at, class: 'label-light'
= f.hidden_field :expires_at, class: "form-control", required: false = f.text_field :expires_at, class: "datepicker form-control", required: false
.datepicker.personal-access-tokens-expires-at
.prepend-top-default .prepend-top-default
= f.submit 'Create Personal Access Token', class: "btn btn-create" = f.submit 'Create Personal Access Token', class: "btn btn-create"
...@@ -63,7 +62,7 @@ ...@@ -63,7 +62,7 @@
= token.expires_at.to_date.to_s(:medium) = token.expires_at.to_date.to_s(:medium)
- else - else
%span.personal-access-tokens-never-expires-label Never %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 - else
.settings-message.text-center .settings-message.text-center
...@@ -95,18 +94,10 @@ ...@@ -95,18 +94,10 @@
var date = $('#personal_access_token_expires_at').val(); var date = $('#personal_access_token_expires_at').val();
var datepicker = $(".datepicker").datepicker({ var datepicker = $(".datepicker").datepicker({
altFormat: "yy-mm-dd", dateFormat: "yy-mm-dd",
altField: "#personal_access_token_expires_at",
minDate: 0 minDate: 0
}); });
if (date) {
datepicker.datepicker("setDate", $.datepicker.parseDate('yy-mm-dd', date));
}
else {
datepicker.datepicker("setDate", null);
}
$("#created-personal-access-token").click(function() { $("#created-personal-access-token").click(function() {
this.select(); this.select();
}); });
......
...@@ -4,26 +4,18 @@ module API ...@@ -4,26 +4,18 @@ module API
PRIVATE_TOKEN_PARAM = :private_token PRIVATE_TOKEN_PARAM = :private_token
SUDO_HEADER = "HTTP_SUDO" SUDO_HEADER = "HTTP_SUDO"
SUDO_PARAM = :sudo SUDO_PARAM = :sudo
PERSONAL_ACCESS_TOKEN_PARAM = PRIVATE_TOKEN_PARAM
PERSONAL_ACCESS_TOKEN_HEADER = PRIVATE_TOKEN_HEADER
def parse_boolean(value) def parse_boolean(value)
[ true, 1, '1', 't', 'T', 'true', 'TRUE', 'on', 'ON' ].include?(value) [ true, 1, '1', 't', 'T', 'true', 'TRUE', 'on', 'ON' ].include?(value)
end end
def find_user_by_private_token def find_user_by_private_token
private_token = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s token_string = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s
User.find_by_authentication_token(private_token) User.find_by_authentication_token(token_string) || User.find_by_personal_access_token(token_string)
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
end end
def current_user 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) unless @current_user && Gitlab::UserAccess.allowed?(@current_user)
return nil return nil
......
...@@ -41,6 +41,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do ...@@ -41,6 +41,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do
fill_in "Name", with: FFaker::Product.brand fill_in "Name", with: FFaker::Product.brand
# Set date to 1st of next month # Set date to 1st of next month
find_field("Expires at").trigger('focus')
find("a[title='Next']").click find("a[title='Next']").click
click_on "1" click_on "1"
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment