Skip to content

Commit

Permalink
ingesting: fixup privileges granted during database restore
Browse files Browse the repository at this point in the history
Previously, all schemas and tables that were ingested as part
of a database restore would "inherit" the privileges of the
database. The database would be granted `CONNECT` for the `public` role and
`ALL` to `admin` and `root`, and so all ingested schemas would have `ALL`
for `admin` and `root`. Since 21.2 we have moved away from
tables/schemas inheriting privileges from the parent database
and so this logic is stale and partly incorrect. It is incorrect
because the restored `public` schema does not have `CREATE` and
`USAGE` granted to the `public` role. These privileges are always
granted to `public` schemas of a database and so there is a discrepancy
in restore's behaviour.

This change simplifies the logic to grant schemas and tables their
default set of privileges if ingested via a database restore. It leaves
the logic for cluster and table restores unchanged.

Release note (bug fix): fixes a bug where a database restore would not
grant `CREATE` and `USAGE` on the public schema to the public role

Fixes: #95456
  • Loading branch information
adityamaru committed Jan 18, 2023
1 parent 51005e4 commit 05cfdac
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 11 deletions.
10 changes: 8 additions & 2 deletions pkg/ccl/backupccl/testdata/backup-restore/restore-grants
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,8 @@ query-sql
SHOW GRANTS ON SCHEMA testdb.public
----
testdb public admin ALL true
testdb public public CREATE false
testdb public public USAGE false
testdb public root ALL true

query-sql
Expand Down Expand Up @@ -554,8 +556,9 @@ query-sql
SHOW GRANTS ON SCHEMA testdb.public
----
testdb public admin ALL true
testdb public public CREATE false
testdb public public USAGE false
testdb public root ALL true
testdb public testuser ALL true

query-sql
SHOW GRANTS ON testdb.sc.othertable
Expand Down Expand Up @@ -603,10 +606,13 @@ SELECT owner FROM [SHOW SCHEMAS] WHERE schema_name = 'sc'
----
testuser

# In postgres, the user "postgres" is the owner of the public schema in a
# newly created db. In CockroachDB, admin is our substitute for the postgres
# user.
query-sql
SELECT owner FROM [SHOW SCHEMAS] WHERE schema_name = 'public'
----
testuser
admin

# Ensure that testuser is the owner of the type.
query-sql
Expand Down
27 changes: 18 additions & 9 deletions pkg/sql/catalog/ingesting/privileges.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,26 @@ func getIngestingPrivilegesForTableOrSchema(
descCoverage tree.DescriptorCoverage,
privilegeType privilege.ObjectType,
) (updatedPrivileges *catpb.PrivilegeDescriptor, err error) {
if wrote, ok := wroteDBs[desc.GetParentID()]; ok {
// If we're creating a new database in this ingestion, the privileges of the
// table and schema should be that of the parent DB.
//
// Leave the privileges of the temp system tables as the default too.
updatedPrivileges = wrote.GetPrivileges()
for i, u := range updatedPrivileges.Users {
updatedPrivileges.Users[i].Privileges =
privilege.ListFromBitField(u.Privileges, privilegeType).ToBitField()
if _, ok := wroteDBs[desc.GetParentID()]; ok {
// If we're creating a new database in this ingestion, the tables and
// schemas in the database should be assigned the default privileges that
// are granted on object creation.
switch privilegeType {
case privilege.Schema:
if desc.GetName() == tree.PublicSchema {
updatedPrivileges = catpb.NewPublicSchemaPrivilegeDescriptor()
} else {
updatedPrivileges = catpb.NewBasePrivilegeDescriptor(user)
}
case privilege.Table:
updatedPrivileges = catpb.NewBasePrivilegeDescriptor(user)
default:
return nil, errors.Newf("unexpected privilege type %T", privilegeType)
}
} else if descCoverage == tree.RequestedDescriptors {
// If we are not creating the database as part of this ingestion, the
// schemas and tables in the database should be given privileges based on
// the parent database's default privileges.
parentDB, err := descsCol.ByID(txn).Get().Database(ctx, desc.GetParentID())
if err != nil {
return nil, errors.Wrapf(err, "failed to lookup parent DB %d", errors.Safe(desc.GetParentID()))
Expand Down

0 comments on commit 05cfdac

Please sign in to comment.