Skip to content

Commit

Permalink
sql: modify HasViewActivityOrViewActivityRedactedRole signature
Browse files Browse the repository at this point in the history 
This commit adds an additional return value to the function
`HasViewActivityOrViewActivityRedactedRole` which returns true if
the user has either `VIEWACTIVITY` or `VIEWACTIVITYREDACTED` roles.

In addition to that, the function will now return a boolean indicating
whether the user is not admin and has the `VIEWACTIVITYREDACTED` role.
This simplifies  areas where we need to redact values based on the
presence of this role, allowing callers to skip an additional role check
for admin and VIEWACTIVITYREDACTED.

Epic: none

Release note: None
  • Loading branch information
@xinhaoz
xinhaoz committed Sep 13, 2023
1 parent 2e84920 commit 0086d68
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 67 deletions.
34 changes: 22 additions & 12 deletions pkg/sql/authorization.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1022,25 +1022,35 @@ func (p *planner) HasOwnershipOnSchema(
return hasOwnership, nil
}

// HasViewActivityOrViewActivityRedactedRole implements the AuthorizationAccessor interface.
// HasViewActivityOrViewActivityRedactedRole is part of the eval.SessionAccessor interface.
// It returns 2 boolean values - the first indicating if we have either privilege requested,
// and the second indicating whether or not it was VIEWACTIVITYREDACTED.
// Requires a valid transaction to be open.
func (p *planner) HasViewActivityOrViewActivityRedactedRole(ctx context.Context) (bool, error) {
func (p *planner) HasViewActivityOrViewActivityRedactedRole(
ctx context.Context,
) (hasPrivs bool, shouldRedact bool, err error) {
if hasAdmin, err := p.HasAdminRole(ctx); err != nil {
return hasAdmin, err
return hasAdmin, false, err
} else if hasAdmin {
return true, nil
}
if hasView, err := p.HasViewActivity(ctx); err != nil {
return false, err
} else if hasView {
return true, nil
return true, false, nil
}

// We check for VIEWACTIVITYREDACTED first as users can have both
// VIEWACTIVITY and VIEWACTIVITYREDACTED, where VIEWACTIVITYREDACTED
// takes precedence (i.e. we must redact senstitive values).
if hasViewRedacted, err := p.HasViewActivityRedacted(ctx); err != nil {
return false, err
return false, false, err
} else if hasViewRedacted {
return true, nil
return true, true, nil
}
return false, nil

if hasView, err := p.HasViewActivity(ctx); err != nil {
return false, false, err
} else if hasView {
return true, false, nil
}

return false, false, nil
}

func (p *planner) HasViewActivityRedacted(ctx context.Context) (bool, error) {
Expand Down
71 changes: 20 additions & 51 deletions pkg/sql/crdb_internal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2016,7 +2016,7 @@ CREATE TABLE crdb_internal.node_transaction_statistics (
)
`,
populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
hasViewActivityOrhasViewActivityRedacted, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasViewActivityOrhasViewActivityRedacted, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -2443,7 +2443,7 @@ var crdbInternalLocalTxnsTable = virtualSchemaTable{
comment: "running user transactions visible by the current user (RAM; local node only)",
schema: fmt.Sprintf(txnsSchemaPattern, "node_transactions"),
populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
hasViewActivityOrhasViewActivityRedacted, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasViewActivityOrhasViewActivityRedacted, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
Expand All @@ -2466,7 +2466,7 @@ var crdbInternalClusterTxnsTable = virtualSchemaTable{
comment: "running user transactions visible by the current user (cluster RPC; expensive!)",
schema: fmt.Sprintf(txnsSchemaPattern, "cluster_transactions"),
populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
hasViewActivityOrhasViewActivityRedacted, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasViewActivityOrhasViewActivityRedacted, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -2574,7 +2574,7 @@ func (p *planner) makeSessionsRequest(
if hasAdmin {
req.Username = ""
} else {
hasViewActivityOrhasViewActivityRedacted, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasViewActivityOrhasViewActivityRedacted, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return serverpb.ListSessionsRequest{}, err
}
Expand Down Expand Up @@ -3122,24 +3122,13 @@ func populateContentionEventsTable(
response *serverpb.ListContentionEventsResponse,
) error {
// Validate users have correct permission/role.
hasPermission, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasPermission, shouldRedactKey, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
if !hasPermission {
return noViewActivityOrViewActivityRedactedRoleError(p.User())
}
isAdmin, err := p.HasAdminRole(ctx)
if err != nil {
return err
}
var shouldRedactKey bool
if !isAdmin {
shouldRedactKey, err = p.HasViewActivityRedacted(ctx)
if err != nil {
return err
}
}
key := tree.NewDBytes("")
for _, ice := range response.Events.IndexContentionEvents {
for _, skc := range ice.Events {
Expand Down Expand Up @@ -4449,7 +4438,7 @@ CREATE TABLE crdb_internal.ranges_no_leases (
if hasAdmin {
return true, nil
}
viewActOrViewActRedact, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
viewActOrViewActRedact, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
// Return if we have permission or encountered an error.
if viewActOrViewActRedact || err != nil {
return viewActOrViewActRedact, err
Expand Down Expand Up @@ -7467,27 +7456,14 @@ CREATE TABLE crdb_internal.transaction_contention_events (
);`,
generator: func(ctx context.Context, p *planner, db catalog.DatabaseDescriptor, stopper *stop.Stopper) (virtualTableGenerator, cleanupFunc, error) {
// Check permission first before making RPC fanout.
hasPermission, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return nil, nil, err
}
if !hasPermission {
return nil, nil, noViewActivityOrViewActivityRedactedRoleError(p.User())
}

// If a user has VIEWACTIVITYREDACTED role option but the user does not
// have the ADMIN role option, then the contending key should be redacted.
isAdmin, err := p.HasAdminRole(ctx)
hasPermission, shouldRedactContendingKey, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return nil, nil, err
}

shouldRedactContendingKey := false
if !isAdmin {
shouldRedactContendingKey, err = p.HasViewActivityRedacted(ctx)
if err != nil {
return nil, nil, err
}
if !hasPermission {
return nil, nil, noViewActivityOrViewActivityRedactedRoleError(p.User())
}

// Account for memory used by the RPC fanout.
Expand Down Expand Up @@ -7756,20 +7732,13 @@ func genClusterLocksGenerator(
if err != nil {
return nil, nil, err
}
hasViewActivityOrViewActivityRedacted, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasViewActivityOrViewActivityRedacted, shouldRedactKeys, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return nil, nil, err
}
if !hasViewActivityOrViewActivityRedacted {
return nil, nil, noViewActivityOrViewActivityRedactedRoleError(p.User())
}
shouldRedactKeys := false
if !hasAdmin {
shouldRedactKeys, err = p.HasViewActivityRedacted(ctx)
if err != nil {
return nil, nil, err
}
}

all, err := p.Descriptors().GetAllDescriptors(ctx, p.txn)
if err != nil {
Expand Down Expand Up @@ -8074,7 +8043,7 @@ func populateTxnExecutionInsights(
addRow func(...tree.Datum) error,
request *serverpb.ListExecutionInsightsRequest,
) (err error) {
hasRoleOption, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasRoleOption, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -8257,11 +8226,11 @@ func populateStmtInsights(
addRow func(...tree.Datum) error,
request *serverpb.ListExecutionInsightsRequest,
) (err error) {
hasRoleOption, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
// Check if the user has sufficient privileges.
hasPrivs, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
if !hasRoleOption {
} else if !hasPrivs {
return noViewActivityOrViewActivityRedactedRoleError(p.User())
}

Expand Down Expand Up @@ -8424,7 +8393,7 @@ CREATE TABLE crdb_internal.node_memory_monitors (
// The memory monitors' names can expose some information about the
// activity on the node, so we require VIEWACTIVITY or
// VIEWACTIVITYREDACTED permissions.
hasRoleOption, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasRoleOption, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -8602,7 +8571,7 @@ CREATE TABLE crdb_internal.kv_flow_controller (
available_elastic_tokens INT NOT NULL
);`,
populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
hasRoleOption, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasRoleOption, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -8642,7 +8611,7 @@ CREATE TABLE crdb_internal.kv_flow_control_handles (
indexes: []virtualIndex{
{
populate: func(ctx context.Context, constraint tree.Datum, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) (matched bool, err error) {
hasRoleOption, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasRoleOption, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return false, err
}
Expand All @@ -8663,7 +8632,7 @@ CREATE TABLE crdb_internal.kv_flow_control_handles (
},
},
populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
hasRoleOption, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasRoleOption, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -8718,7 +8687,7 @@ CREATE TABLE crdb_internal.kv_flow_token_deductions (
indexes: []virtualIndex{
{
populate: func(ctx context.Context, constraint tree.Datum, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) (matched bool, err error) {
hasRoleOption, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasRoleOption, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return false, err
}
Expand All @@ -8739,7 +8708,7 @@ CREATE TABLE crdb_internal.kv_flow_token_deductions (
},
},
populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
hasRoleOption, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
hasRoleOption, _, err := p.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/faketreeeval/evalctx.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -584,8 +584,8 @@ func (ep *DummySessionAccessor) HasRoleOption(
// HasViewActivityOrViewActivityRedactedRole is part of the eval.SessionAccessor interface.
func (ep *DummySessionAccessor) HasViewActivityOrViewActivityRedactedRole(
context.Context,
) (bool, error) {
return false, errors.WithStack(errEvalSessionVar)
) (bool, bool, error) {
return false, false, errors.WithStack(errEvalSessionVar)
}

// DummyClientNoticeSender implements the eval.ClientNoticeSender interface.
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/sem/builtins/generator_builtins.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3408,7 +3408,7 @@ func makeTableSpanStatsGenerator(
ctx context.Context, evalCtx *eval.Context, args tree.Datums,
) (eval.ValueGenerator, error) {
// The user must have ADMIN role or VIEWACTIVITY/VIEWACTIVITYREDACTED permission to use this builtin.
hasViewActivity, err := evalCtx.SessionAccessor.HasViewActivityOrViewActivityRedactedRole(ctx)
hasViewActivity, _, err := evalCtx.SessionAccessor.HasViewActivityOrViewActivityRedactedRole(ctx)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/sem/eval/deps.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ type SessionAccessor interface {

// HasViewActivityOrViewActivityRedactedRole returns true iff the current session user has the
// VIEWACTIVITY or VIEWACTIVITYREDACTED permission.
HasViewActivityOrViewActivityRedactedRole(ctx context.Context) (bool, error)
HasViewActivityOrViewActivityRedactedRole(ctx context.Context) (bool, bool, error)
}

// PreparedStatementState is a limited interface that exposes metadata about
Expand Down

0 comments on commit 0086d68

Please sign in to comment.