sql: fix schema privileges on descriptor read #65750
Conversation
|
This change is |
|
|
||
| validPrivs := privilege.GetValidPrivilegesForObject(privilege.Schema).ToBitField() | ||
|
|
||
| for i := range p.Users { |
There was a problem hiding this comment.
I couldn't think of a way to differentiate if this issue came specifically from ALTER DATABASE ... CONVERT TO SCHEMA or some other way.
I wonder if we should always just remove invalid privileges?
There was a problem hiding this comment.
Maybe annoying but can you write a more integration-level test where we go and write the corruption into a schema descriptor on disk and make sure everything is still happy? You don't need to backport that test to 20.2 if it's painful.
Reviewed 1 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)
pkg/sql/catalog/descpb/privilege.go, line 266 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
I couldn't think of a way to differentiate if this issue came specifically from
ALTER DATABASE ... CONVERT TO SCHEMAor some other way.I wonder if we should always just remove invalid privileges?
I think it's fine to always remove them. Given we don't support the idea of pseudo-default privileges on schemas, we're stuck with just throwing these privileges away.
pkg/sql/catalog/descpb/privilege.go, line 269 at r1 (raw file):
// Users is a slice of values, we need pointers to make them mutable. userPrivileges := &p.Users[i] userPrivileges.Privileges &= validPrivs
If you're going to write out this whole sentence explaining why you're taking the address, might as well just do:
p.Users[i].Privileges &= validPrivs
There was a problem hiding this comment.
What's the best way to do this integration-level test? I'm thinking we can do a restore test or a versionupgrade roachtest.
Is there an alternative / better way than these two?
Reviewable status:
There was a problem hiding this comment.
I was thinking something much much simpler. Just create a schema descriptor, then in a descs.Txn give it the bogus privileges that would have resulted from the relevant bug and then make sure you can still access stuff in the schema. Shouldn't be a very big test.
Reviewable status:
RichardJCai
force-pushed
the
fix_schema_privilege_05262021
branch
from
aabf082 to
b8c4d4f
Compare
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 3 of 3 files at r2.
Reviewable status:
Release note (bug fix): Previously a schema's privilege descriptor could become corrupted upon executing ALTER DATABASE ... CONVERT TO SCHEMA due to privileges that are invalid on a schema being copied over to the schema rendering the schema inusable due to invalid privileges.
RichardJCai
force-pushed
the
fix_schema_privilege_05262021
branch
from
b8c4d4f to
6369e54
Compare
| @@ -97,10 +97,14 @@ func TestExamineDescriptors(t *testing.T) { | |||
| tbl.State = descpb.DescriptorState_DROP | |||
| } | |||
|
|
|||
| inSchemaValidTableDesc := protoutil.Clone(validTableDesc).(*descpb.Descriptor) | |||
| // Use 51 as the Schema ID, we do not want to use a reserved system ID (1-49) | |||
There was a problem hiding this comment.
Had to change schema ids to 51 so it wouldn't be checked for system privileges.
Our SystemAllowedPrivileges check is a bit weird since it doesn't check object type just id. I don't think we have system schemas or system types in this context.
|
Thanks for the review! bors r=ajwerner |
|
Build succeeded: |
Release note (bug fix): Previously a schema's privilege descriptor
could become corrupted upon executing ALTER DATABASE ...
CONVERT TO SCHEMA due to privileges that are invalid on a schema
being copied over to the schema rendering the schema inusable due
to invalid privileges.
Fixes #65697