From 025c6eeaa1e02dce31cb836c39ee4a5f312f202f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 27 Sep 2017 15:56:48 +0200 Subject: [PATCH] Move all API authentication code to APIGuard --- app/models/oauth_access_token.rb | 2 + lib/api/api_guard.rb | 143 ++++++++++++------ lib/api/helpers.rb | 52 +------ spec/requests/api/helpers_spec.rb | 18 +-- .../api/scopes/read_user_shared_examples.rb | 8 +- 5 files changed, 113 insertions(+), 110 deletions(-) diff --git a/app/models/oauth_access_token.rb b/app/models/oauth_access_token.rb index b85f5dbaf2e..f89e60ad9f4 100644 --- a/app/models/oauth_access_token.rb +++ b/app/models/oauth_access_token.rb @@ -1,4 +1,6 @@ class OauthAccessToken < Doorkeeper::AccessToken belongs_to :resource_owner, class_name: 'User' belongs_to :application, class_name: 'Doorkeeper::Application' + + alias_method :user, :resource_owner end diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index e79f988f549..87b9db66efd 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -42,6 +42,38 @@ module API # Helper Methods for Grape Endpoint module HelperMethods + def find_current_user + user = + find_user_from_private_token || + find_user_from_oauth_token || + find_user_from_warden + + return nil unless user + + raise UnauthorizedError unless Gitlab::UserAccess.new(user).allowed? && user.can?(:access_api) + + user + end + + def private_token + params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER] + end + + private + + def find_user_from_private_token + token_string = private_token.to_s + return nil unless token_string.present? + + user = + find_user_by_authentication_token(token_string) || + find_user_by_personal_access_token(token_string) + + raise UnauthorizedError unless user + + user + end + # Invokes the doorkeeper guard. # # If token is presented and valid, then it sets @current_user. @@ -60,9 +92,53 @@ module API # scopes: (optional) scopes required for this guard. # Defaults to empty array. # - def doorkeeper_guard(scopes: []) - access_token = find_access_token - return nil unless access_token + def find_user_from_oauth_token + access_token = find_oauth_access_token + return unless access_token + + find_user_by_access_token(access_token) + end + + 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) + access_token = PersonalAccessToken.find_by_token(token_string) + return unless access_token + + find_user_by_access_token(access_token) + end + + # Check the Rails session for valid authentication details + def find_user_from_warden + warden.try(:authenticate) if verified_request? + end + + def warden + env['warden'] + end + + # Check if the request is GET/HEAD, or if CSRF token is valid. + def verified_request? + Gitlab::RequestForgeryProtection.verified?(env) + end + + def find_oauth_access_token + return @oauth_access_token if defined?(@oauth_access_token) + + token = Doorkeeper::OAuth::Token.from_request(doorkeeper_request, *Doorkeeper.configuration.access_token_methods) + return @oauth_access_token = nil unless token + + @oauth_access_token = OauthAccessToken.by_token(token) + raise UnauthorizedError unless @oauth_access_token + + @oauth_access_token.revoke_previous_refresh_token! + @oauth_access_token + end + + def find_user_by_access_token(access_token) + scopes = scopes_registered_for_endpoint case AccessTokenValidationService.new(access_token, request: request).validate(scopes: scopes) when AccessTokenValidationService::INSUFFICIENT_SCOPE @@ -75,55 +151,30 @@ module API raise RevokedError when AccessTokenValidationService::VALID - User.find(access_token.resource_owner_id) + access_token.user end end - def find_user_by_private_token(scopes: []) - token_string = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s - - return nil unless token_string.present? - - user = - find_user_by_authentication_token(token_string) || - find_user_by_personal_access_token(token_string, scopes) - - raise UnauthorizedError unless user - - user - 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.new(access_token, request: request).include_any_scope?(scopes) - User.find(access_token.user_id) - end - end - - def find_access_token - return @access_token if defined?(@access_token) - - token = Doorkeeper::OAuth::Token.from_request(doorkeeper_request, *Doorkeeper.configuration.access_token_methods) - return @access_token = nil unless token - - @access_token = Doorkeeper::AccessToken.by_token(token) - raise UnauthorizedError unless @access_token - - @access_token.revoke_previous_refresh_token! - @access_token - end - def doorkeeper_request @doorkeeper_request ||= ActionDispatch::Request.new(env) 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?(:allowed_scopes) + memo.concat(endpoint.allowed_scopes) + else + memo + end + end + end + end end module ClassMethods diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a87297a604c..2b316b58ed9 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -3,8 +3,6 @@ module API include Gitlab::Utils include Helpers::Pagination - UnauthorizedError = Class.new(StandardError) - SUDO_HEADER = "HTTP_SUDO".freeze SUDO_PARAM = :sudo @@ -379,47 +377,16 @@ module API private - def private_token - params[APIGuard::PRIVATE_TOKEN_PARAM] || env[APIGuard::PRIVATE_TOKEN_HEADER] - end - - def warden - env['warden'] - end - - # Check if the request is GET/HEAD, or if CSRF token is valid. - def verified_request? - Gitlab::RequestForgeryProtection.verified?(env) - end - - # Check the Rails session for valid authentication details - def find_user_from_warden - warden.try(:authenticate) if verified_request? - end - def initial_current_user return @initial_current_user if defined?(@initial_current_user) begin @initial_current_user = Gitlab::Auth::UniqueIpsLimiter.limit_user! { find_current_user } - rescue APIGuard::UnauthorizedError, UnauthorizedError + rescue APIGuard::UnauthorizedError unauthorized! end end - def find_current_user - user = - find_user_by_private_token(scopes: scopes_registered_for_endpoint) || - doorkeeper_guard(scopes: scopes_registered_for_endpoint) || - find_user_from_warden - - return nil unless user - - raise UnauthorizedError unless Gitlab::UserAccess.new(user).allowed? && user.can?(:access_api) - - user - end - def sudo! return unless sudo_identifier return unless initial_current_user @@ -479,22 +446,5 @@ 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?(:allowed_scopes) - memo.concat(endpoint.allowed_scopes) - else - memo - end - end - end - end end end diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 862920ad7c3..9f3b5a809d7 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -222,13 +222,6 @@ describe API::Helpers do expect { current_user }.to raise_error /401/ end - it "returns a 401 response 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 - - expect { current_user }.to raise_error /401/ - end - it "leaves user as is when sudo not specified" do env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token expect(current_user).to eq(user) @@ -238,18 +231,25 @@ describe API::Helpers do expect(current_user).to eq(user) end + it "does not allow tokens 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 + + expect { current_user }.to raise_error API::APIGuard::InsufficientScopeError + end + it 'does not allow revoked tokens' do personal_access_token.revoke! env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token - expect { current_user }.to raise_error /401/ + expect { current_user }.to raise_error API::APIGuard::RevokedError end it 'does not allow expired tokens' do personal_access_token.update_attributes!(expires_at: 1.day.ago) env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token - expect { current_user }.to raise_error /401/ + expect { current_user }.to raise_error API::APIGuard::ExpiredError 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 57e28e040d7..111534f2f26 100644 --- a/spec/support/api/scopes/read_user_shared_examples.rb +++ b/spec/support/api/scopes/read_user_shared_examples.rb @@ -27,10 +27,10 @@ shared_examples_for 'allows the "read_user" scope' do stub_container_registry_config(enabled: true) end - it 'returns a "401" response' do + it 'returns a "403" response' do get api_call.call(path, user, personal_access_token: token) - expect(response).to have_http_status(401) + expect(response).to have_http_status(403) end end end @@ -74,10 +74,10 @@ 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 + it 'returns a "403" 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) + expect(response).to have_http_status(403) end end end