Commit 4d6da770 authored by Timothy Andrew's avatar Timothy Andrew
Browse files

Implement minor changes from @dbalexandre's review.

- Mainly whitespace changes.

- Require the migration adding the `scope` column to the
  `personal_access_tokens` table to have downtime, since API calls will
  fail if the new code is in place, but the migration hasn't run.

- Minor refactoring - load `@scopes` in a `before_action`, since we're
  doing it in three different places.
parent a4dcfa3552e6
......@@ -2,6 +2,7 @@ class Admin::ApplicationsController < Admin::ApplicationController
include OauthApplications
before_action :set_application, only: [:show, :edit, :update, :destroy]
before_action :load_scopes, only: [:new, :edit]
def index
@applications = Doorkeeper::Application.where("owner_id IS NULL")
......@@ -12,11 +13,9 @@ def show
def new
@application = Doorkeeper::Application.new
@scopes = Doorkeeper.configuration.scopes
end
def edit
@scopes = Doorkeeper.configuration.scopes
end
def create
......
......@@ -7,8 +7,13 @@ module OauthApplications
def prepare_scopes
scopes = params.dig(:doorkeeper_application, :scopes)
if scopes
params[:doorkeeper_application][:scopes] = scopes.join(' ')
end
end
def load_scopes
@scopes = Doorkeeper.configuration.scopes
end
end
......@@ -7,6 +7,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
before_action :verify_user_oauth_applications_enabled
before_action :authenticate_user!
before_action :add_gon_variables
before_action :load_scopes, only: [:index, :create, :edit]
layout 'profile'
......@@ -14,10 +15,6 @@ def index
set_index_vars
end
def edit
@scopes = Doorkeeper.configuration.scopes
end
def create
@application = Doorkeeper::Application.new(application_params)
......@@ -45,7 +42,6 @@ def set_index_vars
@authorized_tokens = current_user.oauth_authorized_tokens
@authorized_anonymous_tokens = @authorized_tokens.reject(&:application)
@authorized_apps = @authorized_tokens.map(&:application).uniq.reject(&:nil?)
@scopes = Doorkeeper.configuration.scopes
# Don't overwrite a value possibly set by `create`
@application ||= Doorkeeper::Application.new
......
......@@ -25,6 +25,5 @@
= label_tag "doorkeeper_application_scopes_#{scope}", scope
%span= "(#{t(scope, scope: [:doorkeeper, :scopes])})"
.prepend-top-default
= f.submit 'Save application', class: "btn btn-create"
......@@ -34,7 +34,6 @@
%span.scope-name= scope
= "(#{t(scope, scope: [:doorkeeper, :scopes])})"
.form-actions
= link_to 'Edit', edit_oauth_application_path(@application), class: 'btn btn-primary wide pull-left'
= render 'delete_form', application: @application, submit_btn_css: 'btn btn-danger prepend-left-10'
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`.
# It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to
# `[]`.
class AddColumnScopesToPersonalAccessTokens < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
# When a migration requires downtime you **must** uncomment the following
# constant and define a short and easy to understand explanation as to why the
# migration requires downtime.
# DOWNTIME_REASON = ''
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
disable_ddl_transaction!
def up
# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`.
# It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to
# `[]`.
add_column_with_default :personal_access_tokens, :scopes, :string, default: ['api'].to_yaml
end
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`.
# It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to
# `[]`.
class ChangePersonalAccessTokensDefaultBackToEmptyArray < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
# When a migration requires downtime you **must** uncomment the following
# constant and define a short and easy to understand explanation as to why the
# migration requires downtime.
# DOWNTIME_REASON = ''
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def up
# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`.
# It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to
# `[]`.
change_column_default :personal_access_tokens, :scopes, [].to_yaml
end
def down
# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`.
# It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to
# `[]`.
change_column_default :personal_access_tokens, :scopes, ['api'].to_yaml
end
end
......@@ -44,17 +44,21 @@ module HelperMethods
# Defaults to empty array.
#
def doorkeeper_guard(scopes: [])
if access_token = find_access_token
case AccessTokenValidationService.validate(access_token, scopes: scopes)
when AccessTokenValidationService::INSUFFICIENT_SCOPE
raise InsufficientScopeError.new(scopes)
when AccessTokenValidationService::EXPIRED
raise ExpiredError
when AccessTokenValidationService::REVOKED
raise RevokedError
when AccessTokenValidationService::VALID
@current_user = User.find(access_token.resource_owner_id)
end
access_token = find_access_token
return nil unless access_token
case AccessTokenValidationService.validate(access_token, scopes: scopes)
when AccessTokenValidationService::INSUFFICIENT_SCOPE
raise InsufficientScopeError.new(scopes)
when AccessTokenValidationService::EXPIRED
raise ExpiredError
when AccessTokenValidationService::REVOKED
raise RevokedError
when AccessTokenValidationService::VALID
@current_user = User.find(access_token.resource_owner_id)
end
end
......
......@@ -107,7 +107,6 @@ def personal_access_token_check(login, password)
if token && token.user == validation && token_has_scope?(token)
Gitlab::Auth::Result.new(validation, nil, :personal_token, full_authentication_abilities)
end
end
end
......
require 'spec_helper'
describe AccessTokenValidationService, services: true do
describe ".sufficient_scope?" do
it "returns true if the required scope is present in the token's scopes" do
token = double("token", scopes: [:api, :read_user])
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment