Commit b8ec1f42 authored by Timothy Andrew's avatar Timothy Andrew

Extract a `Gitlab::Scope` class.

- To represent an authorization scope, such as `api` or `read_user`
- This is a better abstraction than the hash we were previously using.
parent c1fcd730
......@@ -28,17 +28,16 @@ class AccessTokenValidationService
end
# True if the token's scope contains any of the passed scopes.
def include_any_scope?(scopes)
if scopes.blank?
def include_any_scope?(required_scopes)
if required_scopes.blank?
true
else
# Remove any scopes whose `if` condition does not return `true`
scopes = scopes.select { |scope| scope.if.nil? || scope.if.call(request) }
# Check whether the token is allowed access to any of the required scopes.
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?
# We're comparing each required_scope against all token scopes, which would
# 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? { |scope| scope.sufficient?(token_scopes, request) }
end
end
end
......@@ -31,7 +31,7 @@ module API
# the scopes are all aggregated.
def allow_access_with_scope(scopes, options = {})
Array(scopes).each do |scope|
allowed_scopes << OpenStruct.new(name: scope.to_sym, if: options[:if])
allowed_scopes << Scope.new(scope, options)
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)
verify_if_condition(request) && scopes.include?(self.name)
end
private
def verify_if_condition(request)
self.if.nil? || self.if.call(request)
end
end
end
......@@ -136,11 +136,11 @@ module Gitlab
end
def valid_oauth_token?(token)
token && token.accessible? && valid_scoped_token?(token, ['api'])
token && token.accessible? && valid_scoped_token?(token, [:api])
end
def valid_scoped_token?(token, scopes)
scopes = scopes.map { |scope| OpenStruct.new(name: scope) }
scopes = scopes.map { |scope| API::Scope.new(scope) }
AccessTokenValidationService.new(token).include_any_scope?(scopes)
end
......
require 'spec_helper'
describe AccessTokenValidationService, services: true do
def scope(data)
OpenStruct.new(data)
end
describe ".include_any_scope?" do
let(:request) { double("request") }
it "returns true if the required scope is present in the token's scopes" do
token = double("token", scopes: [:api, :read_user])
scopes = [scope({ name: :api })]
scopes = [API::Scope.new(:api)]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end
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])
scopes = [scope({ name: :api }), scope({ name: :other_scope })]
scopes = [API::Scope.new(:api), API::Scope.new(:other_scope)]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end
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])
scopes = [scope({ name: :api }), scope({ name: :read_user }), scope({ name: :other_scope })]
scopes = [API::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end
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])
scopes = [scope({ name: :api }), scope({ name: :read_user }), scope({ name: :other_scope })]
scopes = [API::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end
......@@ -45,7 +41,7 @@ describe AccessTokenValidationService, services: true 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])
scopes = [scope({ name: :other_scope })]
scopes = [API::Scope.new(:other_scope)]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
end
......@@ -53,21 +49,21 @@ describe AccessTokenValidationService, services: true do
context "conditions" do
it "ignores any scopes whose `if` condition returns false" do
token = double("token", scopes: [:api, :read_user])
scopes = [scope({ name: :api, if: ->(_) { false } })]
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 = [scope({ name: :api, if: ->(_) { false } }), scope({ name: :read_user })]
scopes = [API::Scope.new(:api, if: ->(_) { false }), API::Scope.new(: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 = [scope({ name: :api, if: ->(_) { true } }), scope({ name: :read_user, if: ->(_) { false } })]
scopes = [API::Scope.new(:api, if: ->(_) { true }), API::Scope.new(:read_user, if: ->(_) { false })]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
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