Allow API scope declarations to be applied conditionally.
- Scope declarations of the form: allow_access_with_scope :read_user, if: -> (request) { request.get? } will only apply for `GET` requests - Add a negative test to a `POST` endpoint in the `users` API to test this. Also test for this case in the `AccessTokenValidationService` unit tests.
This commit is contained in:
parent
6f1922500b
commit
80c1ebaa83
|
@ -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)
|
||||||
@token = token
|
@token = token
|
||||||
|
@request = request
|
||||||
end
|
end
|
||||||
|
|
||||||
def validate(scopes: [])
|
def validate(scopes: [])
|
||||||
|
@ -31,11 +32,13 @@ class AccessTokenValidationService
|
||||||
if scopes.blank?
|
if scopes.blank?
|
||||||
true
|
true
|
||||||
else
|
else
|
||||||
#scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) }
|
# Remove any scopes whose `if` condition does not return `true`
|
||||||
# Check whether the token is allowed access to any of the required scopes.
|
scopes = scopes.reject { |scope| scope[:if].presence && !scope[:if].call(request) }
|
||||||
|
|
||||||
scope_names = scopes.map { |scope| scope[:name].to_s }
|
# Check whether the token is allowed access to any of the required scopes.
|
||||||
Set.new(scope_names).intersection(Set.new(token.scopes)).present?
|
passed_scope_names = scopes.map { |scope| scope[:name].to_sym }
|
||||||
|
token_scope_names = token.scopes.map(&:to_sym)
|
||||||
|
Set.new(passed_scope_names).intersection(Set.new(token_scope_names)).present?
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -68,7 +68,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).validate(scopes: scopes)
|
||||||
when AccessTokenValidationService::INSUFFICIENT_SCOPE
|
when AccessTokenValidationService::INSUFFICIENT_SCOPE
|
||||||
raise InsufficientScopeError.new(scopes)
|
raise InsufficientScopeError.new(scopes)
|
||||||
|
|
||||||
|
@ -105,7 +105,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).include_any_scope?(scopes)
|
||||||
User.find(access_token.user_id)
|
User.find(access_token.user_id)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -340,7 +340,7 @@ module API
|
||||||
end
|
end
|
||||||
|
|
||||||
def initial_current_user
|
def initial_current_user
|
||||||
endpoint_class = options[:for]
|
endpoint_class = options[:for].presence || ::API::API
|
||||||
|
|
||||||
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
|
||||||
|
|
|
@ -14,6 +14,8 @@ describe API::Helpers do
|
||||||
let(:request) { Rack::Request.new(env) }
|
let(:request) { Rack::Request.new(env) }
|
||||||
let(:header) { }
|
let(:header) { }
|
||||||
|
|
||||||
|
before { allow_any_instance_of(self.class).to receive(:options).and_return({}) }
|
||||||
|
|
||||||
def set_env(user_or_token, identifier)
|
def set_env(user_or_token, identifier)
|
||||||
clear_env
|
clear_env
|
||||||
clear_param
|
clear_param
|
||||||
|
@ -167,7 +169,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
|
||||||
|
|
|
@ -321,6 +321,16 @@ describe API::Users do
|
||||||
.to eq([Gitlab::PathRegex.namespace_format_message])
|
.to eq([Gitlab::PathRegex.namespace_format_message])
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'when the requesting token has the "read_user" scope' do
|
||||||
|
let(:token) { create(:personal_access_token, scopes: ['read_user'], user: admin) }
|
||||||
|
|
||||||
|
it 'returns a "401" response' do
|
||||||
|
post api("/users", admin, personal_access_token: token), attributes_for(:user, projects_limit: 3)
|
||||||
|
|
||||||
|
expect(response).to have_http_status(401)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
it "is not available for non admin users" do
|
it "is not available for non admin users" do
|
||||||
post api("/users", user), attributes_for(:user)
|
post api("/users", user), attributes_for(:user)
|
||||||
expect(response).to have_http_status(403)
|
expect(response).to have_http_status(403)
|
||||||
|
|
|
@ -2,40 +2,64 @@ 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])
|
||||||
|
|
||||||
expect(described_class.new(token).include_any_scope?([:api])).to be(true)
|
expect(described_class.new(token, request).include_any_scope?([{ name: :api }])).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])
|
||||||
|
|
||||||
expect(described_class.new(token).include_any_scope?([:api, :other_scope])).to be(true)
|
expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :other_scope }])).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])
|
||||||
|
|
||||||
expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true)
|
expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).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])
|
||||||
|
|
||||||
expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true)
|
expect(described_class.new(token, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).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: [])
|
||||||
|
|
||||||
expect(described_class.new(token).include_any_scope?([])).to be(true)
|
expect(described_class.new(token, request).include_any_scope?([])).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])
|
||||||
|
|
||||||
expect(described_class.new(token).include_any_scope?([:other_scope])).to be(false)
|
expect(described_class.new(token, request).include_any_scope?([{ name: :other_scope }])).to be(false)
|
||||||
|
end
|
||||||
|
|
||||||
|
context "conditions" do
|
||||||
|
context "if" do
|
||||||
|
it "ignores any scopes whose `if` condition returns false" do
|
||||||
|
token = double("token", scopes: [:api, :read_user])
|
||||||
|
|
||||||
|
expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false)
|
||||||
|
end
|
||||||
|
|
||||||
|
it "does not ignore scopes whose `if` condition is not set" do
|
||||||
|
token = double("token", scopes: [:api, :read_user])
|
||||||
|
|
||||||
|
expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
|
||||||
|
end
|
||||||
|
|
||||||
|
it "does not ignore scopes whose `if` condition returns true" do
|
||||||
|
token = double("token", scopes: [:api, :read_user])
|
||||||
|
|
||||||
|
expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue