sql: prevent dropped user from accessing the cluster

[rafiss]

fixes #20718

Release note (bug fix): Previously if a user was logged in while a different session dropped that user, the dropped user would still inherit privileges from the public role. Now, CockroachDB checks that the user exists before allowing it to inherit privileges from the public role.

In addition, any active web sessions are now revoked when a user is dropped.

[cockroach-teamcity]

This change is 

[andyyang890]

Thanks for fixing this! Just had one minor question

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

---

pkg/sql/authorization.go line 178 at r1 (raw file):

	// Check if the 'public' pseudo-role has privileges.
	if privs.CheckPrivilege(username.PublicRoleName(), privilegeKind) {
		// Before returning true, make sure the user actually exists.

Is there a reason we don't check that the user exists if the public pseudo-role does not have privileges? i.e. why not unnest this check and move it before checking the public pseudo-role?

---

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

		numRoleSettingsRowsDeleted += rowsDeleted

		rowsDeleted, err = params.p.InternalSQLTxn().ExecEx(

The compiler is complaining that this value for rowsDeleted isn't being used. Maybe replace it with an underscore?

---

pkg/sql/tests/drop_role_concurrent_session_test.go line 31 at r1 (raw file):

// Test that if a user is dropped while a session is active, the session can
// no longer access objects in the cluster.
func TestDropRoleConcurrentSession(t *testing.T) {

Nice test!

[rafiss]

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

---

pkg/sql/authorization.go line 178 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Is there a reason we don't check that the user exists if the public pseudo-role does not have privileges? i.e. why not unnest this check and move it before checking the public pseudo-role?

my reasoning was that inheriting from public is the only way that a dropped user can end up passing this privilege check. if the public role does not have privileges, then the next check below (p.checkRolePredicate) is guaranteed to fail since the dropped user will not be anywhere in the role hierarchy. it's better to avoid the call to RoleExists unless it's needed, since it does cause an extra lookup.

i'll add some comments

---

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

Previously, andyyang890 (Andy Yang) wrote…

The compiler is complaining that this value for rowsDeleted isn't being used. Maybe replace it with an underscore?

done

---

pkg/sql/tests/drop_role_concurrent_session_test.go line 31 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Nice test!

thanks!

[andyyang890]

Reviewable status: complete! 1 of 0 LGTMs obtained

[rafiss]

tftr!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)