-
Notifications
You must be signed in to change notification settings - Fork 13
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
Absinthe v1.5, Relay support #27
Draft
jeroenvisser101
wants to merge
9
commits into
jungsoft:master
Choose a base branch
from
koode:jv-absinthe-v1.5
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+153
−98
Draft
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c60f88c
Only compile RateLimiter when Hammer is available
jeroenvisser101 760d3b2
Allow Absinthe 1.5.0
jeroenvisser101 3320148
Project fields to support named fragment spreads
jeroenvisser101 2e59556
Add updated Introspection exception
jeroenvisser101 18c91b6
Don't require QueryAuthorization to be first
jeroenvisser101 22edb56
Add optional default_authorize callback
jeroenvisser101 f0de230
Allow custom errors in unauthorized_message
jeroenvisser101 a5d8a06
Add plugin implementation
jeroenvisser101 9c9ac9c
Add 1.6 compatibility
jeroenvisser101 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,71 +1,73 @@ | ||
defmodule Rajska.RateLimiter do | ||
@moduledoc """ | ||
Rate limiter absinthe middleware. Uses [Hammer](https://github.com/ExHammer/hammer). | ||
if Code.ensure_loaded?(Hammer) do | ||
defmodule Rajska.RateLimiter do | ||
@moduledoc """ | ||
Rate limiter absinthe middleware. Uses [Hammer](https://github.com/ExHammer/hammer). | ||
|
||
## Usage | ||
## Usage | ||
|
||
First configure Hammer, following its documentation. For example: | ||
First configure Hammer, following its documentation. For example: | ||
|
||
config :hammer, | ||
backend: {Hammer.Backend.ETS, [expiry_ms: 60_000 * 60 * 4, | ||
cleanup_interval_ms: 60_000 * 10]} | ||
config :hammer, | ||
backend: {Hammer.Backend.ETS, [expiry_ms: 60_000 * 60 * 4, | ||
cleanup_interval_ms: 60_000 * 10]} | ||
|
||
Add your middleware to the query that should be limited: | ||
Add your middleware to the query that should be limited: | ||
|
||
field :default_config, :string do | ||
middleware Rajska.RateLimiter | ||
resolve fn _, _ -> {:ok, "ok"} end | ||
end | ||
field :default_config, :string do | ||
middleware Rajska.RateLimiter | ||
resolve fn _, _ -> {:ok, "ok"} end | ||
end | ||
|
||
You can also configure it and use multiple rules for limiting in one query: | ||
You can also configure it and use multiple rules for limiting in one query: | ||
|
||
field :login_user, :session do | ||
arg :email, non_null(:string) | ||
arg :password, non_null(:string) | ||
field :login_user, :session do | ||
arg :email, non_null(:string) | ||
arg :password, non_null(:string) | ||
|
||
middleware Rajska.RateLimiter, limit: 10 # Using the default identifier (user IP) | ||
middleware Rajska.RateLimiter, keys: :email, limit: 5 # Using the value provided in the email arg | ||
resolve &AccountsResolver.login_user/2 | ||
end | ||
middleware Rajska.RateLimiter, limit: 10 # Using the default identifier (user IP) | ||
middleware Rajska.RateLimiter, keys: :email, limit: 5 # Using the value provided in the email arg | ||
resolve &AccountsResolver.login_user/2 | ||
end | ||
|
||
The allowed configuration are: | ||
The allowed configuration are: | ||
|
||
* `scale_ms`: The timespan for the maximum number of actions. Defaults to 60_000. | ||
* `limit`: The maximum number of actions in the specified timespan. Defaults to 10. | ||
* `id`: An atom or string to be used as the bucket identifier. Note that this will always be the same, so by using this the limit will be global instead of by user. | ||
* `keys`: An atom or a list of atoms to get a query argument as identifier. Use a list when the argument is nested. | ||
* `error_msg`: The error message to be displayed when rate limit exceeds. Defaults to `"Too many requests"`. | ||
* `scale_ms`: The timespan for the maximum number of actions. Defaults to 60_000. | ||
* `limit`: The maximum number of actions in the specified timespan. Defaults to 10. | ||
* `id`: An atom or string to be used as the bucket identifier. Note that this will always be the same, so by using this the limit will be global instead of by user. | ||
* `keys`: An atom or a list of atoms to get a query argument as identifier. Use a list when the argument is nested. | ||
* `error_msg`: The error message to be displayed when rate limit exceeds. Defaults to `"Too many requests"`. | ||
|
||
Note that when neither `id` or `keys` is provided, the default is to use the user's IP. For that, the default behaviour is to use | ||
`c:Rajska.Authorization.get_ip/1` to fetch the IP from the absinthe context. That means you need to manually insert the user's IP in the | ||
absinthe context before using it as an identifier. See the [absinthe docs](https://hexdocs.pm/absinthe/context-and-authentication.html#content) | ||
for more information. | ||
""" | ||
@behaviour Absinthe.Middleware | ||
Note that when neither `id` or `keys` is provided, the default is to use the user's IP. For that, the default behaviour is to use | ||
`c:Rajska.Authorization.get_ip/1` to fetch the IP from the absinthe context. That means you need to manually insert the user's IP in the | ||
absinthe context before using it as an identifier. See the [absinthe docs](https://hexdocs.pm/absinthe/context-and-authentication.html#content) | ||
for more information. | ||
""" | ||
@behaviour Absinthe.Middleware | ||
|
||
alias Absinthe.Resolution | ||
alias Absinthe.Resolution | ||
|
||
def call(%Resolution{state: :resolved} = resolution, _config), do: resolution | ||
def call(%Resolution{state: :resolved} = resolution, _config), do: resolution | ||
|
||
def call(%Resolution{} = resolution, config) do | ||
scale_ms = Keyword.get(config, :scale_ms, 60_000) | ||
limit = Keyword.get(config, :limit, 10) | ||
identifier = get_identifier(resolution, config[:keys], config[:id]) | ||
error_msg = Keyword.get(config, :error_msg, "Too many requests") | ||
def call(%Resolution{} = resolution, config) do | ||
scale_ms = Keyword.get(config, :scale_ms, 60_000) | ||
limit = Keyword.get(config, :limit, 10) | ||
identifier = get_identifier(resolution, config[:keys], config[:id]) | ||
error_msg = Keyword.get(config, :error_msg, "Too many requests") | ||
|
||
case Hammer.check_rate("query:#{identifier}", scale_ms, limit) do | ||
{:allow, _count} -> resolution | ||
{:deny, _limit} -> Resolution.put_result(resolution, {:error, error_msg}) | ||
case Hammer.check_rate("query:#{identifier}", scale_ms, limit) do | ||
{:allow, _count} -> resolution | ||
{:deny, _limit} -> Resolution.put_result(resolution, {:error, error_msg}) | ||
end | ||
end | ||
end | ||
|
||
defp get_identifier(%Resolution{context: context}, nil, nil), | ||
do: Rajska.apply_auth_mod(context, :get_ip, [context]) | ||
defp get_identifier(%Resolution{context: context}, nil, nil), | ||
do: Rajska.apply_auth_mod(context, :get_ip, [context]) | ||
|
||
defp get_identifier(%Resolution{arguments: arguments}, keys, nil), | ||
do: get_in(arguments, List.wrap(keys)) || raise "Invalid configuration in Rate Limiter. Key not found in arguments." | ||
defp get_identifier(%Resolution{arguments: arguments}, keys, nil), | ||
do: get_in(arguments, List.wrap(keys)) || raise "Invalid configuration in Rate Limiter. Key not found in arguments." | ||
|
||
defp get_identifier(%Resolution{}, nil, id), do: id | ||
defp get_identifier(%Resolution{}, nil, id), do: id | ||
|
||
defp get_identifier(%Resolution{}, _keys, _id), do: raise "Invalid configuration in Rate Limiter. If key is defined, then id must not be defined" | ||
defp get_identifier(%Resolution{}, _keys, _id), do: raise "Invalid configuration in Rate Limiter. If key is defined, then id must not be defined" | ||
end | ||
end |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,7 +65,8 @@ defmodule Rajska do | |
defmacro __using__(opts \\ []) do | ||
super_role = Keyword.get(opts, :super_role, :admin) | ||
valid_roles = Keyword.get(opts, :valid_roles, [super_role]) | ||
default_rule = Keyword.get(opts, :default_rule, :default) | ||
default_rule = Keyword.get(opts, :default_rule, :default) | ||
default_authorize = Keyword.get(opts, :default_authorize, nil) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add this option to the docs on this file (line 40) and on readme? |
||
|
||
quote do | ||
@behaviour Authorization | ||
|
@@ -130,6 +131,8 @@ defmodule Rajska do | |
|> get_current_user() | ||
|> has_user_access?(scoped_struct, rule) | ||
end | ||
|
||
def default_authorize(_context, _object), do: unquote(default_authorize) | ||
rschef marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
defoverridable Authorization | ||
end | ||
|
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did a PR for this and only now realized you already did something similar. But I think this implementation is not the best because we simply ignore the fragments, which could contain further associations.
What do you think about the implementation in #29?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋 , good catch, however, I think I added line 66, which should project the fields down. If you look at the docs for
Absinthe.Resolution.project/2
, they mention it's non-trivial. The fragments might not be simple, and might not always apply to this type (for interfaces for instance).I wonder what would happen if I added the same tests you did, if this would still be passing 🤔
rajska/lib/middlewares/object_authorization.ex
Line 66 in a5d8a06
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't☹️
I believe the problem is "one layer down":
In the tests we have a fragment within a fragment and so the inner fragment is ignored by the authorization and the query returns successfully when it should return an error.
So I think the best option here is for me to merge #29 and then you can rebase with it. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gabrielpra1 noticed it yesterday yeah, have been trying to get it to work but haven't been succesful yet. The thing is, not all fragments are named:
I don't think this one would work, would it?
I still think projection should be the solution here, and as long as we project at the correct level, it should error appropriately, even in nested fragments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gabrielpra1 the thing for us here, is that we'd really like to be able to use the same query for users with different permissions, and those permissions would only check actual access. For instance, in relay, you might see queries like this:
As long as the user only receives what they have access to (e.g. only passes global IDs of types that are allowed), this is fine. The thing is, node is defined as an interface, and only after resolution can we be sure that the user isn't accessing anything it doesn't have access to. I naively assumed that the middleware was being created for each object returned, but it only runs once, before resolution, meaning the details required to determine what the node interface resolves to, are not available yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work, we already support inline fragments. Here is the query being used in the test case:
Absinthe's projection only takes the entire resolution as argument, so I don't think we could use it recursively. Seeing the source code, though, I think we are already covering all cases, except the
skip
directive, which I have never seen being used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do use
description @include(if: $includeProductDescription)
which is similar to@skip
.I can try your PR in our project to see if any issues arise.
[https://hexdocs.pm/absinthe/Absinthe.Resolution.html#project/2](The example from the docs) seem to suggest it would be able to run at any level, I think because of the
path
in the resolution, but again, this would not make it work with interface types (it would work only post-resolution)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge it for now since it is covering our use cases, but we can continue that discussion in another PR if you still have troubles with it.
I'm not sure how to deal with interface types, it looks like we would need to rely only on the ObjectScopeAuthorization for this case, since it is the only one that runs after resolution