change: use RFC 9535 syntax for jwt-role-claim-key config

[taimoorzaeem]

BREAKING CHANGE

Breaks the string comparison operators implemented in #3813. Those can
be replaced with regex searches using JSON Path search() function.

• Implementation
• Tests
• Docs
• Changelog entry

Closes #4914.

[taimoorzaeem]

Now that we are following a standard, I am not sure if we should keep #4603 because it complicates the neat design. Besides, its use case seems very limited to the issue in #4599.

@steve-chavez @wolfgangwalther @merl1n0 WDYT?

[steve-chavez]

@taimoorzaeem One sec, so #4603 was never released so this is not really a breaking change right?

This gives us a chance to just have an amend commit, not change.

On top of it, slice operator can be used by adding the pipe symbol at the end like $.roles[0] | [1:].

Does it cover the same use case as #4599? If so, I agree with removing #4603.

Would like to see some docs to better understand the feature.

[taimoorzaeem]

@taimoorzaeem One sec, so #4603 was never released so this is not really a breaking change right?

Yeah not because of that, it is breaking because we lose #3813, which was released in v13. It is replaced by the standard's $.roles[?@search(<key>, <regex>)].

Does it cover the same use case as #4599? If so, I agree with removing #4603.

It covers it but is not part of the standard and I implemented the slice operator downstream. I was talking about removing the string slice because to me it seems like a bad practice to further slice a selected role value in the JWT claims. Just wanted to know any opinion on it. I will write a detailed comment on this, if that is still not clear.

Would like to see some docs to better understand the feature.

Yeah, will do, that would make it clear.

[taimoorzaeem]

The new syntax for jwt-role-claim-key:

$.postgrest.roles[0] | [1:0]
# ^^^^^^^^^^^^^^^^^^
# The first part here is the standard JSON Path implemented in aeson-jsonpath library

$.postgrest.roles[0] | [1:0]
#                    ^^^^^^^
# The second part is the optional slice part which is not part of the standard. Technically,
# JSON Path is a selector so it can't modify a selected value. We implemented the string slicing
# part in #4603. So to keep it, I have designed it to optionally specify that after
# a pipe '|' symbol in this PR.

Now, I was talking a bit against keeping the slice operator in a previous comment because I think our goal is to follow the standard so we don't frequently change it and break things. That way, it is also easier to document and maintain it.

I am not sure about how useful string slicing feature is. The use case in #4599 seems very niche to me. 😕

[wolfgangwalther]

Now, I was talking a bit against keeping the slice operator in a previous comment because I think our goal is to follow the standard so we don't frequently change it and break things. That way, it is also easier to document and maintain it.

I am not sure about how useful string slicing feature is. The use case in #4599 seems very niche to me. 😕

I agree on removing the slicing again.

[steve-chavez]

$.postgrest.roles[0] | [1:0]

That looks good to me.

Now, I was talking a bit against keeping the slice operator in a previous comment because I think our goal is to follow the standard so we don't frequently change it and break things. That way, it is also easier to document and maintain it.

But the standard doesn't cover the use case in #4599 right? The workaround on #4594 (comment) pollutes the db 😿

I don't think there is a problem if later on we break this config, it's after all server-side so the change is easy and in one place; unlike new functionality at the API level on which a breaking change then requires updating thousands of clients.

I am not sure about how useful string slicing feature is. The use case in #4599 seems very niche to me. 😕

IIRC it was based on another standard (ref) so maybe not that niche. If anything we should make more clear that WLCG is supported in the docs.

[taimoorzaeem]

I don't think there is a problem if later on we break this config, it's after all server-side so the change is easy and in one place; unlike new functionality at the API level on which a breaking change then requires updating thousands of clients.

👍

If anything we should make more clear that WLCG is supported in the docs.

It is supported already if we go by the workaround on #4594 (comment).

The workaround [...] pollutes the db

IMO, the standard (ref) should be responsible for providing clean JWT profiles.

[wolfgangwalther]

Just a thought: Let's not pretend to support that WLCG thing if all we're supporting is extracting that first top-level group. The example in #4594 (comment) has multiple groups, including subgroups - and we're not anywhere close to properly supporting that.

[steve-chavez]

But the standard doesn't cover the use case in #4599 right? The workaround on #4594 (comment) pollutes the db

