Calls to the API are checked for scope.

- Move the `Oauth2::AccessTokenValidationService` class to
  `AccessTokenValidationService`, since it is now being used for
  personal access token validation as well.

- Each API endpoint declares the scopes it accepts (if any). Currently,
  the top level API module declares the `api` scope, and the `Users` API
  module declares the `read_user` scope (for GET requests).

- Move the `find_user_by_private_token` from the API `Helpers` module to
  the `APIGuard` module, to avoid littering `Helpers` with more
  auth-related methods to support `find_user_by_private_token`

app/services/oauth2/access_token_validation_service.rb → app/services/access_token_validation_service.rb

-module Oauth2::AccessTokenValidationService
+module AccessTokenValidationService
   # Results:
   VALID = :valid
   EXPIRED = :expired
@@ -21,21 +21,13 @@ module Oauth2::AccessTokenValidationService
       end
     end
 
-    protected
-
-    # True if the token's scope is a superset of required scopes,
-    # or the required scopes is empty.
-    def sufficient_scope?(token, scopes)
-      if scopes.blank?
-        # if no any scopes required, the scopes of token is sufficient.
-        return true
+    # True if the token's scope contains any of the required scopes.
+    def sufficient_scope?(token, required_scopes)
+      if required_scopes.blank?
+        true
       else
-        # If there are scopes required, then check whether
-        # the set of authorized scopes is a superset of the set of required scopes
-        required_scopes = Set.new(scopes)
-        authorized_scopes = Set.new(token.scopes)
-
-        return authorized_scopes >= required_scopes
+        # Check whether the token is allowed access to any of the required scopes.
+        Set.new(required_scopes).intersection(Set.new(token.scopes)).present?
       end
     end
   end

config/initializers/doorkeeper.rb

@@ -52,8 +52,8 @@ Doorkeeper.configure do
   # Define access token scopes for your provider
   # For more information go to
   # https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes
-  default_scopes  :api
-  # optional_scopes :write, :update
+  default_scopes(*Gitlab::Auth::DEFAULT_SCOPES)
+  optional_scopes(*Gitlab::Auth::OPTIONAL_SCOPES)
 
   # Change the way client credentials are retrieved from the request object.
   # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then

config/locales/doorkeeper.en.yml

@@ -59,6 +59,7 @@ en:
           unknown: "The access token is invalid"
     scopes:
       api: Access your API
+      read_user: Read user information
 
     flash:
       applications:

lib/api/api.rb

@@ -3,6 +3,8 @@ module API
     include APIGuard
     version 'v3', using: :path
 
+    before { allow_access_with_scope :api }
+
     rescue_from Gitlab::Access::AccessDeniedError do
       rack_response({ 'message' => '403 Forbidden' }.to_json, 403)
     end

lib/api/api_guard.rb

@@ -6,6 +6,9 @@ module API
   module APIGuard
     extend ActiveSupport::Concern
 
+    PRIVATE_TOKEN_HEADER = "HTTP_PRIVATE_TOKEN"
+    PRIVATE_TOKEN_PARAM = :private_token
+
     included do |base|
       # OAuth2 Resource Server Authentication
       use Rack::OAuth2::Server::Resource::Bearer, 'The API' do |request|
@@ -41,30 +44,59 @@ module API
       #           Defaults to empty array.
       #
       def doorkeeper_guard(scopes: [])
-        access_token = find_access_token
-        return nil unless access_token
-
-        case validate_access_token(access_token, scopes)
-        when Oauth2::AccessTokenValidationService::INSUFFICIENT_SCOPE
-          raise InsufficientScopeError.new(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
+        end
+      end
 
-        when Oauth2::AccessTokenValidationService::EXPIRED
-          raise ExpiredError
+      def find_user_by_private_token(scopes: [])
+        token_string = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s
 
-        when Oauth2::AccessTokenValidationService::REVOKED
-          raise RevokedError
+        return nil unless token_string.present?
 
-        when Oauth2::AccessTokenValidationService::VALID
-          @current_user = User.find(access_token.resource_owner_id)
-        end
+        find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes)
       end
 
       def current_user
         @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)
+        User.find_by_authentication_token(token_string)
+      end
+
+      def find_user_by_personal_access_token(token_string, scopes)
+        access_token = PersonalAccessToken.active.find_by_token(token_string)
+        return unless access_token
+
+        if AccessTokenValidationService.sufficient_scope?(access_token, scopes)
+          User.find(access_token.user_id)
+        end
+      end
+
       def find_access_token
         @access_token ||= Doorkeeper.authenticate(doorkeeper_request, Doorkeeper.configuration.access_token_methods)
       end
@@ -72,10 +104,6 @@ module API
       def doorkeeper_request
         @doorkeeper_request ||= ActionDispatch::Request.new(env)
       end
-
-      def validate_access_token(access_token, scopes)
-        Oauth2::AccessTokenValidationService.validate(access_token, scopes: scopes)
-      end
     end
 
     module ClassMethods

lib/api/helpers.rb

@@ -2,8 +2,6 @@ module API
   module Helpers
     include Gitlab::Utils
 
-    PRIVATE_TOKEN_HEADER = "HTTP_PRIVATE_TOKEN"
-    PRIVATE_TOKEN_PARAM = :private_token
     SUDO_HEADER = "HTTP_SUDO"
     SUDO_PARAM = :sudo
 
@@ -308,7 +306,7 @@ module API
     private
 
     def private_token
-      params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]
+      params[APIGuard::PRIVATE_TOKEN_PARAM] || env[APIGuard::PRIVATE_TOKEN_HEADER]
     end
 
     def warden
