Commit a82b1ff2 by Tiago

refactors finder and correlated code

parent 9544f4a4
Pipeline #6740780 failed with stages
in 1 minute 50 seconds
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.merge!(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.merge!(order: :expires_at)
@active_impersonation_tokens = finder.execute
finder.params.merge!(state: 'inactive')
@inactive_impersonation_tokens = finder.execute
end
end
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.merge!(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.merge!(order: :expires_at)
@active_personal_access_tokens = finder.execute
finder.params.merge!(state: 'inactive')
@inactive_personal_access_tokens = finder.execute
end
end
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?
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
def execute(token: nil, id: nil)
tokens = by_impersonation
private
return tokens.find_by_token(token) if token
return tokens.find_by_id(id) if id
def state?
@params[:state].presence
tokens = by_state(tokens)
tokens.order(@params[:order]) if @params[:order]
tokens
end
def impersonation?
@params[:impersonation].presence
private
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
return personal_access_tokens unless @params[:impersonation].present?
@params[:impersonation] ? personal_access_tokens.with_impersonation : personal_access_tokens.without_impersonation
end
def find_token_by_id(personal_access_tokens, pat_id)
personal_access_tokens.find_by(id: pat_id)
def by_state(tokens)
return tokens unless @params[:state].present? || @params[:state] != 'all'
@params[:state] == 'active' ? tokens.active : tokens.inactive
end
end
......@@ -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
......
......@@ -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();
});
......@@ -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'));
......
......@@ -6,7 +6,13 @@
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 +26,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
```
......
......@@ -833,16 +833,18 @@ Example response:
It retrieves every personal access token of the user. Note that only administrators can do this.
```
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 +854,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 +883,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 +901,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 |
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
......
......@@ -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
......
......@@ -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)
......
......@@ -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
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(: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) }
describe 'without user' do
let(:finder) { described_class.new }
subject { finder.execute }
it 'returns all tokens without impersonation specified' do
expect(finder.execute).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(: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) }
subject { finder.execute }
it 'returns all tokens without impersonation specified' do
expect(finder.execute).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