From 6f1922500bc9e2c6d53c46dfcbd420687dfe6e6b Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 20 Jun 2017 07:40:24 +0000
Subject: [PATCH 01/12] Initial attempt at refactoring API scope declarations.

- Declaring an endpoint's scopes in a `before` block has proved to be
  unreliable. For example, if we're accessing the `API::Users` endpoint - code
  in a `before` block in `API::API` wouldn't be able to see the scopes set in
  `API::Users` since the `API::API` `before` block runs first.

- This commit moves these declarations to the class level, since they don't need
  to change once set.
---
 .../access_token_validation_service.rb        |  5 ++-
 lib/api/api.rb                                |  3 +-
 lib/api/api_guard.rb                          | 33 ++++++++++++-------
 lib/api/helpers.rb                            |  6 ++--
 lib/api/users.rb                              |  4 ++-
 lib/api/v3/users.rb                           |  4 ++-
 spec/requests/api/users_spec.rb               | 22 +++++++++++++
 spec/support/api_helpers.rb                   |  6 ++--
 8 files changed, 63 insertions(+), 20 deletions(-)

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index b2a543daa00..f171f8194bd 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -31,8 +31,11 @@ class AccessTokenValidationService
     if scopes.blank?
       true
     else
+      #scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) }
       # Check whether the token is allowed access to any of the required scopes.
-      Set.new(scopes).intersection(Set.new(token.scopes)).present?
+
+      scope_names = scopes.map { |scope| scope[:name].to_s }
+      Set.new(scope_names).intersection(Set.new(token.scopes)).present?
     end
   end
 end
diff --git a/lib/api/api.rb b/lib/api/api.rb
index d767af36e8e..efcf0976a81 100644
--- a/lib/api/api.rb
+++ b/lib/api/api.rb
@@ -2,6 +2,8 @@ module API
   class API < Grape::API
     include APIGuard
 
+    allow_access_with_scope :api
+
     version %w(v3 v4), using: :path
 
     version 'v3', using: :path do
@@ -44,7 +46,6 @@ module API
       mount ::API::V3::Variables
     end
 
-    before { allow_access_with_scope :api }
     before { header['X-Frame-Options'] = 'SAMEORIGIN' }
     before { Gitlab::I18n.locale = current_user&.preferred_language }
 
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 9fcf04efa38..9a9e32a0242 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -23,6 +23,27 @@ module API
       install_error_responders(base)
     end
 
+    class_methods do
+      # Set the authorization scope(s) allowed for the current request.
+      #
+      # A call to this method adds to any previous scopes in place, either from the same class, or
+      # higher up in the inheritance chain. For example, if we call `allow_access_with_scope :api` from
+      # `API::API`, and `allow_access_with_scope :read_user` from `API::Users` (which inherits from `API::API`),
+      # `API::Users` will allow access with either the `api` or `read_user` scope. `API::API` will allow
+      # access only with the `api` scope.
+      def allow_access_with_scope(scopes, options = {})
+        @scopes ||= []
+
+        params = Array.wrap(scopes).map { |scope| { name: scope, if: options[:if] } }
+
+        @scopes.concat(params)
+      end
+
+      def scopes
+        @scopes
+      end
+    end
+
     # Helper Methods for Grape Endpoint
     module HelperMethods
       # Invokes the doorkeeper guard.
@@ -74,18 +95,6 @@ module API
         @current_user
       end
 
-      # Set the authorization scope(s) allowed for the current request.
-      #
-      # Note: A call to this method adds to any previous scopes in place. This is done because
-      # `Grape` callbacks run from the outside-in: the top-level callback (API::API) runs first, then
-      # the next-level callback (API::API::Users, for example) runs. All these scopes are valid for the
-      # given endpoint (GET `/api/users` is accessible by the `api` and `read_user` scopes), and so they
-      # need to be stored.
-      def allow_access_with_scope(*scopes)
-        @scopes ||= []
-        @scopes.concat(scopes.map(&:to_s))
-      end
-
       private
 
       def find_user_by_authentication_token(token_string)
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 2c73a6fdc4e..3cf04e6df3c 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -340,10 +340,12 @@ module API
     end
 
     def initial_current_user
+      endpoint_class = options[:for]
+
       return @initial_current_user if defined?(@initial_current_user)
       Gitlab::Auth::UniqueIpsLimiter.limit_user! do
-        @initial_current_user ||= find_user_by_private_token(scopes: @scopes)
-        @initial_current_user ||= doorkeeper_guard(scopes: @scopes)
+        @initial_current_user ||= find_user_by_private_token(scopes: endpoint_class.scopes)
+        @initial_current_user ||= doorkeeper_guard(scopes: endpoint_class.scopes)
         @initial_current_user ||= find_user_from_warden
 
         unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?
diff --git a/lib/api/users.rb b/lib/api/users.rb
index f9555842daf..2cac8c089f2 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -1,9 +1,11 @@
 module API
   class Users < Grape::API
     include PaginationParams
+    include APIGuard
+
+    allow_access_with_scope :read_user, if: -> (request) { request.get? }
 
     before do
-      allow_access_with_scope :read_user if request.get?
       authenticate!
     end
 
diff --git a/lib/api/v3/users.rb b/lib/api/v3/users.rb
index 37020019e07..cf106f2552d 100644
--- a/lib/api/v3/users.rb
+++ b/lib/api/v3/users.rb
@@ -2,9 +2,11 @@ module API
   module V3
     class Users < Grape::API
       include PaginationParams
+      include APIGuard
+
+      allow_access_with_scope :read_user, if: -> (request) { request.get? }
 
       before do
-        allow_access_with_scope :read_user if request.get?
         authenticate!
       end
 
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index c0174b304c8..c8e22799ba4 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -50,6 +50,28 @@ describe API::Users do
         end['username']).to eq(username)
       end
 
+      context "scopes" do
+        context 'when the requesting token has the "read_user" scope' do
+          let(:token) { create(:personal_access_token, scopes: ['read_user']) }
+
+          it 'returns a "200" response' do
+            get api("/users", user, personal_access_token: token)
+
+            expect(response).to have_http_status(200)
+          end
+        end
+
+        context 'when the requesting token does not have any required scope' do
+          let(:token) { create(:personal_access_token, scopes: ['read_registry']) }
+
+          it 'returns a "401" response' do
+            get api("/users", user, personal_access_token: token)
+
+            expect(response).to have_http_status(401)
+          end
+        end
+      end
+
       it "returns an array of blocked users" do
         ldap_blocked_user
         create(:user, state: 'blocked')
diff --git a/spec/support/api_helpers.rb b/spec/support/api_helpers.rb
index 35d1e1cfc7d..163979a2a28 100644
--- a/spec/support/api_helpers.rb
+++ b/spec/support/api_helpers.rb
@@ -17,14 +17,16 @@ module ApiHelpers
   #   => "/api/v2/issues?foo=bar&private_token=..."
   #
   # Returns the relative path to the requested API resource
