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

Jump to bottom

sql,ui: enable non-admin users to see their own jobs #44345

Merged
merged 1 commit into from
Jan 27, 2020
+125 −31
Merged

sql,ui: enable non-admin users to see their own jobs #44345

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 42 additions & 30 deletions pkg/server/admin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1250,14 +1250,16 @@ func TestAdminAPIJobs(t *testing.T) {
status jobs.Status
details jobspb.Details
progress jobspb.ProgressDetails
username string
}{
{1, jobs.StatusRunning, jobspb.RestoreDetails{}, jobspb.RestoreProgress{}},
{2, jobs.StatusRunning, jobspb.BackupDetails{}, jobspb.BackupProgress{}},
{3, jobs.StatusSucceeded, jobspb.BackupDetails{}, jobspb.BackupProgress{}},
{4, jobs.StatusRunning, jobspb.ChangefeedDetails{}, jobspb.ChangefeedProgress{}},
{1, jobs.StatusRunning, jobspb.RestoreDetails{}, jobspb.RestoreProgress{}, security.RootUser},
{2, jobs.StatusRunning, jobspb.BackupDetails{}, jobspb.BackupProgress{}, security.RootUser},
{3, jobs.StatusSucceeded, jobspb.BackupDetails{}, jobspb.BackupProgress{}, security.RootUser},
{4, jobs.StatusRunning, jobspb.ChangefeedDetails{}, jobspb.ChangefeedProgress{}, security.RootUser},
{5, jobs.StatusSucceeded, jobspb.BackupDetails{}, jobspb.BackupProgress{}, authenticatedUserNameNoAdmin},
}
for _, job := range testJobs {
payload := jobspb.Payload{Details: jobspb.WrapPayloadDetails(job.details)}
payload := jobspb.Payload{Username: job.username, Details: jobspb.WrapPayloadDetails(job.details)}
payloadBytes, err := protoutil.Marshal(&payload)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -1289,33 +1291,43 @@ func TestAdminAPIJobs(t *testing.T) {
const invalidJobType = math.MaxInt32

testCases := []struct {
uri string
expectedIDs []int64
uri string
expectedIDsViaAdmin []int64
expectedIDsViaNonAdmin []int64
}{
{"jobs", append([]int64{4, 3, 2, 1}, existingIDs...)},
{"jobs?limit=1", []int64{4}},
{"jobs?status=running", []int64{4, 2, 1}},
{"jobs?status=succeeded", append([]int64{3}, existingIDs...)},
{"jobs?status=pending", []int64{}},
{"jobs?status=garbage", []int64{}},
{fmt.Sprintf("jobs?type=%d", jobspb.TypeBackup), []int64{3, 2}},
{fmt.Sprintf("jobs?type=%d", jobspb.TypeRestore), []int64{1}},
{fmt.Sprintf("jobs?type=%d", invalidJobType), []int64{}},
{fmt.Sprintf("jobs?status=running&type=%d", jobspb.TypeBackup), []int64{2}},
}
for i, testCase := range testCases {
var res serverpb.JobsResponse
if err := getAdminJSONProto(s, testCase.uri, &res); err != nil {
t.Fatal(err)
}
resIDs := []int64{}
for _, job := range res.Jobs {
resIDs = append(resIDs, job.ID)
}
if e, a := testCase.expectedIDs, resIDs; !reflect.DeepEqual(e, a) {
t.Errorf("%d: expected job IDs %v, but got %v", i, e, a)
{"jobs", append([]int64{5, 4, 3, 2, 1}, existingIDs...), []int64{5}},
{"jobs?limit=1", []int64{5}, []int64{5}},
{"jobs?status=running", []int64{4, 2, 1}, []int64{}},
{"jobs?status=succeeded", append([]int64{5, 3}, existingIDs...), []int64{5}},
{"jobs?status=pending", []int64{}, []int64{}},
{"jobs?status=garbage", []int64{}, []int64{}},
{fmt.Sprintf("jobs?type=%d", jobspb.TypeBackup), []int64{5, 3, 2}, []int64{5}},
{fmt.Sprintf("jobs?type=%d", jobspb.TypeRestore), []int64{1}, []int64{}},
{fmt.Sprintf("jobs?type=%d", invalidJobType), []int64{}, []int64{}},
{fmt.Sprintf("jobs?status=running&type=%d", jobspb.TypeBackup), []int64{2}, []int64{}},
}

testutils.RunTrueAndFalse(t, "isAdmin", func(t *testing.T, isAdmin bool) {
for i, testCase := range testCases {
var res serverpb.JobsResponse
if err := getAdminJSONProtoWithAdminOption(s, testCase.uri, &res, isAdmin); err != nil {
t.Fatal(err)
}
resIDs := []int64{}
for _, job := range res.Jobs {
resIDs = append(resIDs, job.ID)
}

expected := testCase.expectedIDsViaAdmin
if !isAdmin {
expected = testCase.expectedIDsViaNonAdmin
}

if e, a := expected, resIDs; !reflect.DeepEqual(e, a) {
t.Errorf("%d: expected job IDs %v, but got %v", i, e, a)
}
}
}
})
}

func TestAdminAPILocations(t *testing.T) {
Expand Down
20 changes: 19 additions & 1 deletion pkg/sql/crdb_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,10 +453,18 @@ CREATE TABLE crdb_internal.jobs (
coordinator_id INT
)`,
populate: func(ctx context.Context, p *planner, _ *DatabaseDescriptor, addRow func(...tree.Datum) error) error {
currentUser := p.SessionData().User
isAdmin, err := p.HasAdminRole(ctx)
if err != nil {
return err
}

// Beware: we're querying system.jobs as root; we need to be careful to filter
// out results that the current user is not able to see.
query := `SELECT id, status, created, payload, progress FROM system.jobs`
rows, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.QueryEx(
ctx, "crdb-internal-jobs-table", p.txn,
sqlbase.InternalExecutorSessionDataOverride{User: p.SessionData().User},
sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
query)
if err != nil {
return err
Expand All @@ -472,6 +480,16 @@ CREATE TABLE crdb_internal.jobs (

// Extract data from the payload.
payload, err := jobs.UnmarshalPayload(payloadBytes)

// We filter out masked rows before we allocate all the
// datums. Needless allocate when not necessary.
sameUser := payload != nil && payload.Username == currentUser
if canAccess := isAdmin || sameUser; !canAccess {
// This user is neither an admin nor the user who created the
// job. They cannot see this row.
continue
}

if err != nil {
errorStr = tree.NewDString(fmt.Sprintf("error decoding payload: %v", err))
} else {
Expand Down
64 changes: 64 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/jobs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# These test verify that a user's job are visible via
# crdb_internal.jobs and SHOW JOBS.

user root

statement ok
GRANT ALL ON DATABASE test TO testuser

statement ok
CREATE TABLE t(x INT); INSERT INTO t(x) VALUES (1); CREATE INDEX ON t(x)

query TTT
SELECT job_type, description, user_name FROM [SHOW JOBS]
----
SCHEMA CHANGE CREATE INDEX ON test.public.t (x) root

query TTT
SELECT job_type, description, user_name FROM crdb_internal.jobs
----
SCHEMA CHANGE CREATE INDEX ON test.public.t (x) root

user testuser

# a non-admin user cannot see the admin jobs

query TTT
SELECT job_type, description, user_name FROM [SHOW JOBS]
----

query TTT
SELECT job_type, description, user_name FROM crdb_internal.jobs
----

# However they can see their own jobs.

statement ok
CREATE TABLE u(x INT); INSERT INTO u(x) VALUES (1); CREATE INDEX ON u(x);


query TTT
SELECT job_type, description, user_name FROM [SHOW JOBS]
----
SCHEMA CHANGE CREATE INDEX ON test.public.u (x) testuser

query TTT
SELECT job_type, description, user_name FROM crdb_internal.jobs
----
SCHEMA CHANGE CREATE INDEX ON test.public.u (x) testuser

# And root can see both.

user root

query TTT
SELECT job_type, description, user_name FROM [SHOW JOBS]
----
SCHEMA CHANGE CREATE INDEX ON test.public.t (x) root
SCHEMA CHANGE CREATE INDEX ON test.public.u (x) testuser

query TTT
SELECT job_type, description, user_name FROM crdb_internal.jobs
----
SCHEMA CHANGE CREATE INDEX ON test.public.t (x) root
SCHEMA CHANGE CREATE INDEX ON test.public.u (x) testuser