Merge branch 'graphql-error-when-authorizing-with-no-permissions-defined' into 'master'
Sanity check for GraphQL authorized? See merge request gitlab-org/gitlab-ce!29921
This commit is contained in:
commit
36db790a17
3 changed files with 46 additions and 19 deletions
|
@ -447,7 +447,7 @@ want to validate the abilities for.
|
||||||
|
|
||||||
Alternatively, we can add a `find_object` method that will load the
|
Alternatively, we can add a `find_object` method that will load the
|
||||||
object on the mutation. This would allow you to use the
|
object on the mutation. This would allow you to use the
|
||||||
`authorized_find!` and `authorized_find!` helper methods.
|
`authorized_find!` helper method.
|
||||||
|
|
||||||
When a user is not allowed to perform the action, or an object is not
|
When a user is not allowed to perform the action, or an object is not
|
||||||
found, we should raise a
|
found, we should raise a
|
||||||
|
|
|
@ -27,12 +27,6 @@ module Gitlab
|
||||||
raise NotImplementedError, "Implement #find_object in #{self.class.name}"
|
raise NotImplementedError, "Implement #find_object in #{self.class.name}"
|
||||||
end
|
end
|
||||||
|
|
||||||
def authorized_find(*args)
|
|
||||||
object = find_object(*args)
|
|
||||||
|
|
||||||
object if authorized?(object)
|
|
||||||
end
|
|
||||||
|
|
||||||
def authorized_find!(*args)
|
def authorized_find!(*args)
|
||||||
object = find_object(*args)
|
object = find_object(*args)
|
||||||
authorize!(object)
|
authorize!(object)
|
||||||
|
@ -48,6 +42,12 @@ module Gitlab
|
||||||
end
|
end
|
||||||
|
|
||||||
def authorized?(object)
|
def authorized?(object)
|
||||||
|
# Sanity check. We don't want to accidentally allow a developer to authorize
|
||||||
|
# without first adding permissions to authorize against
|
||||||
|
if self.class.required_permissions.empty?
|
||||||
|
raise Gitlab::Graphql::Errors::ArgumentError, "#{self.class.name} has no authorizations"
|
||||||
|
end
|
||||||
|
|
||||||
self.class.required_permissions.all? do |ability|
|
self.class.required_permissions.all? do |ability|
|
||||||
# The actions could be performed across multiple objects. In which
|
# The actions could be performed across multiple objects. In which
|
||||||
# case the current user is common, and we could benefit from the
|
# case the current user is common, and we could benefit from the
|
||||||
|
|
|
@ -34,12 +34,6 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe '#authorized_find' do
|
|
||||||
it 'returns the object' do
|
|
||||||
expect(loading_resource.authorized_find).to eq(project)
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
describe '#authorized_find!' do
|
describe '#authorized_find!' do
|
||||||
it 'returns the object' do
|
it 'returns the object' do
|
||||||
expect(loading_resource.authorized_find!).to eq(project)
|
expect(loading_resource.authorized_find!).to eq(project)
|
||||||
|
@ -66,12 +60,6 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
describe '#authorized_find' do
|
|
||||||
it 'returns `nil`' do
|
|
||||||
expect(loading_resource.authorized_find).to be_nil
|
|
||||||
end
|
|
||||||
end
|
|
||||||
|
|
||||||
describe '#authorized_find!' do
|
describe '#authorized_find!' do
|
||||||
it 'raises an error' do
|
it 'raises an error' do
|
||||||
expect { loading_resource.authorize!(project) }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
|
expect { loading_resource.authorize!(project) }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
|
||||||
|
@ -101,6 +89,45 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context 'when the class does not define authorize' do
|
||||||
|
let(:fake_class) do
|
||||||
|
Class.new do
|
||||||
|
include Gitlab::Graphql::Authorize::AuthorizeResource
|
||||||
|
|
||||||
|
attr_reader :user, :found_object
|
||||||
|
|
||||||
|
def initialize(user, found_object)
|
||||||
|
@user, @found_object = user, found_object
|
||||||
|
end
|
||||||
|
|
||||||
|
def find_object(*_args)
|
||||||
|
found_object
|
||||||
|
end
|
||||||
|
|
||||||
|
def current_user
|
||||||
|
user
|
||||||
|
end
|
||||||
|
|
||||||
|
def self.name
|
||||||
|
'TestClass'
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
let(:error) { /#{fake_class.name} has no authorizations/ }
|
||||||
|
|
||||||
|
describe '#authorized_find!' do
|
||||||
|
it 'raises a comprehensive error message' do
|
||||||
|
expect { loading_resource.authorized_find! }.to raise_error(error)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe '#authorized?' do
|
||||||
|
it 'raises a comprehensive error message' do
|
||||||
|
expect { loading_resource.authorized?(project) }.to raise_error(error)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
describe '#authorize' do
|
describe '#authorize' do
|
||||||
it 'adds permissions from subclasses to those of superclasses when used on classes' do
|
it 'adds permissions from subclasses to those of superclasses when used on classes' do
|
||||||
base_class = Class.new do
|
base_class = Class.new do
|
||||||
|
|
Loading…
Reference in a new issue