-  def api(path, user = nil, version: API::API.version)
+  def api(path, user = nil, version: API::API.version, personal_access_token: nil)
     "/api/#{version}#{path}" +
 
       # Normalize query string
       (path.index('?') ? '' : '?') +
 
+      if personal_access_token.present?
+        "&private_token=#{personal_access_token.token}"
       # Append private_token if given a User object
-      if user.respond_to?(:private_token)
+      elsif user.respond_to?(:private_token)
         "&private_token=#{user.private_token}"
       else
         ''

From 80c1ebaa83f346e45346baac584f21878652c350 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 20 Jun 2017 08:27:45 +0000
Subject: [PATCH 02/12] Allow API scope declarations to be applied
 conditionally.

- Scope declarations of the form:

    allow_access_with_scope :read_user, if: -> (request) { request.get? }

  will only apply for `GET` requests

- Add a negative test to a `POST` endpoint in the `users` API to test this. Also
  test for this case in the `AccessTokenValidationService` unit tests.
---
 .../access_token_validation_service.rb        | 15 ++++----
 lib/api/api_guard.rb                          |  4 +--
 lib/api/helpers.rb                            |  2 +-
 spec/requests/api/helpers_spec.rb             |  3 +-
 spec/requests/api/users_spec.rb               | 10 ++++++
 .../access_token_validation_service_spec.rb   | 36 +++++++++++++++----
 6 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index f171f8194bd..6d39ad245d2 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -5,10 +5,11 @@ class AccessTokenValidationService
   REVOKED = :revoked
   INSUFFICIENT_SCOPE = :insufficient_scope
 
-  attr_reader :token
+  attr_reader :token, :request
 
-  def initialize(token)
+  def initialize(token, request)
     @token = token
+    @request = request
   end
 
   def validate(scopes: [])
@@ -31,11 +32,13 @@ class AccessTokenValidationService
     if scopes.blank?
       true
     else
-      #scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) }
-      # Check whether the token is allowed access to any of the required scopes.
+      # Remove any scopes whose `if` condition does not return `true`
+      scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) }
 
-      scope_names = scopes.map { |scope| scope[:name].to_s }
-      Set.new(scope_names).intersection(Set.new(token.scopes)).present?
+      # Check whether the token is allowed access to any of the required scopes.
+      passed_scope_names = scopes.map { |scope| scope[:name].to_sym }
+      token_scope_names = token.scopes.map(&:to_sym)
+      Set.new(passed_scope_names).intersection(Set.new(token_scope_names)).present?
     end
   end
 end
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 9a9e32a0242..ceeecbbc00b 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -68,7 +68,7 @@ module API
         access_token = find_access_token
         return nil unless access_token
 
-        case AccessTokenValidationService.new(access_token).validate(scopes: scopes)
+        case AccessTokenValidationService.new(access_token, request).validate(scopes: scopes)
         when AccessTokenValidationService::INSUFFICIENT_SCOPE
           raise InsufficientScopeError.new(scopes)
 
@@ -105,7 +105,7 @@ module API
         access_token = PersonalAccessToken.active.find_by_token(token_string)
         return unless access_token
 
-        if AccessTokenValidationService.new(access_token).include_any_scope?(scopes)
+        if AccessTokenValidationService.new(access_token, request).include_any_scope?(scopes)
           User.find(access_token.user_id)
         end
       end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 3cf04e6df3c..c69e7afea8c 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -340,7 +340,7 @@ module API
     end
 
     def initial_current_user
-      endpoint_class = options[:for]
+      endpoint_class = options[:for].presence || ::API::API
 
       return @initial_current_user if defined?(@initial_current_user)
       Gitlab::Auth::UniqueIpsLimiter.limit_user! do
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 191c60aba31..87d6f46533e 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -14,6 +14,8 @@ describe API::Helpers do
   let(:request) { Rack::Request.new(env) }
   let(:header) { }
 
+  before { allow_any_instance_of(self.class).to receive(:options).and_return({}) }
+
   def set_env(user_or_token, identifier)
     clear_env
     clear_param
@@ -167,7 +169,6 @@ describe API::Helpers do
       it "returns nil for a token without the appropriate scope" do
         personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user'])
         env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
-        allow_access_with_scope('write_user')
 
         expect(current_user).to be_nil
       end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index c8e22799ba4..982c1a50e3b 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -321,6 +321,16 @@ describe API::Users do
         .to eq([Gitlab::PathRegex.namespace_format_message])
     end
 
+    context 'when the requesting token has the "read_user" scope' do
+      let(:token) { create(:personal_access_token, scopes: ['read_user'], user: admin) }
+
+      it 'returns a "401" response' do
+        post api("/users", admin, personal_access_token: token), attributes_for(:user, projects_limit: 3)
+
+        expect(response).to have_http_status(401)
+      end
+    end
+
     it "is not available for non admin users" do
       post api("/users", user), attributes_for(:user)
       expect(response).to have_http_status(403)
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 87f093ee8ce..0023678dc3b 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -2,40 +2,64 @@ require 'spec_helper'
 
 describe AccessTokenValidationService, services: true do
   describe ".include_any_scope?" do
+    let(:request) { double("request") }
+
     it "returns true if the required scope is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user])
 
-      expect(described_class.new(token).include_any_scope?([:api])).to be(true)
+      expect(described_class.new(token, request).include_any_scope?([{ name: :api }])).to be(true)
     end
 
     it "returns true if more than one of the required scopes is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
 
-      expect(described_class.new(token).include_any_scope?([:api, :other_scope])).to be(true)
+      expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :other_scope }])).to be(true)
     end
 
     it "returns true if the list of required scopes is an exact match for the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
 
-      expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true)
+      expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
     end
 
     it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do
       token = double("token", scopes: [:api, :read_user])
 
-      expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true)
+      expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
     end
 
     it 'returns true if the list of required scopes is blank' do
       token = double("token", scopes: [])
 
-      expect(described_class.new(token).include_any_scope?([])).to be(true)
+      expect(described_class.new(token, request).include_any_scope?([])).to be(true)
     end
 
     it "returns false if there are no scopes in common between the required scopes and the token scopes" do
       token = double("token", scopes: [:api, :read_user])
 
-      expect(described_class.new(token).include_any_scope?([:other_scope])).to be(false)
+      expect(described_class.new(token, request).include_any_scope?([{ name: :other_scope }])).to be(false)
+    end
+
+    context "conditions" do
+      context "if" do
+        it "ignores any scopes whose `if` condition returns false" do
+          token = double("token", scopes: [:api, :read_user])
+
+          expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false)
+        end
+
+        it "does not ignore scopes whose `if` condition is not set" do
+          token = double("token", scopes: [:api, :read_user])
+
+          expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
+        end
+
+        it "does not ignore scopes whose `if` condition returns true" do
+          token = double("token", scopes: [:api, :read_user])
+
+          expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
+        end
+      end
     end
   end
 end

