From 9f2e4742e354f5548b4956060f1bfa5ee3bd6657 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 23 Feb 2017 17:47:06 +0000 Subject: [PATCH] applies relevant changes to the code and code structure --- app/assets/stylesheets/pages/settings.scss | 4 +- .../admin/impersonation_tokens_controller.rb | 49 ++++++++++++ .../personal_access_tokens_controller.rb | 49 ------------ .../personal_access_tokens_controller.rb | 2 +- app/finders/personal_access_tokens_finder.rb | 41 ++++++++++ app/models/personal_access_token.rb | 15 +--- .../impersonation_tokens/index.html.haml | 65 +++++++++++++++ .../personal_access_tokens/index.html.haml | 79 ------------------- app/views/admin/users/_head.html.haml | 4 +- .../personal_access_tokens/index.html.haml | 2 +- .../_personal_access_tokens_form.html.haml} | 20 ++++- .../25367-add-impersonation-token.yml | 4 +- config/routes/admin.rb | 2 +- ...impersonation_to_personal_access_tokens.rb | 2 +- doc/api/personal_access_tokens.md | 2 + lib/api/entities.rb | 9 ++- lib/api/personal_access_tokens.rb | 37 +++------ lib/api/users.rb | 62 ++++++--------- lib/gitlab/auth.rb | 4 +- ... admin_users_impersonation_tokens_spec.rb} | 40 ++++------ .../profiles/personal_access_tokens_spec.rb | 14 +++- spec/models/personal_access_token_spec.rb | 19 +++-- .../api/personal_access_tokens_spec.rb | 12 +-- spec/requests/api/users_spec.rb | 59 ++++++++------ 24 files changed, 317 insertions(+), 279 deletions(-) create mode 100644 app/controllers/admin/impersonation_tokens_controller.rb delete mode 100644 app/controllers/admin/personal_access_tokens_controller.rb create mode 100644 app/finders/personal_access_tokens_finder.rb create mode 100644 app/views/admin/impersonation_tokens/index.html.haml delete mode 100644 app/views/admin/personal_access_tokens/index.html.haml rename app/views/{profiles/personal_access_tokens/_form.html.haml => shared/_personal_access_tokens_form.html.haml} (50%) rename spec/features/admin/{admin_users_personal_access_tokens_spec.rb => admin_users_impersonation_tokens_spec.rb} (59%) 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(", ") : "" + %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(", ") : "" - %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 similarity index 50% rename from app/views/profiles/personal_access_tokens/_form.html.haml rename to 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 similarity index 59% rename from spec/features/admin/admin_users_personal_access_tokens_spec.rb rename to 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 '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}", admin) + 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 '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}", admin) + 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