sql: Ensure role member grantor is always valid #18781
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, role membership was modeled after the implementation of role membership in PostgreSQL v15. Specifically the grantor was set as the role ID of the session executing the `GRANT` statement. However, PostgreSQL v16, which has not been released yet as of this commit, updated role membership to be more consistent with the SQL standard and with object privileges. Specifically the grantor could only be set to someone with ADMIN OPTION on the role being granted or the bootstrap superuser. This commit set's the grantor of all role membership to the mz_system role. We haven't implemented ADMIN OPTION yet, and the 'mz_system' role is our version of the bootstrap superuser. The PostgreSQL commit that made this change can be found here: postgres/postgres@ce6b672 Part of MaterializeInc#11579
Previously, role membership was modeled after the implementation of role membership in PostgreSQL v15. Specifically the grantor was set as the role ID of the session executing the `GRANT` statement. However, PostgreSQL v16, which has not been released yet as of this commit, updated role membership to be more consistent with the SQL standard and with object privileges. Specifically the grantor could only be set to someone with ADMIN OPTION on the role being granted or the bootstrap superuser. This commit set's the grantor of all role membership to the mz_system role. We haven't implemented ADMIN OPTION yet, and the 'mz_system' role is our version of the bootstrap superuser. The PostgreSQL commit that made this change can be found here: postgres/postgres@ce6b672 Part of MaterializeInc#11579
This commit prevents dropping a role, if that role is a grantor fo some other role membership. This helps ensure that the grantor references in the catalog always remain valid. As of this commit, the only valid grantor is mz_system which is impossible to delete. So it's impossible to hit this error scenario. However, this is more future-proof for when we implement ADMIN OPTION and allow other grantors. This is based off of the equivalent commit in PostgreSQL here: postgres/postgres@6566133 That commit will be part of PostgreSQL v16 and was not included in v15. Part of MaterializeInc#11579
jkosh44
force-pushed
the
dangling-grantors
branch
from
3b0424f
to
409e0f4
Compare
maddyblue
approved these changes
Apr 17, 2023
def-
requested changes
Apr 17, 2023
| for role in scx.catalog.get_roles() { | ||
| for (member_id, grantor_id) in role.membership() { | ||
| if &id == grantor_id { | ||
| let member_role = scx.catalog.get_role(member_id); |
There was a problem hiding this comment.
Could you add a SLT test for the block starting here? It seems to be uncovered currently.
There was a problem hiding this comment.
It's not possible right now. The only valid role that can be a grantor is mz_system which can't be dropped. This is mostly a sanity check as well as future proofing for when we add ADMIN OPTION, which will allow other grantors that aren't mz_system.
def-
approved these changes
Apr 17, 2023
…gling-grantors # Conflicts: # src/adapter/src/catalog.rs # src/adapter/src/catalog/storage.rs # src/audit-log/src/lib.rs # src/sql/src/plan.rs
jkosh44
added a commit
to jkosh44/materialize
that referenced
this pull request
Apr 18, 2023
This commit prevents dropping a role, if that role is a grantor for some other role membership. This helps ensure that the grantor references in the catalog always remain valid. As of this commit, the only valid grantor is mz_system which is impossible to delete. So it's impossible to hit this error scenario. However, this is more future-proof for when we implement ADMIN OPTION and allow other grantors. This is based off of the equivalent commit in PostgreSQL here: postgres/postgres@6566133 That commit will be part of PostgreSQL v16 and was not included in v15. Part of MaterializeInc#11579
12 tasks
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit prevents dropping a role, if that role is a grantor fo some
other role membership. This helps ensure that the grantor references in
the catalog always remain valid.
As of this commit, the only valid grantor is mz_system which is
impossible to delete. So it's impossible to hit this error scenario.
However, this is more future-proof for when we implement ADMIN OPTION
and allow other grantors.
This is based off of the equivalent commit in PostgreSQL here:
postgres/postgres@6566133
That commit will be part of PostgreSQL v16 and was not included in v15.
Part of #11579
Tips for Reviewer
Motivation
This PR adds a known-desirable feature.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.