From 157c05f49da1d6992d6b491e4fba8d90a7d821c8 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 20 Jun 2017 09:35:59 +0000
Subject: [PATCH 03/12] Test `/users` endpoints for the `read_user` scope.

- Test `GET` endpoints to check that the scope is allowed.
- Test `POST` endpoints to check that the scope is disallowed.
- Test both `v3` and `v4` endpoints.
---
 spec/requests/api/users_spec.rb               | 54 ++++++++-----------
 spec/requests/api/v3/users_spec.rb            | 21 ++++++++
 .../api/scopes/read_user_shared_examples.rb   | 33 ++++++++++++
 spec/support/api_helpers.rb                   |  4 +-
 4 files changed, 78 insertions(+), 34 deletions(-)
 create mode 100644 spec/support/api/scopes/read_user_shared_examples.rb

diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 982c1a50e3b..f0edc06cd0b 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -50,28 +50,6 @@ describe API::Users do
         end['username']).to eq(username)
       end
 
-      context "scopes" do
-        context 'when the requesting token has the "read_user" scope' do
-          let(:token) { create(:personal_access_token, scopes: ['read_user']) }
-
-          it 'returns a "200" response' do
-            get api("/users", user, personal_access_token: token)
-
-            expect(response).to have_http_status(200)
-          end
-        end
-
-        context 'when the requesting token does not have any required scope' do
-          let(:token) { create(:personal_access_token, scopes: ['read_registry']) }
-
-          it 'returns a "401" response' do
-            get api("/users", user, personal_access_token: token)
-
-            expect(response).to have_http_status(401)
-          end
-        end
-      end
-
       it "returns an array of blocked users" do
         ldap_blocked_user
         create(:user, state: 'blocked')
@@ -104,6 +82,13 @@ describe API::Users do
 
         expect(json_response.first.keys).not_to include 'is_admin'
       end
+
+      context "scopes" do
+        let(:path) { "/users" }
+        let(:api_call) { method(:api) }
+
+        include_examples 'allows the "read_user" scope'
+      end
     end
 
     context "when admin" do
@@ -186,6 +171,13 @@ describe API::Users do
 
       expect(response).to have_http_status(404)
     end
+
+    context "scopes" do
+      let(:path) { "/users/#{user.id}" }
+      let(:api_call) { method(:api) }
+
+      include_examples 'allows the "read_user" scope'
+    end
   end
 
   describe "POST /users" do
@@ -321,16 +313,6 @@ describe API::Users do
         .to eq([Gitlab::PathRegex.namespace_format_message])
     end
 
-    context 'when the requesting token has the "read_user" scope' do
-      let(:token) { create(:personal_access_token, scopes: ['read_user'], user: admin) }
-
-      it 'returns a "401" response' do
-        post api("/users", admin, personal_access_token: token), attributes_for(:user, projects_limit: 3)
-
-        expect(response).to have_http_status(401)
-      end
-    end
-
     it "is not available for non admin users" do
       post api("/users", user), attributes_for(:user)
       expect(response).to have_http_status(403)
@@ -377,6 +359,14 @@ describe API::Users do
         expect(json_response['identities'].first['provider']).to eq('github')
       end
     end
+
+    context "scopes" do
+      let(:user) { admin }
+      let(:path) { '/users' }
+      let(:api_call) { method(:api) }
+
+      include_examples 'does not allow the "read_user" scope'
+    end
   end
 
   describe "GET /users/sign_up" do
diff --git a/spec/requests/api/v3/users_spec.rb b/spec/requests/api/v3/users_spec.rb
index 6d7401f9764..b2c5003c97a 100644
--- a/spec/requests/api/v3/users_spec.rb
+++ b/spec/requests/api/v3/users_spec.rb
@@ -67,6 +67,19 @@ describe API::V3::Users do
         expect(json_response.first['title']).to eq(key.title)
       end
     end
+
+    context "scopes" do
+      let(:user) { admin }
+      let(:path) { "/users/#{user.id}/keys" }
+      let(:api_call) { method(:v3_api) }
+
+      before do
+        user.keys << key
+        user.save
+      end
+
+      include_examples 'allows the "read_user" scope'
+    end
   end
 
   describe 'GET /user/:id/emails' do
@@ -312,5 +325,13 @@ describe API::V3::Users do
 
       expect(json_response['is_admin']).to be_nil
     end
+
+    context "scopes" do
+      let(:user) { admin }
+      let(:path) { '/users' }
+      let(:api_call) { method(:v3_api) }
+
+      include_examples 'does not allow the "read_user" scope'
+    end
   end
 end
diff --git a/spec/support/api/scopes/read_user_shared_examples.rb b/spec/support/api/scopes/read_user_shared_examples.rb
new file mode 100644
index 00000000000..bb5f493f3fd
--- /dev/null
+++ b/spec/support/api/scopes/read_user_shared_examples.rb
@@ -0,0 +1,33 @@
+shared_examples_for 'allows the "read_user" scope' do
+  describe 'when the requesting token has the "read_user" scope' do
+    let(:token) { create(:personal_access_token, scopes: ['read_user'], user: user) }
+
+    it 'returns a "200" response' do
+      get api_call.call(path, user, personal_access_token: token)
+
+      expect(response).to have_http_status(200)
+    end
+  end
+
+  describe 'when the requesting token does not have any required scope' do
+    let(:token) { create(:personal_access_token, scopes: ['read_registry'], user: user) }
+
+    it 'returns a "401" response' do
+      get api_call.call(path, user, personal_access_token: token)
+
+      expect(response).to have_http_status(401)
+    end
+  end
+end
+
+shared_examples_for 'does not allow the "read_user" scope' do
+  context 'when the requesting token has the "read_user" scope' do
+    let(:token) { create(:personal_access_token, scopes: ['read_user'], user: user) }
+
+    it 'returns a "401" response' do
+      post api_call.call(path, user, personal_access_token: token), attributes_for(:user, projects_limit: 3)
+
+      expect(response).to have_http_status(401)
+    end
+  end
+end
diff --git a/spec/support/api_helpers.rb b/spec/support/api_helpers.rb
index 163979a2a28..1becd302d77 100644
--- a/spec/support/api_helpers.rb
+++ b/spec/support/api_helpers.rb
@@ -34,8 +34,8 @@ module ApiHelpers
   end
 
   # Temporary helper method for simplifying V3 exclusive API specs
-  def v3_api(path, user = nil)
-    api(path, user, version: 'v3')
+  def v3_api(path, user = nil, personal_access_token: nil)
+    api(path, user, version: 'v3', personal_access_token: personal_access_token)
   end
 
   def ci_api(path, user = nil)

From d774825f981a73263c9a6c276c672b0c3e9bf104 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 20 Jun 2017 12:00:57 +0000
Subject: [PATCH 04/12] When verifying scopes, manually include scopes from
 `API::API`.

- They are not included automatically since `API::Users` does not inherit from
  `API::API`, as I initially assumed.

