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.

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

jobs: add VIEWJOB role option #96382

Merged
merged 2 commits into from
Feb 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions docs/generated/sql/bnf/stmt_block.bnf
Original file line number Diff line number Diff line change
Expand Up @@ -1241,6 +1241,7 @@ unreserved_keyword ::=
| 'NOVIEWACTIVITY'
| 'NOVIEWACTIVITYREDACTED'
| 'NOVIEWCLUSTERSETTING'
| 'NOVIEWJOB'
| 'NOWAIT'
| 'NULLS'
| 'IGNORE_FOREIGN_KEYS'
Expand Down Expand Up @@ -1435,6 +1436,7 @@ unreserved_keyword ::=
| 'VIEWCLUSTERMETADATA'
| 'VIEWCLUSTERSETTING'
| 'VIEWDEBUG'
| 'VIEWJOB'
| 'VISIBLE'
| 'VOLATILE'
| 'VOTERS'
Expand Down Expand Up @@ -2938,6 +2940,8 @@ role_option ::=
| 'NOSQLLOGIN'
| 'VIEWCLUSTERSETTING'
| 'NOVIEWCLUSTERSETTING'
| 'VIEWJOB'
| 'NOVIEWJOB'
| password_clause
| valid_until_clause

Expand Down
33 changes: 23 additions & 10 deletions pkg/jobs/jobsauth/authorization.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ const (
ViewAccess AccessLevel = iota

// ControlAccess is used to perform authorization for modifying jobs (ex. PAUSE|CANCEL|RESUME JOB).
// This access level performs stricter checks than ViewAccess.
//
// The set of jobs visible via ControlAccess is a subset of jobs visible via
// ViewAccess. In other words: if a user with a given set of privileges is
// authorized to modify a job using ControlAccess, they will be authorized to
// view it using ViewAccess.
ControlAccess
)

