From b2a615c3c60dd3315ecf33d5fdb61061c9d4003e Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Fri, 21 Jun 2019 17:09:02 +1200 Subject: [PATCH 1/2] 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. --- .../graphql/authorize/authorize_resource.rb | 6 +++ .../authorize/authorize_resource_spec.rb | 45 +++++++++++++++++++ 2 files changed, 51 insertions(+) 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 From bbdcbd98aed2bfb4eba008669d3fca500b6e0ace Mon Sep 17 00:00:00 2001 From: Luke Duncalfe Date: Wed, 26 Jun 2019 10:15:14 +1200 Subject: [PATCH 2/2] Remove unused authorized_find method https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/29921#note_184713939 --- doc/development/api_graphql_styleguide.md | 2 +- .../graphql/authorize/authorize_resource.rb | 6 ------ .../authorize/authorize_resource_spec.rb | 18 ------------------ 3 files changed, 1 insertion(+), 25 deletions(-) diff --git a/doc/development/api_graphql_styleguide.md b/doc/development/api_graphql_styleguide.md index 2ed2a905db7..aeddad14995 100644 --- a/doc/development/api_graphql_styleguide.md +++ b/doc/development/api_graphql_styleguide.md @@ -447,7 +447,7 @@ want to validate the abilities for. Alternatively, we can add a `find_object` method that will load 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 found, we should raise a diff --git a/lib/gitlab/graphql/authorize/authorize_resource.rb b/lib/gitlab/graphql/authorize/authorize_resource.rb index c619464f69f..ef5caaf5b0e 100644 --- a/lib/gitlab/graphql/authorize/authorize_resource.rb +++ b/lib/gitlab/graphql/authorize/authorize_resource.rb @@ -27,12 +27,6 @@ module Gitlab raise NotImplementedError, "Implement #find_object in #{self.class.name}" end - def authorized_find(*args) - object = find_object(*args) - - object if authorized?(object) - end - def authorized_find!(*args) object = find_object(*args) authorize!(object) diff --git a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb index f9c4c619330..20842f55014 100644 --- a/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb +++ b/spec/lib/gitlab/graphql/authorize/authorize_resource_spec.rb @@ -34,12 +34,6 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do 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 it 'returns the object' do expect(loading_resource.authorized_find!).to eq(project) @@ -66,12 +60,6 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do end end - describe '#authorized_find' do - it 'returns `nil`' do - expect(loading_resource.authorized_find).to be_nil - end - end - describe '#authorized_find!' do it 'raises an error' do expect { loading_resource.authorize!(project) }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable) @@ -127,12 +115,6 @@ describe Gitlab::Graphql::Authorize::AuthorizeResource do 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)