Repository navigation
support ALTER DEFAULT PRIVILEGES - #2810
Open
jennifersp wants to merge 10 commits into
Open
jennifersp wants to merge 10 commits into
jennifersp wants to merge 10 commits into
Conversation
Contributor
|
@jennifersp DOLT
|
Contributor
|
Ito Test Report ❌17 test cases ran. 3 failed, 3 additional findings, 11 passed. Across 17 test cases, 11 passed and 6 failed: core default-privilege inheritance behaviors were mostly validated end-to-end (including tables, wildcard schemas, sequences, routines, a race scenario, and a non-enumerating unauthorized error path), and one previously unstable privilege-inheritance path now executes successfully in a stabilized environment. The most important confirmed defects are a critical startup crash on malformed auth.db deserialization, a high-severity legacy v1 auth.db role-ID collision issue that can break role lookups, and four medium-severity logic flaws (ALTER DEFAULT PRIVILEGES without FOR ROLE/USER being wrongly denied for non-superusers, in-memory/on-disk ACL drift when persistence fails, CREATE TABLE default ACL persistence asymmetry versus CREATE SEQUENCE, and ALTER INDEX ATTACH PARTITION parsing but always failing due to hardcoded unsupported execution). ❌ Failed (3)
🟠 Default table privileges are not persisted during CREATE TABLE flow
Relevant code:
func (i *createTableDefaultPrivsIter) Next(ctx *sql.Context) (sql.Row, error) {
row, err := i.inner.Next(ctx)
if err == io.EOF && !i.applied {
i.applied = true
var applyErr error
auth.LockWrite(func() {
auth.ApplyDefaultPrivilegesForNewTable(i.ownerID, i.schemaName, i.tableName)
applyErr = auth.PersistChanges()
})
if applyErr != nil {
return nil, applyErr
}
}
return row, err
}
var authErr error
auth.LockWrite(func() {
ownerRole := auth.GetRole(ctx.Client().User)
if ownerRole.IsValid() {
auth.ApplyDefaultPrivilegesForNewSequence(ownerRole.ID(), c.sequence.Id.SchemaName(), c.sequence.Id.SequenceName())
}
authErr = auth.PersistChanges()
})
// ApplyDefaultPrivilegesForNewTable applies any matching default privileges to a newly created table.
// Must be called under LockWrite.
func ApplyDefaultPrivilegesForNewTable(ownerRoleID RoleID, schemaName, tableName string) {
for key, dpv := range globalDatabase.defaultPrivileges.Data {
if key.OwnerRole != ownerRoleID || key.ObjectType != PrivilegeObject_TABLE {
continue
}
if key.Schema != "" && key.Schema != schemaName {
continue
}
for granteeID, granteeValue := range dpv.Grantees {
for _, privilegeMap := range granteeValue.Privileges {
for grantedPriv, withGrantOption := range privilegeMap {🟠 No-FOR-ROLE statement denied before owner fallback resolution
Relevant code:
return vitess.InjectedStatement{
Auth: vitess.AuthInformation{
AuthType: auth.AuthType_CREATE,
TargetType: auth.AuthTargetType_AlterDefaultPrivilegesIdentifiers,
TargetNames: []string{node.TargetRole},
},
Statement: &pgnodes.AlterDefaultPrivileges{
case AuthTargetType_AlterDefaultPrivilegesIdentifiers:
nTargets := len(auth.TargetNames)
if nTargets > 1 {
return errors.Errorf("function identifiers has an unsupported count: %d", len(auth.TargetNames))
}
if state.role.IsSuperUser {
return nil
}
if nTargets == 1 && state.role.Name == auth.TargetNames[0] {
return nil
}
return errors.Errorf("permission denied for %s", auth.TargetNames[0])
func (n *AlterDefaultPrivileges) resolveOwnerRole(ctx *sql.Context) (auth.Role, error) {
// empty means current user
if n.OwnerRole == "" {
userRole := auth.GetRole(ctx.Client().User)
if !userRole.IsValid() {
return auth.Role{}, errors.Errorf(`role "%s" does not exist`, ctx.Client().User)
}
return userRole, nil🟠 Persist error after ALTER leaves in-memory and on-disk ACL drift
Relevant code:
var err error
auth.LockWrite(func() {
err = n.execute(ctx)
if err != nil {
return
}
err = auth.PersistChanges()
})
func PersistChanges() error {
if fileSystem != nil {
return fileSystem.WriteFile(authFileName, globalDatabase.serialize(), 0644)
}
// AddDefaultPrivilege adds a default privilege entry to the global database.
func AddDefaultPrivilege(key DefaultPrivilegeKey, grantee RoleID, privilege GrantedPrivilege, withGrantOption bool) {
dpv, ok := globalDatabase.defaultPrivileges.Data[key]
if !ok {
dpv = DefaultPrivilegeValue{
Key: key,
Grantees: make(map[RoleID]DefaultPrivilegeGranteeValue),
}
}
granteeValue, ok := dpv.Grantees[grantee]
if !ok {
granteeValue = DefaultPrivilegeGranteeValue{
Grantee: grantee,
Privileges: make(map[Privilege]map[GrantedPrivilege]bool),
}
}
privilegeMap, ok := granteeValue.Privileges[privilege.Privilege]
if !ok {✅ Passed (11)
ℹ️ Additional Findings (3)
🟠 ALTER INDEX ATTACH PARTITION cannot execute
Relevant code:
| ALTER INDEX table_index_name ATTACH PARTITION db_object_name
{
$$.val = &tree.AlterIndex{Index: $3.tableIndexName(), Cmd: &tree.AlterIndexAttachPartition{Index: $6.unresolvedObjectName()}}
}
case *tree.AlterIndex:
return nodeAlterIndex(ctx, stmt)
func nodeAlterIndex(ctx *Context, node *tree.AlterIndex) (vitess.Statement, error) {
if node == nil {
return nil, nil
}
return NotYetSupportedError("ALTER INDEX is not yet supported")
}
|
|
Diff SummaryThe run covers default access behavior for newly created tables, sequences, and routines, including owner and schema boundaries, privilege delegation, invalid commands, and persistence across restarts. It also exercises adversarial authorization-data conditions, revealing generally broad coverage but important failures in current-user handling, restart persistence, and damaged-state recovery. Not safe to merge yet — this PR still has high-severity failures affecting authorization after restart and service availability when persisted authorization data is damaged, along with a medium-severity failure in the no-owner privilege form. Separate medium and minor findings appear unrelated to the PR and are flag-for-later caveats, but the attributable high-severity issues are merge blockers. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Revoke grant option is not supported
Evidence Package⚪ Schema and type defaults disappear
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
/testing/go/alter_default_privileges_test.go: some failing tests that…
|
Diff SummaryThe run covers database cursor behavior across normal movement, lifecycle, cleanup, and parameterized access, along with enum defaults, privilege inheritance and authorization boundaries, parsing, and persisted authorization state. It exercises both successful business flows and adversarial or recovery cases, with broad coverage of protocol, catalog, and permission behavior. Merge with caution — two medium-severity issues attributable to this PR remain: parameterized cursor access is broken and configured schema/type defaults are not visible through the catalog, creating functional and administration risks. Separate medium findings in built-in function discovery and malformed authorization-data handling are not attributable to this PR and are caveats rather than merge drivers. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Function lookup misses built-in overloads
Evidence Package🟡 Malformed auth data can crash startup
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
|
Diff SummaryCoverage spans database command parsing and cursor behavior, permission setup and delegation across object types, configuration persistence, concurrency, cleanup, and adversarial recovery cases involving malformed data and routine updates. Happy-path and edge-case behavior was broadly exercised, but routine replacement exposed a serious failure in an unsafe update path. Not safe to merge yet — this PR introduces a high-severity failure in which a malformed routine replacement can discard the working operation and crash the server, making it a merge blocker. Separate medium findings about privilege and catalog behavior are marked unrelated to this PR and should be treated as follow-up caveats rather than merge drivers. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Default privilege mappings are not saved
Evidence Package🟡 New sequence access is not granted
Evidence Package🟡 New routine overloads cannot keep separate default grants
Evidence Package🟡 Function default permissions are not visible after setup
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
| @@ -140,7 +141,9 @@ func (c *CreateFunction) RowIter(ctx *sql.Context, r sql.Row) (sql.RowIter, erro | |||
| return nil, err | |||
| } | |||
| funcID := id.NewFunction(schemaName, c.FunctionName, inputParamTypes...) | |||
There was a problem hiding this comment.
🆕 New Failure: identified in this diff run
Malformed replacement breaks the original routine
What failed: The replacement statement reported success instead of failing cleanly. The next call did not return the original value; it triggered a DoltgresHandler panic, so the original routine was not preserved atomically.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: A malformed routine update can remove the working routine and make later calls crash the database server instead of returning an error. Users and applications that rely on that routine may lose access to the affected operation until it is repaired.
- Steps to Reproduce:
- Create a function that returns 42 and grant a reader role permission to execute it.
- Call the function once and confirm that it returns 42.
- Submit a malformed CREATE OR REPLACE FUNCTION statement that reaches the replacement path.
- Call the routine again and inspect the server response and catalog entry.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR changed server/node/create_function.go at lines 144-150 to compute
replacedand immediately callfuncCollection.DropFunctionbefore the replacement has passed the extension lookup at lines 151-155 or the newfuncCollection.AddFunctioncall at lines 156-171. If either later stage fails, there is no rollback or re-add of the prior function, which is a direct partial-update path. The corresponding procedure implementation has the same ordering at server/node/create_procedure.go lines 125-137, but this test exercised the function path. The captured server stack identifies resultForDefaultIter in server/doltgres_handler.go line 704 and int4out in server/functions/int4.go line 68, whereval.(int32)panics after the malformed replacement produces an invalid value shape. The practical fix is to perform replacement validation and construction before removing the old entry, or retain the old function and restore its definition and privileges on every post-drop error; the existing ACL must also remain attached to the replacement. - Why this is likely a bug: The failure is not limited to a missing test expectation: a malformed update was accepted and a later ordinary call caused an unhandled server panic instead of returning a database error. Existing repository coverage explicitly treats CREATE OR REPLACE as preserving a callable routine and its existing ACL. The PR's drop-before-add ordering creates the partial replacement window, and the recorded stack confirms that the resulting invalid routine data reaches a production type assertion. A targeted transactional or rollback-safe replacement change is sufficient; no broad privilege redesign is needed.
Relevant code
server/node/create_function.go:143-171
funcID := id.NewFunction(schemaName, c.FunctionName, inputParamTypes...)
replaced := c.Replace && funcCollection.HasFunction(ctx, funcID)
if replaced {
if err = funcCollection.DropFunction(ctx, funcID); err != nil {
return nil, err
}
}
if len(c.ExtensionName) > 0 {
if _, err = extensions.GetFunction(c.ExtensionName, c.ExtensionSymbol); err != nil {
return nil, err
}
}
err = funcCollection.AddFunction(ctx, functions.Function{server/doltgres_handler.go:697-705
pan2err := func(err *error) {
if HandlePanics {
if recoveredPanic := recover(); recoveredPanic != nil {
*err = goerrors.Join(*err, errors.Errorf("DoltgresHandler caught panic: %v: %s", recoveredPanic, debug.Stack()))
}
}
}server/functions/int4.go:61-69
var int4out = framework.Function1{
Name: "int4out",
Return: pgtypes.Cstring,
Parameters: [1]*pgtypes.DoltgresType{pgtypes.Int32},
Strict: true,
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
return strconv.FormatInt(int64(val.(int32)), 10), nil
},testing/go/alter_default_privileges_test.go:536-551
Name: "CREATE OR REPLACE FUNCTION preserves existing ACL instead of applying new defaults"
...
{Query: `CREATE OR REPLACE FUNCTION replace_existing() RETURNS INT AS $$ BEGIN RETURN 2; END; $$ LANGUAGE plpgsql;`},
{Query: `SELECT replace_existing();`, Username: "replace_reader", Password: "a", Expected: []sql.Row{{2}}},
{Query: `SELECT replace_existing();`, Username: "replace_new_reader", Password: "a", ExpectedErr: "denied"},Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Malformed replacement breaks the original routine**
**What failed:** The replacement statement reported success instead of failing cleanly. The next call did not return the original value; it triggered a DoltgresHandler panic, so the original routine was not preserved atomically.
- **Impact:** A malformed routine update can remove the working routine and make later calls crash the database server instead of returning an error. Users and applications that rely on that routine may lose access to the affected operation until it is repaired.
- **Steps to reproduce:**
1. Create a function that returns 42 and grant a reader role permission to execute it.
2. Call the function once and confirm that it returns 42.
3. Submit a malformed CREATE OR REPLACE FUNCTION statement that reaches the replacement path.
4. Call the routine again and inspect the server response and catalog entry.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR changed server/node/create_function.go at lines 144-150 to compute `replaced` and immediately call `funcCollection.DropFunction` before the replacement has passed the extension lookup at lines 151-155 or the new `funcCollection.AddFunction` call at lines 156-171. If either later stage fails, there is no rollback or re-add of the prior function, which is a direct partial-update path. The corresponding procedure implementation has the same ordering at server/node/create_procedure.go lines 125-137, but this test exercised the function path. The captured server stack identifies resultForDefaultIter in server/doltgres_handler.go line 704 and int4out in server/functions/int4.go line 68, where `val.(int32)` panics after the malformed replacement produces an invalid value shape. The practical fix is to perform replacement validation and construction before removing the old entry, or retain the old function and restore its definition and privileges on every post-drop error; the existing ACL must also remain attached to the replacement.
- **Why this is likely a bug:** The failure is not limited to a missing test expectation: a malformed update was accepted and a later ordinary call caused an unhandled server panic instead of returning a database error. Existing repository coverage explicitly treats CREATE OR REPLACE as preserving a callable routine and its existing ACL. The PR's drop-before-add ordering creates the partial replacement window, and the recorded stack confirms that the resulting invalid routine data reaches a production type assertion. A targeted transactional or rollback-safe replacement change is sufficient; no broad privilege redesign is needed.
**Relevant code:**
`server/node/create_function.go:143-171`
~~~go
funcID := id.NewFunction(schemaName, c.FunctionName, inputParamTypes...)
replaced := c.Replace && funcCollection.HasFunction(ctx, funcID)
if replaced {
if err = funcCollection.DropFunction(ctx, funcID); err != nil {
return nil, err
}
}
if len(c.ExtensionName) > 0 {
if _, err = extensions.GetFunction(c.ExtensionName, c.ExtensionSymbol); err != nil {
return nil, err
}
}
err = funcCollection.AddFunction(ctx, functions.Function{
~~~
`server/doltgres_handler.go:697-705`
~~~go
pan2err := func(err *error) {
if HandlePanics {
if recoveredPanic := recover(); recoveredPanic != nil {
*err = goerrors.Join(*err, errors.Errorf("DoltgresHandler caught panic: %v: %s", recoveredPanic, debug.Stack()))
}
}
}
~~~
`server/functions/int4.go:61-69`
~~~go
var int4out = framework.Function1{
Name: "int4out",
Return: pgtypes.Cstring,
Parameters: [1]*pgtypes.DoltgresType{pgtypes.Int32},
Strict: true,
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
return strconv.FormatInt(int64(val.(int32)), 10), nil
},
~~~
`testing/go/alter_default_privileges_test.go:536-551`
~~~go
Name: "CREATE OR REPLACE FUNCTION preserves existing ACL instead of applying new defaults"
...
{Query: `CREATE OR REPLACE FUNCTION replace_existing() RETURNS INT AS $$ BEGIN RETURN 2; END; $$ LANGUAGE plpgsql;`},
{Query: `SELECT replace_existing();`, Username: "replace_reader", Password: "a", Expected: []sql.Row{{2}}},
{Query: `SELECT replace_existing();`, Username: "replace_new_reader", Password: "a", ExpectedErr: "denied"},
~~~
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
2 participants
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.
No description provided.