sql: prevent dropping roles with synthetic privileges

[RichardJCai]

Followup work to be done to support DROP OWNED BY and
REASSIGN OWNED BY here.

Release justification: Bug fix to newly introduced feature
Release note: None

[cockroach-teamcity]

This change is 

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

---

pkg/sql/drop_role.go line 583 at r1 (raw file):

		names[i] = username.Normalized()
	}
	rows, err := p.QueryIteratorEx(ctx, `drop-role-get-system-privileges`, sessiondata.NodeUserSessionDataOverride,

should this go through the cache instead of querying the table?

---

pkg/sql/drop_role.go line 587 at r1 (raw file):

	if err != nil {
		return err
	}

need to add:

defer func() { retErr = errors.CombineErrors(retErr, rows.Close()) }()

(and make the return parameter be named)

---

pkg/sql/drop_role.go line 607 at r1 (raw file):

			privilegeObjectFormatter.FormatName(fmt.Sprintf("%s %s", obj.GetObjectType(), obj.GetName()))
		} else {
			privilegeObjectFormatter.FormatName(string(obj.GetObjectType()))

i don't think FormatName should be used for the ObjectType. maybe use:

privilegeObjectFormatter.WriteString(string(obj.GetObjectType()))
if obj.GetName() != "" {
  privilegeObjectFormatter.WriteString(" ")
  privilegeObjectFormatter.FormatNode(obj.GetName())
}

---

pkg/sql/logictest/testdata/logic_test/synthetic_privileges line 256 at r1 (raw file):

GRANT USAGE ON EXTERNAL CONNECTION foo TO testuser

statement error pq: cannot drop role/user testuser: grants still exist on "external_connection foo", global, "virtual_table crdb_internal.tables"

the "global" here in this error message is a little confusing

---

pkg/sql/logictest/testdata/logic_test/synthetic_privileges line 257 at r1 (raw file):

statement error pq: cannot drop role/user testuser: grants still exist on "external_connection foo", global, "virtual_table crdb_internal.tables"
DROP USER testuser

it looks like we should also test that if there are default privileges then the user can't be dropped

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/drop_role.go line 583 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

should this go through the cache instead of querying the table?

Yeah, don't have cache yet, I'll need to add an interface to get synthetic privileges.

---

pkg/sql/drop_role.go line 587 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

need to add:

defer func() { retErr = errors.CombineErrors(retErr, rows.Close()) }()

(and make the return parameter be named)

Done.

---

pkg/sql/drop_role.go line 607 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i don't think FormatName should be used for the ObjectType. maybe use:

privilegeObjectFormatter.WriteString(string(obj.GetObjectType()))
if obj.GetName() != "" {
  privilegeObjectFormatter.WriteString(" ")
  privilegeObjectFormatter.FormatNode(obj.GetName())
}

obj.GetName returns a string so FormatNode won't work, updated to just use another writestring

---

pkg/sql/logictest/testdata/logic_test/synthetic_privileges line 256 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

the "global" here in this error message is a little confusing

I've updated it to be more explicit

---

pkg/sql/logictest/testdata/logic_test/synthetic_privileges line 257 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

it looks like we should also test that if there are default privileges then the user can't be dropped

Done.

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/drop_role.go line 583 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Yeah, don't have cache yet, I'll need to add an interface to get synthetic privileges.

Wait sorry I did add the cache, still need to add an interface for cases like this though

[rafiss]

lgtm with one more nit if you agree with it

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

---

pkg/sql/drop_role.go line 607 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

obj.GetName returns a string so FormatNode won't work, updated to just use another writestring

what about this

privilegeObjectFormatter.WriteString(string(obj.GetObjectType()))
if obj.GetName() != "" {
  privilegeObjectFormatter.WriteString(" ")
  privilegeObjectFormatter.FormatName(obj.GetName())
}

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

---

pkg/sql/drop_role.go line 607 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

what about this

privilegeObjectFormatter.WriteString(string(obj.GetObjectType()))
if obj.GetName() != "" {
  privilegeObjectFormatter.WriteString(" ")
  privilegeObjectFormatter.FormatName(obj.GetName())
}

Good call, should probably do this anyway, if for whatever reason we have to redact the error message (don't think this ever happens right now) then this would be correct.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msbutler, @rafiss, and @RichardJCai)

---

pkg/sql/drop_role.go line 607 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Good call, should probably do this anyway, if for whatever reason we have to redact the error message (don't think this ever happens right now) then this would be correct.

we are redacting the error message to use in telemetry logs now: #83807

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msbutler, @rafiss, and @RichardJCai)

pkg/sql/drop_role.go line 607 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…
we are redacting the error message to use in telemetry logs now: #83807

Ah even more valid then

Also first commit is from: #86823
Gonna wait for that to merge.

[RichardJCai]

Thanks for reviewing!

bors r=rafiss

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)