diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-06-16 05:54:13 +0300 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-06-16 05:54:13 +0300 |
commit | 7ee0898a9ec4a03c9a55841b1cbea67add460c50 (patch) | |
tree | 02715669032caed346c063a1e56db826f167fca2 | |
parent | faa0e3f7580bc38d4d12916b4589c64d6c2678a7 (diff) |
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.
-rw-r--r-- | app/controllers/application_controller.rb | 22 | ||||
-rw-r--r-- | app/controllers/profiles/personal_access_tokens_controller.rb | 6 | ||||
-rw-r--r-- | app/models/user.rb | 5 | ||||
-rw-r--r-- | app/views/layouts/nav/_profile.html.haml | 1 | ||||
-rw-r--r-- | app/views/profiles/personal_access_tokens/index.html.haml | 15 | ||||
-rw-r--r-- | lib/api/helpers.rb | 14 | ||||
-rw-r--r-- | spec/features/profiles/personal_access_tokens_spec.rb | 1 |
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" |