The above is not true as mentioned on #4594 (reply in thread), that spec defines the name to have a /.

I agree on removing the slicing again.

Agree now, no objection from my side.

[taimoorzaeem]

I have run loadtest with:

$ postgrest-loadtest -k jwt-hs

Array Index

Performance loss is little.

# payload = {
#   ... ,
#   "postgrest" : { "roles" : ["other", "postgrest_test_author"] }
# }

# .postgrest.roles[1] (main)

Requests      [total, rate, throughput]         200001, 6272.43, 6272.40
Duration      [total, attack, wait]             31.886s, 31.886s, 420ns
Latencies     [min, mean, 50, 90, 95, 99, max]  420ns, 158.564µs, 143.674µs, 245.273µs, 281.492µs, 344.152µs, 3.764ms
Bytes In      [total, mean]                     0, 0.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      0:1  200:200000

# $.postgrest.roles[1] (HEAD)

Requests      [total, rate, throughput]         200001, 5864.22, 5864.19
Duration      [total, attack, wait]             34.105s, 34.105s, 551ns
Latencies     [min, mean, 50, 90, 95, 99, max]  551ns, 169.711µs, 157.525µs, 273.823µs, 294.209µs, 360.819µs, 20.214ms
Bytes In      [total, mean]                     0, 0.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      0:1  200:200000

Regex search

Performance loss is significant because regex search is expensive as compared to a prefix check.

# payload = {
#   ... ,
#   "postgrest" : { "roles" : ["other", "postgrest_test_author"] }
# }

# .postgrest.roles[?(@ ^== "postgrest_")] (main)

Requests      [total, rate, throughput]         200001, 6096.12, 6096.09
Duration      [total, attack, wait]             32.808s, 32.808s, 401ns
Latencies     [min, mean, 50, 90, 95, 99, max]  401ns, 163.184µs, 155.913µs, 250.21µs, 285.162µs, 349.578µs, 3.733ms
Bytes In      [total, mean]                     0, 0.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      0:1  200:200000

# $.postgrest.roles[?search(@, '^postgrest_')] (HEAD)

Requests      [total, rate, throughput]         200001, 3356.67, 3356.65
Duration      [total, attack, wait]             59.583s, 59.583s, 912ns
Latencies     [min, mean, 50, 90, 95, 99, max]  912ns, 296.792µs, 273.828µs, 432.092µs, 467.252µs, 556.505µs, 3.84ms
Bytes In      [total, mean]                     0, 0.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      0:1  200:200000

[wolfgangwalther]

Array Index

Performance loss is little.

I think we should investigate this, because it affects everyone.

I didn't look at the code again, but are we making sure that we're only parsing or string config value for the jspath once? Where else could we lose that performance?

Regex search

Performance loss is significant because regex search is expensive as compared to a prefix check.

I wouldn't care about this. We're just comparing apples and oranges here, so not really a meaningful number. Also very much a special case anyway.

[taimoorzaeem]

I didn't look at the code again, but are we making sure that we're only parsing or string config value for the jspath once? Where else could we lose that performance?

Yes, only once. I found the culprit, I was doing an extra case check which was impacting performance.

Updated loadtest result:

Array Index

Slight improvement now:

# payload = {
#   ... ,
#   "postgrest" : { "roles" : ["other", "postgrest_test_author"] }
# }

# .postgrest.roles[1]

Requests      [total, rate, throughput]         200001, 6126.28, 6126.25
Duration      [total, attack, wait]             32.646s, 32.646s, 501ns
Latencies     [min, mean, 50, 90, 95, 99, max]  501ns, 162.392µs, 154.355µs, 268.83µs, 289.308µs, 354.268µs, 18.913ms
Bytes In      [total, mean]                     0, 0.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      0:1  200:200000

# $.postgrest.roles[1]

Requests      [total, rate, throughput]         200001, 6281.94, 6281.91
Duration      [total, attack, wait]             31.837s, 31.837s, 501ns
Latencies     [min, mean, 50, 90, 95, 99, max]  501ns, 158.278µs, 146.285µs, 256.881µs, 288.409µs, 354.087µs, 3.472ms
Bytes In      [total, mean]                     0, 0.00
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           100.00%
Status Codes  [code:count]                      0:1  200:200000

[wolfgangwalther]

This LGTM. There are a few nits that can still be improved, but none of them are blocking for me.