From 7fa06ed55d18af4d055041eb27d38fecf9b5548f Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 22 Nov 2016 14:34:23 +0530
Subject: [PATCH] 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`
---
.../access_token_validation_service.rb | 34 ++++++++++
.../oauth2/access_token_validation_service.rb | 42 -------------
config/initializers/doorkeeper.rb | 4 +-
config/locales/doorkeeper.en.yml | 1 +
lib/api/api.rb | 2 +
lib/api/api_guard.rb | 62 ++++++++++++++-----
lib/api/helpers.rb | 15 +----
lib/api/users.rb | 5 +-
lib/gitlab/auth.rb | 4 ++
spec/requests/api/doorkeeper_access_spec.rb | 2 +-
spec/requests/api/helpers_spec.rb | 43 ++++++++-----
.../access_token_validation_service_spec.rb | 42 +++++++++++++
12 files changed, 164 insertions(+), 92 deletions(-)
create mode 100644 app/services/access_token_validation_service.rb
delete mode 100644 app/services/oauth2/access_token_validation_service.rb
create mode 100644 spec/services/access_token_validation_service_spec.rb
diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb
new file mode 100644
index 00000000000..69449f3a445
--- /dev/null
+++ b/app/services/access_token_validation_service.rb
@@ -0,0 +1,34 @@
+module AccessTokenValidationService
+ # Results:
+ VALID = :valid
+ EXPIRED = :expired
+ REVOKED = :revoked
+ INSUFFICIENT_SCOPE = :insufficient_scope
+
+ class << self
+ def validate(token, scopes: [])
+ if token.expired?
+ return EXPIRED
+
+ elsif token.revoked?
+ return REVOKED
+
+ elsif !self.sufficient_scope?(token, scopes)
+ return INSUFFICIENT_SCOPE
+
+ else
+ return VALID
+ end
+ end
+
+ # True if the token's scope contains any of the required scopes.
+ def sufficient_scope?(token, required_scopes)
+ if required_scopes.blank?
+ true
+ else
+ # 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
+end
diff --git a/app/services/oauth2/access_token_validation_service.rb b/app/services/oauth2/access_token_validation_service.rb
deleted file mode 100644
index 264fdccde8f..00000000000
--- a/app/services/oauth2/access_token_validation_service.rb
+++ /dev/null
@@ -1,42 +0,0 @@
-module Oauth2::AccessTokenValidationService
- # Results:
- VALID = :valid
- EXPIRED = :expired
- REVOKED = :revoked
- INSUFFICIENT_SCOPE = :insufficient_scope
-
- class << self
- def validate(token, scopes: [])
- if token.expired?
- return EXPIRED
-
- elsif token.revoked?
- return REVOKED
-
- elsif !self.sufficient_scope?(token, scopes)
- return INSUFFICIENT_SCOPE
-
- else
- return VALID
- 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
- 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
- end
- end
- end
-end
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index fc4b0a72add..88cd0f5f652 100644
--- a/config/initializers/doorkeeper.rb
+++ b/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
diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml
index a4032a21420..1d728282d90 100644
--- a/config/locales/doorkeeper.en.yml
+++ b/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:
diff --git a/lib/api/api.rb b/lib/api/api.rb
index cec2702e44d..9d5adffd8f4 100644
--- a/lib/api/api.rb
+++ b/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
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index 8cc7a26f1fa..cd266669b1e 100644
--- a/lib/api/api_guard.rb
+++ b/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
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 746849ef4c0..4be659fc20b 100644
--- a/lib/api/helpers.rb
+++ b/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?
diff --git a/lib/api/users.rb b/lib/api/users.rb
index c7db2d71017..0842c3874c5 100644
--- a/lib/api/users.rb
+++ b/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
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index aca5d0020cf..c3c464248ef 100644
--- a/lib/gitlab/auth.rb
+++ b/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?
diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb
index 5262a623761..bd9ecaf2685 100644
--- a/spec/requests/api/doorkeeper_access_spec.rb
+++ b/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
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 4035fd97af5..15b93118ee4 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -1,6 +1,7 @@
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
diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb
new file mode 100644
index 00000000000..8808934fa24
--- /dev/null
+++ b/spec/services/access_token_validation_service_spec.rb
@@ -0,0 +1,42 @@
+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
--
GitLab