Commit afbc7520 authored by Timothy Andrew's avatar Timothy Andrew

`AccessTokenValidationService` accepts `String` or `API::Scope` scopes.

- There's no need to use `API::Scope` for scopes that don't have `if`
  conditions, such as in `lib/gitlab/auth.rb`.
parent b8ec1f42
...@@ -37,7 +37,14 @@ class AccessTokenValidationService ...@@ -37,7 +37,14 @@ class AccessTokenValidationService
# small number of records involved. # small number of records involved.
# https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12300/#note_33689006 # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12300/#note_33689006
token_scopes = token.scopes.map(&:to_sym) token_scopes = token.scopes.map(&:to_sym)
required_scopes.any? { |scope| scope.sufficient?(token_scopes, request) }
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
...@@ -11,7 +11,7 @@ module API ...@@ -11,7 +11,7 @@ module API
# Are the `scopes` passed in sufficient to adequately authorize the passed # Are the `scopes` passed in sufficient to adequately authorize the passed
# request for the scope represented by the current instance of this class? # request for the scope represented by the current instance of this class?
def sufficient?(scopes, request) def sufficient?(scopes, request)
verify_if_condition(request) && scopes.include?(self.name) scopes.include?(self.name) && verify_if_condition(request)
end end
private private
......
...@@ -140,7 +140,6 @@ module Gitlab ...@@ -140,7 +140,6 @@ module Gitlab
end end
def valid_scoped_token?(token, scopes) def valid_scoped_token?(token, scopes)
scopes = scopes.map { |scope| API::Scope.new(scope) }
AccessTokenValidationService.new(token).include_any_scope?(scopes) AccessTokenValidationService.new(token).include_any_scope?(scopes)
end end
......
...@@ -6,28 +6,28 @@ describe AccessTokenValidationService, services: true do ...@@ -6,28 +6,28 @@ describe AccessTokenValidationService, services: true do
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::Scope.new(:api)] scopes = [:api]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).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::Scope.new(:api), API::Scope.new(:other_scope)] scopes = [:api, :other_scope]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).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::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)] scopes = [:api, :read_user, :other_scope]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).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::Scope.new(:api), API::Scope.new(:read_user), API::Scope.new(:other_scope)] scopes = [:api, :read_user, :other_scope]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end end
...@@ -41,7 +41,7 @@ describe AccessTokenValidationService, services: true do ...@@ -41,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 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 = [API::Scope.new(:other_scope)] scopes = [:other_scope]
expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(false)
end end
...@@ -56,7 +56,7 @@ describe AccessTokenValidationService, services: true do ...@@ -56,7 +56,7 @@ describe AccessTokenValidationService, services: true do
it "does not ignore scopes whose `if` condition is not set" do it "does not ignore scopes whose `if` condition is not set" do
token = double("token", scopes: [:api, :read_user]) token = double("token", scopes: [:api, :read_user])
scopes = [API::Scope.new(:api, if: ->(_) { false }), API::Scope.new(: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) expect(described_class.new(token, request: request).include_any_scope?(scopes)).to be(true)
end 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