Skip to content

Commit

Permalink
jobs: increase access for CONTROLJOB users
Browse files Browse the repository at this point in the history
This is a preliminary change before adding the `VIEWJOB` role
option in the future. The purpose of the `VIEWJOB` role option
will be to allow TSEs to view jobs in admin clusters without
modifying them. This requires access to view all jobs, including ones
owned by admins.

Adding `VIEWJOB` will conflict with the present `CONTROLJOB` implementation.
It will be stronger than `CONTROLJOB` because it lets one view admin jobs when
`CONTROLJOB` doesn't, yet it will be weaker because it only allows viewing
jobs and not pause/cancel/resume-ing them.

This change is introduced to make `CONTROLJOB` at least as strong
as `VIEWJOB`: it now allows for all jobs to be viewed. In other words,
`VIEWJOB` lets you do a subset of things `CONTROLJOB` lets you do.

Also, the reason that `CONTROLJOB` prevents access to admin-owned
jobs is not well defined. This is tracked in #96432.

Release note (general change):
Previously, users with the `CONTROLJOB` role option could not view
owned by admins (ex. via `SHOW JOBS`). Now, they can.

Epic: none
  • Loading branch information
jayshrivastava committed Feb 2, 2023
1 parent 8ae413b commit 7605bfd
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 19 deletions.
32 changes: 20 additions & 12 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,28 @@ func Authorize(
return nil
}

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

jobOwnerUser := payload.UsernameProto.Decode()
jobOwnerIsAdmin, err := a.UserHasAdminRole(ctx, jobOwnerUser)

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

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

if jobOwnerIsAdmin {
if ownedByRootOrNode {
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
3 changes: 1 addition & 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
8 changes: 3 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

0 comments on commit 7605bfd

Please sign in to comment.