Absinthe v1.5, Relay support #27

jeroenvisser101 · 2020-07-15T22:45:19Z

Includes support for Absinthe v1.5, Absinthe.Relay, and adds default_authorize callback/option that can optionally be used to set the default value where no value was available.

We're still testing this internally, hence the draft. If you have any guidance in the meantime, that's welcome :)

Thanks for open sourcing! 🙏

This would break usage in Relay. This can be improved to check if any Resolution middleware, and if those are found, compilation could still be halted like before. This ensures that there are no accidental 'open doors' when middleware isn't added before QueryAuthorization.

This allows setting the `authorize: ...` value for all fields if none was set. This allows for an easier adoption for APIs that might previously not have any role-based authorization. Defaulting to their default role, for instance, :admin, and only adding `authorize: ...` where authorization can be relaxed.

lib/rajska.ex

rschef · 2020-07-16T10:43:22Z

lib/rajska.ex

@@ -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)


Could you add this option to the docs on this file (line 40) and on readme?

rschef · 2020-07-16T10:51:35Z

Hi @jeroenvisser101, thanks for contributing!

I added two comments about missing docs and minor improvements, but other than that looks good to me.

It seems some tests are also missing, you can check uncovered lines here.

I'll leave to @gabrielpra1 to approve this PR, since he has been using and maintaing this lib more actively than me.

jeroenvisser101 · 2020-07-16T10:58:08Z

@rschef I also noticed that this project isn't using Elixir formatter yet. I tried to adhere to the old formatting as much as possible to not break the history, but I think it would be much nicer to have Elixir auto-format all the code. wdyt?

rschef · 2020-07-16T15:02:41Z

@rschef I also noticed that this project isn't using Elixir formatter yet. I tried to adhere to the old formatting as much as possible to not break the history, but I think it would be much nicer to have Elixir auto-format all the code. wdyt?

I wasn't a big fan of Elixir formatter before, but after using it for a couple of months I think it does save time and ensures a standard format. @gabrielpra1, what do you think? We could also add it in another PR.

gabrielpra1 · 2020-07-16T15:18:47Z

@rschef I also noticed that this project isn't using Elixir formatter yet. I tried to adhere to the old formatting as much as possible to not break the history, but I think it would be much nicer to have Elixir auto-format all the code. wdyt?

I wasn't a big fan of Elixir formatter before, but after using it for a couple of months I think it does save time and ensures a standard format. @gabrielpra1, what do you think? We could also add it in another PR.

I agree, specially for open source libraries. But yes, should be done in a different PR, since this one already introduces 3 unrelated changes haha

jeroenvisser101 · 2020-07-16T15:19:06Z

We could also add it in another PR.

This sound like a plan. I also noticed I accidentally did change some formatting, so will revert those changes and rebase. Some more things came up while testing (in subscriptions mainly, and some cases where an error deeper in the graph would cause non_null to traverse up and fields in %Blueprint{execution: ...} to be nil.

jeroenvisser101 · 2020-07-16T15:22:45Z

But yes, should be done in a different PR, since this one already introduces 3 unrelated changes haha

Yeah not really what I set out to do to be honest 😆

I did commit each change separately so if you'd like, I could take out everything related to default_authorize.

18c91b6

Don't require QueryAuthorization to be first

This would break usage in Relay. This can be improved to check if
any Resolution middleware, and if those are found, compilation
could still be halted like before. This ensures that there are
no accidental 'open doors' when middleware isn't added before
QueryAuthorization.

This didn't come up in the review yet, but I'd like to discuss this if possible. I think it's still a critical check to ensure that the middleware is added before any resolution/resolve middleware.

This can be used as an alternative to pipeline modification.

gabrielpra1 · 2020-08-21T17:41:05Z

lib/middlewares/object_authorization.ex

@@ -114,6 +122,13 @@ defmodule Rajska.ObjectAuthorization do
    authorize(schema_node, selections ++ tail, resolution)
  end

+  defp find_associations(
+    [%Absinthe.Blueprint.Document.Fragment.Spread{} | tail],


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?

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

fields = Resolution.project(resolution)

It doesn't ☹️

I believe the problem is "one layer down":

Project one layer down from where we are right now.

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?

@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:

... on Type { field }

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.

@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:

query NodeQuery($id: ID!) { node(id: $id) { ...Product ...Order ...Customer } }

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.

I don't think this one would work, would it?

It should work, we already support inline fragments. Here is the query being used in the test case:

{ unionQuery { ... on User { name } ... on WalletBalance { total } } }

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.

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)

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

gabrielpra1 · 2020-08-21T17:43:53Z

This didn't come up in the review yet, but I'd like to discuss this if possible. I think it's still a critical check to ensure that the middleware is added before any resolution/resolve middleware.

Agreed. I think the best approach here would be to go through the middlewares until we find QueryAuthorization. If we find a resolution middleware before that, we throw an error.

gabrielpra1 · 2020-08-21T17:44:43Z

By the way @jeroenvisser101, how is the PR going? Were you able to get it working with Absinthe 1.5 or do you need help with something?

jeroenvisser101 · 2020-08-21T19:19:04Z

By the way @jeroenvisser101, how is the PR going? Were you able to get it working with Absinthe 1.5 or do you need help with something?

We've been using this PR in production since end of July, and have not seen any issues, so far so good. I do think field-level authentication is broken because of the issue upstream in Absinthe, which as far as I know is unresolved right now.

Status for this PR is that tests are pending. We are also struggling a little bit with the 'role' idea, because internally it's not as clear-cut: some customer service agents also need access to financial data, and some features might only be enabled for certain users. We were thinking about ways to add multiple roles to users, but haven't spent much if any time on that yet.

jeroenvisser101 · 2020-08-21T19:24:09Z

Agreed. I think the best approach here would be to go through the middlewares until we find QueryAuthorization. If we find a resolution middleware before that, we throw an error.

For sure, it would be a matter of changing the Enum.find statement to raise on any Resolution middleware:

rajska/lib/schema.ex

Line 26 in 18c91b6

|> Enum.find(&match?({{QueryAuthorization, :call}, _config}, &1))

gabrielpra1 · 2021-02-15T19:56:47Z

Hey @jeroenvisser101, I noticed this PR is still active. Our latest release (1.1.0) has been working well with Absinthe 1.5.4+, have you had a chance to try it out?

From what I've seen, this PR still adds two extra functionalities: relay support and the default_authorize callback, so if you want to to open new PRs for that, I would be glad to accept them 😄

Other things, like support for Absinthe 1.5, and not requiring QueryAuthorization to be the first middleware are already on master.

jeroenvisser101 · 2021-02-15T20:02:16Z

@gabrielpra1 hi :)

