sql: fix excess privileges being created from default privileges.#72323
Conversation
|
This change is |
| // it as the default case where the user has all privileges. | ||
| role := descpb.DefaultPrivilegesRole{Role: user} | ||
| if _, found := d.GetDefaultPrivilegesForRole(role); !found { | ||
| if defaultPrivilegesForRole, found := d.GetDefaultPrivilegesForRole(role); !found { |
There was a problem hiding this comment.
does the comment above need adjusting?
There was a problem hiding this comment.
No this is still accurate but it might be more fitting to put this into the if !found clause? Thoughts?
There was a problem hiding this comment.
yeah i'm in favor of putting it inside of if !found and then one more comment inside of the else
5d1a574 to
110f641
Compare
Release note (bug fix): Previously, when creating an object default privileges from users that were not the user creating the object would be added to the privileges of the object. This fix ensures only the relevant default privileges are applied.
110f641 to
aeb60d0
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Part of me wonders if the thing we were missing here was table-driven unit testing of the functions in catprivilege.
Reviewed 1 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @rafiss, and @RichardJCai)
|
TFTR! |
Yeah we're definitely missing some unit tests here. I'll add some in, this isn't good |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed: |
|
Looks like the test failure is a flake. |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
Release note (bug fix): Previously, when creating an object
default privileges from users that were not the user creating
the object would be added to the privileges of the object.
This fix ensures only the relevant default privileges are applied.
Resolves #72322