- Scopes declared in `API::API` are considered global (to the API), and need to
  be included in all cases.
---
 lib/api/api_guard.rb | 10 ++++------
 lib/api/helpers.rb   | 23 +++++++++++++++++++----
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index ceeecbbc00b..29ca760ec25 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -24,13 +24,11 @@ module API
     end
 
     class_methods do
-      # Set the authorization scope(s) allowed for the current request.
+      # Set the authorization scope(s) allowed for an API endpoint.
       #
-      # A call to this method adds to any previous scopes in place, either from the same class, or
-      # higher up in the inheritance chain. For example, if we call `allow_access_with_scope :api` from
-      # `API::API`, and `allow_access_with_scope :read_user` from `API::Users` (which inherits from `API::API`),
-      # `API::Users` will allow access with either the `api` or `read_user` scope. `API::API` will allow
-      # access only with the `api` scope.
+      # A call to this method maps the given scope(s) to the current API
+      # endpoint class. If this method is called multiple times on the same class,
+      # the scopes are all aggregated.
       def allow_access_with_scope(scopes, options = {})
         @scopes ||= []
 
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index c69e7afea8c..5c0b82587ab 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -340,12 +340,10 @@ module API
     end
 
     def initial_current_user
-      endpoint_class = options[:for].presence || ::API::API
-
       return @initial_current_user if defined?(@initial_current_user)
       Gitlab::Auth::UniqueIpsLimiter.limit_user! do
-        @initial_current_user ||= find_user_by_private_token(scopes: endpoint_class.scopes)
-        @initial_current_user ||= doorkeeper_guard(scopes: endpoint_class.scopes)
+        @initial_current_user ||= find_user_by_private_token(scopes: scopes_registered_for_endpoint)
+        @initial_current_user ||= doorkeeper_guard(scopes: scopes_registered_for_endpoint)
         @initial_current_user ||= find_user_from_warden
 
         unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?
@@ -409,5 +407,22 @@ module API
 
       exception.status == 500
     end
+
+    # An array of scopes that were registered (using `allow_access_with_scope`)
+    # for the current endpoint class. It also returns scopes registered on
+    # `API::API`, since these are meant to apply to all API routes.
+    def scopes_registered_for_endpoint
+      @scopes_registered_for_endpoint ||=
+        begin
+          endpoint_classes = [options[:for].presence, ::API::API].compact
+          endpoint_classes.reduce([]) do |memo, endpoint|
+            if endpoint.respond_to?(:scopes)
+              memo.concat(endpoint.scopes)
+            else
+              memo
+            end
+          end
+        end
+    end
   end
 end

From 0ff1d161920a083e07b5f1629aa395642609b251 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 20 Jun 2017 12:02:25 +0000
Subject: [PATCH 05/12] Test OAuth token scope verification in the `API::Users`
 endpoint

---
 spec/requests/api/helpers_spec.rb             |  4 +-
 .../api/scopes/read_user_shared_examples.rb   | 67 ++++++++++++++++---
 spec/support/api_helpers.rb                   | 14 +++-
 3 files changed, 71 insertions(+), 14 deletions(-)

diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 87d6f46533e..25ec44fa036 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -14,7 +14,9 @@ describe API::Helpers do
   let(:request) { Rack::Request.new(env) }
   let(:header) { }
 
-  before { allow_any_instance_of(self.class).to receive(:options).and_return({}) }
+  before do
+    allow_any_instance_of(self.class).to receive(:options).and_return({})
+  end
 
   def set_env(user_or_token, identifier)
     clear_env
diff --git a/spec/support/api/scopes/read_user_shared_examples.rb b/spec/support/api/scopes/read_user_shared_examples.rb
index bb5f493f3fd..cae6099a0c2 100644
--- a/spec/support/api/scopes/read_user_shared_examples.rb
+++ b/spec/support/api/scopes/read_user_shared_examples.rb
@@ -1,21 +1,68 @@
 shared_examples_for 'allows the "read_user" scope' do
-  describe 'when the requesting token has the "read_user" scope' do
-    let(:token) { create(:personal_access_token, scopes: ['read_user'], user: user) }
+  context 'for personal access tokens' do
+    context 'when the requesting token has the "api" scope' do
+      let(:token) { create(:personal_access_token, scopes: ['api'], user: user) }
 
-    it 'returns a "200" response' do
-      get api_call.call(path, user, personal_access_token: token)
+      it 'returns a "200" response' do
+        get api_call.call(path, user, personal_access_token: token)
 
-      expect(response).to have_http_status(200)
+        expect(response).to have_http_status(200)
+      end
+    end
+
+    context 'when the requesting token has the "read_user" scope' do
+      let(:token) { create(:personal_access_token, scopes: ['read_user'], user: user) }
+
+      it 'returns a "200" response' do
+        get api_call.call(path, user, personal_access_token: token)
+
+        expect(response).to have_http_status(200)
+      end
+    end
+
+    context 'when the requesting token does not have any required scope' do
+      let(:token) { create(:personal_access_token, scopes: ['read_registry'], user: user) }
+
+      it 'returns a "401" response' do
+        get api_call.call(path, user, personal_access_token: token)
+
+        expect(response).to have_http_status(401)
+      end
     end
   end
 
-  describe 'when the requesting token does not have any required scope' do
-    let(:token) { create(:personal_access_token, scopes: ['read_registry'], user: user) }
+  context 'for doorkeeper (OAuth) tokens' do
+    let!(:user) {create(:user)}
+    let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) }
 
-    it 'returns a "401" response' do
-      get api_call.call(path, user, personal_access_token: token)
+    context 'when the requesting token has the "api" scope' do
+      let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "api" }
 
-      expect(response).to have_http_status(401)
+      it 'returns a "200" response' do
+        get api_call.call(path, user, oauth_access_token: token)
+
+        expect(response).to have_http_status(200)
+      end
+    end
+
+    context 'when the requesting token has the "read_user" scope' do
+      let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "read_user" }
+
+      it 'returns a "200" response' do
+        get api_call.call(path, user, oauth_access_token: token)
+
+        expect(response).to have_http_status(200)
+      end
+    end
+
+    context 'when the requesting token does not have any required scope' do
+      let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "invalid" }
+
+      it 'returns a "403" response' do
+        get api_call.call(path, user, oauth_access_token: token)
+
+        expect(response).to have_http_status(403)
+      end
     end
   end
 end
diff --git a/spec/support/api_helpers.rb b/spec/support/api_helpers.rb
index 1becd302d77..ac0aaa524b7 100644
--- a/spec/support/api_helpers.rb
+++ b/spec/support/api_helpers.rb
@@ -17,7 +17,7 @@ module ApiHelpers
   #   => "/api/v2/issues?foo=bar&private_token=..."
   #
   # Returns the relative path to the requested API resource
-  def api(path, user = nil, version: API::API.version, personal_access_token: nil)
+  def api(path, user = nil, version: API::API.version, personal_access_token: nil, oauth_access_token: nil)
     "/api/#{version}#{path}" +
 
       # Normalize query string
