From 7fa06ed55d18af4d055041eb27d38fecf9b5548f Mon Sep 17 00:00:00 2001 From: Timothy Andrew 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