Fix external access setting persistence#1730
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e736b69cb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static func resolvedExternalAccess(for connection: DatabaseConnection) -> ExternalAccessLevel { | ||
| connection.additionalFields[persistedExternalAccessFieldKey] | ||
| .flatMap(ExternalAccessLevel.init(rawValue:)) | ||
| ?? connection.externalAccess |
There was a problem hiding this comment.
When a user saves a connection as blocked and then reloads it from StoredConnection, the new side-channel value remains only in additionalFields; DatabaseConnection.externalAccess is still the default .readOnly. This helper fixes MCPAuthPolicy, but I checked MCPConnectionBridge.listConnections and MCPTabSnapshotProvider.blockedExternalConnectionIds, and they still read connection.externalAccess directly, so blocked connections can still be listed and their tabs/history exposed to external clients after a restart. Please centralize this resolution or hydrate externalAccess on load so every MCP filter sees the persisted value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for catching this. I centralized the fallback in DatabaseConnection.resolvedExternalAccess and updated all MCP paths that were reading connection.externalAccess directly:
MCPAuthPolicynow usesconn.resolvedExternalAccessMCPConnectionBridge.listConnectionsnow filters blocked connections usingresolvedExternalAccessMCPTabSnapshotProvider.blockedExternalConnectionIdsnow usesresolvedExternalAccess- The advanced pane loads/saves through the same persisted key
I also verified the MCPConnectionBridge.swift diff is now only the intended one-line filter change.
|
I have read the CLA Document and I hereby sign the CLA. |
…ersistence # Conflicts: # CHANGELOG.md # TablePro/Core/Storage/StoredConnection.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d50a2c4793
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| externalAccess = try container.decodeIfPresent( | ||
| String.self, forKey: .externalAccess | ||
| ) ?? ExternalAccessLevel.readOnly.rawValue |
There was a problem hiding this comment.
Fresh evidence in this revision is that the decoder only reads the new top-level externalAccess key before defaulting. For users who already saved this setting in the previous additionalFields["externalAccess"] side channel, the file has no top-level key, so loading it sets DatabaseConnection.externalAccess to .readOnly while the side-channel remains ignored by MCPConnectionBridge and MCPTabSnapshotProvider; a Blocked connection becomes visible after restart. Please fall back to the additional-fields value, or hydrate it in toConnection, before defaulting.
Useful? React with 👍 / 👎.
| try container.encodeIfPresent(groupId, forKey: .groupId) | ||
| try container.encodeIfPresent(sshProfileId, forKey: .sshProfileId) | ||
| try container.encode(safeModeLevel, forKey: .safeModeLevel) | ||
| try container.encode(externalAccess, forKey: .externalAccess) |
There was a problem hiding this comment.
With iCloud sync enabled, this only writes the new setting to the local StoredConnection JSON. ConnectionStorage.add/update marks dirty connections and SyncCoordinator.uploadPendingChanges serializes them through SyncRecordMapper.toCKRecord, but that mapper still writes fields like safeModeLevel and never writes or reads externalAccess, so a connection set to Blocked or Read & Write on one Mac arrives on another as the default Read Only. Please add the field to the CloudKit record mapping alongside this local persistence.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes an issue where the per-connection External Clients access level could revert to read-only after saving and reopening the connection editor.
Users could select Read & Write, save the connection, and then reopen it to find Read Only selected again. This also caused MCP clients to remain read-only, preventing write operations even when the connection was intended to allow them.
Changes
Notes
The model default remains
.readOnlyfor backward compatibility when no persisted value exists.