Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql: Ensure priv role refs are always valid #18783

Merged
merged 13 commits into from
Apr 17, 2023
Merged

sql: Ensure priv role refs are always valid #18783

merged 13 commits into from
Apr 17, 2023

Conversation

jkosh44
Copy link
Contributor

@jkosh44 jkosh44 commented Apr 16, 2023

This commit prevents dropping a role, if that role is a grantee or
grantor for some object privilege. This helps ensure that the grantee
and grantor references in the catalog always remain valid.

Part of #11579

Motivation

This PR adds a known-desirable feature.

Tips for reviewer

  • This builds off of adapter: Add privileges to objects #18700, only the final commit is unique to this PR. Currently the only valid grantor or grantee is the object owner or the public role. We can't drop the public role and there is already a check preventing us from dropping the role of object owners. Therefore, there's no additional tests that we can write for this feature. However, it's important to get this feature in before we add the ability to add more grantees and grantors.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered.
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • This PR includes the following user-facing behavior changes:

@jkosh44
adapter: Add privileges to objects
877f5a0 
This commit adds privileges to the following objects:

  - tables
  - views
  - materialized views
  - sources
  - types
  - secrets
  - connections
  - secrets
  - clusters
  - databases
  - schemas

This commit updates the on-disk stash representation and the catalog
tables/views that present this information to users.

Currently, privileges cannot be modified, and they are not looked at
when executing statements. These features will be implemented in a
future commit.

Part of MaterializeInc#11579
@jkosh44
Fix expect typos
7f2f7ee
@jkosh44
Remove create privilege from new public schemas
b5b3abb
@jkosh44
Switch from bitor_assign to union
94482d0
@jkosh44
Fix clippy
cc035e9
@jkosh44
Fix test
ff6c922
@jkosh44
constify all_bject_privileges
f3d665f
@jkosh44
Remove public create privileges from default user database, schema, a…
e328e9b 
…nd cluster
@jkosh44
Fix tests
15fdc31
@jkosh44
sql: Ensure priv role refs are always valid
ddf733a 
This commit prevents dropping a role, if that role is a grantee or
grantor for some object privilege. This helps ensure that the grantee
and grantor references in the catalog always remain valid.

Part of MaterializeInc#11579
@jkosh44 jkosh44 marked this pull request as ready for review April 16, 2023 15:40
@jkosh44 jkosh44 requested review from a team as code owners April 16, 2023 15:40
@jkosh44 jkosh44 requested review from def- and removed request for a team April 16, 2023 15:40
maddyblue
maddyblue reviewed Apr 17, 2023
View reviewed changes
Copy link
Contributor

@maddyblue maddyblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for this?

@jkosh44
Copy link
Contributor Author

jkosh44 commented Apr 17, 2023

Test for this?

We already have a bunch of tests that ensure that roles can't be dropped while they are the owner of an object, for example:

materialize/test/sqllogictest/object_ownership.slt

Lines 942 to 943 in d6e4716

statement error role "owner" cannot be dropped because some objects depend on it
DROP ROLE owner
. It's impossible right now, without implementing GRANT, to create a privilege where the grantee or the grantor isn't the owner. So any test we write for this will be identical to the existing tests.

However, I think it's good to merge this before the GRANT and REVOKE features so we don't have a window where people can corrupt their catalogs.

I'll update the existing tests to include the error details. Then when implementing GRANT and REVOKE, I'll add some more tests.

@jkosh44
Merge branch 'main' of github.com:MaterializeInc/materialize into dro…
a4fef07 
…p-role-privilege-check

# Conflicts:
#	src/adapter/src/catalog.rs
#	src/adapter/src/catalog/storage.rs
@jkosh44
Fix merge skew
8708bd1
@jkosh44
Update and fix tests
38a9e5e
@jkosh44
Copy link
Contributor Author

jkosh44 commented Apr 17, 2023

I just updated the tests so that it includes the error messages with the privileges.

def-
def- approved these changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the tests

@jkosh44 jkosh44 merged commit 148297d into MaterializeInc:main Apr 17, 2023
@jkosh44 jkosh44 deleted the drop-role-privilege-check branch April 17, 2023 19:38
@materialize-bot materialize-bot mentioned this pull request Apr 21, 2023
Closed
12 tasks
@jkosh44 jkosh44 mentioned this pull request Apr 24, 2023
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maddyblue maddyblue maddyblue approved these changes

@def- def- def- approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jkosh44 @maddyblue @def-