Merge #96382 #97746 #97802

96382: jobs: add VIEWJOB role option r=HonoreDB a=jayshrivastava

#### jobs: allow CONTROLJOB users to view all jobs 
This is a preliminary change before adding the `VIEWJOB` role
option. 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

---

#### jobs: add VIEWJOB role option
Release note (general change):
Users can now be granted the `VIEWJOB` role option
to be able to view all jobs (ex. via `SHOW JOBS`).
This role can be revoked using `NOVIEWJOB`.

Epic: none

97746: sql,schemachanger: ADD PRIMARY KEY NOT VALID returns an error r=Xiang-Gu a=Xiang-Gu

Fixes #96729

Release note (sql change): When we add an unvalidated PK constraint to a table, assuming its PK was on the implicit "rowid" column, will return an error. This is compatible to what PG will do.

97802: sql: return droppedView instead of nil r=chengxiong-ruan a=chengxiong-ruan

In #97631 we refactor the method to drorp index and column cascade dependents. But we didn't return a correct droppedViews for event logging, instead, we returned a nil. This wasn't caught by the `event_log` logic test because we `local-legacy-schema-changer` test config. Though this was caught when backporting to v22.2 because there was a fallback.

Epic: None

Release note: None

Co-authored-by: Jayant Shrivastava <[email protected]>
Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Chengxiong Ruan <[email protected]>

docs/generated/sql/bnf/stmt_block.bnf

@@ -1241,6 +1241,7 @@ unreserved_keyword ::=
 	| 'NOVIEWACTIVITY'
 	| 'NOVIEWACTIVITYREDACTED'
 	| 'NOVIEWCLUSTERSETTING'
+	| 'NOVIEWJOB'
 	| 'NOWAIT'
 	| 'NULLS'
 	| 'IGNORE_FOREIGN_KEYS'
@@ -1435,6 +1436,7 @@ unreserved_keyword ::=
 	| 'VIEWCLUSTERMETADATA'
 	| 'VIEWCLUSTERSETTING'
 	| 'VIEWDEBUG'
+	| 'VIEWJOB'
 	| 'VISIBLE'
 	| 'VOLATILE'
 	| 'VOTERS'
@@ -2938,6 +2940,8 @@ role_option ::=
 	| 'NOSQLLOGIN'
 	| 'VIEWCLUSTERSETTING'
 	| 'NOVIEWCLUSTERSETTING'
+	| 'VIEWJOB'
+	| 'NOVIEWJOB'
 	| password_clause
 	| valid_until_clause
 

pkg/jobs/jobsauth/authorization.go

@@ -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
 )
 
@@ -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,
@@ -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
 	}
 

pkg/jobs/jobsauth/authorization_test.go

@@ -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",
@@ -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{

pkg/sql/alter_table.go

@@ -37,6 +37,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/semenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -229,6 +230,9 @@ func (n *alterTableNode) startExec(params runParams) error {
 				}
 
 				if d.PrimaryKey {
+					if t.ValidationBehavior == tree.ValidationSkip {
+						return sqlerrors.NewUnsupportedUnvalidatedConstraintError(catconstants.ConstraintTypePK)
+					}
 					// Translate this operation into an ALTER PRIMARY KEY command.
 					alterPK := &tree.AlterTableAlterPrimaryKey{
 						Columns:       d.Columns,

pkg/sql/crdb_internal_test.go

@@ -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

pkg/sql/drop_index.go

@@ -555,5 +555,5 @@ func (p *planner) removeDependents(
 			}
 		}
 	}
-	return nil, nil
+	return droppedViews, nil
 }

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -3135,3 +3135,21 @@ CREATE TABLE public.t_twocol (
   CONSTRAINT t_twocol_pkey PRIMARY KEY (id ASC),
   FAMILY fam_0 (id, a)
 )
+
+# This subtests ensures we cannot add an unvalidated PK constraint
+subtest add_unvalidated_pk_96729
+
+statement ok
+CREATE TABLE t_96729 (i INT NOT NULL)
+
+statement error pgcode 0A000 PRIMARY KEY constraints cannot be marked NOT VALID
+ALTER TABLE t_96729 ADD PRIMARY KEY (i) NOT VALID;
+
+statement ok
+ALTER TABLE t_96729 ADD PRIMARY KEY (i);
+
+query TTTTB colnames
+SHOW CONSTRAINTS FROM t_96729
+----
+table_name  constraint_name  constraint_type  details              validated
+t_96729     t_96729_pkey     PRIMARY KEY      PRIMARY KEY (i ASC)  true

pkg/sql/logictest/testdata/logic_test/event_log

@@ -1,5 +1,5 @@
 # Legacy schema changer is also skipped because mutation IDs throughout log
-# entries will differ.
+# entries will differ. "event_log_legacy" file is for the legacy schema changer.
 # LogicTest: !local-legacy-schema-changer
 # knob-opt: sync-event-log