Commit 3209c071 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'dm-api-unauthorized' into 'master'

Make sure API responds with 401 when invalid authentication info is provided

Closes #38438

See merge request gitlab-org/gitlab-ce!14525
parents 06daba75 b6c5a73c
---
title: Make sure API responds with 401 when invalid authentication info is provided
merge_request:
author:
type: fixed
...@@ -75,7 +75,7 @@ module API ...@@ -75,7 +75,7 @@ module API
raise RevokedError raise RevokedError
when AccessTokenValidationService::VALID when AccessTokenValidationService::VALID
@current_user = User.find(access_token.resource_owner_id) User.find(access_token.resource_owner_id)
end end
end end
...@@ -84,11 +84,13 @@ module API ...@@ -84,11 +84,13 @@ module API
return nil unless token_string.present? return nil unless token_string.present?
find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes) user =
end find_user_by_authentication_token(token_string) ||
find_user_by_personal_access_token(token_string, scopes)
raise UnauthorizedError unless user
def current_user user
@current_user
end end
private private
...@@ -107,7 +109,16 @@ module API ...@@ -107,7 +109,16 @@ module API
end end
def find_access_token def find_access_token
@access_token ||= Doorkeeper.authenticate(doorkeeper_request, Doorkeeper.configuration.access_token_methods) 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 end
def doorkeeper_request def doorkeeper_request
...@@ -169,6 +180,7 @@ module API ...@@ -169,6 +180,7 @@ module API
TokenNotFoundError = Class.new(StandardError) TokenNotFoundError = Class.new(StandardError)
ExpiredError = Class.new(StandardError) ExpiredError = Class.new(StandardError)
RevokedError = Class.new(StandardError) RevokedError = Class.new(StandardError)
UnauthorizedError = Class.new(StandardError)
class InsufficientScopeError < StandardError class InsufficientScopeError < StandardError
attr_reader :scopes attr_reader :scopes
......
...@@ -3,6 +3,8 @@ module API ...@@ -3,6 +3,8 @@ module API
include Gitlab::Utils include Gitlab::Utils
include Helpers::Pagination include Helpers::Pagination
UnauthorizedError = Class.new(StandardError)
SUDO_HEADER = "HTTP_SUDO".freeze SUDO_HEADER = "HTTP_SUDO".freeze
SUDO_PARAM = :sudo SUDO_PARAM = :sudo
...@@ -139,7 +141,7 @@ module API ...@@ -139,7 +141,7 @@ module API
end end
def authenticate! def authenticate!
unauthorized! unless current_user && can?(initial_current_user, :access_api) unauthorized! unless current_user
end end
def authenticate_non_get! def authenticate_non_get!
...@@ -397,17 +399,25 @@ module API ...@@ -397,17 +399,25 @@ module API
def initial_current_user def initial_current_user
return @initial_current_user if defined?(@initial_current_user) 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_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? begin
@initial_current_user = nil @initial_current_user = Gitlab::Auth::UniqueIpsLimiter.limit_user! { find_current_user }
rescue APIGuard::UnauthorizedError, UnauthorizedError
unauthorized!
end end
@initial_current_user
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 end
def sudo! def sudo!
......
...@@ -159,18 +159,25 @@ describe API::Helpers do ...@@ -159,18 +159,25 @@ describe API::Helpers do
end end
describe "when authenticating using a user's private token" do describe "when authenticating using a user's private token" do
it "returns nil for an invalid token" do it "returns a 401 response for an invalid token" do
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token' env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token'
allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false } allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false }
expect(current_user).to be_nil expect { current_user }.to raise_error /401/
end end
it "returns nil for a user without access" do it "returns a 401 response for a user without access" do
env[API::APIGuard::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) allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
expect(current_user).to be_nil expect { current_user }.to raise_error /401/
end
it 'returns a 401 response for a user who is blocked' do
user.block!
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token
expect { current_user }.to raise_error /401/
end end
it "leaves user as is when sudo not specified" do it "leaves user as is when sudo not specified" do
...@@ -193,24 +200,31 @@ describe API::Helpers do ...@@ -193,24 +200,31 @@ describe API::Helpers do
allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false } allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false }
end end
it "returns nil for an invalid token" do it "returns a 401 response for an invalid token" do
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token' env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token'
expect(current_user).to be_nil expect { current_user }.to raise_error /401/
end end
it "returns nil for a user without access" do it "returns a 401 response for a user without access" do
env[API::APIGuard::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) allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false)
expect(current_user).to be_nil expect { current_user }.to raise_error /401/
end
it 'returns a 401 response for a user who is blocked' do
user.block!
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
expect { current_user }.to raise_error /401/
end end
it "returns nil for a token without the appropriate scope" do 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']) personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user'])
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
expect(current_user).to be_nil expect { current_user }.to raise_error /401/
end end
it "leaves user as is when sudo not specified" do it "leaves user as is when sudo not specified" do
...@@ -226,14 +240,14 @@ describe API::Helpers do ...@@ -226,14 +240,14 @@ describe API::Helpers do
personal_access_token.revoke! personal_access_token.revoke!
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
expect(current_user).to be_nil expect { current_user }.to raise_error /401/
end end
it 'does not allow expired tokens' do it 'does not allow expired tokens' do
personal_access_token.update_attributes!(expires_at: 1.day.ago) personal_access_token.update_attributes!(expires_at: 1.day.ago)
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
expect(current_user).to be_nil expect { current_user }.to raise_error /401/
end end
end end
...@@ -351,6 +365,18 @@ describe API::Helpers do ...@@ -351,6 +365,18 @@ describe API::Helpers do
end end
end end
end end
context 'when user is blocked' do
before do
user.block!
end
it 'changes current_user to sudo' do
set_env(admin, user.id)
expect(current_user).to eq(user)
end
end
end end
context 'with regular user' do context 'with regular user' do
...@@ -490,11 +516,10 @@ describe API::Helpers do ...@@ -490,11 +516,10 @@ describe API::Helpers do
context 'current_user is nil' do context 'current_user is nil' do
before do before do
expect_any_instance_of(self.class).to receive(:current_user).and_return(nil) expect_any_instance_of(self.class).to receive(:current_user).and_return(nil)
allow_any_instance_of(self.class).to receive(:initial_current_user).and_return(nil)
end end
it 'returns a 401 response' do it 'returns a 401 response' do
expect { authenticate! }.to raise_error '401 - {"message"=>"401 Unauthorized"}' expect { authenticate! }.to raise_error /401/
end end
end end
...@@ -502,35 +527,12 @@ describe API::Helpers do ...@@ -502,35 +527,12 @@ describe API::Helpers do
let(:user) { build(:user) } let(:user) { build(:user) }
before do before do
expect_any_instance_of(self.class).to receive(:current_user).at_least(:once).and_return(user) expect_any_instance_of(self.class).to receive(:current_user).and_return(user)
expect_any_instance_of(self.class).to receive(:initial_current_user).and_return(user)
end end
it 'does not raise an error' do it 'does not raise an error' do
expect { authenticate! }.not_to raise_error expect { authenticate! }.not_to raise_error
end end
end end
context 'current_user is blocked' do
let(:user) { build(:user, :blocked) }
before do
expect_any_instance_of(self.class).to receive(:current_user).at_least(:once).and_return(user)
end
it 'raises an error' do
expect_any_instance_of(self.class).to receive(:initial_current_user).and_return(user)
expect { authenticate! }.to raise_error '401 - {"message"=>"401 Unauthorized"}'
end
it "doesn't raise an error if an admin user is impersonating a blocked user (via sudo)" do
admin_user = build(:user, :admin)
expect_any_instance_of(self.class).to receive(:initial_current_user).and_return(admin_user)
expect { authenticate! }.not_to raise_error
end
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment