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

sql: deprecate GRANT privilege #74210

Merged
merged 1 commit into from
Dec 23, 2021
Merged

sql: deprecate GRANT privilege #74210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions pkg/ccl/backupccl/testdata/backup-restore/restore-grants
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,17 @@ CREATE TABLE testdb.sc.othertable (a INT);
# Give some grants to user1.
# User1 has access to testdb.sc.othertable.
exec-sql
GRANT ALL ON DATABASE testdb TO user1;
GRANT ALL ON SCHEMA public TO user1;
GRANT ALL ON SCHEMA public TO testuser;
GRANT ALL ON DATABASE testdb TO user1 WITH GRANT OPTION;
GRANT ALL ON SCHEMA public TO user1 WITH GRANT OPTION;
GRANT ALL ON SCHEMA public TO testuser WITH GRANT OPTION;
GRANT USAGE ON SCHEMA sc TO user1;
GRANT SELECT ON testdb.sc.othertable TO user1;
----

# Grant privs to testuser.
# Test user has access to testdb.testtable_greeting_usage and testtable_greeting_owner.
exec-sql
GRANT ALL ON DATABASE testdb TO testuser;
GRANT ALL ON DATABASE testdb TO testuser WITH GRANT OPTION;
GRANT USAGE ON TYPE testdb.greeting_usage TO testuser;
GRANT UPDATE ON testdb.testtable_greeting_usage TO testuser;
----
Expand Down
63 changes: 59 additions & 4 deletions pkg/sql/alter_default_privileges.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
Expand Down Expand Up @@ -197,14 +198,41 @@ func (n *alterDefaultPrivilegesNode) alterDefaultPrivilegesForSchemas(
if err != nil {
return err
}

grantPresent, allPresent := false, false
if params.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.ValidateGrantOption) {
for _, priv := range privileges {
grantPresent = grantPresent || priv == privilege.GRANT
allPresent = allPresent || priv == privilege.ALL
}

noticeMessage := ""
// we only output the message for ALL privilege if it is being granted without the WITH GRANT OPTION flag
// if GRANT privilege is involved, we must always output the message
if allPresent && n.n.IsGrant && !grantOption {
noticeMessage = "grant options were automatically applied but this behavior is deprecated"
} else if grantPresent {
noticeMessage = "the GRANT privilege is deprecated"
}

if len(noticeMessage) > 0 {
params.p.noticeSender.BufferNotice(
errors.WithHint(
pgnotice.Newf("%s", noticeMessage),
"please use WITH GRANT OPTION",
),
)
}
}

