Commit 07f5be6b authored by Timothy Andrew's avatar Timothy Andrew

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 c400cae3
...@@ -2,6 +2,7 @@ class Admin::ApplicationsController < Admin::ApplicationController ...@@ -2,6 +2,7 @@ class Admin::ApplicationsController < Admin::ApplicationController
include OauthApplications include OauthApplications
before_action :set_application, only: [:show, :edit, :update, :destroy] before_action :set_application, only: [:show, :edit, :update, :destroy]
before_action :load_scopes, only: [:new, :edit]
def index def index
@applications = Doorkeeper::Application.where("owner_id IS NULL") @applications = Doorkeeper::Application.where("owner_id IS NULL")
...@@ -12,11 +13,9 @@ class Admin::ApplicationsController < Admin::ApplicationController ...@@ -12,11 +13,9 @@ class Admin::ApplicationsController < Admin::ApplicationController
def new def new
@application = Doorkeeper::Application.new @application = Doorkeeper::Application.new
@scopes = Doorkeeper.configuration.scopes
end end
def edit def edit
@scopes = Doorkeeper.configuration.scopes
end end
def create def create
......
...@@ -7,8 +7,13 @@ module OauthApplications ...@@ -7,8 +7,13 @@ module OauthApplications
def prepare_scopes def prepare_scopes
scopes = params.dig(:doorkeeper_application, :scopes) scopes = params.dig(:doorkeeper_application, :scopes)
if scopes if scopes
params[:doorkeeper_application][:scopes] = scopes.join(' ') params[:doorkeeper_application][:scopes] = scopes.join(' ')
end end
end end
def load_scopes
@scopes = Doorkeeper.configuration.scopes
end
end end
...@@ -7,6 +7,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController ...@@ -7,6 +7,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
before_action :verify_user_oauth_applications_enabled before_action :verify_user_oauth_applications_enabled
before_action :authenticate_user! before_action :authenticate_user!
before_action :add_gon_variables before_action :add_gon_variables
before_action :load_scopes, only: [:index, :create, :edit]
layout 'profile' layout 'profile'
...@@ -14,10 +15,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController ...@@ -14,10 +15,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
set_index_vars set_index_vars
end end
def edit
@scopes = Doorkeeper.configuration.scopes
end
def create def create
@application = Doorkeeper::Application.new(application_params) @application = Doorkeeper::Application.new(application_params)
...@@ -45,7 +42,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController ...@@ -45,7 +42,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
@authorized_tokens = current_user.oauth_authorized_tokens @authorized_tokens = current_user.oauth_authorized_tokens
@authorized_anonymous_tokens = @authorized_tokens.reject(&:application) @authorized_anonymous_tokens = @authorized_tokens.reject(&:application)
@authorized_apps = @authorized_tokens.map(&:application).uniq.reject(&:nil?) @authorized_apps = @authorized_tokens.map(&:application).uniq.reject(&:nil?)
@scopes = Doorkeeper.configuration.scopes
# Don't overwrite a value possibly set by `create` # Don't overwrite a value possibly set by `create`
@application ||= Doorkeeper::Application.new @application ||= Doorkeeper::Application.new
......
...@@ -25,6 +25,5 @@ ...@@ -25,6 +25,5 @@
= label_tag "doorkeeper_application_scopes_#{scope}", scope = label_tag "doorkeeper_application_scopes_#{scope}", scope
%span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})"
.prepend-top-default .prepend-top-default
= f.submit 'Save application', class: "btn btn-create" = f.submit 'Save application', class: "btn btn-create"
...@@ -34,7 +34,6 @@ ...@@ -34,7 +34,6 @@
%span.scope-name= scope %span.scope-name= scope
= "(#{t(scope, scope: [:doorkeeper, :scopes])})" = "(#{t(scope, scope: [:doorkeeper, :scopes])})"
.form-actions .form-actions
= link_to 'Edit', edit_oauth_application_path(@application), class: 'btn btn-primary wide pull-left' = 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' = 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 # The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`.
# for more information on how to write migrations for GitLab. # It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to
# `[]`.
class AddColumnScopesToPersonalAccessTokens < ActiveRecord::Migration class AddColumnScopesToPersonalAccessTokens < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false 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! disable_ddl_transaction!
def up 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 add_column_with_default :personal_access_tokens, :scopes, :string, default: ['api'].to_yaml
end end
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html # The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`.
# for more information on how to write migrations for GitLab. # It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to
# `[]`.
class ChangePersonalAccessTokensDefaultBackToEmptyArray < ActiveRecord::Migration class ChangePersonalAccessTokensDefaultBackToEmptyArray < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false 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 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 change_column_default :personal_access_tokens, :scopes, [].to_yaml
end end
def down 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 change_column_default :personal_access_tokens, :scopes, ['api'].to_yaml
end end
end end
...@@ -44,19 +44,23 @@ module API ...@@ -44,19 +44,23 @@ module API
# Defaults to empty array. # Defaults to empty array.
# #
def doorkeeper_guard(scopes: []) def doorkeeper_guard(scopes: [])
if access_token = find_access_token access_token = find_access_token
return nil unless access_token
case AccessTokenValidationService.validate(access_token, scopes: scopes) case AccessTokenValidationService.validate(access_token, scopes: scopes)
when AccessTokenValidationService::INSUFFICIENT_SCOPE when AccessTokenValidationService::INSUFFICIENT_SCOPE
raise InsufficientScopeError.new(scopes) raise InsufficientScopeError.new(scopes)
when AccessTokenValidationService::EXPIRED when AccessTokenValidationService::EXPIRED
raise ExpiredError raise ExpiredError
when AccessTokenValidationService::REVOKED when AccessTokenValidationService::REVOKED
raise RevokedError raise RevokedError
when AccessTokenValidationService::VALID when AccessTokenValidationService::VALID
@current_user = User.find(access_token.resource_owner_id) @current_user = User.find(access_token.resource_owner_id)
end end
end end
end
def find_user_by_private_token(scopes: []) def find_user_by_private_token(scopes: [])
token_string = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s token_string = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s
......
...@@ -112,7 +112,6 @@ module Gitlab ...@@ -112,7 +112,6 @@ module Gitlab
if token && token.user == validation && token_has_scope?(token) if token && token.user == validation && token_has_scope?(token)
Gitlab::Auth::Result.new(validation, nil, :personal_token, full_authentication_abilities) Gitlab::Auth::Result.new(validation, nil, :personal_token, full_authentication_abilities)
end end
end end
end end
......
require 'spec_helper' require 'spec_helper'
describe AccessTokenValidationService, services: true do describe AccessTokenValidationService, services: true do
describe ".sufficient_scope?" do describe ".sufficient_scope?" do
it "returns true if the required scope is present in the token's scopes" do it "returns true if the required scope is present in the token's scopes" do
token = double("token", scopes: [:api, :read_user]) 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