Skip to content

Commit

Permalink
sql: grant to all tables did not skip privileges on sequences
Browse files Browse the repository at this point in the history
Previously, grant privilege to all tables attempted to apply privileges
meant only for tables on to sequences. This could lead to validation
error preventing certain combinations like "GRANT BACKUP ON ALL
TABLES..." from working correctly. To address this, this patch adds
support for correctly propogating the object type and skipping over
unsupported privileges. When an unsupported privilege is encountered on
an object other then a sequence a warning is now logged.

Fixes: #117861

Release note (bug fix): GRANT <...> ON ALL TABLES could fail if
sequences existed and they did not support a privilege (for example
BACKUP).
  • Loading branch information
fqazi committed Mar 19, 2024
1 parent 6454abd commit 431ec65
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 3 deletions.
27 changes: 26 additions & 1 deletion pkg/sql/grant_revoke.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
"github.com/cockroachdb/cockroach/pkg/util/log/logpb"
"github.com/cockroachdb/errors"
Expand Down Expand Up @@ -282,7 +283,31 @@ func (n *changeDescriptorBackedPrivilegesNode) startExec(params runParams) error

privileges := descriptor.GetPrivileges()
for _, grantee := range n.grantees {
changed, err := n.changePrivilege(privileges, n.desiredprivs, grantee)
// Ensure we are only setting privilleges valid for this object type.
// i.e. We only expect this for sequences.
validPrivs, err := privilege.GetValidPrivilegesForObject(objType)
if err != nil {
return err
}
targetPrivs := n.desiredprivs.ToBitField() & validPrivs.ToBitField()
// If all privileges are discarded for a given object, and it's not
// a sequence log a message.
if targetPrivs == 0 {
missingPrivs, err := privilege.ListFromBitField(
n.desiredprivs.ToBitField()&^targetPrivs, privilege.Any)
if err != nil {
return err
}
if objType != privilege.Sequence {
log.Warningf(ctx, "object type %s does not support privilege %v", objType, missingPrivs.SortedDisplayNames())
}
break
}
privsToSet, err := privilege.ListFromBitField(targetPrivs, objType)
if err != nil {
return err
}
changed, err := n.changePrivilege(privileges, privsToSet, grantee)
if err != nil {
return err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,22 @@ SET ROLE root
statement ok
GRANT SELECT ON ALL SEQUENCES IN SCHEMA s TO testuser

query TTTTTB colnames
# This should be a no-op, since backup privellege is not
# supported on sequences.
query T noticetrace
GRANT BACKUP ON ALL TABLES IN SCHEMA S TO testuser
----
NOTICE: some privileges have no effect on sequences: [BACKUP]

statement error pgcode 0LP01 invalid privilege type BACKUP for sequence
GRANT BACKUP ON ALL SEQUENCES IN SCHEMA S TO testuser

query TTTTTB colnames,rowsort
SHOW GRANTS FOR testuser
----
database_name schema_name relation_name grantee privilege_type is_grantable
test s q testuser SELECT false
test s t testuser BACKUP false

statement ok
GRANT USAGE ON ALL SEQUENCES IN SCHEMA s TO testuser
Expand All @@ -47,6 +58,7 @@ SHOW GRANTS FOR testuser
database_name schema_name relation_name grantee privilege_type is_grantable
test s q testuser SELECT false
test s q testuser USAGE false
test s t testuser BACKUP false

statement ok
GRANT SELECT ON ALL SEQUENCES IN SCHEMA s, s2 TO testuser, testuser2
Expand All @@ -58,6 +70,7 @@ database_name schema_name relation_name grantee privilege_type is_grantab
test s q testuser SELECT false
test s q testuser USAGE false
test s q testuser2 SELECT false
test s t testuser BACKUP false
test s2 q testuser SELECT false
test s2 q testuser2 SELECT false

Expand All @@ -70,6 +83,7 @@ SHOW GRANTS FOR testuser, testuser2
database_name schema_name relation_name grantee privilege_type is_grantable
test s q testuser ALL false
test s q testuser2 ALL false
test s t testuser BACKUP false
test s2 q testuser ALL false
test s2 q testuser2 ALL false

Expand Down Expand Up @@ -102,6 +116,7 @@ SHOW GRANTS FOR testuser, testuser2
database_name schema_name relation_name grantee privilege_type is_grantable
test s q testuser ALL false
test s q testuser2 ALL false
test s t testuser BACKUP false
test s2 q testuser ALL false
test s2 q testuser2 ALL false

Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ func (p *planner) getDescriptorsFromTargetListForPrivilegeChange(
descs,
DescriptorWithObjectType{
descriptor: mut,
objectType: privilege.Table,
objectType: mut.GetObjectType(),
})
}
}
Expand Down

0 comments on commit 431ec65

Please sign in to comment.