diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb index ee2e93a0d63..bf5aef0055e 100644 --- a/app/services/access_token_validation_service.rb +++ b/app/services/access_token_validation_service.rb @@ -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 diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index 56f6da57555..0d2d71e336a 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -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 diff --git a/lib/api/scope.rb b/lib/api/scope.rb new file mode 100644 index 00000000000..c23846d1e7d --- /dev/null +++ b/lib/api/scope.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index ec73255b20a..6d0d638ba14 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -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 diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb index 279f4ed93ac..660a05e0b6d 100644 --- a/spec/services/access_token_validation_service_spec.rb +++ b/spec/services/access_token_validation_service_spec.rb @@ -1,37 +1,33 @@ 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