for _, role := range roles {
if n.n.IsGrant {
defaultPrivs.GrantDefaultPrivileges(
role, privileges, granteeSQLUsernames, objectType, grantOption,
role, privileges, granteeSQLUsernames, objectType, grantOption, grantPresent || allPresent,
)
} else {
defaultPrivs.RevokeDefaultPrivileges(
role, privileges, granteeSQLUsernames, objectType, grantOption,
role, privileges, granteeSQLUsernames, objectType, grantOption, grantPresent || allPresent,
)
}

Expand Down Expand Up @@ -273,14 +301,41 @@ func (n *alterDefaultPrivilegesNode) alterDefaultPrivilegesForDatabase(
if err != nil {
return err
}

grantPresent, allPresent := false, false
if params.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.ValidateGrantOption) {
for _, priv := range privileges {
grantPresent = grantPresent || priv == privilege.GRANT
allPresent = allPresent || priv == privilege.ALL
}

noticeMessage := ""
// we only output the message for ALL privilege if it is being granted without the WITH GRANT OPTION flag
// if GRANT privilege is involved, we must always output the message
if allPresent && n.n.IsGrant && !grantOption {
noticeMessage = "grant options were automatically applied but this behavior is deprecated"
} else if grantPresent {
noticeMessage = "the GRANT privilege is deprecated"
}

if len(noticeMessage) > 0 {
params.p.noticeSender.BufferNotice(
errors.WithHint(
pgnotice.Newf("%s", noticeMessage),
"please use WITH GRANT OPTION",
),
)
}
}

for _, role := range roles {
if n.n.IsGrant {
defaultPrivs.GrantDefaultPrivileges(
role, privileges, granteeSQLUsernames, objectType, grantOption,
role, privileges, granteeSQLUsernames, objectType, grantOption, grantPresent || allPresent,
)
} else {
defaultPrivs.RevokeDefaultPrivileges(
role, privileges, granteeSQLUsernames, objectType, grantOption,
role, privileges, granteeSQLUsernames, objectType, grantOption, grantPresent || allPresent,
)
}

Expand Down
12 changes: 10 additions & 2 deletions pkg/sql/catalog/catprivilege/default_privilege.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ func (d *immutable) grantOrRevokeDefaultPrivilegesHelper(
privList privilege.List,
withGrantOption bool,
isGrant bool,
deprecateGrant bool,
) {
defaultPrivileges := defaultPrivilegesForRole.DefaultPrivilegesPerObject[targetObject]
// expandPrivileges turns flags on the DefaultPrivilegesForRole representing
Expand All @@ -97,6 +98,11 @@ func (d *immutable) grantOrRevokeDefaultPrivilegesHelper(
} else {
defaultPrivileges.Revoke(grantee, privList, targetObject.ToPrivilegeObjectType(), withGrantOption)
}

if deprecateGrant {
defaultPrivileges.GrantPrivilegeToGrantOptions(grantee, isGrant)
}

if d.IsDatabaseDefaultPrivilege() {
foldPrivileges(defaultPrivilegesForRole, role, &defaultPrivileges, targetObject)
}
Expand All @@ -110,10 +116,11 @@ func (d *Mutable) GrantDefaultPrivileges(
grantees []security.SQLUsername,
targetObject tree.AlterDefaultPrivilegesTargetObject,
withGrantOption bool,
deprecateGrant bool,
) {
defaultPrivilegesForRole := d.defaultPrivilegeDescriptor.FindOrCreateUser(role)
for _, grantee := range grantees {
d.grantOrRevokeDefaultPrivilegesHelper(defaultPrivilegesForRole, role, targetObject, grantee, privileges, withGrantOption, true /* isGrant */)
d.grantOrRevokeDefaultPrivilegesHelper(defaultPrivilegesForRole, role, targetObject, grantee, privileges, withGrantOption, true /* isGrant */, deprecateGrant)
}
}

Expand All @@ -124,10 +131,11 @@ func (d *Mutable) RevokeDefaultPrivileges(
grantees []security.SQLUsername,
targetObject tree.AlterDefaultPrivilegesTargetObject,
grantOptionFor bool,
deprecateGrant bool,
) {
defaultPrivilegesForRole := d.defaultPrivilegeDescriptor.FindOrCreateUser(role)
for _, grantee := range grantees {
d.grantOrRevokeDefaultPrivilegesHelper(defaultPrivilegesForRole, role, targetObject, grantee, privileges, grantOptionFor, false /* isGrant */)
d.grantOrRevokeDefaultPrivilegesHelper(defaultPrivilegesForRole, role, targetObject, grantee, privileges, grantOptionFor, false /* isGrant */, deprecateGrant)
}

defaultPrivilegesPerObject := defaultPrivilegesForRole.DefaultPrivilegesPerObject
Expand Down
17 changes: 13 additions & 4 deletions pkg/sql/catalog/catprivilege/default_privilege_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ func TestGrantDefaultPrivileges(t *testing.T) {
defaultPrivilegeDescriptor := MakeDefaultPrivilegeDescriptor(descpb.DefaultPrivilegeDescriptor_DATABASE)
defaultPrivileges := NewMutableDefaultPrivileges(defaultPrivilegeDescriptor)

defaultPrivileges.GrantDefaultPrivileges(tc.defaultPrivilegesRole, tc.privileges, tc.grantees, tc.targetObject, false /* withGrantOption */)
defaultPrivileges.GrantDefaultPrivileges(tc.defaultPrivilegesRole, tc.privileges, tc.grantees, tc.targetObject, false /* withGrantOption */, false /*deprecateGrant*/)

newPrivileges := CreatePrivilegesFromDefaultPrivileges(
defaultPrivileges, nil, /* schemaDefaultPrivilegeDescriptor */
Expand Down Expand Up @@ -284,8 +284,8 @@ func TestRevokeDefaultPrivileges(t *testing.T) {
defaultPrivilegeDescriptor := MakeDefaultPrivilegeDescriptor(descpb.DefaultPrivilegeDescriptor_DATABASE)
defaultPrivileges := NewMutableDefaultPrivileges(defaultPrivilegeDescriptor)

defaultPrivileges.GrantDefaultPrivileges(tc.defaultPrivilegesRole, tc.grantPrivileges, tc.grantees, tc.targetObject, false /* withGrantOption */)
defaultPrivileges.RevokeDefaultPrivileges(tc.defaultPrivilegesRole, tc.revokePrivileges, tc.grantees, tc.targetObject, false /* grantOptionFor */)
defaultPrivileges.GrantDefaultPrivileges(tc.defaultPrivilegesRole, tc.grantPrivileges, tc.grantees, tc.targetObject, false /* withGrantOption */, false /*deprecateGrant*/)
defaultPrivileges.RevokeDefaultPrivileges(tc.defaultPrivilegesRole, tc.revokePrivileges, tc.grantees, tc.targetObject, false /* grantOptionFor */, false /*deprecateGrant*/)

newPrivileges := CreatePrivilegesFromDefaultPrivileges(
defaultPrivileges, nil, /* schemaDefaultPrivilegeDescriptor */
Expand All @@ -311,7 +311,7 @@ func TestRevokeDefaultPrivilegesFromEmptyList(t *testing.T) {
fooUser := security.MakeSQLUsernameFromPreNormalizedString("foo")
defaultPrivileges.RevokeDefaultPrivileges(descpb.DefaultPrivilegesRole{
Role: creatorUser,
}, privilege.List{privilege.ALL}, []security.SQLUsername{fooUser}, tree.Tables, false /* grantOptionFor */)
}, privilege.List{privilege.ALL}, []security.SQLUsername{fooUser}, tree.Tables, false /* grantOptionFor */, false /*deprecateGrant*/)

newPrivileges := CreatePrivilegesFromDefaultPrivileges(
defaultPrivileges, nil, /* schemaDefaultPrivilegeDescriptor */
Expand Down Expand Up @@ -673,6 +673,7 @@ func TestDefaultPrivileges(t *testing.T) {
userAndGrant.grants,
[]security.SQLUsername{userAndGrant.user},
tc.targetObject, false, /* withGrantOption */
false, /*deprecateGrant*/
)
}

Expand All @@ -683,6 +684,7 @@ func TestDefaultPrivileges(t *testing.T) {
[]security.SQLUsername{userAndGrant.user},
tc.targetObject,
false, /* withGrantOption */
false, /*deprecateGrant*/
)
}

Expand Down Expand Up @@ -745,6 +747,7 @@ func TestModifyDefaultDefaultPrivileges(t *testing.T) {
tc.revokeAndGrantPrivileges,
[]security.SQLUsername{creatorUser},
tc.targetObject, false, /* grantOptionFor */
false, /*deprecateGrant*/
)
if GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForCreator, tc.targetObject) {
t.Errorf("expected role to not have ALL privileges on %s", tc.targetObject)
Expand All @@ -754,6 +757,7 @@ func TestModifyDefaultDefaultPrivileges(t *testing.T) {
tc.revokeAndGrantPrivileges,
[]security.SQLUsername{creatorUser},
tc.targetObject, false, /* withGrantOption */
false, /*deprecateGrant*/
)
if !GetRoleHasAllPrivilegesOnTargetObject(defaultPrivilegesForCreator, tc.targetObject) {
t.Errorf("expected role to have ALL privileges on %s", tc.targetObject)
Expand All @@ -778,6 +782,7 @@ func TestModifyDefaultDefaultPrivilegesForPublic(t *testing.T) {
privilege.List{privilege.USAGE},
[]security.SQLUsername{security.PublicRoleName()},
tree.Types, false, /* grantOptionFor */
false, /*deprecateGrant*/
)
if GetPublicHasUsageOnTypes(defaultPrivilegesForCreator) {
t.Errorf("expected public to not have USAGE privilege on types")
Expand All @@ -787,6 +792,7 @@ func TestModifyDefaultDefaultPrivilegesForPublic(t *testing.T) {
privilege.List{privilege.USAGE},
[]security.SQLUsername{security.PublicRoleName()},
tree.Types, false, /* withGrantOption */
false, /*deprecateGrant*/
)
if !GetPublicHasUsageOnTypes(defaultPrivilegesForCreator) {
t.Errorf("expected public to have USAGE privilege on types")
Expand All @@ -798,6 +804,7 @@ func TestModifyDefaultDefaultPrivilegesForPublic(t *testing.T) {
privilege.List{privilege.USAGE},
[]security.SQLUsername{security.PublicRoleName()},
tree.Types, true, /* withGrantOption */
false, /*deprecateGrant*/
)

privDesc := defaultPrivilegesForCreator.DefaultPrivilegesPerObject[tree.Types]
Expand All @@ -820,6 +827,7 @@ func TestModifyDefaultDefaultPrivilegesForPublic(t *testing.T) {
privilege.List{privilege.USAGE},
[]security.SQLUsername{security.PublicRoleName()},
tree.Types, true, /* grantOptionFor */
false, /*deprecateGrant*/
)

privDesc = defaultPrivilegesForCreator.DefaultPrivilegesPerObject[tree.Types]
Expand All @@ -838,6 +846,7 @@ func TestModifyDefaultDefaultPrivilegesForPublic(t *testing.T) {
privilege.List{privilege.USAGE},
[]security.SQLUsername{security.PublicRoleName()},
tree.Types, false, /* grantOptionFor */
false, /*deprecateGrant*/
)
if GetPublicHasUsageOnTypes(defaultPrivilegesForCreator) {
t.Errorf("expected public to not have USAGE privilege on types")
Expand Down
21 changes: 21 additions & 0 deletions pkg/sql/catalog/descpb/privilege.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,27 @@ func (p *PrivilegeDescriptor) Revoke(

}

// GrantPrivilegeToGrantOptions adjusts a user's grant option bits based on whether the GRANT or ALL
// privilege was just granted or revoked. If GRANT/ALL was just granted, the user should obtain grant
// options for each privilege it currently has. If GRANT/ALL was just revoked, the user should lose
// grant options for each privilege it has
// TODO(jackcwu): delete this function once the GRANT privilege is finally removed
func (p *PrivilegeDescriptor) GrantPrivilegeToGrantOptions(
user security.SQLUsername, isGrant bool,
) {
if isGrant {
userPriv := p.FindOrCreateUser(user)
userPriv.WithGrantOption = userPriv.Privileges
} else {
userPriv, ok := p.FindUser(user)
if !ok || userPriv.Privileges == 0 {
// Removing privileges from a user without privileges is a no-op.
return
}
userPriv.WithGrantOption = 0
}
}

// ValidateSuperuserPrivileges ensures that superusers have exactly the maximum
// allowed privilege set for the object.
// It requires the ID of the descriptor it is applied on to determine whether
Expand Down
32 changes: 32 additions & 0 deletions pkg/sql/grant_revoke.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,14 @@ func (n *changePrivilegesNode) startExec(params runParams) error {
if len(n.desiredprivs) > 0 {
// Only allow granting/revoking privileges that the requesting
// user themselves have on the descriptor.

grantPresent, allPresent := false, false
for _, priv := range n.desiredprivs {
if err := p.CheckPrivilege(ctx, descriptor, priv); err != nil {
return err
}
grantPresent = grantPresent || priv == privilege.GRANT
allPresent = allPresent || priv == privilege.ALL
}
privileges := descriptor.GetPrivileges()

Expand All @@ -204,10 +208,38 @@ func (n *changePrivilegesNode) startExec(params runParams) error {
if err != nil {
return err
}

noticeMessage := ""
// we only output the message for ALL privilege if it is being granted without the WITH GRANT OPTION flag
// if GRANT privilege is involved, we must always output the message
if allPresent && n.isGrant && !n.withGrantOption {
noticeMessage = "grant options were automatically applied but this behavior is deprecated"
} else if grantPresent {
noticeMessage = "the GRANT privilege is deprecated"
}

if len(noticeMessage) > 0 {
params.p.noticeSender.BufferNotice(
errors.WithHint(
pgnotice.Newf("%s", noticeMessage),
"please use WITH GRANT OPTION",
),
)
}
}

for _, grantee := range n.grantees {
n.changePrivilege(privileges, n.desiredprivs, grantee)

if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.ValidateGrantOption) {
if grantPresent || allPresent {
if n.isGrant {
privileges.GrantPrivilegeToGrantOptions(grantee, true /*isGrant*/)
} else if !n.isGrant && !n.withGrantOption {
privileges.GrantPrivilegeToGrantOptions(grantee, false /*isGrant*/)
}
}
}
}

// Ensure superusers have exactly the allowed privilege set.
Expand Down
Loading