Mount auth operations in main GQL schema #96
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcelicalderon
force-pushed
the
mount-auth-on-main-schema
branch
2 times, most recently
from
a7de750
to
d178626
Compare
Closed
mcelicalderon
force-pushed
the
mount-auth-on-main-schema
branch
from
d178626
to
d8ea28d
Compare
mcelicalderon
force-pushed
the
mount-auth-on-main-schema
branch
from
38f3685
to
510ac1e
Compare
mcelicalderon
force-pushed
the
mount-auth-on-main-schema
branch
from
510ac1e
to
2e63cda
Compare
mcelicalderon
force-pushed
the
mount-auth-on-main-schema
branch
from
393db52
to
6898d21
Compare
00dav00
reviewed
Jun 9, 2020
| @@ -35,7 +35,7 @@ def resolve(confirm_success_url: nil, **attrs) | |||
|
|
|||
| { authenticatable: resource } | |||
| else | |||
| clean_up_passwords(resource) | |||
| resource.clean_up_passwords if resource.respond_to?(:clean_up_passwords) | |||
There was a problem hiding this comment.
Since rails is a dependency, did you considered try ?
lib/graphql_devise/schema_plugin.rb
Outdated
| field = traced_field(trace_data) | ||
| provided_value = authenticate_option(field, trace_data) | ||
|
|
||
| if (!provided_value.nil? && provided_value) || @authenticate_default |
There was a problem hiding this comment.
What about
provided_value.presenceThere was a problem hiding this comment.
provided_value might be false so I cannot use presence. But now I see this won't skip authentication if the provided value in the field is false and the default is true. I'll fix it.
00dav00
reviewed
Jun 10, 2020
README.md
Outdated
| option is provided, the route will be `/graphql_auth`. This has no effect on your own application schema. | ||
| More on this in the next section. | ||
| ### Important | ||
| Remember this gem mounts a completely separate GraphQL schema on a separate controller in the route |
There was a problem hiding this comment.
What about Remember that by default....
| before_action -> { set_resource_by_token(:user) } | ||
|
|
||
| def my_action | ||
| render json: DummySchema.execute(params[:query], context: graphql_context) |
There was a problem hiding this comment.
Is graphql_context supposed to be set by the main project?
In that case I think it may be useful to add a comment or something like
graphql_context = the_gql_context_created_by_your_application
render json: DummySchema.execute(params[:query], context: graphql_context) In that way it is clear that graphql_context is not provided by the egm and shouldn't be copypasta
There was a problem hiding this comment.
no, this is a method in our concern
README.md
Outdated
| provided by the `at` option in the `mount_graphql_devise_for` method in the `config/routes.rb` file by default. If no `at` | ||
| option is provided, the route will be `/graphql_auth`. | ||
|
|
||
| **Starting with `v0.12.0`** you can opt-in to a new behavior where you actually load this gem's |
There was a problem hiding this comment.
Suggested change
| **Starting with `v0.12.0`** you can opt-in to a new behavior where you actually load this gem's | |
| **Starting with `v0.12.0`** you can opt-in to load this auth queries and mutations into into your own application's schema. |
00dav00
approved these changes
Jun 11, 2020
| 1. `query`: This param is mandatory unless you skip all queries via the resource loader | ||
| options. This should be the same `QueryType` you provide to the `query` method | ||
| in your schema. | ||
| 1. `mutation`: This param mandatory unless you skip all mutations via the resource loader |
There was a problem hiding this comment.
Suggested change
| 1. `mutation`: This param mandatory unless you skip all mutations via the resource loader | |
| 1. `mutation`: This param is mandatory unless you skip all mutations via the resource loader |
| SetUserByToken.module_eval do | ||
| attr_accessor :client_id, :token, :resource | ||
|
|
||
| alias_method :set_resource_by_token, :set_user_by_token |
mcelicalderon
force-pushed
the
mount-auth-on-main-schema
branch
from
3a12f80
to
687ebaf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GQL specification is not very clear on how/where exactly authentication should be handled (here). After some reading and discussing with @00dav00 we have decided to implement this while still keeping a way to mount the schema in a separate route. This will allow to require/not_require authentication per field on the schema and having a default.
Users::Adminwould generate the wrong query name on the mailer helper.