edgedb / edgedb

A graph-relational database with declarative schema, built-in migration system, and a next-generation query language
https://edgedb.com
Apache License 2.0
12.93k stars 396 forks source link

ISE using auth::ClientTokenIdentity within access policies #6535

Closed aljazerzen closed 8 months ago

aljazerzen commented 9 months ago

Schema:

using extension auth;

module default {
  type User {
    required identity: ext::auth::Identity;
  };

  type Todo {
    required owner: User;

    access policy ownerHasAccess
      allow all
      using ((select User filter .identity = global ext::auth::ClientTokenIdentity limit 1) ?= .owner);
  };
}

Query:

_localdev:edgedb> insert Todo { owner := (insert User { identity := global ext::auth::ClientTokenIdentity }) };
edgedb error: InternalServerError: function edgedbstd.47e3264c-8091-5a3c-b2c5-b55df212c73c(jsonb, jsonb[], text) does not exist

Problem:

We call edgedbstd."std::json_get" with (jsonb, jsonb[], text) instead of (jsonb, jsonb, text).

Call site:

FROM LATERAL (
    SELECT/*<pg.SelectStmt at 0x7f7aa5172990>*/
        edgedbstd."std::json_get"("v~4"."v~5", "json_claims~4"."claims_value~3", ('sub')::text) AS "expr~32_value~1"
) AS "q~13"

The problematic "json_claims~4"."claims_value~3" comes from here:

FROM LATERAL (
  FROM ROWS FROM (unnest(ARRAY["set_as_subquery~7"."FreeObject_serialized~2"]) AS (_t0 uuid, _t1 jsonb[])) AS "unpack~3"
  SELECT/*<pg.SelectStmt at 0x7f7aa5172110>*/
    _t1 AS "claims_value~1"
) AS "q~11"
scotttrinh commented 9 months ago

This could be related to the cardinality bug for ClientTokenIdentity that we hardcoded a fix for in the compiler. Maybe it gets missed in this specific use case? Fixed in https://github.com/edgedb/edgedb/pull/6393 but 4.x still has the old hardcoded "fix" introduced in https://github.com/edgedb/edgedb/pull/6369

msullivan commented 8 months ago

I don't get this failure on master or 4.2? (Though the access policy does fail, since select User won't have the newly created User). Or even in 4.0?

aljazerzen commented 8 months ago

Hmmm, I'm able to reproduce this on c78a7d638, but not on 9c49debb2 or current master (b078d5de2).

I don't want to bisect this, but since it's fixed now, I assume that it was fixed intentionally and already has a test, so we can close.