Skip to content

Commit

Permalink
sql: refactor pg_builtin to use actual grant options
Browse files Browse the repository at this point in the history
Release note (sql change): The builtins has_database_privilege, has_table_privilege, pg_has_role now use privileges.Priv.GrantOption instead of privileges.Kind.GRANT
  • Loading branch information
ecwall committed Jan 4, 2022
1 parent 5bbffac commit dd8a8d8
Show file tree
Hide file tree
Showing 6 changed files with 154 additions and 146 deletions.
2 changes: 1 addition & 1 deletion pkg/ccl/importccl/import_table_creation.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ func (so *importSequenceOperators) HasPrivilege(
ctx context.Context,
specifier tree.HasPrivilegeSpecifier,
user security.SQLUsername,
kind privilege.Kind,
priv privilege.Priv,
) (bool, error) {
return false, errors.WithStack(errSequenceOperators)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/faketreeeval/evalctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (so *DummySequenceOperators) HasPrivilege(
ctx context.Context,
specifier tree.HasPrivilegeSpecifier,
user security.SQLUsername,
kind privilege.Kind,
priv privilege.Priv,
) (bool, error) {
return false, errors.WithStack(errEvalPlanner)
}
Expand Down Expand Up @@ -305,7 +305,7 @@ func (ep *DummyEvalPlanner) HasPrivilege(
ctx context.Context,
specifier tree.HasPrivilegeSpecifier,
user security.SQLUsername,
kind privilege.Kind,
priv privilege.Priv,
) (bool, error) {
return false, errors.WithStack(errEvalPlanner)
}
Expand Down
42 changes: 42 additions & 0 deletions pkg/sql/privilege/privilege.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,48 @@ const (
RULE Kind = 12
)

// Priv represents a privilege parsed from an Access Privilege Inquiry
// Function's privilege string argument. The structure is distinct from
// privilege.Kind due to differences in how PostgreSQL and CockroachDB
// handle the GRANT privilege.
//
// In PostgreSQL, each privilege (SELECT, INSERT, etc.) has an optional
// "grant option" bit associated with it. A role can only grant a privilege
// on an object to others if it is the owner of the object or if it itself
// holds that privilege **with grant option** on the object. With this
// construction, there is no need for a separate GRANT privilege.
//
// In CockroachDB, there exists a distinct GRANT privilege and no concept of
// a "grant option" on other privileges. A role can only grant a privilege
// on an object to others if it is the owner of the object or if it itself
// holds both (1) that privilege on the object and (2) the GRANT privilege
// on the object. However, this behavior may change in the future, see
// https://github.com/cockroachdb/cockroach/issues/67410.
//
// For the sake of parsing the privilege argument of these builtins, it is
// helpful to represent privileges more closely to how they are represented
// in PostgreSQL. This allows us to represent a single Priv with a fake
// "grant option", which is later computed as a conjunction between that
// Priv's Kind and the GRANT privilege, while also computing a disjunction
// across all comma-separated privilege strings.
//
// For instance, consider the following argument string:
//
// arg = "SELECT, UPDATE WITH GRANT OPTION, DELETE"
//
// This would be represented as the following list of Priv structs:
//
// privs = []Priv{{SELECT, false}, {UPDATE, true}, {DELETE, false}}
//
// Which would be evaluated as:
//
// res = check(SELECT) || (check(UPDATE) && check(GRANT)) || check(DELETE)
//
type Priv struct {
Kind Kind
GrantOption bool
}

// ObjectType represents objects that can have privileges.
type ObjectType string

Expand Down
22 changes: 15 additions & 7 deletions pkg/sql/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ package sql
import (
"context"
"fmt"

"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/kv"
Expand Down Expand Up @@ -304,7 +303,7 @@ func (p *planner) HasPrivilege(
ctx context.Context,
specifier tree.HasPrivilegeSpecifier,
user security.SQLUsername,
kind privilege.Kind,
priv privilege.Priv,
) (bool, error) {
desc, err := p.ResolveDescriptorForPrivilegeSpecifier(
ctx,
Expand All @@ -315,32 +314,41 @@ func (p *planner) HasPrivilege(
}

// hasPrivilegeFunc checks whether any role has the given privilege.
hasPrivilegeFunc := func(priv privilege.Kind) (bool, error) {
err := p.CheckPrivilegeForUser(ctx, desc, priv, user)
hasPrivilegeFunc := func(priv privilege.Priv) (bool, error) {
err := p.CheckPrivilegeForUser(ctx, desc, priv.Kind, user)
if err != nil {
if pgerror.GetPGCode(err) == pgcode.InsufficientPrivilege {
return false, nil
}
return false, err
}
if priv.GrantOption {
err = p.CheckGrantOptionsForUser(ctx, desc, []privilege.Kind{priv.Kind}, true)
if err != nil {
if pgerror.GetPGCode(err) == pgcode.InsufficientPrivilege {
return false, nil
}
return false, err
}
}
return true, nil
}

if kind == privilege.RULE {
if priv.Kind == privilege.RULE {
// RULE was only added for compatibility with Postgres, and Postgres
// never allows RULE to be granted, even if the user has ALL privileges.
// See https://www.postgresql.org/docs/8.1/sql-grant.html
// and https://www.postgresql.org/docs/release/8.2.0/.
return false, nil
}
hasPrivilege, err := hasPrivilegeFunc(privilege.ALL)
hasPrivilege, err := hasPrivilegeFunc(privilege.Priv{Kind: privilege.ALL})
if err != nil {
return false, err
}
if hasPrivilege {
return true, nil
}
return hasPrivilegeFunc(kind)
return hasPrivilegeFunc(priv)
}

// ResolveDescriptorForPrivilegeSpecifier resolves a tree.HasPrivilegeSpecifier
Expand Down
Loading

0 comments on commit dd8a8d8

Please sign in to comment.