sql: refactor pg_has_role to remove privilege.GRANT usage#75726
sql: refactor pg_has_role to remove privilege.GRANT usage#75726craig[bot] merged 1 commit intocockroachdb:masterfrom ecwall:refactor-pg_has_rule
Conversation
|
This change is |
| user security.SQLUsername, | ||
| priv privilege.Privilege, | ||
| ) (bool, error) { | ||
| privs []privilege.Privilege, |
There was a problem hiding this comment.
Checks multiple privileges now to avoid multiple calls to ResolveDescriptorForPrivilegeSpecifier.
Returns tree.HasAnyPrivilegeResult to avoid needing to unwrap and inspect error pg codes at call sites.
| // and https://www.postgresql.org/docs/release/8.2.0/. | ||
| return false, nil | ||
| } | ||
| hasPrivilege, err := hasPrivilegeFunc(privilege.Privilege{Kind: privilege.ALL}) |
There was a problem hiding this comment.
privilege.ALL does not need special handling here because CheckPrivilegeForUser and CheckGrantOptionsForUser handle that case internally.
|
import file change LGTM, but the majority of the changes here are for SQL Experience to review! |
RichardJCai
left a comment
There was a problem hiding this comment.
Reviewed 1 of 5 files at r1, 1 of 2 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ecwall and @RichardJCai)
pkg/sql/resolver.go, line 412 at r2 (raw file):
_, table, err = p.Descriptors().GetImmutableTableByName( ctx, p.txn, tn, tree.ObjectLookupFlags{ CommonLookupFlags: tree.CommonLookupFlags{
Why do we no longer require the table to be found?
Can't this result in calling IsSequence on a nil pointer below?
pkg/sql/sem/tree/eval.go, line 3084 at r2 (raw file):
const ( // At least one of the specified privileges is granted HasPrivilege HasAnyPrivilegeResult = 1
I think these constants make more sense to be defined in the privilege package.
Also I'm not entirely convinced we need this, looking at the usages of ObjectNotFound seem to be mostly in the builtins. I think we can continue to require the builtins to check if the object exist before checking for privileges and we should probably leave the privilege check itself to return true or false. Or at least I don't think we need HasAnyPrivilege itself to return ObjectNotFound
ecwall
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/resolver.go, line 412 at r2 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Why do we no longer require the table to be found?
Can't this result in calling IsSequence on a nil pointer below?
This code is confusing because the case of it being "required" is handled above by
if _, err = p.ResolveTableName(ctx, tn); err != nil {
return nil, err
}
GetImmutableTableByName requires that ResolveTableName be called first to populate fields inside tn so passing Required: true doesn't do anything.
pkg/sql/sem/tree/eval.go, line 3084 at r2 (raw file):
I think these constants make more sense to be defined in the privilege package.
I defined HasAnyPrivilegeResult in the same package as the function that returns it.
Also I'm not entirely convinced we need this, looking at the usages of ObjectNotFound seem to be mostly in the builtins.
HasAnyPrivilege is only used by builtins so HasAnyPrivilegeResult is only used there.
I think we can continue to require the builtins to check if the object exist before checking for privileges and we should probably leave the privilege check itself to return true or false.
It should probably be split into different functions for each of the different object types (database, schema, table, etc) being checked, but I would do that in a separate PR. There we can also look at separating the check for if an object exists vs. its privileges.
Or at least I don't think we need HasAnyPrivilege itself to return ObjectNotFound
I think making ObjectNotFound a return value is more obvious to the caller than unwrapping specific errors that are created multiple layers down then looking for them in handlers like these https://github.com/cockroachdb/cockroach/pull/75726/files#diff-58a8869993736aa2fc4a2e10f1c9ca93190d71b6fc673988d1d92ce9e0fc00efL2316
RichardJCai
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/eval.go, line 3084 at r2 (raw file):
HasAnyPrivilege is only used by builtins so HasAnyPrivilegeResult is only used there.
Ah missed this part, makes sense to me.
pkg/sql/sem/tree/eval.go, line 3084 at r2 (raw file):
const ( // At least one of the specified privileges is granted HasPrivilege HasAnyPrivilegeResult = 1
super nit, end sentences for this the two below with periods.
ecwall
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/eval.go, line 3084 at r2 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
super nit, end sentences for this the two below with periods.
Done.
refs #73129 Also combines some layers of privilege checking code. Release note: None
|
bors r=RichardJCai |
|
Build succeeded: |
refs #73129
Also combines some layers of privilege checking code.
Release note: None