@@ -25,6 +25,8 @@ module ApiHelpers
 
       if personal_access_token.present?
         "&private_token=#{personal_access_token.token}"
+      elsif oauth_access_token.present?
+        "&access_token=#{oauth_access_token.token}"
       # Append private_token if given a User object
       elsif user.respond_to?(:private_token)
         "&private_token=#{user.private_token}"
@@ -34,8 +36,14 @@ module ApiHelpers
   end
 
   # Temporary helper method for simplifying V3 exclusive API specs
-  def v3_api(path, user = nil, personal_access_token: nil)
-    api(path, user, version: 'v3', personal_access_token: personal_access_token)
+  def v3_api(path, user = nil, personal_access_token: nil, oauth_access_token: nil)
+    api(
+      path,
+      user,
+      version: 'v3',
+      personal_access_token: personal_access_token,
+      oauth_access_token: oauth_access_token
+    )
   end
 
   def ci_api(path, user = nil)

From 8b399b185cf72f396be8d6b7caae37f2a3aa4279 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 21 Jun 2017 07:13:36 +0000
Subject: [PATCH 06/12] Add CHANGELOG entry for CE MR 12300

---
 changelogs/unreleased/33580-fix-api-scoping.yml | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changelogs/unreleased/33580-fix-api-scoping.yml

diff --git a/changelogs/unreleased/33580-fix-api-scoping.yml b/changelogs/unreleased/33580-fix-api-scoping.yml
new file mode 100644
index 00000000000..f4ebb13c082
--- /dev/null
+++ b/changelogs/unreleased/33580-fix-api-scoping.yml
@@ -0,0 +1,4 @@
+---
+title: Fix API Scoping
+merge_request: 12300
+author:

From 1b8223dd51345f6075172a92dab610f9dee89d84 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 21 Jun 2017 09:22:39 +0000
Subject: [PATCH 07/12] Fix remaining spec failures for !12300.

1. Get the spec for `lib/gitlab/auth.rb` passing.

  - Make the `request` argument to `AccessTokenValidationService` optional -
  `auth.rb` doesn't need to pass in a request.

  - Pass in scopes in the format `[{ name: 'api' }]` rather than `['api']`, which
  is what `AccessTokenValidationService` now expects.

2. Get the spec for `API::V3::Users` passing

2. Get the spec for `AccessTokenValidationService` passing
---
 .../access_token_validation_service.rb         |  2 +-
 lib/api/api_guard.rb                           |  4 ++--
 lib/gitlab/auth.rb                             |  4 ++--
 spec/requests/api/v3/users_spec.rb             |  2 +-
 .../access_token_validation_service_spec.rb    | 18 +++++++++---------
 .../api/scopes/read_user_shared_examples.rb    |  1 -
 6 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index 6d39ad245d2..450e90d947d 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -7,7 +7,7 @@ class AccessTokenValidationService
 
   attr_reader :token, :request
 
-  def initialize(token, request)
+  def initialize(token, request: nil)
     @token = token
     @request = request
   end
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 29ca760ec25..d4599aaeed0 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -66,7 +66,7 @@ module API
         access_token = find_access_token
         return nil unless access_token
 
-        case AccessTokenValidationService.new(access_token, request).validate(scopes: scopes)
+        case AccessTokenValidationService.new(access_token, request: request).validate(scopes: scopes)
         when AccessTokenValidationService::INSUFFICIENT_SCOPE
           raise InsufficientScopeError.new(scopes)
 
@@ -103,7 +103,7 @@ module API
         access_token = PersonalAccessToken.active.find_by_token(token_string)
         return unless access_token
 
-        if AccessTokenValidationService.new(access_token, request).include_any_scope?(scopes)
+        if AccessTokenValidationService.new(access_token, request: request).include_any_scope?(scopes)
           User.find(access_token.user_id)
         end
       end
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 3933c3b04dd..37ac8ecc2f0 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -130,13 +130,13 @@ module Gitlab
 
         token = PersonalAccessTokensFinder.new(state: 'active').find_by(token: password)
 
-        if token && valid_scoped_token?(token, AVAILABLE_SCOPES.map(&:to_s))
+        if token && valid_scoped_token?(token, AVAILABLE_SCOPES.map { |scope| { name: scope.to_s }})
           Gitlab::Auth::Result.new(token.user, nil, :personal_token, abilities_for_scope(token.scopes))
         end
       end
 
       def valid_oauth_token?(token)
-        token && token.accessible? && valid_scoped_token?(token, ["api"])
+        token && token.accessible? && valid_scoped_token?(token, [{ name: "api" }])
       end
 
       def valid_scoped_token?(token, scopes)
diff --git a/spec/requests/api/v3/users_spec.rb b/spec/requests/api/v3/users_spec.rb
index b2c5003c97a..de7499a4e43 100644
--- a/spec/requests/api/v3/users_spec.rb
+++ b/spec/requests/api/v3/users_spec.rb
@@ -300,7 +300,7 @@ describe API::V3::Users do
     end
 
     it 'returns a 404 error if not found' do
-      get v3_api('/users/42/events', user)
+      get v3_api('/users/420/events', user)
 
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 User Not Found')
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 0023678dc3b..eff4269a4d5 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -7,37 +7,37 @@ describe AccessTokenValidationService, services: true do
     it "returns true if the required scope is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user])
 
-      expect(described_class.new(token, request).include_any_scope?([{ name: :api }])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }])).to be(true)
     end
 
     it "returns true if more than one of the required scopes is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
 
-      expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :other_scope }])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :other_scope }])).to be(true)
     end
 
     it "returns true if the list of required scopes is an exact match for the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
 
-      expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
     end
 
     it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do
       token = double("token", scopes: [:api, :read_user])
 
-      expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
     end
 
     it 'returns true if the list of required scopes is blank' do
       token = double("token", scopes: [])
 
-      expect(described_class.new(token, request).include_any_scope?([])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?([])).to be(true)
     end
 
     it "returns false if there are no scopes in common between the required scopes and the token scopes" do
       token = double("token", scopes: [:api, :read_user])
 
-      expect(described_class.new(token, request).include_any_scope?([{ name: :other_scope }])).to be(false)
+      expect(described_class.new(token, request: request).include_any_scope?([{ name: :other_scope }])).to be(false)
     end
 
     context "conditions" do
@@ -45,19 +45,19 @@ describe AccessTokenValidationService, services: true do
         it "ignores any scopes whose `if` condition returns false" do
           token = double("token", scopes: [:api, :read_user])
 
-          expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false)
+          expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false)
         end
 
         it "does not ignore scopes whose `if` condition is not set" do
           token = double("token", scopes: [:api, :read_user])
 
-          expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
+          expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
         end
 
         it "does not ignore scopes whose `if` condition returns true" do
           token = double("token", scopes: [:api, :read_user])
 
