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

pkg/ccl/serverccl/role_authentication_test.go, line 104 at r1 (raw file):

		{"druidia", "12345", false, true, ""},
		{"druidia", "hunter2", false, false, "crypto/bcrypt"},
		{"root", "", true, false, "crypto/bcrypt"},

looks like root is the only true case in these tests. can we add more tests for other admin roles having is_superuser true also?

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

// we could save detailed memberships (as opposed to fully expanded) and reuse them
// across users. We may then want to lookup more than just this user.
func (p *planner) resolveMemberOfWithAdminOption(

this function wasn't really supposed to be used externally, since it does a KV lookup -- the idea was to always go through the RoleMembershipCache to avoid unexpected round trips

pkg/sql/user.go, line 257 at r1 (raw file):

	)
	if err != nil {
		return sessioninit.AuthInfo{}, err

could this all be done outside of retrieveAuthInfo, so that it doesn't end up in the session init cache? instead, is there a way we could do this in the GetUserSessionInitInfo function, and reuse the execCfg.RoleMemberCache? it would address the concerns i had about cache inconsistency, and the danger of exposing a non-cached ResolveMemberOfWithAdminOption function

pkg/sql/sessioninit/cache.go, line 63 at r1 (raw file):

	UserExists bool
	// IsSuperuser is set to true for any admin users.
	IsSuperuser bool

hm i think there's a cache consistency issue here now

when GRANT admin TO user happens, the role membership cache is invalidated:

however, IsSuperUser is now going to be cached in the SessionInitCache, and as per the clearCacheIfStale method below, it won't be cleared out even if the role memberships change.

you should be able to repro with:

- login as testuser
- SHOW is_superuser should be false
- disconnect
- login as root
- GRANT admin TO testuser
- disconnect
- login as testuser
- SHOW is_superuser should be true at this point, but i think it will show false.

this points to the fact that this sessioninit cache is undertested right now -- i wanted to write tests for it during stability period

pkg/sql/sqlutil/auth.go, line 23 at r1 (raw file):

// ResolveMemberOfWithAdminOption explores all the memberships of the user.
// It returns a map of memberships, as well as whether that membership
// is an admin role.

i think this deserves some more disclaimers around how it can cause unexpected latency, and if possible you should use the cached version (see sql.MemberOfWithAdminOption)

even better would be to make this function here use the RoleMemberCache. see if we can pull the sql.planner.MemberOfWithAdminOption function in here and refactor it so it doesn't use the planner directly.

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

pkg/ccl/serverccl/role_authentication_test.go, line 104 at r1 (raw file):

Done.

pkg/sql/user.go, line 257 at r1 (raw file):

urgh, that's annoying. not sure where to get p.Descriptors() from which is what held me back.

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

pkg/sql/user.go, line 257 at r1 (raw file):

i think what we can do (not the prettiest...) is change the signature of retrieveSessionInitInfoWithCache (in pkg/sql/user.go) to

func retrieveSessionInitInfoWithCache(
	ctx context.Context,
	execCfg *ExecutorConfig,
	ie *InternalExecutor,
	username security.SQLUsername,
	databaseName string,
) (aInfo sessioninit.AuthInfo, settingsEntries []sessioninit.SettingsCacheEntry, isSuperuser bool, err error) {

and somewhere in the impl of that you can use execCfg.CollectionFactory.Txn() which will give you the descs.Collection you need.

IMO, the func (p *planner) MemberOfWithAdminOption( method should actually be a method on the RoleMemberCache. similar to how SessionInitCache has a GetAuthInfo method.

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

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

Done.

pkg/sql/user.go, line 257 at r1 (raw file):

Done.

pkg/sql/sessioninit/cache.go, line 63 at r1 (raw file):

Done.

pkg/sql/sqlutil/auth.go, line 23 at r1 (raw file):

Done.

looking nice!

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

pkg/sql/user.go, line 109 at r10 (raw file):

	}

	err = execCfg.CollectionFactory.Txn(

there's one sneaky thing here -- since this can result in a KV lookup and it's during the connection init flow, this is supposed to be behind the server.user_login.timeout setting. see the comment on GetUserSessionInitInfo. the timeout itself is applied inside of the retrieveSessionInitInfoWithCache function right now.

there's also a test for it in user_test.go (it's named TestGetUserHashedPasswordTimeout since i missed renaming it in my last refactor)

so could you move this lookup to be behind the timeout, and verify with that test that the timeout applies to this operation?

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

pkg/sql/user.go, line 109 at r10 (raw file):

gotcha, done

lgtm! thanks for the tests

Reviewed 5 of 6 files at r10, 13 of 13 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)