Support JsonPath union operator

[lsmor]

Problem

The current implementation of JsonPath does not allow for union operator [,] in its full form: only array indices are allowed. Such operator is part of the RFC9535. The idea is that you can specify different keys in you jwt-role-claim-key and pick the first match.

Here there is an example in the JSONPath Online Evaluator. The example shows the syntax and the results

What's the use case?

If you have multiple Identity providers, they could send the role in different claims. It is not clear to me how you handle such a situation. Currently I was able to put a custom claim in both IdP's because I have permissions to do so, but that's not true always.

Solution

For example, in your config file you could have

# proposed syntax
jwt-role-claim-key=".[role,userinfo.role]" 

That will search for role claim, and if fails will search for useinfo.role. I am bearly familiar with Pgrst codebase, but I think the implementation should be something in the line of:

-- module PostgREST.Config.JSPath
-- Completely untested code
type JSPath = [JSPathExp]

data JSPathExp
  = JSPKey Text                      -- .property or ."property-dash"
  | JSPIdx Int                       -- [0]
  | JSPSlice (Maybe Int) (Maybe Int) -- [0:5] or [0:] or [:5] or [:]
  | JSPFilter FilterExp              -- [?(@ == "match")]
  | JSPUnion [JSPath]   -- <<<NEW>>> An union is a list of JSPaths (notice the nested list because of JSPath)


-- <<<NEW>>> Parser for the union case
pJSPUnion :: P.Parser JSPathExp
pJSPUnion = do
  P.char '['
  inner <- P.many1 pJSPKey `P.sepBy` ','
  P.char ']'
  return (JSPUnion inner ) <?> "pJSPUnion: JSPath union operator"

-- <<NEW>> the walk for the new case. Something tells me is a simple usage of the Alternative instance of Maybe
walkJSPath jsonObj (JSPUnion jpaths:rest) = asum $  fmap (walkJSPath jsonObj)  jpaths -- apply walkJSPath to all and take the first Just value

-- Other things should be implemented as well

The above is far from a real implementation, but moreless you can have an idea.

[wolfgangwalther]

We should make a basic decision on whether we want to support full JsonPath or not.

[taimoorzaeem]

I think it looks like we should support full RFC 9535. We already have a library aeson-jsonpath available which is fully compliant (except for some advanced features which can be added later, if needed).

This would be a breaking change for sure and we'll lose feature #3813, however this can be easily patched into aeson-jsonpath. #4599 is also implemented in the library and so is the union operator.

[wolfgangwalther]

We already have a library aeson-jsonpath available

I briefly looked into it:

• I believe this library is more useful to the public than "a copy of PG's fuzzy match into haskell". While it's not widely (or at all?) used by other hackage packages, it has at least a bit of history of being maintained already. So in contrast to what I said in fix: remove package fuzzyset in favor of fuzzystrmatch-pg #4837, I'd not oppose the usage of the library on the grounds mentioned there.
• Even though it uses template-haskell for quasi quoters, as long as we don't use them, we should be fine in our attempts to stay TH-free for "cross-compilation without emulators". Or in other words: I tested building it in the static nixpkgs overlay with the external interpreter disabled: It compiles just fine. No blocker here either.

This would be a breaking change for sure and we'll lose feature #3813,

Can this be expressed differently with JSONPath?

however this can be easily patched into aeson-jsonpath.

In which way? Can we extend aeson-jsonpath downstream somehow or do you mean to patch it in upstream? The latter doesn't sound compelling, because that would violate RFC 9535 compliance, right? I mean... even the former doesn't seem like a great idea, because we could then not say "it's just RFC 9535, that's all".

[lsmor]

May I share some thoughts/questions?

• As far as I understand, Jsonpanth should start with $.<expr>, but pgrst implementation seems to drop initial $. Is this correct?
• Maybe implementing the full union operator is not the solution, but would be better to simply allow jwt-role-claim-key to be a comma-separated list of expressions as currently implemented. The role is the first match.
• Another solution could be to allow role extraction delegation to an external tool/filter, like jq for example. I guess depending on thirdparty tool is not the best solution
• Following the previous idea, what about a new config parameter jwt-role-pre-filter=./path/to/some/executable in the spirit of pandoc's filters. some/executable is a user-made executable which takes the decoded jwt from stdin and outputs a json to stdout. That output json is the one checked by jwt-role-claim-key. The user is responsable for including that exectuable in the docker container or virtual machine pgrst is deployed, that way pgrst does not add a new dependency

How happy are you with new contributors? I know haskell enough to help in small tasks

[taimoorzaeem]

This would be a breaking change for sure and we'll lose feature #3813,

Can this be expressed differently with JSONPath?

Yes, using the function extension like search(). This allows doing a complete regex search so that gives us more control than #3813. It is currently not implemented, but can be added of course.

however this can be easily patched into aeson-jsonpath.

In which way? Can we extend aeson-jsonpath downstream somehow or do you mean to patch it in upstream? The latter doesn't sound compelling, because that would violate RFC 9535 compliance, right?

I meant doing it upstream in the library.

I don't think it violates the compliance because everything that is currently implemented continues to work exactly as it is. To me it's just an extra feature which keeps the existing functionality unaffected. I am not saying we should do this though, to me the search() function is obviously more graceful. WDYT?

[taimoorzaeem]

How happy are you with new contributors? I know haskell enough to help in small tasks

@lsmor We appreciate new contributors very much 💯. I don't think this is a small task but YMMV. I would recommend to please wait sometime so we can take a decision on this first.

[wolfgangwalther]

I don't think it violates the compliance because everything that is currently implemented continues to work exactly as it is. To me it's just an extra feature which keeps the existing functionality unaffected.

For a library that follows a standard, this is always risky: The standard could be extended in the future, and this change might be incompatible with the extra features that were implemented.

I am not saying we should do this though, to me the search() function is obviously more graceful. WDYT?

Yeah, if there's an RFC-way of doing this, we should go that way.

[taimoorzaeem]

I released aeson-jsonpath with search() function now implemented. With this, we have everything we need. How about we do the breaking change now? We'd still need to keep #4603 downstream because it is not supported in the standard. I will a open a PR and we can see how it goes.

@steve-chavez @wolfgangwalther WDYT?

[steve-chavez]

I will a open a PR and we can see how it goes.

I'd say go ahead 👍