Require UPDATE_SCHEMA permission on all schema mutators#4423
Conversation
Extends the authorization check previously applied to createProperty to every public schema-mutating method, closing an incorrect-authorization gap where a non-admin identity could still alter the schema. - LocalDocumentType: rename, dropProperty, addSuperType, removeSuperType, setSuperTypes, setAliases, addBucket, removeBucket now check UPDATE_SCHEMA via a shared checkForSchemaMutation() helper. - LocalProperty: setDefaultValue, setOfType, setReadonly, setMandatory, setNotNull, setHidden, setExternal, setCompression, setMax, setMin, setRegexp, setCustomValue now check UPDATE_SCHEMA. The check is a no-op in embedded mode and in system contexts with no bound user (schema load at startup, HA replication apply), so internal paths are unaffected. Adds a server integration regression test.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces centralized schema mutation authorization checks (checkForSchemaMutation()) in LocalDocumentType and LocalProperty to enforce the UPDATE_SCHEMA permission, addressing an incomplete fix for schema mutation access. It also adds a regression integration test to verify these restrictions. The reviewer feedback points out that some schema-mutating methods in LocalDocumentType (setBucketSelectionStrategy and setCustomValue) are still missing these checks, and suggests expanding the integration tests to cover them.
| protected void checkForSchemaMutation() { | ||
| ((DatabaseInternal) schema.getDatabase()).checkPermissionsOnDatabase(SecurityDatabaseUser.DATABASE_ACCESS.UPDATE_SCHEMA); | ||
| } |
There was a problem hiding this comment.
While checkForSchemaMutation() is a great addition to centralize permission checks, there are other public schema-mutating methods in LocalDocumentType that are currently missing this check:
setBucketSelectionStrategy(final BucketSelectionStrategy selectionStrategy)(and its overload)setCustomValue(final String key, final Object value)
Without gating these methods, a non-administrator identity could still alter the database schema's bucket selection strategy or custom metadata. Please add checkForSchemaMutation(); at the beginning of these methods as well.
| assertThat(command(serverIndex, token, "DROP PROPERTY Memory.salience")) | ||
| .as("read-only token must not DROP PROPERTY").isEqualTo(403); | ||
| assertThat(command(serverIndex, token, "ALTER TYPE Memory SUPERTYPE +Persisted")) | ||
| .as("read-only token must not add a SUPERTYPE").isEqualTo(403); | ||
| assertThat(command(serverIndex, token, "ALTER TYPE Memory SUPERTYPE -Persisted")) | ||
| .as("read-only token must not remove a SUPERTYPE").isEqualTo(403); | ||
| assertThat(command(serverIndex, token, "ALTER TYPE Memory NAME MemoryRenamed")) | ||
| .as("read-only token must not rename a type").isEqualTo(403); | ||
| assertThat(command(serverIndex, token, "ALTER PROPERTY Memory.salience MANDATORY true")) | ||
| .as("read-only token must not alter a property constraint").isEqualTo(403); |
There was a problem hiding this comment.
Please add test cases for ALTER TYPE ... BUCKETSELECTIONSTRATEGY and ALTER TYPE ... CUSTOM to ensure complete coverage of all schema mutators once those methods are gated with checkForSchemaMutation() in LocalDocumentType.
assertThat(command(serverIndex, token, "DROP PROPERTY Memory.salience"))
.as("read-only token must not DROP PROPERTY").isEqualTo(403);
assertThat(command(serverIndex, token, "ALTER TYPE Memory SUPERTYPE +Persisted"))
.as("read-only token must not add a SUPERTYPE").isEqualTo(403);
assertThat(command(serverIndex, token, "ALTER TYPE Memory SUPERTYPE -Persisted"))
.as("read-only token must not remove a SUPERTYPE").isEqualTo(403);
assertThat(command(serverIndex, token, "ALTER TYPE Memory NAME MemoryRenamed"))
.as("read-only token must not rename a type").isEqualTo(403);
assertThat(command(serverIndex, token, "ALTER PROPERTY Memory.salience MANDATORY true"))
.as("read-only token must not alter a property constraint").isEqualTo(403);
assertThat(command(serverIndex, token, "ALTER TYPE Memory BUCKETSELECTIONSTRATEGY partitioned(salience)"))
.as("read-only token must not alter bucket selection strategy").isEqualTo(403);
assertThat(command(serverIndex, token, "ALTER TYPE Memory CUSTOM description='test'"))
.as("read-only token must not alter custom metadata").isEqualTo(403);
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
🟢 Coverage 100.00% diff coverage
Metric Results Coverage variation Report missing for bdc414c1 Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (bdc414c) Report Missing Report Missing Report Missing Head commit (fb7755a) 159976 105856 66.17% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4423) 26 26 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Code ReviewOverviewThis PR closes a real authorization gap: Strengths
Issues / Suggestions1. Redundant permission checks in
This is currently harmless - the check is a cheap thread-local lookup and batch sizes are always tiny. But the PR description claims the two delegates are the chokepoints while 2. The method is declared 3. Test does not clean up types between test methods (low risk)
4. Test covers only one of the 12 Only 5. CVE reference in test Javadoc The class-level Javadoc cites "CVE-2026-44221" (a year-2026 identifier). If this is an internal tracking ID rather than a published CVE, removing the CVE prefix or adding a clarifying note would avoid confusion. 6. In SummaryThe security fix is correct, minimal, and well-tested. The items above are all minor quality notes - none block merging. The most actionable one is tightening |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4423 +/- ##
============================================
+ Coverage 64.45% 64.49% +0.03%
Complexity 431 431
============================================
Files 1648 1649 +1
Lines 128194 128270 +76
Branches 27477 27507 +30
============================================
+ Hits 82630 82729 +99
+ Misses 33948 33927 -21
+ Partials 11616 11614 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
Extends the
UPDATE_SCHEMAauthorization check (previously applied only tocreateProperty) to every public schema-mutating method, closing an incorrect-authorization gap where a non-administrator identity could still alter the database schema.LocalDocumentType- added a sharedcheckForSchemaMutation()helper and gated:rename,dropProperty,addSuperType(chokepoint covering all overloads),removeSuperType(chokepoint),setSuperTypes,setAliases,addBucket,removeBucket. The existingcreatePropertycheck now uses the same helper.LocalProperty- gated all 12 setters:setDefaultValue,setOfType,setReadonly,setMandatory,setNotNull,setHidden,setExternal,setCompression,setMax,setMin,setRegexp,setCustomValue.Safety of internal paths
The check is a no-op in embedded mode and in system contexts with no bound user. Schema reload (
DocumentType.createProperty(String, JSONObject)) calls the already-gatedcreatePropertyplus everyLocalPropertysetter, and has shipped that way since 26.4.2 without breaking reload - so reload and HA replication apply run in a no-current-user context where the new checks are no-ops.dropTypeandTypeBuilderalready checkUPDATE_SCHEMAat entry, so their internal mutator calls are harmless redundant re-checks.Tests
SchemaMutationAuthorizationIT(server): a read-only API token gets HTTP 403 onDROP PROPERTY,ALTER TYPE SUPERTYPE +/-,ALTER TYPE NAME, andALTER PROPERTY MANDATORY, while an administrator still succeeds (positive control).CrossDatabaseAccessIT, HARaftReplicationChangeSchemaIT(replication intact), OpenCypher label tests.