Fixes granted by cascade/restrict statements for revoke #7517
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7517 +/- ##
=======================================
Coverage 89.59% 89.60%
=======================================
Files 282 282
Lines 60322 60322
Branches 7512 7512
=======================================
+ Hits 54048 54051 +3
Misses 4118 4118
+ Partials 2156 2153 -3 |
| @@ -137,7 +137,7 @@ REVOKE dist_role_3 from dist_role_4 granted by test_admin_role; | |||
| SELECT roleid::regrole::text AS role, member::regrole::text, (grantor::regrole::text IN ('postgres', 'non_dist_role_1', 'dist_role_1','test_admin_role')) AS grantor, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2; | |||
| SELECT objid::regrole FROM pg_catalog.pg_dist_object WHERE classid='pg_authid'::regclass::oid AND objid::regrole::text LIKE '%dist\_%' ORDER BY 1; | |||
|
|
|||
| REVOKE dist_role_3 from non_dist_role_3 granted by test_admin_role; | |||
There was a problem hiding this comment.
Rather than changing an existing test case that already works nicely before the fix, it'd be great to add a test that would fail on main but succeeds on this branch etc.
There was a problem hiding this comment.
This error is tricky actually. Since granted by statement is not being propagated, revoke statement never parsed before; therefore the error never exists. This appeared after I need to use granted by statement in revoke statement with cascade. Since there is no such test in main branch, there has been no errors actually.
* Adds catalog table checks to show propagation
| --------------------------------------------------------------------- | ||
| [{"member":"non_dist_role_3","role":"dist_role_3","grantor":"test_admin_role","admin_option":false}, + | ||
| {"member":"test_admin_role","role":"dist_role_3","grantor":"postgres","admin_option":true}] | ||
| [{"member":"dist_role_4","role":"dist_role_3","grantor":"test_admin_role","admin_option":false}, + |
There was a problem hiding this comment.
The following line disappeared from coordinator (see the first array in result) and but not from workers, do you think what's going wrong?
{"member":"dist_role_4","role":"dist_role_3","grantor":"test_admin_role","admin_option":false}There was a problem hiding this comment.
Also, flaky test detection jobs are failing.
There was a problem hiding this comment.
Yes I noticed right now. It is successful in local. Let me check it
There was a problem hiding this comment.
The following line disappeared from coordinator (see the first array in
result) and but not from workers, do you think what's going wrong?{"member":"dist_role_4","role":"dist_role_3","grantor":"test_admin_role","admin_option":false}
Disappearing from coordinator's output (first array in the result) is the correct behavior but the fact that the same doesn't happen for workers doesn't sound correct.
There was a problem hiding this comment.
Problem was I didn't perform a fresh build before getting test outputs. Now pushed test outputs with fresh builds
gurkanindibay
force-pushed
the
granted-by-restricted
branch
from
cba9d64
to
b5e83e0
Compare
|
@onurctirtir kindly request your re-review |
DESCRIPTION: Fixes incorrect propagating of
GRANTED BYandCASCADE/RESTRICTclauses forREVOKEstatementsThere are two issues fixed in this PR
Since granted by statements does not appear in statements, this bug hasn't been visible until now. However, after activating the granted by statement for revoke, order problem arised and this issue was fixed order problem for cascade/revoke as well
In summary, this PR provides usage of granted by statements properly now with the correct order of statements.
We can verify the both errors, fixed with just single statement
REVOKE dist_role_3 from non_dist_role_3 granted by test_admin_role cascade;