Commit 98768953 authored by Douwe Maan's avatar Douwe Maan

Merge branch '33580-fix-api-scoping' into 'master'

Fix API Scoping

Closes #33580 and #33022

See merge request !12300
parents 6df61942 94258a65
...@@ -5,10 +5,11 @@ class AccessTokenValidationService ...@@ -5,10 +5,11 @@ class AccessTokenValidationService
REVOKED = :revoked REVOKED = :revoked
INSUFFICIENT_SCOPE = :insufficient_scope INSUFFICIENT_SCOPE = :insufficient_scope
attr_reader :token attr_reader :token, :request
def initialize(token) def initialize(token, request: nil)
@token = token @token = token
@request = request
end end
def validate(scopes: []) def validate(scopes: [])
...@@ -27,12 +28,23 @@ class AccessTokenValidationService ...@@ -27,12 +28,23 @@ class AccessTokenValidationService
end end
# True if the token's scope contains any of the passed scopes. # True if the token's scope contains any of the passed scopes.
def include_any_scope?(scopes) def include_any_scope?(required_scopes)
if scopes.blank? if required_scopes.blank?
true true
else else
# Check whether the token is allowed access to any of the required scopes. # We're comparing each required_scope against all token scopes, which would
Set.new(scopes).intersection(Set.new(token.scopes)).present? # take quadratic time. This consideration is irrelevant here because of the
# small number of records involved.
# https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12300/#note_33689006
token_scopes = token.scopes.map(&:to_sym)
required_scopes.any? do |scope|
if scope.respond_to?(:sufficient?)
scope.sufficient?(token_scopes, request)
else
API::Scope.new(scope).sufficient?(token_scopes, request)
end
end
end end
end end
end end
---
title: Fix API Scoping
merge_request: 12300
author:
...@@ -2,6 +2,8 @@ module API ...@@ -2,6 +2,8 @@ module API
class API < Grape::API class API < Grape::API
include APIGuard include APIGuard
allow_access_with_scope :api
version %w(v3 v4), using: :path version %w(v3 v4), using: :path
version 'v3', using: :path do version 'v3', using: :path do
...@@ -44,7 +46,6 @@ module API ...@@ -44,7 +46,6 @@ module API
mount ::API::V3::Variables mount ::API::V3::Variables
end end
before { allow_access_with_scope :api }
before { header['X-Frame-Options'] = 'SAMEORIGIN' } before { header['X-Frame-Options'] = 'SAMEORIGIN' }
before { Gitlab::I18n.locale = current_user&.preferred_language } before { Gitlab::I18n.locale = current_user&.preferred_language }
......
...@@ -23,6 +23,23 @@ module API ...@@ -23,6 +23,23 @@ module API
install_error_responders(base) install_error_responders(base)
end end
class_methods do
# Set the authorization scope(s) allowed for an API endpoint.
#
# A call to this method maps the given scope(s) to the current API
# endpoint class. If this method is called multiple times on the same class,
# the scopes are all aggregated.
def allow_access_with_scope(scopes, options = {})
Array(scopes).each do |scope|
allowed_scopes << Scope.new(scope, options)
end
end
def allowed_scopes
@scopes ||= []
end
end
# Helper Methods for Grape Endpoint # Helper Methods for Grape Endpoint
module HelperMethods module HelperMethods
# Invokes the doorkeeper guard. # Invokes the doorkeeper guard.
...@@ -47,7 +64,7 @@ module API ...@@ -47,7 +64,7 @@ module API
access_token = find_access_token access_token = find_access_token
return nil unless access_token return nil unless access_token
case AccessTokenValidationService.new(access_token).validate(scopes: scopes) case AccessTokenValidationService.new(access_token, request: request).validate(scopes: scopes)
when AccessTokenValidationService::INSUFFICIENT_SCOPE when AccessTokenValidationService::INSUFFICIENT_SCOPE
raise InsufficientScopeError.new(scopes) raise InsufficientScopeError.new(scopes)
...@@ -74,18 +91,6 @@ module API ...@@ -74,18 +91,6 @@ module API
@current_user @current_user
end 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 private
def find_user_by_authentication_token(token_string) def find_user_by_authentication_token(token_string)
...@@ -96,7 +101,7 @@ module API ...@@ -96,7 +101,7 @@ module API
access_token = PersonalAccessToken.active.find_by_token(token_string) access_token = PersonalAccessToken.active.find_by_token(token_string)
return unless access_token return unless access_token
if AccessTokenValidationService.new(access_token).include_any_scope?(scopes) if AccessTokenValidationService.new(access_token, request: request).include_any_scope?(scopes)
User.find(access_token.user_id) User.find(access_token.user_id)
end end
end end
......
...@@ -342,8 +342,8 @@ module API ...@@ -342,8 +342,8 @@ 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 Gitlab::Auth::UniqueIpsLimiter.limit_user! do
@initial_current_user ||= find_user_by_private_token(scopes: @scopes) @initial_current_user ||= find_user_by_private_token(scopes: scopes_registered_for_endpoint)
@initial_current_user ||= doorkeeper_guard(scopes: @scopes) @initial_current_user ||= doorkeeper_guard(scopes: scopes_registered_for_endpoint)
@initial_current_user ||= find_user_from_warden @initial_current_user ||= find_user_from_warden
unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed? unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed?
...@@ -407,5 +407,22 @@ module API ...@@ -407,5 +407,22 @@ module API
exception.status == 500 exception.status == 500
end 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
end end
# Encapsulate a scope used for authorization, such as `api`, or `read_user`
module API
class Scope
attr_reader :name, :if
def initialize(name, options = {})
@name = name.to_sym
@if = options[:if]
end
# Are the `scopes` passed in sufficient to adequately authorize the passed
# request for the scope represented by the current instance of this class?
def sufficient?(scopes, request)
scopes.include?(self.name) && verify_if_condition(request)
end
private
def verify_if_condition(request)
self.if.nil? || self.if.call(request)
end
end
end
module API module API
class Users < Grape::API class Users < Grape::API
include PaginationParams include PaginationParams
include APIGuard
before do allow_access_with_scope :read_user, if: -> (request) { request.get? }
allow_access_with_scope :read_user if request.get?
end
resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
before do before do
......
...@@ -2,9 +2,11 @@ module API ...@@ -2,9 +2,11 @@ module API
module V3 module V3
class Users < Grape::API class Users < Grape::API
include PaginationParams include PaginationParams
include APIGuard
allow_access_with_scope :read_user, if: -> (request) { request.get? }
before do before do
allow_access_with_scope :read_user if request.get?
authenticate! authenticate!
end end
......
...@@ -130,13 +130,13 @@ module Gitlab ...@@ -130,13 +130,13 @@ module Gitlab
token = PersonalAccessTokensFinder.new(state: 'active').find_by(token: password) token = PersonalAccessTokensFinder.new(state: 'active').find_by(token: password)
if token && valid_scoped_token?(token, AVAILABLE_SCOPES.map(&:to_s)) if token && valid_scoped_token?(token, AVAILABLE_SCOPES)
Gitlab::Auth::Result.new(token.user, nil, :personal_token, abilities_for_scope(token.scopes)) Gitlab::Auth::Result.new(token.user, nil, :personal_token, abilities_for_scope(token.scopes))
end end
end end
def valid_oauth_token?(token) def valid_oauth_token?(token)
token && token.accessible? && valid_scoped_token?(token, ["api"]) token && token.accessible? && valid_scoped_token?(token, [:api])
end end
def valid_scoped_token?(token, scopes) def valid_scoped_token?(token, scopes)
......
...@@ -14,6 +14,10 @@ describe API::Helpers do ...@@ -14,6 +14,10 @@ describe API::Helpers do
let(:request) { Rack::Request.new(env) } let(:request) { Rack::Request.new(env) }
let(:header) { } let(:header) { }
before do
allow_any_instance_of(self.class).to receive(:options).and_return({})
end
def set_env(user_or_token, identifier) def set_env(user_or_token, identifier)
clear_env clear_env
clear_param clear_param
...@@ -167,7 +171,6 @@ describe API::Helpers do ...@@ -167,7 +171,6 @@ describe API::Helpers do
it "returns nil for a token without the appropriate scope" do it "returns nil 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
allow_access_with_scope('write_user')
expect(current_user).to be_nil expect(current_user).to be_nil
end end
......
...@@ -390,6 +390,14 @@ describe API::Users do ...@@ -390,6 +390,14 @@ describe API::Users do
expect(json_response['identities'].first['provider']).to eq('github') expect(json_response['identities'].first['provider']).to eq('github')
end end
end end
context "scopes" do
let(:user) { admin }
let(:path) { '/users' }
let(:api_call) { method(:api) }
include_examples 'does not allow the "read_user" scope'
end
end end
describe "GET /users/sign_up" do describe "GET /users/sign_up" do
...@@ -887,6 +895,13 @@ describe API::Users do ...@@ -887,6 +895,13 @@ describe API::Users do
expect(response).to match_response_schema('public_api/v4/user/public') expect(response).to match_response_schema('public_api/v4/user/public')
expect(json_response['id']).to eq(user.id) expect(json_response['id']).to eq(user.id)
end end
context "scopes" do
let(:path) { "/user" }
let(:api_call) { method(:api) }
include_examples 'allows the "read_user" scope'
end
end end
context 'with admin' do context 'with admin' do
...@@ -956,6 +971,13 @@ describe API::Users do ...@@ -956,6 +971,13 @@ describe API::Users do
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first["title"]).to eq(key.title) expect(json_response.first["title"]).to eq(key.title)
end end
context "scopes" do
let(:path) { "/user/keys" }
let(:api_call) { method(:api) }
include_examples 'allows the "read_user" scope'
end
end end
end end
...@@ -989,6 +1011,13 @@ describe API::Users do ...@@ -989,6 +1011,13 @@ describe API::Users do
expect(response).to have_http_status(404) expect(response).to have_http_status(404)
end end
context "scopes" do
let(:path) { "/user/keys/#{key.id}" }
let(:api_call) { method(:api) }
include_examples 'allows the "read_user" scope'
end
end end
describe "POST /user/keys" do describe "POST /user/keys" do
...@@ -1078,6 +1107,13 @@ describe API::Users do ...@@ -1078,6 +1107,13 @@ describe API::Users do
expect(json_response).to be_an Array expect(json_response).to be_an Array
expect(json_response.first["email"]).to eq(email.email) expect(json_response.first["email"]).to eq(email.email)
end end
context "scopes" do
let(:path) { "/user/emails" }
let(:api_call) { method(:api) }
include_examples 'allows the "read_user" scope'
end
end end
end end
...@@ -1110,6 +1146,13 @@ describe API::Users do ...@@ -1110,6 +1146,13 @@ describe API::Users do
expect(response).to have_http_status(404) expect(response).to have_http_status(404)
end end
context "scopes" do
let(:path) { "/user/emails/#{email.id}" }
let(:api_call) { method(:api) }
include_examples 'allows the "read_user" scope'
end
end end
describe "POST /user/emails" do describe "POST /user/emails" do
......
...@@ -67,6 +67,19 @@ describe API::V3::Users do ...@@ -67,6 +67,19 @@ describe API::V3::Users do
expect(json_response.first['title']).to eq(key.title) expect(json_response.first['title']).to eq(key.title)
end end
end end
context "scopes" do
let(:user) { admin }
let(:path) { "/users/#{user.id}/keys" }
let(:api_call) { method(:v3_api) }
before do
user.keys << key
user.save
end
include_examples 'allows the "read_user" scope'
end
end end
describe 'GET /user/:id/emails' do describe 'GET /user/:id/emails' do
...@@ -287,7 +300,7 @@ describe API::V3::Users do ...@@ -287,7 +300,7 @@ describe API::V3::Users do
end end
it 'returns a 404 error if not found' do it 'returns a 404 error if not found' do
get v3_api('/users/42/events', user) get v3_api('/users/420/events', user)
expect(response).to have_http_status(404) expect(response).to have_http_status(404)
expect(json_response['message']).to eq('404 User Not Found') expect(json_response['message']).to eq('404 User Not Found')
...@@ -312,5 +325,13 @@ describe API::V3::Users do ...@@ -312,5 +325,13 @@ describe API::V3::Users do
expect(json_response['is_admin']).to be_nil expect(json_response['is_admin']).to be_nil
end end
context "scopes" do
let(:user) { admin }
let(:path) { '/users' }
let(:api_call) { method(:v3_api) }
include_examples 'does not allow the "read_user" scope'
end
end end
end end
...@@ -2,40 +2,71 @@ require 'spec_helper' ...@@ -2,40 +2,71 @@ require 'spec_helper'
describe AccessTokenValidationService, services: true do describe AccessTokenValidationService, services: true do
describe ".include_any_scope?" do describe ".include_any_scope?" do
let(:request) { double("request") }
it "returns true if the required scope is present in the token's scopes" do it "returns true if the required scope is present in the token's scopes" do
token = double("token", scopes: [:api, :read_user]) token = double("token", scopes: [:api, :read_user])
scopes = [:api]
expect(described_class.new(token).include_any_scope?([:api])).to be(true) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end end
it "returns true if more than one of the required scopes is present in the token's scopes" do 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]) token = double("token", scopes: [:api, :read_user, :other_scope])
scopes = [:api, :other_scope]
expect(described_class.new(token).include_any_scope?([:api, :other_scope])).to be(true) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end end
it "returns true if the list of required scopes is an exact match for the token's scopes" do 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]) token = double("token", scopes: [:api, :read_user, :other_scope])
scopes = [:api, :read_user, :other_scope]
expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end end
it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do 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]) token = double("token", scopes: [:api, :read_user])
scopes = [:api, :read_user, :other_scope]
expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end end
it 'returns true if the list of required scopes is blank' do it 'returns true if the list of required scopes is blank' do
token = double("token", scopes: []) token = double("token", scopes: [])
scopes = []
expect(described_class.new(token).include_any_scope?([])).to be(true) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end end
it "returns false if there are no scopes in common between the required scopes and the token scopes" do 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]) token = double("token", scopes: [:api, :read_user])
scopes = [:other_scope]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
end
context "conditions" do
it "ignores any scopes whose `if` condition returns false" do
token = double("token", scopes: [:api, :read_user])
scopes = [API::Scope.new(:api, if: ->(_) { false })]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
end
it "does not ignore scopes whose `if` condition is not set" do
token = double("token", scopes: [:api, :read_user])
scopes = [API::Scope.new(:api, if: ->(_) { false }), :read_user]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end
it "does not ignore scopes whose `if` condition returns true" do
token = double("token", scopes: [:api, :read_user])
scopes = [API::Scope.new(:api, if: ->(_) { true }), API::Scope.new(:read_user, if: ->(_) { false })]
expect(described_class.new(token).include_any_scope?([:other_scope])).to be(false) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end
end end
end end
end end
shared_examples_for 'allows the "read_user" scope' do
context 'for personal access tokens' do
context 'when the requesting token has the "api" scope' do
let(:token) { create(:personal_access_token, scopes: ['api'], user: user) }
it 'returns a "200" response' do
get api_call.call(path, user, personal_access_token: token)
expect(response).to have_http_status(200)
end
end
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 "200" response' do
get api_call.call(path, user, personal_access_token: token)
expect(response).to have_http_status(200)
end
end
context 'when the requesting token does not have any required scope' do
let(:token) { create(:personal_access_token, scopes: ['read_registry'], user: user) }
it 'returns a "401" response' do
get api_call.call(path, user, personal_access_token: token)
expect(response).to have_http_status(401)
end
end
end
context 'for doorkeeper (OAuth) tokens' do
let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) }
context 'when the requesting token has the "api" scope' do
let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "api" }
it 'returns a "200" response' do
get api_call.call(path, user, oauth_access_token: token)
expect(response).to have_http_status(200)
end
end
context 'when the requesting token has the "read_user" scope' do
let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "read_user" }
it 'returns a "200" response' do
get api_call.call(path, user, oauth_access_token: token)
expect(response).to have_http_status(200)
end
end
context 'when the requesting token does not have any required scope' do
let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "invalid" }
it 'returns a "403" response' do
get api_call.call(path, user, oauth_access_token: token)
expect(response).to have_http_status(403)
end
end
end
end
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
post api_call.call(path, user, personal_access_token: token), attributes_for(:user, projects_limit: 3)
expect(response).to have_http_status(401)
end
end
end
...@@ -17,14 +17,18 @@ module ApiHelpers ...@@ -17,14 +17,18 @@ module ApiHelpers
# => "/api/v2/issues?foo=bar&private_token=..." # => "/api/v2/issues?foo=bar&private_token=..."
# #
# Returns the relative path to the requested API resource # Returns the relative path to the requested API resource
def api(path, user = nil, version: API::API.version) def api(path, user = nil, version: API::API.version, personal_access_token: nil, oauth_access_token: nil)
"/api/#{version}#{path}" + "/api/#{version}#{path}" +
# Normalize query string # Normalize query string
(path.index('?') ? '' : '?') + (path.index('?') ? '' : '?') +
if personal_access_token.present?
"&private_token=#{personal_access_token.token}"
elsif oauth_access_token.present?
"&access_token=#{oauth_access_token.token}"
# Append private_token if given a User object # Append private_token if given a User object
if user.respond_to?(:private_token) elsif user.respond_to?(:private_token)
"&private_token=#{user.private_token}" "&private_token=#{user.private_token}"
else else
'' ''
...@@ -32,8 +36,14 @@ module ApiHelpers ...@@ -32,8 +36,14 @@ module ApiHelpers
end end
# Temporary helper method for simplifying V3 exclusive API specs # Temporary helper method for simplifying V3 exclusive API specs
def v3_api(path, user = nil) def v3_api(path, user = nil, personal_access_token: nil, oauth_access_token: nil)
api(path, user, version: 'v3') api(
path,
user,
version: 'v3',
personal_access_token: personal_access_token,
oauth_access_token: oauth_access_token
)
end end
def ci_api(path, user = nil) def ci_api(path, user = nil)
......
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