sql/privilege: clean up and increase compatibility of new acl functions#166185
Open
rafiss wants to merge 7 commits intocockroachdb:masterfrom
Open
sql/privilege: clean up and increase compatibility of new acl functions#166185rafiss wants to merge 7 commits intocockroachdb:masterfrom
rafiss wants to merge 7 commits intocockroachdb:masterfrom
Conversation
Replace the hardcoded orderedPrivs list with a sort by Kind value, which corresponds to the privilege bit position. This produces a deterministic ACL character ordering and also fixes a pre-existing bug where `make([]string, len(privileges))` created zero-value entries before appending. Epic: none Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Use ACLCharToPrivName map lookup instead of maintaining a duplicate list of valid ACL characters. Epic: none Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Update the acldefault builtin to return privilege defaults that match
what CockroachDB actually assigns to newly created objects:
- Include admin and root roles with ALL privileges, matching
NewCustomSuperuserPrivilegeDescriptor behavior.
- If the owner is distinct from admin/root, include an owner entry too.
- Adjust privilege chars per object type to match CRDB's defaults
(e.g. tables get "admrtw" not "arwdDxtm").
- Add parameter ('p') defaults with SET and ALTER SYSTEM privileges.
- Do not mark implicit grant options with '*' in the ACL string.
PostgreSQL documents that "the owner's implicit grant options are not
marked in the access privileges display", so we follow suit for
admin, root, and the owner.
Add TestAclDefaultPublicPrivileges to verify that the public role's
privileges from acldefault match the actual defaults on newly created
objects.
Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Contributor
|
Merging to
|
Member
Populate the relacl, nspacl, datacl, proacl, and typacl columns in pg_catalog tables using a new privilegeDescriptorToACLArray helper. Only privileges with a PostgreSQL ACL character equivalent are included; CockroachDB-specific privileges (BACKUP, CHANGEFEED, ZONECONFIG, etc.) are skipped. To match PostgreSQL behavior: - Return NULL when an object's privileges match its defaults, since pg_dump compares ACL columns against acldefault() to decide whether to emit GRANT/REVOKE statements. - Strip grant option '*' markers for admin, root, and the owner, since their grant options are implicit (as in PostgreSQL for the owner/superuser). - Update acldefault() to use a shared DefaultACLItems function, ensuring consistency between ACL column output and acldefault(). Virtual tables and schemas continue to return NULL ACLs. Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Add a new ACLItem struct that encapsulates PostgreSQL-compatible aclitem construction (grantee=privchars/grantor format). This replaces ad-hoc fmt.Sprintf-based ACL string building with a structured type that: - Implements redact.SafeFormatter (role names are PII, privilege chars are safe) - Provides NewACLItem constructor and ParseACLItem parser with round-trip consistency - Moves DefaultACLItems to return []ACLItem instead of []string - Adds IsDefaultACL for order-independent default privilege comparison - Removes ValidateACLItemString (superseded by ParseACLItem) and the now-unused skipACLIdentifier helper Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Update all callers that previously constructed aclitem strings via fmt.Sprintf to use the structured ACLItem type: - NewDACLItemFromDString: use ParseACLItem for validation - acldefault builtin: use DefaultACLItems ([]ACLItem) for descriptor-backed types; use NewACLItem for non-descriptor types (except PARAMETER which needs raw ACL chars for SET/ALTER SYSTEM) - privilegeDescriptorToACLArray: use NewACLItem with ALL expansion and IsDefaultACL for default detection - createDefACLItem: accept username.SQLUsername, use NewACLItem The aclexplode builtin retains manual char iteration because ACL chars 's' (SET) and 'A' (ALTER SYSTEM) have no privilege.Kind equivalent. Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Change `tree.NewDACLItem` to accept a `privilege.ACLItem` instead of a string. This eliminates the string→parse→validate round-trip at each call site where a structured ACLItem is already available. Callers that still need to construct an aclitem datum from a raw string (pgwire decoding, casts, random datum generation, string literal resolution) now use `tree.NewDACLItemFromDString`. Also change `createDefACLItem` in pg_catalog.go to return `privilege.ACLItem` instead of `string`, since all callers immediately pass it to `tree.NewDACLItem`. Release note: None Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review note: Please review each commit individually to make this easy to review.
See the Claude Code plan here.
sql/privilege: sort ACL characters by privilege bit position
Replace the hardcoded orderedPrivs list with a sort by Kind value,
which corresponds to the privilege bit position. This produces a
deterministic ACL character ordering and also fixes a pre-existing
bug where
make([]string, len(privileges))created zero-valueentries before appending.
sql/privilege: remove redundant ValidACLChars constant
Use ACLCharToPrivName map lookup instead of maintaining a duplicate
list of valid ACL characters.
sql: update acldefault to reflect actual default privileges
Update the acldefault builtin to return privilege defaults that match
what CockroachDB actually assigns to newly created objects:
NewCustomSuperuserPrivilegeDescriptor behavior.
(e.g. tables get "admrtw" not "arwdDxtm").
PostgreSQL documents that "the owner's implicit grant options are not
marked in the access privileges display", so we follow suit for
admin, root, and the owner.
Add TestAclDefaultPublicPrivileges to verify that the public role's
privileges from acldefault match the actual defaults on newly created
objects.
sql: populate ACL columns in pg_catalog tables
Populate the relacl, nspacl, datacl, proacl, and typacl columns in
pg_catalog tables using a new privilegeDescriptorToACLArray helper.
Only privileges with a PostgreSQL ACL character equivalent are included;
CockroachDB-specific privileges (BACKUP, CHANGEFEED, ZONECONFIG, etc.)
are skipped.
To match PostgreSQL behavior:
pg_dump compares ACL columns against acldefault() to decide whether
to emit GRANT/REVOKE statements.
since their grant options are implicit (as in PostgreSQL for the
owner/superuser).
ensuring consistency between ACL column output and acldefault().
Virtual tables and schemas continue to return NULL ACLs.
sql/privilege: add ACLItem type for structured aclitem construction
Add a new ACLItem struct that encapsulates PostgreSQL-compatible aclitem
construction (grantee=privchars/grantor format). This replaces ad-hoc
fmt.Sprintf-based ACL string building with a structured type that:
are safe)
round-trip consistency
now-unused skipACLIdentifier helper
sql: migrate aclitem callers to use ACLItem type
Update all callers that previously constructed aclitem strings via
fmt.Sprintf to use the structured ACLItem type:
descriptor-backed types; use NewACLItem for non-descriptor types
(except PARAMETER which needs raw ACL chars for SET/ALTER SYSTEM)
and IsDefaultACL for default detection
The aclexplode builtin retains manual char iteration because ACL chars
's' (SET) and 'A' (ALTER SYSTEM) have no privilege.Kind equivalent.
sql: accept privilege.ACLItem in tree.NewDACLItem
Change
tree.NewDACLItemto accept aprivilege.ACLIteminstead of astring. This eliminates the string→parse→validate round-trip at each
call site where a structured ACLItem is already available.
Callers that still need to construct an aclitem datum from a raw string
(pgwire decoding, casts, random datum generation, string literal
resolution) now use
tree.NewDACLItemFromDString.Also change
createDefACLItemin pg_catalog.go to returnprivilege.ACLIteminstead ofstring, since all callers immediatelypass it to
tree.NewDACLItem.informs #20296