Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass query/mutation context into the field privacy detection for better access control #727

Merged
merged 12 commits into from
Apr 3, 2021

Conversation

torunar
Copy link
Contributor

@torunar torunar commented Mar 24, 2021

Summary

Currently privacy in the field specification can access only the query/mutation arguments.
This creates difficulties in the applications where access to the specific field is based on the current query/mutation context, and the context itself is the robust object which contains user, site area (back office / client area), etc.

This results in quirky workarounds like registering context globally in the app and accessing it via facades.
This PR is fixes this issue by passing query/mutation context to the privacy handlers, so this:

'privacy' => static function (array $args) {
    $ctx = app(Context::class);
    return $ctx->isAdministrator();
},

becomes this:

'privacy' => static function (array $args, $ctx) {
    return $ctx->isAdministrator();
},

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist:

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

@mfn
Copy link
Collaborator

mfn commented Mar 24, 2021

Just a quick feedback:

  • makes sense to me!
  • might be a breaking change though? (function signature change)
    "no a problem" though, we can just bump the major version and document it properly

=> will review later

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far I can already say: please add a test!

Also, a changelog entry as I think this is the way forward.

Are there any other arguments we might want to consider passing (like we do to the resolver)?

@torunar
Copy link
Contributor Author

torunar commented Mar 25, 2021

might be a breaking change though? (function signature change)

Existing privacy handlers will be compatible: calling function with more parameters that defined in its signature won't produce an error.

You are right, it breaks compatibility for the privacy policies defined in the class, this looks like a change for the major version then.

@torunar
Copy link
Contributor Author

torunar commented Mar 25, 2021

Hi @mfn,

I've added tests, changelog entries and marked this PR as the one with breaking changes. Thank you for the feedback, I was so occupied with closure-based privacy handlers, I've completely missed the class-based ones.

Are there any other arguments we might want to consider passing (like we do to the resolver)?

I currently don't see use cases where we need anything but query arguments and its context to decide whether a field is accessible in the context of the current operation. All the extra can be passed via the class privacy handler constructor and auto-wired here:

$instance = app($privacyClass);

@torunar
Copy link
Contributor Author

torunar commented Mar 26, 2021

@mfn

May I ask for your help with the code analysis checks?
When I run composer phpstan locally, I get the following errors:

 ------ -----------------------------------------------------------------------
  Line   src/Support/SelectFields.php
 ------ -----------------------------------------------------------------------
         Ignored error pattern #^Parameter \#1 \$callback of function
         call_user_func expects callable\(\)\: mixed, array\(\*NEVER\*,
         'fire'\) given\.$# in path
         /Users/mschekotov/srv/src/graphql-laravel/src/Support/SelectFields.ph
         p was not matched in reported errors.
         Ignored error pattern #^Parameter \#1 \$callback of function
         call_user_func expects callable\(\)\: mixed, array\(\*NEVER\*,
         mixed\) given\.$# in path
         /Users/mschekotov/srv/src/graphql-laravel/src/Support/SelectFields.ph
         p was not matched in reported errors.
  203    Parameter #1 $function of function call_user_func expects callable():
         mixed, array(*NEVER*, mixed) given.
 ------ -----------------------------------------------------------------------

 ------ ----------------------------------------------------------------------
  Line   src/Support/Type.php
 ------ ----------------------------------------------------------------------
         Ignored error pattern #^Parameter \#1 \$callback of function
         call_user_func_array expects callable\(\)\: mixed,
         array\(\$this\(Rebing\\GraphQL\\Support\\Type\), string\) given\.$#
         in path
         /Users/mschekotov/srv/src/graphql-laravel/src/Support/Type.php was
         not matched in reported errors.
  68     Parameter #1 $function of function call_user_func_array expects
         callable(): mixed, array($this(Rebing\GraphQL\Support\Type), string)
         given.
 ------ ----------------------------------------------------------------------

 ------ ------------------------------------------------------------------
  Line   src/routes.php
 ------ ------------------------------------------------------------------
         Ignored error pattern #^Parameter \#2 \$callback of function
         array_filter expects \(callable\(mixed, mixed\)\: bool\)\|null,
         Closure\(array, string\)\: bool\|null given\.$# in path
         /Users/mschekotov/srv/src/graphql-laravel/src/routes.php was not
         matched in reported errors.
  40     Parameter #2 $callback of function array_filter expects
         callable(mixed, mixed): bool, Closure(array, string): bool|null
         given.
 ------ ------------------------------------------------------------------

 ------ ------------------------------------------------------------------------
  Line   tests/Database/SelectFields/ValidateFieldTests/PrivacyQueryContext.php
 ------ ------------------------------------------------------------------------
  15     Method
         Rebing\GraphQL\Tests\Database\SelectFields\ValidateFieldTests\Privacy
         QueryContext::validate() has parameter $queryArgs with no value type
         specified in iterable type array.
         💡 See:
         https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-i
         terable-type
 ------ ------------------------------------------------------------------------

But when the same action is performed via GitHub, it produces only 2 errors:

 ------ ----------------------------------------------------------------------- 
  Line   src/Support/SelectFields.php                                           
 ------ ----------------------------------------------------------------------- 
         Ignored error pattern #^Parameter \#1 \$callback of function           
         call_user_func expects callable\(\)\: mixed, array\(\*NEVER\*,         
         'fire'\) given\.$# in path                                             
         /home/runner/work/graphql-laravel/graphql-laravel/src/Support/SelectF  
         ields.php was not matched in reported errors.                          
 ------ ----------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------ 
  Line   tests/Database/SelectFields/ValidateFieldTests/PrivacyQueryContext.php  
 ------ ------------------------------------------------------------------------ 
  15     Method                                                                  
         Rebing\GraphQL\Tests\Database\SelectFields\ValidateFieldTests\Privacy   
         QueryContext::validate() has parameter $queryArgs with no value type    
         specified in iterable type array.                                       
         💡 See:                                                                  
         https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-i   
         terable-type                                                            
 ------ ------------------------------------------------------------------------ 

@mfn
Copy link
Collaborator

mfn commented Mar 26, 2021

But when the same action is performed via GitHub, it produces only 2 errors:

Ah yeah, super annoying: because of the difference of the PHP version I think. Github Action is using 8.0, see

But: don't worry, I'm not rejecting anyones work due to this ;) Once I've time for the review, I'll take care of it!

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the PR!

I fixed the phpstan baseline and some other small things since I was already on it.

@mfn mfn merged commit ab6bd09 into rebing:master Apr 3, 2021
@mfn
Copy link
Collaborator

mfn commented Apr 3, 2021

PS: that was really as solid PR for a first time contribution, thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants