summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--changelogs/unreleased/dm-api-unauthorized.yml5
-rw-r--r--lib/api/api_guard.rb24
-rw-r--r--lib/api/helpers.rb30
-rw-r--r--spec/requests/api/helpers_spec.rb78
4 files changed, 83 insertions, 54 deletions
diff --git a/changelogs/unreleased/dm-api-unauthorized.yml b/changelogs/unreleased/dm-api-unauthorized.yml
new file mode 100644
index 00000000000..26b45bd4c40
--- /dev/null
+++ b/changelogs/unreleased/dm-api-unauthorized.yml
@@ -0,0 +1,5 @@
+---
+title: Make sure API responds with 401 when invalid authentication info is provided
+merge_request:
+author:
+type: fixed
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index c4c0fdda665..e79f988f549 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -75,7 +75,7 @@ module API
raise RevokedError
when AccessTokenValidationService::VALID
- @current_user = User.find(access_token.resource_owner_id)
+ User.find(access_token.resource_owner_id)
end
end
@@ -84,11 +84,13 @@ module API
return nil unless token_string.present?
- find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes)
- end
+ user =
+ find_user_by_authentication_token(token_string) ||
+ find_user_by_personal_access_token(token_string, scopes)
+
+ raise UnauthorizedError unless user
- def current_user
- @current_user
+ user
end
private
@@ -107,7 +109,16 @@ module API
end
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
def doorkeeper_request
@@ -169,6 +180,7 @@ module API
TokenNotFoundError = Class.new(StandardError)
ExpiredError = Class.new(StandardError)
RevokedError = Class.new(StandardError)
+ UnauthorizedError = Class.new(StandardError)
class InsufficientScopeError < StandardError
attr_reader :scopes
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 00dbc2aee7a..1e8475ba3ec 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -3,6 +3,8 @@ module API
include Gitlab::Utils
include Helpers::Pagination
+ UnauthorizedError = Class.new(StandardError)
+
SUDO_HEADER = "HTTP_SUDO".freeze
SUDO_PARAM = :sudo
@@ -139,7 +141,7 @@ module API
end
def authenticate!
- unauthorized! unless current_user && can?(initial_current_user, :access_api)
+ unauthorized! unless current_user
end
def authenticate_non_get!
@@ -397,19 +399,27 @@ module API
def 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?
- @initial_current_user = nil
- end
- @initial_current_user
+ begin
+ @initial_current_user = Gitlab::Auth::UniqueIpsLimiter.limit_user! { find_current_user }
+ rescue APIGuard::UnauthorizedError, 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
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index d4006fe71a2..98c49d3364c 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -159,18 +159,25 @@ describe API::Helpers do
end
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'
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
- 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
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
it "leaves user as is when sudo not specified" do
@@ -193,24 +200,31 @@ describe API::Helpers do
allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false }
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'
- expect(current_user).to be_nil
+ expect { current_user }.to raise_error /401/
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
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
- 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'])
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
- expect(current_user).to be_nil
+ expect { current_user }.to raise_error /401/
end
it "leaves user as is when sudo not specified" do
@@ -226,14 +240,14 @@ describe API::Helpers do
personal_access_token.revoke!
env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token
- expect(current_user).to be_nil
+ expect { current_user }.to raise_error /401/
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 be_nil
+ expect { current_user }.to raise_error /401/
end
end
@@ -351,6 +365,18 @@ describe API::Helpers do
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
context 'with regular user' do
@@ -490,11 +516,10 @@ describe API::Helpers do
context 'current_user is nil' do
before do
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
it 'returns a 401 response' do
- expect { authenticate! }.to raise_error '401 - {"message"=>"401 Unauthorized"}'
+ expect { authenticate! }.to raise_error /401/
end
end
@@ -502,35 +527,12 @@ describe API::Helpers do
let(:user) { build(:user) }
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(:initial_current_user).and_return(user)
+ expect_any_instance_of(self.class).to receive(:current_user).and_return(user)
end
it 'does not raise an error' do
expect { authenticate! }.not_to raise_error
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