Skip to content

Add keychain types for per-app access groups#25648

Draft
crazytonyli wants to merge 7 commits into
trunkfrom
task/keychain-shared-1-types
Draft

Add keychain types for per-app access groups#25648
crazytonyli wants to merge 7 commits into
trunkfrom
task/keychain-shared-1-types

Conversation

@crazytonyli

@crazytonyli crazytonyli commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Note

First of a three-PR stack, followed by #25649 and #25650. Nothing in this PR is called by app code yet.

Description

The WordPress and Jetpack apps share a single keychain access group, and because it is the only group in the entitlements, every keychain item both apps write is cross-app by accident. This stack moves each app onto its own private access group, keeping the shared group only for the WordPress-to-Jetpack migration contract.

This PR adds the types, with no call sites yet:

  1. AppKeychain scopes keychain access to the app's private group. Reads fall back to the legacy shared group during the transition, writes go to the private group only, and deletes clear both groups so a logout cannot be undone by the fallback read.
  2. SharedKeychain is the explicit window into the legacy shared group. Its doc comment restricts it to the three migration call sites, which the next PRs wire up.
  3. KeychainGroupMigrator performs a one-time, insert-only copy of the shared group into the private group, then sweeps the shared group's leftovers once both apps have recorded a copy at the current migration version.

@dangermattic

dangermattic commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator
2 Warnings
⚠️ Modules/Package.swift was changed without updating its corresponding Package.resolved.

If the change includes adding, removing, or editing a dependency please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running swift package resolve).

If the change to the Package.swift did not modify dependencies, ignoring this warning should be safe, but we recommend double checking and running the package resolution just in case.
.

⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@crazytonyli crazytonyli marked this pull request as draft June 12, 2026 08:31
The suites share KeychainStub's class-level state, and .serialized on an individual suite only orders the tests inside it; without a common serialized ancestor, separate suites race on the stub.
The copy is insert-only: a retried copy after a partial failure must not revert credentials the app wrote after the update. It also skips the counterpart's WP.com token, which would otherwise outlive the counterpart's logout inside this app's private group. The sweep runs only when both apps have recorded a copy at the current migration version; there is deliberately no counterpart-not-installed shortcut, because keychain items survive uninstalls and scheme probing is unreliable outside production builds.
@crazytonyli crazytonyli force-pushed the task/keychain-shared-1-types branch from c0f969d to 9149194 Compare June 12, 2026 08:43
@crazytonyli crazytonyli marked this pull request as ready for review June 12, 2026 08:49
@crazytonyli crazytonyli requested a review from jkmassel June 12, 2026 08:49
@crazytonyli crazytonyli added this to the 27.0 milestone Jun 12, 2026
@wpmobilebot

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number32586
VersionPR #25648
Bundle IDcom.jetpack.alpha
Commit9149194
Installation URL3sm0v0ak744do
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

Copy link
Copy Markdown
Contributor

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

@crazytonyli crazytonyli marked this pull request as draft June 12, 2026 09:32
@wpmobilebot

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number32586
VersionPR #25648
Bundle IDorg.wordpress.alpha
Commit9149194
Installation URL6lgsn3ek7m6go
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@jkmassel jkmassel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early review – there's a potential bug here if only one of the keychain deletions succeeds on logout. The app will still have a keychain entry, so on the next read I think it'll repopulate it?

I think the easiest fix here might just be to write a flag to UserDefaults when we copy from the shared keychain, then never do it again once that flag is written?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants