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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ctx.params and ctx.query available to graphql policies #4272

Merged
merged 4 commits into from Oct 21, 2019

Conversation

@donmasakayan
Copy link
Contributor

donmasakayan commented Oct 16, 2019

Currently

When a policy is triggered via a GraphQL route, it's not possible to read the query params (i.e. ctx.params, ctx.query or ctx.request.body).

This makes it impossible to perform some policies such as verifying ownership of documents, etc.

If this was by design, please do let me know why.

What this PR does

Overall, I'm just moving up a bunch of already-existing code earlier, so that policies have access to the ctx values.

GraphQL Queries

  • The convertToParams() and convertToQuery() functions are executed and attached to the ctx before the policies are executed.

GraphQL Mutations

  • The convertToParams() function is executed and attached to the ctx before the policies are executed.
  • The options values are attached to ctx.request.body prior to the policies being executed.

  • 馃挜 Breaking change
  • 馃悰 Bug fix
  • 馃拝 Enhancement
  • 馃殌 New feature

Main update on the:

  • Admin
  • Documentation
  • Framework
  • Plugin

Manual testing done on the following databases:

  • Not applicable
  • MongoDB
  • MySQL
  • Postgres
  • SQLite
Copy link
Collaborator

alexandrebodin left a comment

Just a comment but will be good to have ! thanks

@alexandrebodin alexandrebodin added this to the 3.0.0-beta.17.2 milestone Oct 18, 2019
@alexandrebodin alexandrebodin changed the title ctx.params and ctx.query available to graphql policies Make ctx.params and ctx.query available to graphql policies Oct 18, 2019
donmasakayan and others added 3 commits Oct 19, 2019
Copy link
Collaborator

alexandrebodin left a comment

LGTM

@alexandrebodin alexandrebodin merged commit 95985ff into strapi:master Oct 21, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JelmerV-WFC

This comment has been minimized.

Copy link
Contributor

JelmerV-WFC commented Oct 22, 2019

@alexandrebodin @donmasakayan this merge breaks GraphQL queries (and possibly mutations) that queries multiple content types in one query (error message: Your filters contain a field 'x' that doesn't appear on your model definition nor it's relations). Will investigate.

@donmasakayan

This comment has been minimized.

Copy link
Contributor Author

donmasakayan commented Oct 23, 2019

I didn't get to test that usecase. Thanks for the head's up, will investigate that too.

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Oct 23, 2019

@donmasakayan a PR is already open don't bother doing one too ;) The issue isn't coming from this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.