-          expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
+          expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
         end
       end
     end
diff --git a/spec/support/api/scopes/read_user_shared_examples.rb b/spec/support/api/scopes/read_user_shared_examples.rb
index cae6099a0c2..3bd589d64b9 100644
--- a/spec/support/api/scopes/read_user_shared_examples.rb
+++ b/spec/support/api/scopes/read_user_shared_examples.rb
@@ -32,7 +32,6 @@ shared_examples_for 'allows the "read_user" scope' do
   end
 
   context 'for doorkeeper (OAuth) tokens' do
-    let!(:user) {create(:user)}
     let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) }
 
     context 'when the requesting token has the "api" scope' do

From 4dbfa14e160e0d9bca11941adcf04b3d272aa1a2 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 23 Jun 2017 11:18:44 +0000
Subject: [PATCH 08/12] Implement review comments from @dbalexandre for !12300.

---
 lib/api/api_guard.rb                          | 12 ++++------
 lib/api/helpers.rb                            |  4 ++--
 .../access_token_validation_service_spec.rb   | 24 +++++++++----------
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index d4599aaeed0..8411ad8ec34 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -30,15 +30,13 @@ module API
       # endpoint class. If this method is called multiple times on the same class,
       # the scopes are all aggregated.
       def allow_access_with_scope(scopes, options = {})
-        @scopes ||= []
-
-        params = Array.wrap(scopes).map { |scope| { name: scope, if: options[:if] } }
-
-        @scopes.concat(params)
+        Array(scopes).each do |scope|
+          allowed_scopes << { name: scope, if: options[:if] }
+        end
       end
 
-      def scopes
-        @scopes
+      def allowed_scopes
+        @scopes ||= []
       end
     end
 
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 5c0b82587ab..a2a661b205c 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -416,8 +416,8 @@ module API
         begin
           endpoint_classes = [options[:for].presence, ::API::API].compact
           endpoint_classes.reduce([]) do |memo, endpoint|
-            if endpoint.respond_to?(:scopes)
-              memo.concat(endpoint.scopes)
+            if endpoint.respond_to?(:allowed_scopes)
+              memo.concat(endpoint.allowed_scopes)
             else
               memo
             end
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index eff4269a4d5..c8189aa14d8 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -41,24 +41,22 @@ describe AccessTokenValidationService, services: true do
     end
 
     context "conditions" do
-      context "if" do
-        it "ignores any scopes whose `if` condition returns false" do
-          token = double("token", scopes: [:api, :read_user])
+      it "ignores any scopes whose `if` condition returns false" do
+        token = double("token", scopes: [:api, :read_user])
 
-          expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false)
-        end
+        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false)
+      end
 
-        it "does not ignore scopes whose `if` condition is not set" do
-          token = double("token", scopes: [:api, :read_user])
+      it "does not ignore scopes whose `if` condition is not set" do
+        token = double("token", scopes: [:api, :read_user])
 
-          expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
-        end
+        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
+      end
 
-        it "does not ignore scopes whose `if` condition returns true" do
-          token = double("token", scopes: [:api, :read_user])
+      it "does not ignore scopes whose `if` condition returns true" do
+        token = double("token", scopes: [:api, :read_user])
 
-          expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
-        end
+        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
       end
     end
   end

From c1fcd730cc9dbee5b41ce2a6a12f8d84416b1a4a Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 26 Jun 2017 04:14:10 +0000
Subject: [PATCH 09/12] Implement review comments from @DouweM for !12300.

- Use a struct for scopes, so we can call `scope.if` instead of `scope[:if]`

- Refactor the "remove scopes whose :if condition returns false" logic to use a
  `select` rather than a `reject`.
---
 .../access_token_validation_service.rb        |  4 +--
 lib/api/api_guard.rb                          |  2 +-
 lib/gitlab/auth.rb                            |  5 +--
 .../access_token_validation_service_spec.rb   | 31 +++++++++++++------
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index 450e90d947d..ee2e93a0d63 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -33,10 +33,10 @@ class AccessTokenValidationService
       true
     else
       # Remove any scopes whose `if` condition does not return `true`
-      scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) }
+      scopes = scopes.select { |scope| scope.if.nil? || scope.if.call(request) }
 
       # Check whether the token is allowed access to any of the required scopes.
-      passed_scope_names = scopes.map { |scope| scope[:name].to_sym }
+      passed_scope_names = scopes.map { |scope| scope.name.to_sym }
       token_scope_names = token.scopes.map(&:to_sym)
       Set.new(passed_scope_names).intersection(Set.new(token_scope_names)).present?
     end
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 8411ad8ec34..56f6da57555 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -31,7 +31,7 @@ module API
       # the scopes are all aggregated.
       def allow_access_with_scope(scopes, options = {})
         Array(scopes).each do |scope|
-          allowed_scopes << { name: scope, if: options[:if] }
+          allowed_scopes << OpenStruct.new(name: scope.to_sym, if: options[:if])
         end
       end
 
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 37ac8ecc2f0..ec73255b20a 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -130,16 +130,17 @@ module Gitlab
 
         token = PersonalAccessTokensFinder.new(state: 'active').find_by(token: password)
 
-        if token && valid_scoped_token?(token, AVAILABLE_SCOPES.map { |scope| { name: scope.to_s }})
+        if token && valid_scoped_token?(token, AVAILABLE_SCOPES)
           Gitlab::Auth::Result.new(token.user, nil, :personal_token, abilities_for_scope(token.scopes))
         end
       end
 
       def valid_oauth_token?(token)
-        token && token.accessible? && valid_scoped_token?(token, [{ name: "api" }])
+        token && token.accessible? && valid_scoped_token?(token, ['api'])
       end
 
       def valid_scoped_token?(token, scopes)
+        scopes = scopes.map { |scope| OpenStruct.new(name: scope) }
         AccessTokenValidationService.new(token).include_any_scope?(scopes)
       end
 
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index c8189aa14d8..279f4ed93ac 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -1,62 +1,75 @@
 require 'spec_helper'
 
 describe AccessTokenValidationService, services: true do
+  def scope(data)
+    OpenStruct.new(data)
+  end
+
   describe ".include_any_scope?" do
     let(:request) { double("request") }
 
     it "returns true if the required scope is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user])
