diff options
| -rw-r--r-- | app/controllers/admin/impersonation_tokens_controller.rb | 20 | ||||
| -rw-r--r-- | app/controllers/profiles/personal_access_tokens_controller.rb | 19 | ||||
| -rw-r--r-- | app/finders/personal_access_tokens_finder.rb | 56 | ||||
| -rw-r--r-- | app/models/personal_access_token.rb | 7 | ||||
| -rw-r--r-- | app/views/profiles/personal_access_tokens/index.html.haml | 13 | ||||
| -rw-r--r-- | app/views/shared/_personal_access_tokens_form.html.haml | 2 | ||||
| -rw-r--r-- | doc/api/personal_access_tokens.md | 24 | ||||
| -rw-r--r-- | doc/api/users.md | 43 | ||||
| -rw-r--r-- | lib/api/personal_access_tokens.rb | 21 | ||||
| -rw-r--r-- | lib/api/users.rb | 30 | ||||
| -rw-r--r-- | lib/gitlab/auth.rb | 2 | ||||
| -rw-r--r-- | spec/features/admin/admin_users_impersonation_tokens_spec.rb | 57 | ||||
| -rw-r--r-- | spec/finders/personal_access_tokens_finder_spec.rb | 192 | ||||
| -rw-r--r-- | spec/lib/gitlab/auth_spec.rb | 11 | ||||
| -rw-r--r-- | spec/models/personal_access_token_spec.rb | 1 | ||||
| -rw-r--r-- | spec/requests/api/personal_access_tokens_spec.rb | 15 | ||||
| -rw-r--r-- | spec/requests/api/users_spec.rb | 67 |
17 files changed, 378 insertions, 202 deletions
diff --git a/app/controllers/admin/impersonation_tokens_controller.rb b/app/controllers/admin/impersonation_tokens_controller.rb index 43e718bcc97..448f2c881a1 100644 --- a/app/controllers/admin/impersonation_tokens_controller.rb +++ b/app/controllers/admin/impersonation_tokens_controller.rb @@ -1,13 +1,12 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController - before_action :user + before_action :user, :finder 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)) + @impersonation_token = finder.execute.build(impersonation_token_params) if @impersonation_token.save flash[:impersonation_token] = @impersonation_token.token @@ -19,7 +18,7 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController end def revoke - @impersonation_token = user.personal_access_tokens.impersonation.find(params[:id]) + @impersonation_token = finder.execute(id: params[:id]) if @impersonation_token.revoke! flash[:notice] = "Revoked impersonation token #{@impersonation_token.name}!" @@ -36,14 +35,21 @@ class Admin::ImpersonationTokensController < Admin::ApplicationController @user ||= User.find_by!(username: params[:user_id]) end + def finder + @finder ||= PersonalAccessTokensFinder.new(user: user, impersonation: true) + 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 + finder.params[:state] = 'active' + @impersonation_token ||= finder.execute.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 + finder.params[:order] = :expires_at + @active_impersonation_tokens = finder.execute + finder.params[:state] = 'inactive' + @inactive_impersonation_tokens = finder.execute end end diff --git a/app/controllers/profiles/personal_access_tokens_controller.rb b/app/controllers/profiles/personal_access_tokens_controller.rb index a7eca876c2f..2188350f2fd 100644 --- a/app/controllers/profiles/personal_access_tokens_controller.rb +++ b/app/controllers/profiles/personal_access_tokens_controller.rb @@ -1,10 +1,12 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController + before_action :finder + def index set_index_vars end def create - @personal_access_token = current_user.personal_access_tokens.build(personal_access_token_params) + @personal_access_token = finder.execute.build(personal_access_token_params) if @personal_access_token.save flash[:personal_access_token] = @personal_access_token.token @@ -16,7 +18,7 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController end def revoke - @personal_access_token = current_user.personal_access_tokens.find(params[:id]) + @personal_access_token = finder.execute(id: params[:id]) if @personal_access_token.revoke! flash[:notice] = "Revoked personal access token #{@personal_access_token.name}!" @@ -29,14 +31,21 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController private + def finder + @finder ||= PersonalAccessTokensFinder.new(user: current_user, impersonation: false) + end + def personal_access_token_params params.require(:personal_access_token).permit(:name, :expires_at, scopes: []) end def set_index_vars - @personal_access_token ||= current_user.personal_access_tokens.build + finder.params[:state] = 'active' + @personal_access_token ||= finder.execute.build @scopes = Gitlab::Auth::SCOPES - @active_personal_access_tokens = current_user.personal_access_tokens.active.order(:expires_at) - @inactive_personal_access_tokens = current_user.personal_access_tokens.inactive + finder.params[:order] = :expires_at + @active_personal_access_tokens = finder.execute + finder.params[:state] = 'inactive' + @inactive_personal_access_tokens = finder.execute end end diff --git a/app/finders/personal_access_tokens_finder.rb b/app/finders/personal_access_tokens_finder.rb index ad3f7f4a437..7b9a2f6c0bb 100644 --- a/app/finders/personal_access_tokens_finder.rb +++ b/app/finders/personal_access_tokens_finder.rb @@ -1,41 +1,47 @@ class PersonalAccessTokensFinder - def initialize(user, params = {}) - @user = user + attr_accessor :params + + def initialize(params = {}) @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? + def execute(token: nil, id: nil) + tokens = by_impersonation - return find_token_by_id(personal_access_tokens, pat_id) if pat_id + return tokens.find_by_token(token) if token + return tokens.find_by_id(id) if id - case state? - when 'active' - personal_access_tokens.active - when 'inactive' - personal_access_tokens.inactive - else - personal_access_tokens - end + tokens = by_state(tokens) + tokens.order(@params[:order]) if @params[:order] + + tokens end private - def state? - @params[:state].presence - end - - def impersonation? - @params[:impersonation].presence + def personal_access_tokens + @params[:user] ? @params[:user].personal_access_tokens : PersonalAccessToken.all end - def token_id? - @params[:personal_access_token_id].presence + def by_impersonation + case @params[:impersonation] + when true + personal_access_tokens.with_impersonation + when false + personal_access_tokens.without_impersonation + else + personal_access_tokens + end end - def find_token_by_id(personal_access_tokens, pat_id) - personal_access_tokens.find_by(id: pat_id) + def by_state(tokens) + case @params[:state] + when 'active' + tokens.active + when 'inactive' + tokens.inactive + else + tokens + end end end diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index 676e0832d54..22809fe1487 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -9,11 +9,10 @@ class PersonalAccessToken < ActiveRecord::Base before_save :ensure_token - default_scope { where(impersonation: false) } - scope :active, -> { where(revoked: false).where("expires_at >= NOW() OR expires_at IS NULL") } + scope :active, -> { where("revoked = false AND (expires_at >= NOW() OR expires_at IS NULL)") } scope :inactive, -> { where("revoked = true OR expires_at < NOW()") } - scope :impersonation, -> { unscoped.where(impersonation: true) } - scope :with_impersonation_tokens, -> { unscoped } + scope :with_impersonation, -> { where(impersonation: true) } + scope :without_impersonation, -> { where(impersonation: false) } def revoke! self.revoked = true diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml index 8d5008642c9..7b5121ed550 100644 --- a/app/views/profiles/personal_access_tokens/index.html.haml +++ b/app/views/profiles/personal_access_tokens/index.html.haml @@ -86,19 +86,6 @@ :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')); - } - }); - $("#created-personal-access-token").click(function() { this.select(); }); diff --git a/app/views/shared/_personal_access_tokens_form.html.haml b/app/views/shared/_personal_access_tokens_form.html.haml index 546e90a1d62..074eeb7d038 100644 --- a/app/views/shared/_personal_access_tokens_form.html.haml +++ b/app/views/shared/_personal_access_tokens_form.html.haml @@ -29,7 +29,7 @@ new Pikaday({ field: $dateField.get(0), theme: 'gitlab-theme', - format: 'YYYY-MM-DD', + format: 'yyyy-mm-dd', minDate: new Date(), onSelect: function(dateText) { $dateField.val(dateFormat(new Date(dateText), 'yyyy-mm-dd')); diff --git a/doc/api/personal_access_tokens.md b/doc/api/personal_access_tokens.md index 81e6f10f0c6..ea156d92dc8 100644 --- a/doc/api/personal_access_tokens.md +++ b/doc/api/personal_access_tokens.md @@ -2,11 +2,19 @@ ## List +This function takes pagination parameters `page` and `per_page` to restrict the list of personal access tokens. + ``` GET /personal_access_tokens ``` -An example: +Parameters: + +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `state` | string | no | filter tokens based on state (all, active, inactive) | + +Example response: ```json [ { @@ -20,20 +28,6 @@ An example: ] ``` -In addition, you can filter tokens based on state: `all`, `active` and `inactive` - -``` -GET /personal_access_tokens?state=all -``` - -``` -GET /personal_access_tokens?state=active -``` - -``` -GET /personal_access_tokens?state=inactive -``` - ## Show ``` diff --git a/doc/api/users.md b/doc/api/users.md index 2b4099227bc..91168c05dbe 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -831,18 +831,21 @@ Example response: ## Retrieve user personal access tokens It retrieves every personal access token of the user. Note that only administrators can do this. +This function takes pagination parameters `page` and `per_page` to restrict the list of personal access tokens. ``` -GET /users/:user_id/personal_access_tokens +GET /users/:id/personal_access_tokens ``` Parameters: | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | -| `user_id` | integer | yes | The ID of the user | +| `id` | integer | yes | The ID of the user | +| `state` | string | no | filter tokens based on state (all, active, inactive) | +| `impersonation` | boolean | no | The impersonation flag of the personal access token | -An example: +Example response: ```json [ { @@ -852,45 +855,25 @@ An example: "expires_at": "2017-01-04", "scopes": ['api'], "active": true, - "impersonation": false, + "impersonation": true, "token": "9koXpg98eAheJpvBs5tK" } ] ``` -In addition, you can filter tokens based on state: `all`, `active` and `inactive` - -``` -GET /users/:user_id/personal_access_tokens?state=all -``` - -``` -GET /users/:user_id/personal_access_tokens?state=active -``` - -``` -GET /users/:user_id/personal_access_tokens?state=inactive -``` - -Finally, you can filter based on impersonation: `true` or `false`. - -``` -GET /users/:user_id/personal_access_tokens?impersonation=true -``` - ## Show a user personal access token It shows a user's personal access token. Note that only administrators can do this. ``` -GET /users/:user_id/personal_access_tokens/:personal_access_token_id +GET /users/:id/personal_access_tokens/:personal_access_token_id ``` Parameters: | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | -| `user_id` | integer | yes | The ID of the user | +| `id` | integer | yes | The ID of the user | | `personal_access_token_id` | integer | yes | The ID of the personal access token | ## Create a personal access token @@ -901,14 +884,14 @@ both API calls and Git reads and writes. The user will not see these tokens in h settings page. ``` -POST /users/:user_id/personal_access_tokens +POST /users/:id/personal_access_tokens ``` Parameters: | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | -| `user_id` | integer | yes | The ID of the user | +| `id` | integer | yes | The ID of the user | | `name` | string | yes | The name of the personal access token | | `expires_at` | date | no | The expiration date of the personal access token | | `scopes` | array | no | The array of scopes of the personal access token | @@ -919,12 +902,12 @@ Parameters: It revokes a personal access token. Note that only administrators can revoke impersonation tokens. ``` -DELETE /users/:user_id/personal_access_tokens/:personal_access_token_id +DELETE /users/:id/personal_access_tokens/:personal_access_token_id ``` Parameters: | Attribute | Type | Required | Description | | --------- | ---- | -------- | ----------- | -| `user_id` | integer | yes | The ID of the user | +| `id` | integer | yes | The ID of the user | | `personal_access_token_id` | integer | yes | The ID of the personal access token | diff --git a/lib/api/personal_access_tokens.rb b/lib/api/personal_access_tokens.rb index 763888bb57e..7f1a54ac12f 100644 --- a/lib/api/personal_access_tokens.rb +++ b/lib/api/personal_access_tokens.rb @@ -1,6 +1,11 @@ module API class PersonalAccessTokens < Grape::API - before { authenticate! } + include PaginationParams + + before do + authenticate! + @finder = PersonalAccessTokensFinder.new(user: current_user, impersonation: false) + end resource :personal_access_tokens do desc 'Retrieve personal access tokens' do @@ -9,8 +14,12 @@ module API end params do optional :state, type: String, default: 'all', values: %w[all active inactive], desc: 'Filters (all|active|inactive) personal_access_tokens' + use :pagination + end + get do + @finder.params.merge!(declared_params(include_missing: false)) + present paginate(@finder.execute), with: Entities::PersonalAccessToken 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' @@ -20,7 +29,7 @@ module API 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 = PersonalAccessTokensFinder.new(current_user, declared_params(include_missing: false)).execute + personal_access_token = @finder.execute(id: declared_params[:personal_access_token_id]) not_found!('Personal Access Token') unless personal_access_token present personal_access_token, with: Entities::PersonalAccessToken @@ -36,7 +45,7 @@ module API optional :scopes, type: Array, desc: 'The array of scopes of the personal access token' end post do - personal_access_token = current_user.personal_access_tokens.build(declared_params(include_missing: false)) + personal_access_token = @finder.execute.build(declared_params(include_missing: false)) if personal_access_token.save present personal_access_token, with: Entities::PersonalAccessTokenWithToken @@ -52,12 +61,10 @@ module API 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 = PersonalAccessTokensFinder.new(current_user, declared_params(include_missing: false)).execute + personal_access_token = @finder.execute(id: declared_params[:personal_access_token_id]) not_found!('Personal Access Token') unless personal_access_token personal_access_token.revoke! - - no_content! end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index d29f6dde210..37117049007 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -373,7 +373,11 @@ module API end segment ':id' do resource :personal_access_tokens do - before { authenticated_as_admin! } + before do + authenticated_as_admin! + user = find_user(params) + @finder = PersonalAccessTokensFinder.new(user: user) + end desc 'Retrieve personal access tokens. Available only for admins.' do detail 'This feature was introduced in GitLab 9.0' @@ -381,11 +385,12 @@ module API 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' + optional :impersonation, type: Boolean, desc: 'Filters only impersonation personal_access_tokens' + use :pagination end get do - user = find_user(params) - present PersonalAccessTokensFinder.new(user, params).execute, with: Entities::ImpersonationToken + @finder.params.merge!(declared_params(include_missing: false)) + present paginate(@finder.execute), with: Entities::ImpersonationToken end desc 'Create a personal access token. Available only for admins.' do @@ -396,11 +401,10 @@ module API requires :name, type: String, desc: 'The name of the personal access token' optional :expires_at, type: Date, desc: 'The expiration date in the format YEAR-MONTH-DAY of the personal access token' optional :scopes, type: Array, desc: 'The array of scopes of the personal access token' - optional :impersonation, type: Boolean, default: false, desc: 'The impersonation flag of the personal access token' + optional :impersonation, type: Boolean, desc: 'The impersonation flag of the personal access token' end post do - user = find_user(params) - personal_access_token = PersonalAccessTokensFinder.new(user).execute.build(declared_params(include_missing: false)) + personal_access_token = @finder.execute.build(declared_params(include_missing: false)) if personal_access_token.save present personal_access_token, with: Entities::ImpersonationToken @@ -415,12 +419,9 @@ module API 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 = find_user(params) - - personal_access_token = PersonalAccessTokensFinder.new(user, declared_params(include_missing: false)).execute + personal_access_token = @finder.execute(id: declared_params[:personal_access_token_id]) not_found!('Personal Access Token') unless personal_access_token present personal_access_token, with: Entities::ImpersonationToken @@ -431,17 +432,12 @@ module API 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 = find_user(params) - - personal_access_token = PersonalAccessTokensFinder.new(user, declared_params(include_missing: false)).execute + personal_access_token = @finder.execute(id: declared_params[:personal_access_token_id]) not_found!('Personal Access Token') unless personal_access_token personal_access_token.revoke! - - no_content! end end end diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index ef261d08b1d..6f84288654f 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -105,7 +105,7 @@ module Gitlab def personal_access_token_check(password) return unless password.present? - token = PersonalAccessToken.with_impersonation_tokens.active.find_by_token(password) + token = PersonalAccessTokensFinder.new(state: 'active').execute(token: password) if token && valid_api_token?(token) Gitlab::Auth::Result.new(token.user, nil, :personal_token, full_authentication_abilities) diff --git a/spec/features/admin/admin_users_impersonation_tokens_spec.rb b/spec/features/admin/admin_users_impersonation_tokens_spec.rb index c37cf1178df..804723cea32 100644 --- a/spec/features/admin/admin_users_impersonation_tokens_spec.rb +++ b/spec/features/admin/admin_users_impersonation_tokens_spec.rb @@ -4,24 +4,14 @@ describe 'Admin > Users > Impersonation Tokens', feature: true, js: true do let(:admin) { create(:admin) } let!(:user) { create(:user) } - def active_personal_access_tokens + def active_impersonation_tokens find(".table.active-impersonation-tokens") end - def inactive_personal_access_tokens + def inactive_impersonation_tokens find(".table.inactive-impersonation-tokens") end - def created_personal_access_token - find("#created-impersonation-token").value - end - - def disallow_personal_access_token_saves! - allow_any_instance_of(PersonalAccessToken).to receive(:save).and_return(false) - errors = ActiveModel::Errors.new(PersonalAccessToken.new).tap { |e| e.add(:name, "cannot be nil") } - allow_any_instance_of(PersonalAccessToken).to receive(:errors).and_return(errors) - end - before { login_as(admin) } describe "token creation" do @@ -40,44 +30,43 @@ describe 'Admin > Users > Impersonation Tokens', feature: true, js: true do check "api" check "read_user" - 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 { click_on "Create Impersonation Token" }.to change { PersonalAccessToken.with_impersonation.count } + expect(active_impersonation_tokens).to have_text(name) + expect(active_impersonation_tokens).to have_text('In') + expect(active_impersonation_tokens).to have_text('api') + expect(active_impersonation_tokens).to have_text('read_user') + 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 impersonation tokens' do + visit admin_user_impersonation_tokens_path(user_id: user.username) + + expect(active_impersonation_tokens).to have_text(impersonation_token.name) + expect(active_impersonation_tokens).not_to have_text(personal_access_token.name) end end describe "inactive tokens" do - let!(:personal_access_token) { create(:impersonation_personal_access_token, user: user) } + let!(:impersonation_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) click_on "Revoke" - expect(inactive_personal_access_tokens).to have_text(personal_access_token.name) + expect(inactive_impersonation_tokens).to have_text(impersonation_token.name) end it "moves expired tokens to the 'inactive' section" do - personal_access_token.update(expires_at: 5.days.ago) + impersonation_token.update(expires_at: 5.days.ago) 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 - 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 + expect(inactive_impersonation_tokens).to have_text(impersonation_token.name) end end end diff --git a/spec/finders/personal_access_tokens_finder_spec.rb b/spec/finders/personal_access_tokens_finder_spec.rb new file mode 100644 index 00000000000..91e746f295a --- /dev/null +++ b/spec/finders/personal_access_tokens_finder_spec.rb @@ -0,0 +1,192 @@ +require 'spec_helper' + +describe PersonalAccessTokensFinder do + describe '#execute' do + let(:user) { create(:user) } + let!(:active_personal_access_token) { create(:personal_access_token, user: user) } + let!(:expired_personal_access_token) { create(:expired_personal_access_token, user: user) } + let!(:revoked_personal_access_token) { create(:revoked_personal_access_token, user: user) } + let!(:active_impersonation_token) { create(:impersonation_personal_access_token, user: user, impersonation: true) } + let!(:expired_impersonation_token) { create(:expired_personal_access_token, user: user, impersonation: true) } + let!(:revoked_impersonation_token) { create(:revoked_personal_access_token, user: user, impersonation: true) } + + subject { finder.execute } + + describe 'without user' do + let(:finder) { described_class.new } + + it do + is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token, + revoked_personal_access_token, expired_personal_access_token, + revoked_impersonation_token, expired_impersonation_token) + end + + describe 'without impersonation' do + before { finder.params.merge!(impersonation: false) } + + it { is_expected.to contain_exactly(active_personal_access_token, revoked_personal_access_token, expired_personal_access_token) } + + describe 'with active state' do + before { finder.params.merge!(state: 'active') } + + it { is_expected.to contain_exactly(active_personal_access_token) } + end + + describe 'with inactive state' do + before { finder.params.merge!(state: 'inactive') } + + it { is_expected.to contain_exactly(revoked_personal_access_token, expired_personal_access_token) } + end + end + + describe 'with impersonation' do + before { finder.params.merge!(impersonation: true) } + + it { is_expected.to contain_exactly(active_impersonation_token, revoked_impersonation_token, expired_impersonation_token) } + + describe 'with active state' do + before { finder.params.merge!(state: 'active') } + + it { is_expected.to contain_exactly(active_impersonation_token) } + end + + describe 'with inactive state' do + before { finder.params.merge!(state: 'inactive') } + + it { is_expected.to contain_exactly(revoked_impersonation_token, expired_impersonation_token) } + end + end + + describe 'with active state' do + before { finder.params.merge!(state: 'active') } + + it { is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token) } + end + + describe 'with inactive state' do + before { finder.params.merge!(state: 'inactive') } + + it do + is_expected.to contain_exactly(expired_personal_access_token, revoked_personal_access_token, + expired_impersonation_token, revoked_impersonation_token) + end + end + + describe 'with id' do + subject { finder.execute(id: active_personal_access_token.id) } + + it { is_expected.to eq(active_personal_access_token) } + + describe 'with impersonation' do + before { finder.params.merge!(impersonation: true) } + + it { is_expected.to be_nil } + end + end + + describe 'with token' do + subject { finder.execute(token: active_personal_access_token.token) } + + it { is_expected.to eq(active_personal_access_token) } + + describe 'with impersonation' do + before { finder.params.merge!(impersonation: true) } + + it { is_expected.to be_nil } + end + end + end + + describe 'with user' do + let(:user2) { create(:user) } + let(:finder) { described_class.new(user: user) } + let!(:other_user_active_personal_access_token) { create(:personal_access_token, user: user2) } + let!(:other_user_expired_personal_access_token) { create(:expired_personal_access_token, user: user2) } + let!(:other_user_revoked_personal_access_token) { create(:revoked_personal_access_token, user: user2) } + let!(:other_user_active_impersonation_token) { create(:impersonation_personal_access_token, user: user2, impersonation: true) } + let!(:other_user_expired_impersonation_token) { create(:expired_personal_access_token, user: user2, impersonation: true) } + let!(:other_user_revoked_impersonation_token) { create(:revoked_personal_access_token, user: user2, impersonation: true) } + + it do + is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token, + revoked_personal_access_token, expired_personal_access_token, + revoked_impersonation_token, expired_impersonation_token) + end + + describe 'without impersonation' do + before { finder.params.merge!(impersonation: false) } + + it { is_expected.to contain_exactly(active_personal_access_token, revoked_personal_access_token, expired_personal_access_token) } + + describe 'with active state' do + before { finder.params.merge!(state: 'active') } + + it { is_expected.to contain_exactly(active_personal_access_token) } + end + + describe 'with inactive state' do + before { finder.params.merge!(state: 'inactive') } + + it { is_expected.to contain_exactly(revoked_personal_access_token, expired_personal_access_token) } + end + end + + describe 'with impersonation' do + before { finder.params.merge!(impersonation: true) } + + it { is_expected.to contain_exactly(active_impersonation_token, revoked_impersonation_token, expired_impersonation_token) } + + describe 'with active state' do + before { finder.params.merge!(state: 'active') } + + it { is_expected.to contain_exactly(active_impersonation_token) } + end + + describe 'with inactive state' do + before { finder.params.merge!(state: 'inactive') } + + it { is_expected.to contain_exactly(revoked_impersonation_token, expired_impersonation_token) } + end + end + + describe 'with active state' do + before { finder.params.merge!(state: 'active') } + + it { is_expected.to contain_exactly(active_personal_access_token, active_impersonation_token) } + end + + describe 'with inactive state' do + before { finder.params.merge!(state: 'inactive') } + + it do + is_expected.to contain_exactly(expired_personal_access_token, revoked_personal_access_token, + expired_impersonation_token, revoked_impersonation_token) + end + end + + describe 'with id' do + subject { finder.execute(id: active_personal_access_token.id) } + + it { is_expected.to eq(active_personal_access_token) } + + describe 'with impersonation' do + before { finder.params.merge!(impersonation: true) } + + it { is_expected.to be_nil } + end + end + + describe 'with token' do + subject { finder.execute(token: active_personal_access_token.token) } + + it { is_expected.to eq(active_personal_access_token) } + + describe 'with impersonation' do + before { finder.params.merge!(impersonation: true) } + + it { is_expected.to be_nil } + end + end + end + end +end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index f9be51302d9..ca297fead4a 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -118,10 +118,10 @@ describe Gitlab::Auth, lib: true do end it 'succeeds if it is an impersonation token' do - personal_access_token = create(:personal_access_token, impersonation: true, scopes: []) + impersonation_token = create(:personal_access_token, impersonation: true, scopes: ['api']) expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: '') - expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(personal_access_token.user, nil, :personal_token, full_authentication_abilities)) + expect(gl_auth.find_for_git_client('', impersonation_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(impersonation_token.user, nil, :personal_token, full_authentication_abilities)) end it 'fails for personal access tokens with other scopes' do @@ -131,6 +131,13 @@ describe Gitlab::Auth, lib: true do expect(gl_auth.find_for_git_client('', personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil)) end + it 'fails for impersonation token with other scopes' do + impersonation_token = create(:personal_access_token, scopes: ['read_user']) + + expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '') + expect(gl_auth.find_for_git_client('', impersonation_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil)) + end + it 'fails if password is nil' do expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: '') expect(gl_auth.find_for_git_client('', nil, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil)) diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index b98a4d7fd1c..173136ef899 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -16,6 +16,7 @@ describe PersonalAccessToken, models: true do 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 98c8794efa4..4037ce483ea 100644 --- a/spec/requests/api/personal_access_tokens_spec.rb +++ b/spec/requests/api/personal_access_tokens_spec.rb @@ -5,8 +5,10 @@ describe API::PersonalAccessTokens, api: true do let(:user) { create(:user) } let(:not_found_token) { (PersonalAccessToken.maximum('id') || 0) + 10 } + let(:finder) { PersonalAccessTokensFinder.new(user: user, impersonation: false) } describe "GET /personal_access_tokens" do + let!(:active_impersonation_token) { create(:impersonation_personal_access_token, user: user) } 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) } @@ -16,7 +18,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(user.personal_access_tokens.count) + expect(json_response.size).to eq(finder.execute.count) json_personal_access_token = json_response.detect do |personal_access_token| personal_access_token['id'] == active_personal_access_token.id @@ -27,18 +29,24 @@ describe API::PersonalAccessTokens, api: true do end it 'returns an array of active personal access tokens if active is set to true' do + finder.params[:state] = 'active' + get api("/personal_access_tokens?state=active", user) expect(response).to have_http_status(200) expect(json_response).to be_an Array + expect(json_response.size).to eq(finder.execute.count) expect(json_response).to all(include('active' => true)) end it 'returns an array of inactive personal access tokens if active is set to false' do + finder.params[:state] = 'inactive' + get api("/personal_access_tokens?state=inactive", user) expect(response).to have_http_status(200) expect(json_response).to be_an Array + expect(json_response.size).to eq(finder.execute.count) expect(json_response).to all(include('active' => false)) end end @@ -46,7 +54,7 @@ describe API::PersonalAccessTokens, api: true do describe 'POST /personal_access_tokens' do let(:name) { 'my new pat' } let(:expires_at) { '2016-12-28' } - let(:scopes) { ['api', 'read_user'] } + let(:scopes) { %w(api read_user) } it 'returns validation error if personal access token miss some attributes' do post api("/personal_access_tokens", user) @@ -73,7 +81,8 @@ 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 be_nil + expect(json_response['impersonation']).not_to be_present + expect(finder.execute(id: personal_access_token_id)).not_to be_nil end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index f5b6d30b9f6..39c94edd44a 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -12,6 +12,7 @@ describe API::Users, api: true do let(:ldap_blocked_user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') } let(:not_existing_user_id) { (User.maximum('id') || 0 ) + 10 } let(:not_existing_pat_id) { (PersonalAccessToken.maximum('id') || 0 ) + 10 } + let(:finder) { PersonalAccessTokensFinder.new(user: user) } describe "GET /users" do context "when unauthenticated" do @@ -1178,41 +1179,60 @@ describe API::Users, api: true do expect(json_response['message']).to eq('403 Forbidden') end - it 'returns an array of non impersonated personal access tokens' do + it 'returns an array of all impersonated and non-impersonated tokens' do get api("/users/#{user.id}/personal_access_tokens", admin) expect(response).to have_http_status(200) + expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(json_response.size).to eq(user.personal_access_tokens.count) + expect(json_response.size).to eq(finder.execute.count) + end + + it 'returns an array of non impersonated personal access tokens' do + finder.params[:impersonation] = false + + get api("/users/#{user.id}/personal_access_tokens?impersonation=false", admin) + + expect(response).to have_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.size).to eq(finder.execute.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) end it 'returns an array of active personal access tokens if active is set to true' do - get api("/users/#{user.id}/personal_access_tokens?state=active", admin) + finder.params.merge!(state: 'active', impersonation: false) + + get api("/users/#{user.id}/personal_access_tokens?state=active&impersonation=false", admin) expect(response).to have_http_status(200) + expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(json_response.size).to eq(user.personal_access_tokens.active.count) + expect(json_response.size).to eq(finder.execute.count) expect(json_response).to all(include('active' => true)) end it 'returns an array of inactive personal access tokens if active is set to false' do - get api("/users/#{user.id}/personal_access_tokens?state=inactive", admin) + finder.params.merge!(state: 'inactive', impersonation: false) + get api("/users/#{user.id}/personal_access_tokens?impersonation=false&state=inactive", admin) 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.size).to eq(finder.execute.count) expect(json_response).to all(include('active' => false)) end it 'returns an array of impersonation personal access tokens if impersonation is set to true' do + finder.params[:impersonation] = true + get api("/users/#{user.id}/personal_access_tokens?impersonation=true", admin) expect(response).to have_http_status(200) + expect(response).to include_pagination_headers expect(json_response).to be_an Array - expect(json_response.size).to eq(user.personal_access_tokens.impersonation.count) + expect(json_response.size).to eq(finder.execute.count) expect(json_response).to all(include('impersonation' => true)) end end @@ -1220,7 +1240,7 @@ describe API::Users, api: true 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(:scopes) { %w(api read_user) } let(:impersonation) { true } it 'returns validation error if personal access token misses some attributes' do @@ -1265,7 +1285,7 @@ describe API::Users, api: true do expect(json_response['revoked']).to be_falsey expect(json_response['token']).to be_present expect(json_response['impersonation']).to eq(impersonation) - expect(PersonalAccessToken.with_impersonation_tokens.find(json_response['id'])).not_to be_nil + expect(finder.execute(id: json_response['id'])).not_to be_nil end end @@ -1301,19 +1321,6 @@ describe API::Users, api: true do 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}?impersonation=true", admin) - - expect(response).to have_http_status(200) - expect(json_response['impersonation']).to be_truthy - end end describe 'DELETE /users/:id/personal_access_tokens/:personal_access_token_id' do @@ -1348,21 +1355,5 @@ describe API::Users, api: true do 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}?impersonation=true", admin) - - expect(response).to have_http_status(204) - expect(impersonation_token.revoked).to be_falsey - expect(impersonation_token.reload.revoked).to be_truthy - end end end |