Commit 1b8223dd authored by Timothy Andrew's avatar Timothy Andrew

Fix remaining spec failures for !12300.

1. Get the spec for `lib/gitlab/auth.rb` passing.

  - Make the `request` argument to `AccessTokenValidationService` optional -
  `auth.rb` doesn't need to pass in a request.

  - Pass in scopes in the format `[{ name: 'api' }]` rather than `['api']`, which
  is what `AccessTokenValidationService` now expects.

2. Get the spec for `API::V3::Users` passing

2. Get the spec for `AccessTokenValidationService` passing
parent 8b399b18
...@@ -7,7 +7,7 @@ class AccessTokenValidationService ...@@ -7,7 +7,7 @@ class AccessTokenValidationService
attr_reader :token, :request attr_reader :token, :request
def initialize(token, request) def initialize(token, request: nil)
@token = token @token = token
@request = request @request = request
end end
......
...@@ -66,7 +66,7 @@ module API ...@@ -66,7 +66,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, request).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)
...@@ -103,7 +103,7 @@ module API ...@@ -103,7 +103,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, request).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
......
...@@ -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.map { |scope| { name: scope.to_s }})
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, [{ name: "api" }])
end end
def valid_scoped_token?(token, scopes) def valid_scoped_token?(token, scopes)
......
...@@ -300,7 +300,7 @@ describe API::V3::Users do ...@@ -300,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')
......
...@@ -7,37 +7,37 @@ describe AccessTokenValidationService, services: true do ...@@ -7,37 +7,37 @@ 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])
expect(described_class.new(token, request).include_any_scope?([{ name: :api }])).to be(true) expect(described_class.new(token, request: 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, request).include_any_scope?([{ name: :api }, { name: :other_scope }])).to be(true) expect(described_class.new(token, request: 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, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true) expect(described_class.new(token, request: 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, request).include_any_scope?([{ name: :api }, { name: :read_user }, { name: :other_scope }])).to be(true) expect(described_class.new(token, request: 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, request).include_any_scope?([])).to be(true) expect(described_class.new(token, request: 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, request).include_any_scope?([{ name: :other_scope }])).to be(false) expect(described_class.new(token, request: request).include_any_scope?([{ name: :other_scope }])).to be(false)
end end
context "conditions" do context "conditions" do
...@@ -45,19 +45,19 @@ describe AccessTokenValidationService, services: true do ...@@ -45,19 +45,19 @@ describe AccessTokenValidationService, services: true do
it "ignores any scopes whose `if` condition returns false" do it "ignores any scopes whose `if` condition returns false" do
token = double("token", scopes: [:api, :read_user]) token = double("token", scopes: [:api, :read_user])
expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false) expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }])).to be(false)
end end
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])
expect(described_class.new(token, request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true) expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { false } }, { name: :read_user }])).to be(true)
end end
it "does not ignore scopes whose `if` condition returns true" do it "does not ignore scopes whose `if` condition returns true" do
token = double("token", scopes: [:api, :read_user]) 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) expect(described_class.new(token, request: request).include_any_scope?([{ name: :api, if: ->(_) { true } }, { name: :read_user, if: ->(_) { false } }])).to be(true)
end end
end end
end end
......
...@@ -32,7 +32,6 @@ shared_examples_for 'allows the "read_user" scope' do ...@@ -32,7 +32,6 @@ shared_examples_for 'allows the "read_user" scope' do
end end
context 'for doorkeeper (OAuth) tokens' do context 'for doorkeeper (OAuth) tokens' do
let!(:user) {create(:user)}
let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) } let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) }
context 'when the requesting token has the "api" scope' do context 'when the requesting token has the "api" scope' do
......
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