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.

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])