From 4d6da770de94f4bf140507cdf43461b67269ce28 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 24 Nov 2016 13:07:22 +0530 Subject: [PATCH] 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. --- .../admin/applications_controller.rb | 3 +- .../concerns/oauth_applications.rb | 5 ++++ .../oauth/applications_controller.rb | 6 +--- .../doorkeeper/applications/_form.html.haml | 1 - .../doorkeeper/applications/show.html.haml | 1 - ...column_scopes_to_personal_access_tokens.rb | 23 ++------------- ...cess_tokens_default_back_to_empty_array.rb | 28 ++----------------- lib/api/api_guard.rb | 26 +++++++++-------- lib/gitlab/auth.rb | 1 - .../access_token_validation_service_spec.rb | 1 - 10 files changed, 28 insertions(+), 67 deletions(-) diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb index 759044910bb..62f62e99a97 100644 --- a/app/controllers/admin/applications_controller.rb +++ b/app/controllers/admin/applications_controller.rb @@ -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 @@ class Admin::ApplicationsController < Admin::ApplicationController def new @application = Doorkeeper::Application.new - @scopes = Doorkeeper.configuration.scopes end def edit - @scopes = Doorkeeper.configuration.scopes end def create diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb index 34ad43ededd..7210ed3eb32 100644 --- a/app/controllers/concerns/oauth_applications.rb +++ b/app/controllers/concerns/oauth_applications.rb @@ -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 diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index b5449a6c30e..2ae4785b12c 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -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 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController set_index_vars end - def edit - @scopes = Doorkeeper.configuration.scopes - end - def create @application = Doorkeeper::Application.new(application_params) @@ -45,7 +42,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController @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 diff --git a/app/views/doorkeeper/applications/_form.html.haml b/app/views/doorkeeper/applications/_form.html.haml index 6fdb04077b6..96677dc1a4d 100644 --- a/app/views/doorkeeper/applications/_form.html.haml +++ b/app/views/doorkeeper/applications/_form.html.haml @@ -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" diff --git a/app/views/doorkeeper/applications/show.html.haml b/app/views/doorkeeper/applications/show.html.haml index a18e133c8de..5473a8e0ddc 100644 --- a/app/views/doorkeeper/applications/show.html.haml +++ b/app/views/doorkeeper/applications/show.html.haml @@ -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' diff --git a/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb b/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb index ab7f0365603..91479de840b 100644 --- a/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb +++ b/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb @@ -1,32 +1,15 @@ -# 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 diff --git a/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb b/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb index 018cc3d4747..c8ceb116b8a 100644 --- a/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb +++ b/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb @@ -1,39 +1,17 @@ -# 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 diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index cd266669b1e..563224a580f 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -44,17 +44,21 @@ module API # 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 diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index c6a23aa2bdf..c425702fd75 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -107,7 +107,6 @@ module Gitlab if token && token.user == validation && token_has_scope?(token) Gitlab::Auth::Result.new(validation, nil, :personal_token, full_authentication_abilities) end - end end diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb index 8808934fa24..332e745aa36 100644 --- a/spec/services/access_token_validation_service_spec.rb +++ b/spec/services/access_token_validation_service_spec.rb @@ -1,7 +1,6 @@ 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]) -- GitLab