diff options
author | Luke Duncalfe <lduncalfe@eml.cc> | 2019-06-21 08:09:02 +0300 |
---|---|---|
committer | Luke Duncalfe <lduncalfe@eml.cc> | 2019-06-26 01:12:35 +0300 |
commit | b2a615c3c60dd3315ecf33d5fdb61061c9d4003e (patch) | |
tree | 847a8806465ff076159c9b1018f204cd9371cf63 | |
parent | 87b468c254e3e1e3b6b1d86e003f1f90a39935a2 (diff) |
Sanity check for GraphQL authorized?
Raise an exception if a developer calls any of the GraphQL authorization
methods and a `authorize :permission` is missing from a mutation class.
Previously `authorized?` would return `true` in this situation, which
although technically is accurate is not what a developer is intending.
-rw-r--r-- | lib/gitlab/graphql/authorize/authorize_resource.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb | 45 |
2 files changed, 51 insertions, 0 deletions
diff --git a/lib/gitlab/graphql/authorize/authorize_resource.rb b/lib/gitlab/graphql/authorize/authorize_resource.rb index b367a97105c..c619464f69f 100644 --- a/lib/gitlab/graphql/authorize/authorize_resource.rb +++ b/lib/gitlab/graphql/authorize/authorize_resource.rb @@ -48,6 +48,12 @@ module Gitlab end 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| # The actions could be performed across multiple objects. In which # case the current user is common, and we could benefit from the diff --git a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb index 13cf52fd795..f9c4c619330 100644 --- a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb @@ -101,6 +101,51 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do 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_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 it 'adds permissions from subclasses to those of superclasses when used on classes' do base_class = Class.new do |