I haven't, but that's mostly due to an expected incomptibility with how rasjka:master handles fragments on interfaces. I was just in the process of updating the version constraint on Absinthe, since they bumped to 1.6, which isn't supported in master yet.

I'll try to see if I can rebase these changes onto master and if the fragment support is sufficient, if it is, I'll open PRs for:

1.6 support
relay support
default_authorize
add compile guard for RateLimiter when Hammer isn't available (c60f88c)
add plugin implementation (a5d8a06)

churcho · 2023-03-01T23:23:08Z

@jeroenvisser101 is is possible to have an outline of possible things that might need to be updated/fixed to move this forward?

jeroenvisser101 added 3 commits July 16, 2020 00:30

Only compile RateLimiter when Hammer is available

c60f88c

Allow Absinthe 1.5.0

760d3b2

Project fields to support named fragment spreads

3320148

jeroenvisser101 force-pushed the jv-absinthe-v1.5 branch from f86fd56 to 6ea9689 Compare July 15, 2020 23:16

jeroenvisser101 added 4 commits July 16, 2020 02:45

Add updated Introspection exception

2e59556

Allow custom errors in unauthorized_message

f0de230

jeroenvisser101 force-pushed the jv-absinthe-v1.5 branch from 6ea9689 to f0de230 Compare July 16, 2020 00:45

rschef reviewed Jul 16, 2020

View reviewed changes

lib/rajska.ex Show resolved Hide resolved

rschef reviewed Jul 16, 2020

View reviewed changes

rschef assigned gabrielpra1 Jul 16, 2020

Add plugin implementation

a5d8a06

This can be used as an alternative to pipeline modification.

gabrielpra1 reviewed Aug 21, 2020

View reviewed changes

gabrielpra1 mentioned this pull request Oct 5, 2020

Support absinthe 1.5 #30

Merged

Add 1.6 compatibility

9c9ac9c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Absinthe v1.5, Relay support #27

Absinthe v1.5, Relay support #27

jeroenvisser101 commented Jul 15, 2020

rschef Jul 16, 2020

rschef commented Jul 16, 2020

jeroenvisser101 commented Jul 16, 2020

rschef commented Jul 16, 2020

gabrielpra1 commented Jul 16, 2020

jeroenvisser101 commented Jul 16, 2020

jeroenvisser101 commented Jul 16, 2020

gabrielpra1 Aug 21, 2020

jeroenvisser101 Aug 21, 2020

gabrielpra1 Aug 21, 2020

jeroenvisser101 Aug 22, 2020

jeroenvisser101 Aug 22, 2020

gabrielpra1 Aug 22, 2020

jeroenvisser101 Aug 22, 2020

gabrielpra1 Aug 26, 2020

gabrielpra1 commented Aug 21, 2020

gabrielpra1 commented Aug 21, 2020

jeroenvisser101 commented Aug 21, 2020

jeroenvisser101 commented Aug 21, 2020

gabrielpra1 commented Feb 15, 2021

jeroenvisser101 commented Feb 15, 2021 •

edited

Loading

churcho commented Mar 1, 2023

Absinthe v1.5, Relay support #27

Are you sure you want to change the base?

Absinthe v1.5, Relay support #27

Conversation

jeroenvisser101 commented Jul 15, 2020

Choose a reason for hiding this comment

rschef commented Jul 16, 2020

jeroenvisser101 commented Jul 16, 2020

rschef commented Jul 16, 2020

gabrielpra1 commented Jul 16, 2020

jeroenvisser101 commented Jul 16, 2020

jeroenvisser101 commented Jul 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielpra1 commented Aug 21, 2020

gabrielpra1 commented Aug 21, 2020

jeroenvisser101 commented Aug 21, 2020

jeroenvisser101 commented Aug 21, 2020

gabrielpra1 commented Feb 15, 2021

jeroenvisser101 commented Feb 15, 2021 • edited Loading

churcho commented Mar 1, 2023

jeroenvisser101 commented Feb 15, 2021 •

edited

Loading