+      scopes = [scope({ name: :api })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if more than one of the required scopes is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
+      scopes = [scope({ name: :api }), scope({ name: :other_scope })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :other_scope }])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if the list of required scopes is an exact match for the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
+      scopes = [scope({ name: :api }), scope({ name: :read_user }), scope({ name: :other_scope })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do
       token = double("token", scopes: [:api, :read_user])
+      scopes = [scope({ name: :api }), scope({ name: :read_user }), scope({ name: :other_scope })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it 'returns true if the list of required scopes is blank' do
       token = double("token", scopes: [])
+      scopes = []
 
-      expect(described_class.new(token, request: request).include_any_scope?([])).to be(true)
+      expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns false if there are no scopes in common between the required scopes and the token scopes" do
       token = double("token", scopes: [:api, :read_user])
+      scopes = [scope({ name: :other_scope })]
 
-      expect(described_class.new(token, request: request).include_any_scope?([{ name: :other_scope }])).to be(false)
+      expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
     end
 
     context "conditions" do
       it "ignores any scopes whose `if` condition returns false" do
         token = double("token", scopes: [:api, :read_user])
+        scopes = [scope({ name: :api, if: ->(_) { false } })]
 
-        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false)
+        expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
       end
 
       it "does not ignore scopes whose `if` condition is not set" do
         token = double("token", scopes: [:api, :read_user])
+        scopes = [scope({ name: :api, if: ->(_) { false } }), scope({ name: :read_user })]
 
-        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
+        expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
       end
 
       it "does not ignore scopes whose `if` condition returns true" do
         token = double("token", scopes: [:api, :read_user])
+        scopes = [scope({ name: :api, if: ->(_) { true } }), scope({ name: :read_user, if: ->(_) { false } })]
 
-        expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
+        expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
       end
     end
   end

From b8ec1f4201c74c500e4f7010b238c7920599da7a Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 28 Jun 2017 07:12:23 +0000
Subject: [PATCH 10/12] Extract a `Gitlab::Scope` class.

- To represent an authorization scope, such as `api` or `read_user`
- This is a better abstraction than the hash we were previously using.
---
 .../access_token_validation_service.rb        | 17 +++++++-------
 lib/api/api_guard.rb                          |  2 +-
 lib/api/scope.rb                              | 23 +++++++++++++++++++
 lib/gitlab/auth.rb                            |  4 ++--
 .../access_token_validation_service_spec.rb   | 20 +++++++---------
 5 files changed, 42 insertions(+), 24 deletions(-)
 create mode 100644 lib/api/scope.rb

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index ee2e93a0d63..bf5aef0055e 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -28,17 +28,16 @@ class AccessTokenValidationService
   end
 
   # True if the token's scope contains any of the passed scopes.
-  def include_any_scope?(scopes)
-    if scopes.blank?
+  def include_any_scope?(required_scopes)
+    if required_scopes.blank?
       true
     else
-      # Remove any scopes whose `if` condition does not return `true`
-      scopes = scopes.select { |scope| scope.if.nil? || scope.if.call(request) }
-
-      # Check whether the token is allowed access to any of the required scopes.
-      passed_scope_names = scopes.map { |scope| scope.name.to_sym }
-      token_scope_names = token.scopes.map(&:to_sym)
-      Set.new(passed_scope_names).intersection(Set.new(token_scope_names)).present?
+      # We're comparing each required_scope against all token scopes, which would
+      # take quadratic time. This consideration is irrelevant here because of the
+      # small number of records involved.
+      # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12300/#note_33689006
+      token_scopes = token.scopes.map(&:to_sym)
+      required_scopes.any? { |scope| scope.sufficient?(token_scopes, request) }
     end
   end
 end
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 56f6da57555..0d2d71e336a 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -31,7 +31,7 @@ module API
       # the scopes are all aggregated.
       def allow_access_with_scope(scopes, options = {})
         Array(scopes).each do |scope|
-          allowed_scopes << OpenStruct.new(name: scope.to_sym, if: options[:if])
+          allowed_scopes << Scope.new(scope, options)
         end
       end
 
diff --git a/lib/api/scope.rb b/lib/api/scope.rb
new file mode 100644
index 00000000000..c23846d1e7d
--- /dev/null
+++ b/lib/api/scope.rb
@@ -0,0 +1,23 @@
+# Encapsulate a scope used for authorization, such as `api`, or `read_user`
+module API
+  class Scope
+    attr_reader :name, :if
+
+    def initialize(name, options = {})
+      @name = name.to_sym
+      @if = options[:if]
+    end
+
+    # Are the `scopes` passed in sufficient to adequately authorize the passed
+    # request for the scope represented by the current instance of this class?
+    def sufficient?(scopes, request)
+      verify_if_condition(request) && scopes.include?(self.name)
+    end
+
+    private
+
+    def verify_if_condition(request)
+      self.if.nil? || self.if.call(request)
+    end
+  end
+end
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index ec73255b20a..6d0d638ba14 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -136,11 +136,11 @@ module Gitlab
       end
 
       def valid_oauth_token?(token)
-        token && token.accessible? && valid_scoped_token?(token, ['api'])
+        token && token.accessible? && valid_scoped_token?(token, [:api])
       end
 
       def valid_scoped_token?(token, scopes)
-        scopes = scopes.map { |scope| OpenStruct.new(name: scope) }
+        scopes = scopes.map { |scope| API::Scope.new(scope) }
         AccessTokenValidationService.new(token).include_any_scope?(scopes)
       end
 
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 279f4ed93ac..660a05e0b6d 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -1,37 +1,33 @@
 require 'spec_helper'
 
 describe AccessTokenValidationService, services: true do
-  def scope(data)
-    OpenStruct.new(data)
-  end
-
   describe ".include_any_scope?" do
     let(:request) { double("request") }
 
     it "returns true if the required scope is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [scope({ name: :api })]
+      scopes = [API::Scope.new(:api)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if more than one of the required scopes is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
-      scopes = [scope({ name: :api }), scope({ name: :other_scope })]
+      scopes = [API::Scope.new(:api), API::Scope.new(:other_scope)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if the list of required scopes is an exact match for the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
-      scopes = [scope({ name: :api }), scope({ name: :read_user }), scope({ name: :other_scope })]
+      scopes = [API::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [scope({ name: :api }), scope({ name: :read_user }), scope({ name: :other_scope })]
+      scopes = [API::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
@@ -45,7 +41,7 @@ describe AccessTokenValidationService, services: true do
 
     it "returns false if there are no scopes in common between the required scopes and the token scopes" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [scope({ name: :other_scope })]
+      scopes = [API::Scope.new(:other_scope)]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
     end
@@ -53,21 +49,21 @@ describe AccessTokenValidationService, services: true do
     context "conditions" do
       it "ignores any scopes whose `if` condition returns false" do
         token = double("token", scopes: [:api, :read_user])
-        scopes = [scope({ name: :api, if: ->(_) { false } })]
+        scopes = [API::Scope.new(:api, if: ->(_) { false })]
 
         expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
       end
 
       it "does not ignore scopes whose `if` condition is not set" do
         token = double("token", scopes: [:api, :read_user])
-        scopes = [scope({ name: :api, if: ->(_) { false } }), scope({ name: :read_user })]
+        scopes = [API::Scope.new(:api, if: ->(_) { false }), API::Scope.new(:read_user)]
 
         expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
       end
 
       it "does not ignore scopes whose `if` condition returns true" do
         token = double("token", scopes: [:api, :read_user])
-        scopes = [scope({ name: :api, if: ->(_) { true } }), scope({ name: :read_user, if: ->(_) { false } })]
+        scopes = [API::Scope.new(:api, if: ->(_) { true }), API::Scope.new(:read_user, if: ->(_) { false })]
 
         expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
       end

From afbc7520c296196d0f3f95d4a24a9e42c0e41f3c Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 30 Jun 2017 07:32:25 +0000
Subject: [PATCH 11/12] `AccessTokenValidationService` accepts `String` or
 `API::Scope` scopes.

- There's no need to use `API::Scope` for scopes that don't have `if`
  conditions, such as in `lib/gitlab/auth.rb`.
---
 app/services/access_token_validation_service.rb      |  9 ++++++++-
 lib/api/scope.rb                                     |  2 +-
 lib/gitlab/auth.rb                                   |  1 -
 .../services/access_token_validation_service_spec.rb | 12 ++++++------
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
index bf5aef0055e..9c00ea789ec 100644
--- a/app/services/access_token_validation_service.rb
+++ b/app/services/access_token_validation_service.rb
@@ -37,7 +37,14 @@ class AccessTokenValidationService
       # small number of records involved.
       # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12300/#note_33689006
       token_scopes = token.scopes.map(&:to_sym)
-      required_scopes.any? { |scope| scope.sufficient?(token_scopes, request) }
+
+      required_scopes.any? do |scope|
+        if scope.respond_to?(:sufficient?)
+          scope.sufficient?(token_scopes, request)
+        else
+          API::Scope.new(scope).sufficient?(token_scopes, request)
+        end
+      end
     end
   end
 end
diff --git a/lib/api/scope.rb b/lib/api/scope.rb
index c23846d1e7d..d5165b2e482 100644
--- a/lib/api/scope.rb
+++ b/lib/api/scope.rb
@@ -11,7 +11,7 @@ module API
     # Are the `scopes` passed in sufficient to adequately authorize the passed
     # request for the scope represented by the current instance of this class?
     def sufficient?(scopes, request)
-      verify_if_condition(request) && scopes.include?(self.name)
+      scopes.include?(self.name) && verify_if_condition(request)
     end
 
     private
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 6d0d638ba14..ccb5d886bab 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -140,7 +140,6 @@ module Gitlab
       end
 
       def valid_scoped_token?(token, scopes)
-        scopes = scopes.map { |scope| API::Scope.new(scope) }
         AccessTokenValidationService.new(token).include_any_scope?(scopes)
       end
 
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
index 660a05e0b6d..11225fad18a 100644
--- a/spec/services/access_token_validation_service_spec.rb
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -6,28 +6,28 @@ describe AccessTokenValidationService, services: true do
 
     it "returns true if the required scope is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [API::Scope.new(:api)]
+      scopes = [:api]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if more than one of the required scopes is present in the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
-      scopes = [API::Scope.new(:api), API::Scope.new(:other_scope)]
+      scopes = [:api, :other_scope]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if the list of required scopes is an exact match for the token's scopes" do
       token = double("token", scopes: [:api, :read_user, :other_scope])
-      scopes = [API::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)]
+      scopes = [:api, :read_user, :other_scope]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
 
     it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [API::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)]
+      scopes = [:api, :read_user, :other_scope]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
     end
@@ -41,7 +41,7 @@ describe AccessTokenValidationService, services: true do
 
     it "returns false if there are no scopes in common between the required scopes and the token scopes" do
       token = double("token", scopes: [:api, :read_user])
-      scopes = [API::Scope.new(:other_scope)]
+      scopes = [:other_scope]
 
       expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
     end
@@ -56,7 +56,7 @@ describe AccessTokenValidationService, services: true do
 
       it "does not ignore scopes whose `if` condition is not set" do
         token = double("token", scopes: [:api, :read_user])
-        scopes = [API::Scope.new(:api, if: ->(_) { false }), API::Scope.new(:read_user)]
+        scopes = [API::Scope.new(:api, if: ->(_) { false }), :read_user]
 
         expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
       end

From 94258a6500855ca37e42e442ede642091a8d4366 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 5 Jul 2017 03:44:40 +0000
Subject: [PATCH 12/12] Fix build for !12300.

- The `/users` and `/users/:id` APIs are now accessible without
  authentication (!12445), and so scopes are not relevant for these endpoints.

- Previously, we were testing our scope declaration against these two methods.
  This commit moves these tests to other `GET` user endpoints which still
  require authentication.
---
 spec/requests/api/users_spec.rb | 49 +++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index b8109ce401c..70b94a09e6b 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -113,13 +113,6 @@ describe API::Users do
 
         expect(json_response.first.keys).not_to include 'is_admin'
       end
-
-      context "scopes" do
-        let(:path) { "/users" }
-        let(:api_call) { method(:api) }
-
-        include_examples 'allows the "read_user" scope'
-      end
     end
 
     context "when admin" do
@@ -216,13 +209,6 @@ describe API::Users do
 
       expect(response).to have_http_status(404)
     end
-
-    context "scopes" do
-      let(:path) { "/users/#{user.id}" }
-      let(:api_call) { method(:api) }
-
-      include_examples 'allows the "read_user" scope'
-    end
   end
 
   describe "POST /users" do
@@ -909,6 +895,13 @@ describe API::Users do
         expect(response).to match_response_schema('public_api/v4/user/public')
         expect(json_response['id']).to eq(user.id)
       end
+
+      context "scopes" do
+        let(:path) { "/user" }
+        let(:api_call) { method(:api) }
+
+        include_examples 'allows the "read_user" scope'
+      end
     end
 
     context 'with admin' do
@@ -978,6 +971,13 @@ describe API::Users do
         expect(json_response).to be_an Array
         expect(json_response.first["title"]).to eq(key.title)
       end
+
+      context "scopes" do
+        let(:path) { "/user/keys" }
+        let(:api_call) { method(:api) }
+
+        include_examples 'allows the "read_user" scope'
+      end
     end
   end
 
@@ -1011,6 +1011,13 @@ describe API::Users do
 
       expect(response).to have_http_status(404)
     end
+
+    context "scopes" do
+      let(:path) { "/user/keys/#{key.id}" }
+      let(:api_call) { method(:api) }
+
+      include_examples 'allows the "read_user" scope'
+    end
   end
 
   describe "POST /user/keys" do
@@ -1100,6 +1107,13 @@ describe API::Users do
         expect(json_response).to be_an Array
         expect(json_response.first["email"]).to eq(email.email)
       end
+
+      context "scopes" do
+        let(:path) { "/user/emails" }
+        let(:api_call) { method(:api) }
+
+        include_examples 'allows the "read_user" scope'
+      end
     end
   end
 
@@ -1132,6 +1146,13 @@ describe API::Users do
 
       expect(response).to have_http_status(404)
     end
+
+    context "scopes" do
+      let(:path) { "/user/emails/#{email.id}" }
+      let(:api_call) { method(:api) }
+
+      include_examples 'allows the "read_user" scope'
+    end
   end
 
   describe "POST /user/emails" do