Repository navigation
[2.0] fix(security): restrict service connection reads to bots and editors - #36265
gh-bot-collate[bot] wants to merge 1 commit into
Conversation
…36219) * fix(security): restrict service connection reads to bots and editors Return a service connection in full only to bots, masked to callers who can edit the service (admins and explicitly delegated operators with EDIT_ALL), and not at all to anyone else. The built-in OpenMetadata service no longer stores the Elasticsearch sink or the ingestion-bot JWT (ingestion builds its own securityConfig on the fly and reindexing reads the search config from the application config); it keeps only host/port/auth provider. The server-connection JWT is masked for non-bot readers, and a migration drops those fields from existing metadata service rows and their version history. The decision is recorded in docs/decisions. * test: align messaging connection visibility with edit permissions (cherry picked from commit 4934556)
gh-bot-collate
Bot
added
the
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } else if (canEditConnection(securityContext, service)) { | ||
| service.getConnection().setConfig(retrieveServiceConnectionConfig(service, true)); | ||
| } else { | ||
| return nullifyConnection(service); |
There was a problem hiding this comment.
decryptOrNullify now returns inside the viewer branch as well as at the end. The repository guide requires one return statement per method, placed at the end. Assign the result of nullifyConnection(service) to service, then use the existing trailing return. This requirement must be satisfied before merging.
| return nullifyConnection(service); | |
| service = nullifyConnection(service); |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| return service; | ||
| } | ||
|
|
||
| private boolean canEditConnection(SecurityContext securityContext, T service) { |
There was a problem hiding this comment.
canEditConnection adds two parameters without final, although neither changes. The repository guide requires final on unchanged parameters. Add it to both parameters and to the new TestNamespace ns parameter in serviceConnection_maskedForEditor_nullForViewer_fullForBot. This requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // An editor (admin holds EDIT_ALL) gets the connection with the password masked, not cleartext. | ||
| final MetadataService asEditor = SdkClients.adminClient().metadataServices().getByName(fqn); | ||
| assertNotNull(asEditor.getConnection()); | ||
| final AmundsenConnection editorConn = | ||
| JsonUtils.convertValue(asEditor.getConnection().getConfig(), AmundsenConnection.class); | ||
| assertEquals(MASKED, editorConn.getPassword()); |
There was a problem hiding this comment.
The editor check uses only an administrator. Administrators bypass normal policy checks, so this test does not cover the main promise of canEditConnection: preserving access for non-admin owners and delegated editors. A change that hides their connections could still pass this test.
Add a non-admin caller with a conditional EDIT_ALL grant and verify a masked connection. Also check that a caller with only field-scoped edits receives no connection.
Knowledge Base Used: Platform security and governance
|
Automated backport of #36219 to
2.0.4934556c77bbfcbe41845b02f5bb7ee4c3e03328Resolution notes
Backport notes: #36219 → 2.0
Source PR:
fix(security): restrict service connection reads to bots and editors(#36219)Merge commit:
4934556c77bbfcbe41845b02f5bb7ee4c3e03328What merged cleanly
git cherry-pickauto-merged the production code and most tests withoutconflict:
ServiceEntityResource.java—decryptOrNullifynow branches on bot vs.EDIT_ALLvs. everyone else, via the newcanEditConnectionhelper.MetadataServiceResource.java— the built-inOpenMetadataservice nolonger stores the Elasticsearch sink or the ingestion-bot JWT.
EntityMasker.java—OpenMetadataJWTClientConfigremoved from the"do not mask" exemption list, so its
jwtTokenis masked for non-botreaders.
2.0.7/postDataMigrationSQLScript.sql)scrubbing the ES sink / bot JWT from existing
OpenMetadataservice rowsand their version history.
docs/decisions/2026-10-26-service-connection-visibility-follows-edit-permission.md(new file).
MessagingServiceResourceIT.java— rewritten assertions (view-only usersnow get
nullconnection instead of a masked one).Conflicts resolved (import-only, both files)
Both unmerged files had only import-block conflicts; the actual test
bodies the PR added had already auto-merged cleanly on top of the
2.0content. In each case I kept the imports the backported test code actually
uses and dropped the
HEAD/mainside imports that belong to tests/codenot present on
2.0:openmetadata-integration-tests/.../MetadataServiceResourceIT.javaKept
org.openmetadata.schema.utils.JsonUtils(used by the two new teststhis PR adds:
serviceConnection_maskedForEditor_nullForViewer_fullForBotand
builtInOpenMetadataService_hasNoEsSinkOrStoredBotToken). Droppedorg.openmetadata.sdk.client.OpenMetadataClient— onmainthat import isneeded by a
putCreateOrUpdatehelper and other tests added by later,unrelated commits that don't exist on
2.0; it would be unused here.openmetadata-service/.../masker/PasswordEntityMaskerTest.javaKept
org.openmetadata.schema.services.connections.metadata.OpenMetadataConnection(used by the new
testOpenMetadataServerJwtTokenIsMaskedAndRestoredtestthis PR adds). Dropped the
microsoftFabric,drive/sftp, andmcpimports — those belong to other connector test methods added on
mainbyunrelated commits and aren't present in this file on
2.0.What a reviewer should double check
mvn compile, since thisenvironment doesn't have the sandbox for a full build) that every API the
ported code references exists on
2.0:Authorizer.getPermission(ctx, user, ResourceContextInterface),EntityResource.getResourceContextById,MetadataOperation.EDIT_ALL,Permission.Access.ALLOW,OpenMetadataConnectionBuilder#withSecurityConfig. Worth a real build/testrun before merging.
2.0.7/postDataMigrationSQLScript.sql, which was anempty placeholder on
2.0— confirm2.0.7is still the correctnext-release migration bucket at merge time.
mvn spotless:apply— the auto-mergeddecryptOrNullifyhas anawkwardly line-wrapped comment that formatting will clean up.
Opened from openmetadata-collate, which holds the credentials needed to open a PR here. It merges itself once this branch's required checks pass — no review needed. If a check fails it stays open and the author of #36219 is asked to look.
resolver run
No runtime blocker was established, but the explicit repository style requirements must be satisfied before merging.
Findings
Summary
Service connections now follow the caller’s edit permission:
The built-in
OpenMetadataservice no longer stores the search sink or bot JWT. Both database migrations remove those fields from current rows and stored versions. JWT masking gains a round-trip test.No concrete runtime regression was established. Two explicit repository style requirements need fixes, and delegated-editor coverage should be added.
Acknowledged by gh-bot-collate[bot]: Java formatting was not run; the PR description requests
mvn spotless:apply.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Service response with a connection] --> B{Bot caller?} B -->|Yes| C[Return decrypted connection] B -->|No| D{EDIT_ALL allowed?} D -->|Yes| E[Return masked connection] D -->|No| F[Return null connection]Reviews (1) · Last reviewed commit: "fix(security): restrict service connecti..." · Reviewed by Greptile