Expand Down Expand Up @@ -79,6 +73,15 @@ type AuthorizationAccessor interface {
// Authorize returns nil if the user is authorized to access the job.
// If the user is not authorized, then a pgcode.InsufficientPrivilege
// error will be returned.
//
// TODO(#96432): sort out internal job owners and rules for accessing them
// Authorize checks these rules in order:
// 1. If the user is an admin, grant access.
// 2. If the AccessLevel is ViewAccess, grant access if the user has CONTROLJOB
// or if the user owns the job.
// 3. If the AccessLevel is ControlAccess, grant access if the user has CONTROLJOB
// and the job owner is not an admin.
// 4. If there is an authorization check for this job type that passes, grant the user access.
func Authorize(
ctx context.Context,
a AuthorizationAccessor,
Expand All @@ -94,23 +97,33 @@ func Authorize(
return nil
}

userHasControlJob, err := a.HasRoleOption(ctx, roleoption.CONTROLJOB)
hasControlJob, err := a.HasRoleOption(ctx, roleoption.CONTROLJOB)
if err != nil {
return err
}

hasViewJob, err := a.HasRoleOption(ctx, roleoption.VIEWJOB)
if err != nil {
return err
}

jobOwnerUser := payload.UsernameProto.Decode()

if accessLevel == ViewAccess {
if a.User() == jobOwnerUser || hasControlJob || hasViewJob {
return nil
}
}

jobOwnerIsAdmin, err := a.UserHasAdminRole(ctx, jobOwnerUser)
if err != nil {
return err
}

if jobOwnerIsAdmin {
return pgerror.Newf(pgcode.InsufficientPrivilege,
"only admins can control jobs owned by other admins")
}

if (userHasControlJob) || (accessLevel == ViewAccess && a.User() == jobOwnerUser) {
if hasControlJob {
return nil
}

Expand Down
22 changes: 20 additions & 2 deletions pkg/jobs/jobsauth/authorization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,14 @@ func TestAuthorization(t *testing.T) {
accessLevel: jobsauth.ControlAccess,
},
{
name: "controljob-insufficient-for-admin-jobs",
name: "controljob-sufficient-to-view-admin-jobs",
user: username.MakeSQLUsernameFromPreNormalizedString("user1"),
roleOptions: map[roleoption.Option]struct{}{
roleoption.CONTROLJOB: {},
},
admins: map[string]struct{}{"user2": {}},
payload: makeBackupPayload("user2"),
accessLevel: jobsauth.ViewAccess,
userErr: pgerror.New(pgcode.InsufficientPrivilege, "foo"),
},
{
name: "users-access-their-own-jobs",
Expand Down Expand Up @@ -206,6 +205,25 @@ func TestAuthorization(t *testing.T) {
payload: makeChangefeedPayload("user2", []descpb.ID{0, 1, 2}),
accessLevel: jobsauth.ControlAccess,
},
{
name: "viewjob-allows-read-access",
user: username.MakeSQLUsernameFromPreNormalizedString("user1"),
roleOptions: map[roleoption.Option]struct{}{roleoption.VIEWJOB: {}},
admins: map[string]struct{}{},

payload: makeBackupPayload("user2"),
accessLevel: jobsauth.ViewAccess,
},
{
name: "viewjob-disallows-control-access",
user: username.MakeSQLUsernameFromPreNormalizedString("user1"),
roleOptions: map[roleoption.Option]struct{}{roleoption.VIEWJOB: {}},
admins: map[string]struct{}{},

payload: makeBackupPayload("user2"),
accessLevel: jobsauth.ControlAccess,
userErr: pgerror.New(pgcode.InsufficientPrivilege, "foo"),
},
} {
t.Run(tc.name, func(t *testing.T) {
testAuth := &testAuthAccessor{
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/crdb_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1380,9 +1380,9 @@ func TestInternalSystemJobsAccess(t *testing.T) {
_, err = registry.CreateJobWithTxn(ctx, rec, 3, nil /* txn */)
assert.NoError(t, err)

// user1 can see all jobs not owned by admins because they have the CONTROLJOB role option.
// user1 can see all jobs because they have the CONTROLJOB role option.
asUser("user1", func(userDB *sqlutils.SQLRunner) {
userDB.CheckQueryResults(t, "SELECT id FROM crdb_internal.system_jobs WHERE id IN (1,2,3) ORDER BY id", [][]string{{"1"}, {"2"}})
userDB.CheckQueryResults(t, "SELECT id FROM crdb_internal.system_jobs WHERE id IN (1,2,3) ORDER BY id", [][]string{{"1"}, {"2"}, {"3"}})
})

// user2 can only see their own job
Expand Down
38 changes: 33 additions & 5 deletions pkg/sql/logictest/testdata/logic_test/jobs
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,12 @@ DROP TABLE t1

user testuser

# testuser should be able to see jobs created by non-admin users.
# testuser should be able to see all jobs
query TTT
SELECT job_type, description, user_name FROM crdb_internal.jobs ORDER BY 1, 2, 3
SELECT job_type, description, user_name FROM crdb_internal.jobs WHERE job_type = 'SCHEMA CHANGE GC' ORDER BY 1, 2, 3
----
SCHEMA CHANGE CREATE INDEX ON test.public.t1 (x) testuser2
SCHEMA CHANGE CREATE INDEX ON test.public.u (x) testuser
SCHEMA CHANGE DROP TABLE test.public.t1 testuser2
SCHEMA CHANGE GC GC for DROP TABLE test.public.t1 testuser2
SCHEMA CHANGE GC GC for temporary index used during index backfill root
SCHEMA CHANGE GC GC for temporary index used during index backfill testuser
SCHEMA CHANGE GC GC for temporary index used during index backfill testuser2

Expand Down Expand Up @@ -186,6 +184,36 @@ RESUME JOB (SELECT $job_id)

user root

statement ok
ALTER ROLE testuser VIEWJOB

user testuser

# testuser should be able to see all jobs
query TTT
SELECT job_type, description, user_name FROM crdb_internal.jobs WHERE job_type = 'SCHEMA CHANGE GC' ORDER BY 1, 2, 3
----
SCHEMA CHANGE GC GC for DROP TABLE test.public.t1 testuser2
SCHEMA CHANGE GC GC for DROP TABLE test.public.t2 root
SCHEMA CHANGE GC GC for temporary index used during index backfill root
SCHEMA CHANGE GC GC for temporary index used during index backfill testuser
SCHEMA CHANGE GC GC for temporary index used during index backfill testuser2

user root

statement ok
ALTER ROLE testuser NOVIEWJOB

user testuser

# testuser can only see their own job
query TTT
SELECT job_type, description, user_name FROM crdb_internal.jobs WHERE job_type = 'SCHEMA CHANGE GC' ORDER BY 1, 2, 3
----
SCHEMA CHANGE GC GC for temporary index used during index backfill testuser

user root

# Validate that the schema_change_successful metric
query T
SELECT feature_name FROM crdb_internal.feature_usage
Expand Down
9 changes: 9 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/role
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,15 @@ ALTER ROLE testrole WITH CREATEROLE CREATEROLE
statement error pq: redundant role options
ALTER ROLE testrole WITH NOCREATEROLE NOCREATEROLE

statement error pq: conflicting role options
ALTER ROLE testrole WITH VIEWJOB NOVIEWJOB

statement error pq: redundant role options
ALTER ROLE testrole WITH VIEWJOB VIEWJOB

statement error pq: redundant role options
ALTER ROLE testrole WITH NOVIEWJOB NOVIEWJOB

statement ok
CREATE ROLE rolewithcreate WITH CREATEROLE

Expand Down
14 changes: 12 additions & 2 deletions pkg/sql/parser/sql.y
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,7 @@ func (u *sqlSymUnion) showTenantOpts() tree.ShowTenantOptions {
%token <str> NOSQLLOGIN NO_INDEX_JOIN NO_ZIGZAG_JOIN NO_FULL_SCAN NONE NONVOTERS NORMAL NOT
%token <str> NOTHING NOTHING_AFTER_RETURNING
%token <str> NOTNULL
%token <str> NOVIEWACTIVITY NOVIEWACTIVITYREDACTED NOVIEWCLUSTERSETTING NOWAIT NULL NULLIF NULLS NUMERIC
%token <str> NOVIEWACTIVITY NOVIEWACTIVITYREDACTED NOVIEWCLUSTERSETTING NOVIEWJOB NOWAIT NULL NULLIF NULLS NUMERIC

%token <str> OF OFF OFFSET OID OIDS OIDVECTOR OLD_KMS ON ONLY OPT OPTION OPTIONS OR
%token <str> ORDER ORDINALITY OTHERS OUT OUTER OVER OVERLAPS OVERLAY OWNED OWNER OPERATOR
Expand Down Expand Up @@ -984,7 +984,7 @@ func (u *sqlSymUnion) showTenantOpts() tree.ShowTenantOptions {
%token <str> UPDATE UPSERT UNSET UNTIL USE USER USERS USING UUID

%token <str> VALID VALIDATE VALUE VALUES VARBIT VARCHAR VARIADIC VERIFY_BACKUP_TABLE_DATA VIEW VARYING VIEWACTIVITY VIEWACTIVITYREDACTED VIEWDEBUG
%token <str> VIEWCLUSTERMETADATA VIEWCLUSTERSETTING VIRTUAL VISIBLE INVISIBLE VOLATILE VOTERS
%token <str> VIEWCLUSTERMETADATA VIEWCLUSTERSETTING VIEWJOB VIRTUAL VISIBLE INVISIBLE VOLATILE VOTERS

%token <str> WHEN WHERE WINDOW WITH WITHIN WITHOUT WORK WRITE

Expand Down Expand Up @@ -10216,6 +10216,14 @@ role_option:
{
$$.val = tree.KVOption{Key: tree.Name($1), Value: nil}
}
| VIEWJOB
{
$$.val = tree.KVOption{Key: tree.Name($1), Value: nil}
}
| NOVIEWJOB
{
$$.val = tree.KVOption{Key: tree.Name($1), Value: nil}
}
| password_clause
| valid_until_clause

Expand Down Expand Up @@ -16239,6 +16247,7 @@ unreserved_keyword:
| NOVIEWACTIVITY
| NOVIEWACTIVITYREDACTED
| NOVIEWCLUSTERSETTING
| NOVIEWJOB
| NOWAIT
| NULLS
| IGNORE_FOREIGN_KEYS
Expand Down Expand Up @@ -16433,6 +16442,7 @@ unreserved_keyword:
| VIEWCLUSTERMETADATA
| VIEWCLUSTERSETTING
| VIEWDEBUG
| VIEWJOB
| VISIBLE
| VOLATILE
| VOTERS
Expand Down
6 changes: 4 additions & 2 deletions pkg/sql/roleoption/option_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 11 additions & 1 deletion pkg/sql/roleoption/role_option.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ const (
NOSQLLOGIN
VIEWCLUSTERSETTING
NOVIEWCLUSTERSETTING
VIEWJOB
NOVIEWJOB
)

// ControlChangefeedDeprecationNoticeMsg is a user friendly notice which should be shown when CONTROLCHANGEFEED is used
Expand Down Expand Up @@ -103,6 +105,8 @@ var toSQLStmts = map[Option]string{
NOVIEWACTIVITYREDACTED: `DELETE FROM system.role_options WHERE username = $1 AND option = 'VIEWACTIVITYREDACTED'`,
VIEWCLUSTERSETTING: `INSERT INTO system.role_options (username, option) VALUES ($1, 'VIEWCLUSTERSETTING') ON CONFLICT DO NOTHING`,
NOVIEWCLUSTERSETTING: `DELETE FROM system.role_options WHERE username = $1 AND option = 'VIEWCLUSTERSETTING'`,
VIEWJOB: `INSERT INTO system.role_options (username, option) VALUES ($1, 'VIEWJOB') ON CONFLICT DO NOTHING`,
NOVIEWJOB: `DELETE FROM system.role_options WHERE username = $1 AND option = 'VIEWJOB'`,
}

// toSQLStmtsWithID is a map of Kind -> SQL statement string for applying the
Expand Down Expand Up @@ -134,6 +138,8 @@ var toSQLStmtsWithID = map[Option]string{
NOVIEWACTIVITYREDACTED: `DELETE FROM system.role_options WHERE username = $1 AND user_id = $2 AND option = 'VIEWACTIVITYREDACTED'`,
VIEWCLUSTERSETTING: `INSERT INTO system.role_options (username, option, user_id) VALUES ($1, 'VIEWCLUSTERSETTING', $2) ON CONFLICT DO NOTHING`,
NOVIEWCLUSTERSETTING: `DELETE FROM system.role_options WHERE username = $1 AND user_id = $2 AND option = 'VIEWCLUSTERSETTING'`,
VIEWJOB: `INSERT INTO system.role_options (username, option, user_id) VALUES ($1, 'VIEWJOB', $2) ON CONFLICT DO NOTHING`,
NOVIEWJOB: `DELETE FROM system.role_options WHERE username = $1 AND user_id = $2 AND option = 'VIEWJOB'`,
}

// Mask returns the bitmask for a given role option.
Expand Down Expand Up @@ -169,6 +175,8 @@ var ByName = map[string]Option{
"NOSQLLOGIN": NOSQLLOGIN,
"VIEWCLUSTERSETTING": VIEWCLUSTERSETTING,
"NOVIEWCLUSTERSETTING": NOVIEWCLUSTERSETTING,
"VIEWJOB": VIEWJOB,
"NOVIEWJOB": NOVIEWJOB,
}

// ToOption takes a string and returns the corresponding Option.
Expand Down Expand Up @@ -321,7 +329,9 @@ func (rol List) CheckRoleOptionConflicts() error {
(roleOptionBits&SQLLOGIN.Mask() != 0 &&
roleOptionBits&NOSQLLOGIN.Mask() != 0) ||
(roleOptionBits&VIEWCLUSTERSETTING.Mask() != 0 &&
roleOptionBits&NOVIEWCLUSTERSETTING.Mask() != 0) {
roleOptionBits&NOVIEWCLUSTERSETTING.Mask() != 0) ||
(roleOptionBits&VIEWJOB.Mask() != 0 &&
roleOptionBits&NOVIEWJOB.Mask() != 0) {
return pgerror.Newf(pgcode.Syntax, "conflicting role options")
}
return nil
Expand Down
6 changes: 4 additions & 2 deletions pkg/sql/testdata/telemetry/role_options
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ iam.roles.create.*
----

feature-usage
CREATE ROLE testuser CREATEROLE LOGIN VALID UNTIL '2020-01-01' CONTROLJOB CONTROLCHANGEFEED CREATEDB CREATELOGIN VIEWACTIVITY CANCELQUERY MODIFYCLUSTERSETTING
CREATE ROLE testuser CREATEROLE LOGIN VALID UNTIL '2020-01-01' CONTROLJOB CONTROLCHANGEFEED CREATEDB CREATELOGIN VIEWACTIVITY CANCELQUERY MODIFYCLUSTERSETTING VIEWJOB
----
iam.roles.create.cancelquery
iam.roles.create.controlchangefeed
Expand All @@ -19,9 +19,10 @@ iam.roles.create.modifyclustersetting
iam.roles.create.role
iam.roles.create.valid until
iam.roles.create.viewactivity
iam.roles.create.viewjob

feature-usage
ALTER ROLE testuser NOCREATEROLE NOLOGIN NOCONTROLJOB NOCONTROLCHANGEFEED NOCREATEDB NOCREATELOGIN NOVIEWACTIVITY NOCANCELQUERY NOMODIFYCLUSTERSETTING
ALTER ROLE testuser NOCREATEROLE NOLOGIN NOCONTROLJOB NOCONTROLCHANGEFEED NOCREATEDB NOCREATELOGIN NOVIEWACTIVITY NOCANCELQUERY NOMODIFYCLUSTERSETTING NOVIEWJOB
----
iam.roles.alter.nocancelquery
iam.roles.alter.nocontrolchangefeed
Expand All @@ -32,4 +33,5 @@ iam.roles.alter.nocreaterole
iam.roles.alter.nologin
iam.roles.alter.nomodifyclustersetting
iam.roles.alter.noviewactivity
iam.roles.alter.noviewjob
iam.roles.alter.role