@@ -323,18 +321,11 @@ module API
       warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD'])
     end
 
-    def find_user_by_private_token
-      token = private_token
-      return nil unless token.present?
-
-      User.find_by_authentication_token(token) || User.find_by_personal_access_token(token)
-    end
-
     def initial_current_user
       return @initial_current_user if defined?(@initial_current_user)
 
-      @initial_current_user ||= find_user_by_private_token
-      @initial_current_user ||= doorkeeper_guard
+      @initial_current_user ||= find_user_by_private_token(scopes: @scopes)
+      @initial_current_user ||= doorkeeper_guard(scopes: @scopes)
       @initial_current_user ||= find_user_from_warden
 
       unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?

lib/api/users.rb

@@ -2,7 +2,10 @@ module API
   class Users < Grape::API
     include PaginationParams
 
-    before { authenticate! }
+    before do
+      allow_access_with_scope :read_user if request.get?
+      authenticate!
+    end
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
       helpers do

lib/gitlab/auth.rb

@@ -2,6 +2,10 @@ module Gitlab
   module Auth
     class MissingPersonalTokenError < StandardError; end
 
+    SCOPES = [:api, :read_user]
+    DEFAULT_SCOPES = [:api]
+    OPTIONAL_SCOPES = SCOPES - DEFAULT_SCOPES
+
     class << self
       def find_for_git_client(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?

spec/requests/api/doorkeeper_access_spec.rb

@@ -5,7 +5,7 @@ describe API::API, api: true  do
 
   let!(:user) { create(:user) }
   let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) }
-  let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id }
+  let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "api" }
 
   describe "when unauthenticated" do
     it "returns authentication success" do

spec/requests/api/helpers_spec.rb

 require 'spec_helper'
 
 describe API::Helpers, api: true do
+  include API::APIGuard::HelperMethods
   include API::Helpers
   include SentryHelper
 
@@ -15,24 +16,24 @@ describe API::Helpers, api: true do
   def set_env(user_or_token, identifier)
     clear_env
     clear_param
-    env[API::Helpers::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
+    env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
     env[API::Helpers::SUDO_HEADER] = identifier.to_s
   end
 
   def set_param(user_or_token, identifier)
     clear_env
     clear_param
-    params[API::Helpers::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
+    params[API::APIGuard::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token
     params[API::Helpers::SUDO_PARAM] = identifier.to_s
   end
 
   def clear_env
-    env.delete(API::Helpers::PRIVATE_TOKEN_HEADER)
+    env.delete(API::APIGuard::PRIVATE_TOKEN_HEADER)
     env.delete(API::Helpers::SUDO_HEADER)
   end
 
   def clear_param
-    params.delete(API::Helpers::PRIVATE_TOKEN_PARAM)
+    params.delete(API::APIGuard::PRIVATE_TOKEN_PARAM)
     params.delete(API::Helpers::SUDO_PARAM)
   end
 
@@ -94,22 +95,22 @@ describe API::Helpers, api: true do
 
     describe "when authenticating using a user's private token" do
       it "returns nil for an invalid token" do
-        env[API::Helpers::PRIVATE_TOKEN_HEADER] = 'invalid token'
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token'
         allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false }
         expect(current_user).to be_nil
       end
 
       it "returns nil for a user without access" do
-        env[API::Helpers::PRIVATE_TOKEN_HEADER] = user.private_token
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token
         allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
         expect(current_user).to be_nil
       end
 
       it "leaves user as is when sudo not specified" do
-        env[API::Helpers::PRIVATE_TOKEN_HEADER] = user.private_token
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token
         expect(current_user).to eq(user)
         clear_env
-        params[API::Helpers::PRIVATE_TOKEN_PARAM] = user.private_token
+        params[API::APIGuard::PRIVATE_TOKEN_PARAM] = user.private_token
         expect(current_user).to eq(user)
       end
     end
@@ -117,37 +118,45 @@ describe API::Helpers, api: true do
     describe "when authenticating using a user's personal access tokens" do
       let(:personal_access_token) { create(:personal_access_token, user: user) }
 
+      before do
+        allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false }
+      end
+
       it "returns nil for an invalid token" do
-        env[API::Helpers::PRIVATE_TOKEN_HEADER] = 'invalid token'
-        allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false }
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token'
         expect(current_user).to be_nil
       end
 
       it "returns nil for a user without access" do
-        env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
         allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
         expect(current_user).to be_nil
       end
 
+      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
+
       it "leaves user as is when sudo not specified" do
-        env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
         expect(current_user).to eq(user)
         clear_env
-        params[API::Helpers::PRIVATE_TOKEN_PARAM] = personal_access_token.token
+        params[API::APIGuard::PRIVATE_TOKEN_PARAM] = personal_access_token.token
         expect(current_user).to eq(user)
       end
 
       it 'does not allow revoked tokens' do
         personal_access_token.revoke!
-        env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token
-        allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false }
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
         expect(current_user).to be_nil
       end
 
       it 'does not allow expired tokens' do
         personal_access_token.update_attributes!(expires_at: 1.day.ago)
-        env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token
-        allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false }
+        env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
         expect(current_user).to be_nil
       end
     end

spec/services/access_token_validation_service_spec.rb

+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])
+
+      expect(described_class.sufficient_scope?(token, [: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.sufficient_scope?(token, [:api, :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.sufficient_scope?(token, [:api, :read_user, :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.sufficient_scope?(token, [:api, :read_user, :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.sufficient_scope?(token, [])).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.sufficient_scope?(token, [:other_scope])).to be(false)
+    end
+  end
+end