feat: delete the account from the app - #38
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (2)
Limit details: You’ve used all 10 included reviews currently available. 📝 WalkthroughWalkthroughThe iOS app adds an HTTPS cloud client, account reauthentication, and account deletion with keep-or-forget pairing choices. Deletion retries after required reauthentication, then handles local pairings and session state. The account screen presents the deletion flow and reports progress and errors. Release builds configure the cloud URL. Tests cover cloud requests, reauthentication, deletion, and pairing handoff. Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Account deletion is irreversible, but its local cleanup can fail or be interrupted after the cloud succeeds. The app can then leave pairings or a saved session in a state that does not match the apparent deletion outcome. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: 67aee5df-d91c-4b22-8c21-0a92f3765e21
📒 Files selected for processing (11)
apps/ios/Account.xcconfigapps/ios/Origin89/AccountView.swiftapps/ios/Origin89/Info.plistapps/ios/Origin89/Origin89App.swiftapps/ios/Origin89/SetupView.swiftapps/ios/SetupKit/Sources/SetupKit/Account.swiftapps/ios/SetupKit/Sources/SetupKit/AccountDeletion.swiftapps/ios/SetupKit/Sources/SetupKit/AuthKitClient.swiftapps/ios/SetupKit/Sources/SetupKit/CloudClient.swiftapps/ios/SetupKit/Sources/SetupKit/KeychainEnrolmentStore.swiftapps/ios/SetupKit/Tests/SetupKitTests/CloudTests.swift
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| query[kSecMatchLimit as String] = kSecMatchLimitAll | ||
| query[kSecReturnAttributes as String] = true | ||
| var result: CFTypeRef? | ||
| guard SecItemCopyMatching(query as CFDictionary, &result) == errSecSuccess else { return [] } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not report a failed Keychain query as "no enrolments".
deviceIDs() returns [] for every status other than errSecSuccess. That includes errors such as errSecInteractionNotAllowed, not only errSecItemNotFound. KeychainAccountPairings.keep in AccountDeletion.swift (Lines 81-88) goes through this list and then calls own.removeAll(). If the attribute query fails and the delete succeeds, "keep pairings" copies nothing and deletes every pairing of the account. The code then reports .deleted. The code at Lines 82-84 tries to prevent this exact loss for unreadable items, but a failure at this point skips that check.
Make deviceIDs() throw on any status other than success or not-found. removeEveryAccount() already does this.
🐛 Proposed fix
- public func deviceIDs() -> [String] {
+ public func deviceIDs() throws -> [String] {
var query = items()
query[kSecMatchLimit as String] = kSecMatchLimitAll
query[kSecReturnAttributes as String] = true
var result: CFTypeRef?
- guard SecItemCopyMatching(query as CFDictionary, &result) == errSecSuccess else { return [] }
+ let status = SecItemCopyMatching(query as CFDictionary, &result)
+ if status == errSecItemNotFound { return [] }
+ guard status == errSecSuccess else { throw Failure(status: status) }In AccountDeletion.swift, change the loop to for deviceID in try own.deviceIDs().
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
apps/ios/SetupKit/Tests/SetupKitTests/CloudTests.swift-225-225 (1)
225-225: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHold the first cloud response until the account switch completes.
The request count shows that
StubHTTP.sendrecorded the request; it does not show that the deletion task is still waiting. The stub yields once and can then return its 401 response. If deletion starts reauthentication first, it can consume the only authentication response beforesignInuses it. Make the stub wait for an explicit release after the account switch, so this test exercises the intended race reliably. (raw.githubusercontent.com)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: c6af7091-42ca-4d9c-b5f7-7dcd83bfec41
📒 Files selected for processing (3)
apps/ios/SetupKit/Sources/SetupKit/AccountDeletion.swiftapps/ios/SetupKit/Sources/SetupKit/KeychainEnrolmentStore.swiftapps/ios/SetupKit/Tests/SetupKitTests/CloudTests.swift
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
Change
Closes #36. Signed-in people can delete their account from the account sheet, as App Store guideline 5.1.1(v) requires. The app calls
DELETE /v1/accounton the Origin89 cloud. When the cloud answersreauthentication_required(the sign-in is older than 5 minutes), the app opens AuthKit again withmax_age=0andlogin_hint, which starts a new session. It keeps that session only if the same user signed in, then retries once. If another account signs in at that point, or the person switches accounts while the request is out, the app deletes nothing and says so.Nothing on the phone changes until the cloud answers 204. The confirmation asks what happens to the pairings made in the account. Keep moves them to the signed-out scope, where a pairing replaces a signed-out one for the same controller and the account's last controller carries over. Remove deletes them. The app then signs out. A
provider_unavailable502 leaves everything as it was, and a retry finishes.KeychainEnrolmentStore.savenow updates an existing entry in place, so a failed save never loses the older pairing. Keep lists the account's pairings with a throwingstoredDeviceIDs(), so a Keychain that can't be read (a locked phone) stops the move instead of reading as no pairings. If the pairings cannot be moved or removed, the account is still signed out and its pairings stay hidden under the deleted account. Controllers are never touched.CloudClient(SetupKit) sends v1 requests with the account's access token and maps the contract's{"error":{"code","message"}}to a typedCloudError. Link enrolled controllers to a site after sign-in #35 builds its sites and linking calls on it.Account.reauthenticate(using:)handles the fresh sign-in that linking also needs.Account.xcconfigsetsCLOUD_BASE_URL. Release useshttps://cloud.origin89.com, paired with the production WorkOS client. Debug has no cloud until a staging Worker is deployed, since each cloud accepts only its own environment's tokens; in Debug, Delete account is disabled with a note.Validation
just checkpasses: swift-format, Rust fmt/clippy/tests, 196 SetupKit tests, the simulator app build and the bench build.xcodebuild -showBuildSettingsgives Release the production client and cloud URL, and Debug the staging client and no cloud.max_age=0,login_hint) and retries with the new token; another account signing in to confirm, an account switch during the request (checked to fail without the guard), or a cancel, deletes nothing; a 502 changes nothing and the retry succeeds; pairings that cannot be handed over are reported; a Keychain that keeps the session still signs out; signed out cannot delete.Not done: a deletion against the production cloud from a device, and a device check that AuthKit's
max_age=0starts a new session that the cloud's 5-minute check accepts.KeychainAccountPairingshas no unit test; the tests use an in-memory store.