Skip to content

Commit

Permalink
Merge pull request cockroachdb#84272 from rytaft/backport21.2-83959
Browse files Browse the repository at this point in the history 
release-21.2: sql: ensure revalidate_unique_constraint* builtins respect privileges
  • Loading branch information
@rytaft
rytaft authored Jul 19, 2022
2 parents ea44105 + a0d20fe commit 97a382e
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 11 deletions.
6 changes: 3 additions & 3 deletions pkg/ccl/importccl/import_stmt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2145,7 +2145,7 @@ func (r *importResumer) Resume(ctx context.Context, execCtx interface{}) error {
}
}

if err := r.checkVirtualConstraints(ctx, p.ExecCfg(), r.job); err != nil {
if err := r.checkVirtualConstraints(ctx, p.ExecCfg(), r.job, p.User()); err != nil {
return err
}

Expand Down Expand Up @@ -2311,7 +2311,7 @@ func (r *importResumer) publishSchemas(ctx context.Context, execCfg *sql.Executo
// checkVirtualConstraints checks constraints that are enforced via runtime
// checks, such as uniqueness checks that are not directly backed by an index.
func (*importResumer) checkVirtualConstraints(
ctx context.Context, execCfg *sql.ExecutorConfig, job *jobs.Job,
ctx context.Context, execCfg *sql.ExecutorConfig, job *jobs.Job, user security.SQLUsername,
) error {
for _, tbl := range job.Details().(jobspb.ImportDetails).Tables {
desc := tabledesc.NewBuilder(tbl.Desc).BuildExistingMutableTable()
Expand All @@ -2329,7 +2329,7 @@ func (*importResumer) checkVirtualConstraints(
ie := job.MakeSessionBoundInternalExecutor(ctx, sql.NewFakeSessionData(execCfg.SV()))
defer ie.Close(ctx)
return ie.WithSyntheticDescriptors([]catalog.Descriptor{desc}, func() error {
return sql.RevalidateUniqueConstraintsInTable(ctx, txn, ie, desc)
return sql.RevalidateUniqueConstraintsInTable(ctx, txn, user, ie, desc)
})
}); err != nil {
return err
Expand Down
47 changes: 47 additions & 0 deletions pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,3 +223,50 @@ user testuser
statement ok
ALTER DATABASE repartition_privs ADD REGION "ap-southeast-2";
ALTER DATABASE repartition_privs DROP REGION "us-east-1"

subtest revalidate_privs

user root

statement ok
CREATE DATABASE revalidate_privs PRIMARY REGION "ca-central-1" REGIONS "us-east-1"

statement ok
CREATE TABLE revalidate_privs.rbr () LOCALITY REGIONAL BY ROW

user testuser

statement ok
USE revalidate_privs

# Check that revalidate_unique_constraint* builtins respect privileges.
query error user testuser does not have SELECT privilege on relation rbr
SELECT crdb_internal.revalidate_unique_constraints_in_all_tables()

query error user testuser does not have SELECT privilege on relation rbr
SELECT crdb_internal.revalidate_unique_constraints_in_table('revalidate_privs.rbr')

query error user testuser does not have SELECT privilege on relation rbr
SELECT crdb_internal.revalidate_unique_constraint('revalidate_privs.rbr', 'primary')

user root

statement ok
GRANT SELECT ON revalidate_privs.rbr TO testuser

user testuser

query B
SELECT crdb_internal.revalidate_unique_constraints_in_all_tables()
----
true

query B
SELECT crdb_internal.revalidate_unique_constraints_in_table('repartition_privs.rbr')
----
true

query B
SELECT crdb_internal.revalidate_unique_constraint('repartition_privs.rbr', 'primary')
----
true
2 changes: 1 addition & 1 deletion pkg/sql/alter_table.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ func (n *alterTableNode) startExec(params runParams) error {
"constraint %q in the middle of being added, try again later", t.Constraint)
}
if err := validateUniqueWithoutIndexConstraintInTxn(
params.ctx, params.EvalContext(), n.tableDesc, params.EvalContext().Txn, name,
params.ctx, params.EvalContext(), n.tableDesc, params.EvalContext().Txn, params.p.User(), name,
); err != nil {
return err
}
Expand Down
19 changes: 16 additions & 3 deletions pkg/sql/backfill.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/settings"
"github.com/cockroachdb/cockroach/pkg/sql/backfill"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
Expand Down Expand Up @@ -742,7 +743,9 @@ func (sc *SchemaChanger) validateConstraints(
return err
}
} else if c.IsUniqueWithoutIndex() {
if err := validateUniqueWithoutIndexConstraintInTxn(ctx, &evalCtx.EvalContext, desc, txn, c.GetName()); err != nil {
if err := validateUniqueWithoutIndexConstraintInTxn(
ctx, &evalCtx.EvalContext, desc, txn, evalCtx.SessionData().User(), c.GetName(),
); err != nil {
return err
}
} else if c.IsNotNull() {
Expand Down Expand Up @@ -1838,6 +1841,7 @@ func ValidateForwardIndexes(
idx.GetPredicate(),
ie,
txn,
security.NodeUserName(),
false, /* preExisting */
); err != nil {
return err
Expand Down Expand Up @@ -2268,7 +2272,7 @@ func runSchemaChangesInTxn(
uwi := &c.ConstraintToUpdateDesc().UniqueWithoutIndexConstraint
if uwi.Validity == descpb.ConstraintValidity_Validating {
if err := validateUniqueWithoutIndexConstraintInTxn(
ctx, planner.EvalContext(), tableDesc, planner.txn, c.GetName(),
ctx, planner.EvalContext(), tableDesc, planner.txn, planner.User(), c.GetName(),
); err != nil {
return err
}
Expand Down Expand Up @@ -2416,6 +2420,7 @@ func validateUniqueWithoutIndexConstraintInTxn(
evalCtx *tree.EvalContext,
tableDesc *tabledesc.Mutable,
txn *kv.Txn,
user security.SQLUsername,
constraintName string,
) error {
ie := evalCtx.InternalExecutor.(*InternalExecutor)
Expand All @@ -2438,7 +2443,15 @@ func validateUniqueWithoutIndexConstraintInTxn(

return ie.WithSyntheticDescriptors(syntheticDescs, func() error {
return validateUniqueConstraint(
ctx, tableDesc, uc.Name, uc.ColumnIDs, uc.Predicate, ie, txn, false, /* preExisting */
ctx,
tableDesc,
uc.Name,
uc.ColumnIDs,
uc.Predicate,
ie,
txn,
user,
false, /* preExisting */
)
})
}
Expand Down
24 changes: 20 additions & 4 deletions pkg/sql/check.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (

"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
Expand Down Expand Up @@ -382,7 +383,9 @@ func (p *planner) RevalidateUniqueConstraintsInCurrentDB(ctx context.Context) er
}

for _, tableDesc := range tableDescs {
if err = RevalidateUniqueConstraintsInTable(ctx, p.Txn(), p.ExecCfg().InternalExecutor, tableDesc); err != nil {
if err = RevalidateUniqueConstraintsInTable(
ctx, p.Txn(), p.User(), p.ExecCfg().InternalExecutor, tableDesc,
); err != nil {
return err
}
}
Expand All @@ -403,7 +406,9 @@ func (p *planner) RevalidateUniqueConstraintsInTable(ctx context.Context, tableI
if err != nil {
return err
}
return RevalidateUniqueConstraintsInTable(ctx, p.Txn(), p.ExecCfg().InternalExecutor, tableDesc)
return RevalidateUniqueConstraintsInTable(
ctx, p.Txn(), p.User(), p.ExecCfg().InternalExecutor, tableDesc,
)
}

// RevalidateUniqueConstraint verifies that the given unique constraint on the
Expand Down Expand Up @@ -439,6 +444,7 @@ func (p *planner) RevalidateUniqueConstraint(
index.GetPredicate(),
p.ExecCfg().InternalExecutor,
p.Txn(),
p.User(),
true, /* preExisting */
)
}
Expand All @@ -458,6 +464,7 @@ func (p *planner) RevalidateUniqueConstraint(
uc.Predicate,
p.ExecCfg().InternalExecutor,
p.Txn(),
p.User(),
true, /* preExisting */
)
}
Expand Down Expand Up @@ -491,7 +498,11 @@ func HasVirtualUniqueConstraints(tableDesc catalog.TableDescriptor) bool {
// enforced by an index. This includes implicitly partitioned UNIQUE indexes
// and UNIQUE WITHOUT INDEX constraints.
func RevalidateUniqueConstraintsInTable(
ctx context.Context, txn *kv.Txn, ie sqlutil.InternalExecutor, tableDesc catalog.TableDescriptor,
ctx context.Context,
txn *kv.Txn,
user security.SQLUsername,
ie sqlutil.InternalExecutor,
tableDesc catalog.TableDescriptor,
) error {
// Check implicitly partitioned UNIQUE indexes.
for _, index := range tableDesc.ActiveIndexes() {
Expand All @@ -504,6 +515,7 @@ func RevalidateUniqueConstraintsInTable(
index.GetPredicate(),
ie,
txn,
user,
true, /* preExisting */
); err != nil {
log.Errorf(ctx, "validation of unique constraints failed for table %s: %s", tableDesc.GetName(), err)
Expand All @@ -523,6 +535,7 @@ func RevalidateUniqueConstraintsInTable(
uc.Predicate,
ie,
txn,
user,
true, /* preExisting */
); err != nil {
log.Errorf(ctx, "validation of unique constraints failed for table %s: %s", tableDesc.GetName(), err)
Expand Down Expand Up @@ -551,6 +564,7 @@ func validateUniqueConstraint(
pred string,
ie sqlutil.InternalExecutor,
txn *kv.Txn,
user security.SQLUsername,
preExisting bool,
) error {
query, colNames, err := duplicateRowQuery(
Expand All @@ -567,7 +581,9 @@ func validateUniqueConstraint(
query,
)

values, err := ie.QueryRow(ctx, "validate unique constraint", txn, query)
sessionDataOverride := sessiondata.NoSessionDataOverride
sessionDataOverride.User = user
values, err := ie.QueryRowEx(ctx, "validate unique constraint", txn, sessionDataOverride, query)
if err != nil {
return err
}
Expand Down

0 comments on commit 97a382e

Please sign in to comment.