*: stop swallowing errors from privilege checks #95276
Conversation
|
This change is |
rafiss
changed the title
rafiss
force-pushed
the
refactor-check-privilege
branch
3 times, most recently
from
5cc31e3 to
f0e4740
Compare
rafiss
requested review from
a team and
rhu713
and removed request for
a team
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @rhu713)
pkg/sql/authorization.go line 199 at r1 (raw file):
// we don't get the risk to say "OK" to root requests // with an invalid API usage. if p.txn == nil {
This is a confusing place to check the txn since it looks like it is not used by this function.
Why is the function being called in the first place if there is no transaction?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/authorization.go line 199 at r1 (raw file):
Previously, ecwall (Evan Wall) wrote…
This is a confusing place to check the
txnsince it looks like it is not used by this function.Why is the function being called in the first place if there is no transaction?
i've just copied the existing check over from CheckAnyPrivilege
the txn is used in the function - the getPrivilegeDescriptor and MemberofWithAdminOption calls both use the txn.
the check was probably added as a defensive check since privilege checks occur early on during query planning and in the conn_executor, and it's better to have an assertion error instead of a panic if the txn was incorrectly set up. since it was already there, i think removing it should be done as a separate change after making sure it's ok to remove
There was a problem hiding this comment.
Just took a look at the status/admin server stuff. LGTM. My understanding is that the errs returned from any of the checks are not permission gates, but errors reading/retrieving the permission.
Reviewed 2 of 8 files at r1, 2 of 6 files at r2, 2 of 2 files at r5.
Reviewable status:
tftr! yeah, that's the right understanding |
|
tftr! bors r=ecwall |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
there's merge skew bors r- |
|
Canceled. |
The next commit will use these functions. Release note: None
Instead of swallowing errors, we use the new HasPrivilege function. If there was an error that causes the transaction to abort, it's important to propagate it. Release note: None
Instead of swallowing errors, we use the new HasAnyPrivilege function. If there was an error that causes the transaction to abort, it's important to propagate it. Release note: None
Instead of swallowing errors, we use the new HasPrivilege function. If there was an error that causes the transaction to abort, it's important to propagate it. Release note: None
The privilege check can return an internal error, so it should not be swallowed. Release note: None
rafiss
force-pushed
the
refactor-check-privilege
branch
from
f0e4740 to
6fb8cc2
Compare
|
bors r=ecwall |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
sql: add HasPrivilege and HasAnyPrivilege to AuthorizationAccessor
*: stop swallowing errors from CheckPrivilegeForUser
*: stop swallowing errors from CheckAnyPrivilege
*: stop swallowing errors from CheckPrivilege
server: don't swallow error in hasGlobalPrivilege
Instead of swallowing errors, we use the new Has*Privilege functions. If
there was an error that causes the transaction to abort, it's important
to propagate it.
Epic: None
Release note: None