graphql-engine icon indicating copy to clipboard operation
graphql-engine copied to clipboard

Using X-Hasura-User-Id with a UUID in _contains permissions does not work

Open sean-bennett112 opened this issue 3 years ago • 2 comments

Doing something like the below, where member_ids is a jsonb array on a view:

      filter:
        _or:
          - member_ids:
              _contains: X-Hasura-User-Id
          - assignee_id:
              _eq: X-Hasura-User-Id

does not work. Specifically I'm getting the following error back:

{
  "errors": [
    {
      "extensions": {
        "path": "$",
        "code": "data-exception"
      },
      "message": "invalid input syntax for type json"
    }
  ]
}

Which appears to be returned directly from Postgres. The query Hasura appears to be generating contains the following snippet:

WHERE ((((\"public\".\"project_tasks\".\"member_ids\") @> ((($1->>'x-hasura-user-id'))::jsonb)) OR (((\"public\".\"project_tasks\".\"assignee_id\") = ((($1->>'x-hasura-user-id'))::uuid))

So it is correctly trying to grab the session variable, but then I'm assuming it's failing to correctly wrap the UUID in quotes. If I manually do this with a UUID I already have, where member_ids @> (('6bda2a0f-8ecd-4469-9e04-81c29f9accf3'))::jsonb, then it also fails. Note that if I instead call where member_ids @> ((public.gen_random_uuid()::text))::jsonb, then this seems to work correctly.

It does appear to work just fine if someone is using integer IDs in X-Hasura-User-Id, but I think using UUIDs is fairly common at this point, so probably should be supported.

I think the actual issue is that ::jsonb is being used, rather then to_jsonb, which does a better job converting arbitrary types to jsonb. If I do where member_ids @> ((to_jsonb('6bda2a0f-8ecd-4469-9e04-81c29f9accf3'::text))), then everything works just fine. I think this is a more robust solution in general when working with _contains, so maybe that's the switch that should be made?

This is also related to #4817, but I think is somewhat separate. That issue seems to be about using session variables inside more complex column expressions, where the session variables aren't getting replaced in the larger structure, which isn't quite the same issue as here.

I've confirmed this is occurring in 1.3.0, 1.3.3, and 2.0.0-alpha.8

sean-bennett112 avatar Apr 20 '21 19:04 sean-bennett112

For anyone else who runs across this, there's a somewhat hacky workaround, at least if the underlying relation is a view rather then a table: convert the jsonb entity to a string of some kind containing the relevant X-Hasura-User-Id value using string_agg and then switch _contains to _regex. This does require version 2.0.0, unfortunately.

sean-bennett112 avatar Apr 21 '21 03:04 sean-bennett112

So for one json_b() isn't just a safer way to cast to jsonb, it does different things:

postgres=# select ('hello'::text)::jsonb;
ERROR:  invalid input syntax for type json
DETAIL:  Token "hello" is invalid.
CONTEXT:  JSON data, line 1: hello
postgres=# select to_jsonb('hello'::text);
 to_jsonb 
----------
 "hello"
(1 row)

postgres=# select ('"hello"'::text)::jsonb;
  jsonb  
---------
 "hello"
(1 row)

postgres=# select to_jsonb('"hello"'::text);
  to_jsonb   
-------------
 "\"hello\""
(1 row)

As we can see, where '"hello"'::jsonb used to be the json string hello, the function json_b will instead quote the original string. So it's not that surprising that making this change everywhere breaks things.

We could try to make this change only for session variables, in which case it's less likely to have negative impact, but we need to be careful, since this might still well break existing usecases. E.g., the hasura.user session variable currently is a json object with string values (as far as I can tell), so currently an x-hasura-user-id: 1 would result in "1"::jsonb which gives the jsonb number 1. After the change, this would turn into the jsonb string "1".

I'm going to push that change, and hope that tests fail for this integer user id reason. If not, we'll definitely need to add a test that does check this case, too. (I.e., an integer user id that's checked for jsonb array membership.)

robx avatar Oct 26 '21 12:10 robx