Skip to content

Commit

Permalink
server: fix UserSQLRoles to account for global privileges
Browse files Browse the repository at this point in the history
Release note (bug fix): DB Console features that check for the
VIEWACTIVITYREDACTED privilege now also account for global privileges.
  • Loading branch information
rafiss committed Jan 18, 2023
1 parent 582d845 commit 2a33966
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 14 deletions.
17 changes: 14 additions & 3 deletions pkg/server/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"context"

"github.com/cockroachdb/cockroach/pkg/server/serverpb"
"github.com/cockroachdb/cockroach/pkg/sql/privilege"
"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
)

Expand All @@ -31,13 +32,23 @@ func (s *baseStatusServer) UserSQLRoles(

var resp serverpb.UserSQLRolesResponse
if !isAdmin {
for name := range roleoption.ByName {
hasRole, err := s.privilegeChecker.hasRoleOption(ctx, username, roleoption.ByName[name])
for _, privKind := range privilege.GlobalPrivileges {
privName := privKind.String()
hasPriv := s.privilegeChecker.checkHasGlobalPrivilege(ctx, username, privKind)
if hasPriv {
resp.Roles = append(resp.Roles, privName)
continue
}
roleOpt, ok := roleoption.ByName[privName]
if !ok {
continue
}
hasRole, err := s.privilegeChecker.hasRoleOption(ctx, username, roleOpt)
if err != nil {
return nil, err
}
if hasRole {
resp.Roles = append(resp.Roles, name)
resp.Roles = append(resp.Roles, privName)
}
}
} else {
Expand Down
25 changes: 14 additions & 11 deletions pkg/server/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,33 +87,36 @@ func TestSQLRolesAPI(t *testing.T) {
expRoles := []string{"ADMIN"}
err := getStatusJSONProtoWithAdminOption(s, "sqlroles", &res, true)
require.NoError(t, err)
require.Equal(t, expRoles, res.Roles)
require.ElementsMatch(t, expRoles, res.Roles)

// No roles added to a non-admin user.
expRoles = []string{}
err = getStatusJSONProtoWithAdminOption(s, "sqlroles", &res, false)
require.NoError(t, err)
require.Equal(t, expRoles, res.Roles)
require.ElementsMatch(t, expRoles, res.Roles)

// One role added to the non-admin user.
// Role option and global privilege added to the non-admin user.
db.Exec(t, fmt.Sprintf("ALTER USER %s VIEWACTIVITY", authenticatedUserNameNoAdmin().Normalized()))
expRoles = []string{"VIEWACTIVITY"}
db.Exec(t, fmt.Sprintf("GRANT SYSTEM MODIFYCLUSTERSETTING TO %s", authenticatedUserNameNoAdmin().Normalized()))
expRoles = []string{"MODIFYCLUSTERSETTING", "VIEWACTIVITY"}
err = getStatusJSONProtoWithAdminOption(s, "sqlroles", &res, false)
require.NoError(t, err)
require.Equal(t, expRoles, res.Roles)
require.ElementsMatch(t, expRoles, res.Roles)

// Two roles added to the non-admin user.
// Two role options and two global privileges added to the non-admin user.
db.Exec(t, fmt.Sprintf("ALTER USER %s VIEWACTIVITYREDACTED", authenticatedUserNameNoAdmin().Normalized()))
expRoles = []string{"VIEWACTIVITY", "VIEWACTIVITYREDACTED"}
db.Exec(t, fmt.Sprintf("GRANT SYSTEM CANCELQUERY TO %s", authenticatedUserNameNoAdmin().Normalized()))
expRoles = []string{"CANCELQUERY", "MODIFYCLUSTERSETTING", "VIEWACTIVITY", "VIEWACTIVITYREDACTED"}
err = getStatusJSONProtoWithAdminOption(s, "sqlroles", &res, false)
sort.Strings(res.Roles)
require.NoError(t, err)
require.Equal(t, expRoles, res.Roles)
require.ElementsMatch(t, expRoles, res.Roles)

// Remove one role from non-admin user.
// Remove one role option and one global privilege from non-admin user.
db.Exec(t, fmt.Sprintf("ALTER USER %s NOVIEWACTIVITY", authenticatedUserNameNoAdmin().Normalized()))
expRoles = []string{"VIEWACTIVITYREDACTED"}
db.Exec(t, fmt.Sprintf("REVOKE SYSTEM MODIFYCLUSTERSETTING FROM %s", authenticatedUserNameNoAdmin().Normalized()))
expRoles = []string{"CANCELQUERY", "VIEWACTIVITYREDACTED"}
err = getStatusJSONProtoWithAdminOption(s, "sqlroles", &res, false)
require.NoError(t, err)
require.Equal(t, expRoles, res.Roles)
require.ElementsMatch(t, expRoles, res.Roles)
}

0 comments on commit 2a33966

Please sign in to comment.