feat: add optional WorkOS sign-in with per-account pairings - #37
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. 📝 WalkthroughWalkthroughThe iOS app adds WorkOS sign-in, account session storage and refresh, and account-specific controller enrolments. The setup flow uses the current account owner and refreshes account state or replaces flow state when the owner changes. A new account screen provides sign-in and sign-out actions. Build settings supply separate staging and production client IDs. Tests cover authentication, token refresh, enrolment storage, and flow resumption. Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Setup remains available while signed out, but users cannot complete WorkOS sign-in or use account-scoped pairings. Register the callback scheme before merging this feature. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Rapid account changes can leave the setup screen using a previous account’s controller pairings. The exposure is local to the app and device, but it undermines the new account-isolation behavior. No broader remote attack path was established. 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.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
apps/ios/SetupKit/Sources/SetupKit/AuthKitClient.swift-152-155 (1)
152-155: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClassify refresh failures by the OAuth
errorfield.
AuthKitClient.authenticatemaps408,invalid_request, andinvalid_clientresponses to.refused.Account.refreshthen callsendSession(), removes the Keychain session, and forces sign-in. WorkOS classifies408as transient. OAuthinvalid_requestandinvalid_clientdo not establish refresh-token revocation.Parse the response body's
errorfield. Return.refusedonly forinvalid_grant. Keep the session for408and other non-terminal failures. Do not treat bodyless401or403as revocation without an explicit WorkOS contract that defines them that way. Add fixtures for these responses and assert that the session remains stored.apps/ios/SetupKit/Sources/SetupKit/Account.swift-224-235 (1)
224-235: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOnly report signed out after Keychain removal succeeds.
When a refused refresh reaches
endSession(), a failedstore.remove()is ignored. The method then clears memory and reports.signedOut, although the Keychain can still contain the revoked session. A later launch loads that session as.signedInand retries it.Propagate the removal error and update the in-memory state only after removal succeeds. Keep the HTTP refusal classification unchanged.
Suggested fix
- if error == .refused { endSession() } + if error == .refused { try endSession() } throw error } } - private func endSession() { + private func endSession() throws(AccountError) { SetupLog.account.notice("WorkOS refused the refresh token: the session ended") - do { - try store.remove() - } catch { - SetupLog.account.error("the ended session could not be removed from the Keychain") - } + try store.remove() generation += 1apps/ios/SetupKit/Sources/SetupKit/Account.swift-206-222 (1)
206-222: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not return a refreshed session when persistence fails.
store.save(refreshed)can fail before removing the old session. The current branch then keepsself.session, reports.signedIn, and returnsrefreshed. Clearing only the in-memory state would not remove the old persisted token, so the next launch could load that spent token as signed in.Retry saving
refreshedfirst. If the retry fails, clear the in-memory session, remove the persisted session, and throw the persistence error. This preserves the rotated token when the retry succeeds and removes the stale token only after persistence has failed again.Suggested fix
- } catch { - // The kept refresh token is spent: the next launch signs in again. - SetupLog.account.error("the refreshed session could not be kept") + } catch let persistenceError { + do { + try store.save(refreshed) + } catch { + self.session = nil + status = .signedOut + try store.remove() + SetupLog.account.error("the refreshed session could not be kept") + throw persistenceError + } } return refreshed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: c126b171-8804-4bd7-bb50-a46abebcd836
📒 Files selected for processing (15)
apps/ios/Account.xcconfigapps/ios/Origin89.xcodeproj/project.pbxprojapps/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/AccountEnrolmentStore.swiftapps/ios/SetupKit/Sources/SetupKit/AuthKitClient.swiftapps/ios/SetupKit/Sources/SetupKit/KeychainAccountSessionStore.swiftapps/ios/SetupKit/Sources/SetupKit/KeychainEnrolmentStore.swiftapps/ios/SetupKit/Sources/SetupKit/SetupLog.swiftapps/ios/SetupKit/Tests/SetupKitTests/AccountEnrolmentTests.swiftapps/ios/SetupKit/Tests/SetupKitTests/AccountTests.swiftapps/ios/Signing.xcconfig
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ios/SetupKit/Tests/SetupKitTests/AccountTests.swift (1)
269-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the expected error for each HTTP response.
The
any Errorassertion only checks thataccessToken()throws. It can miss incorrect error classification. Assert.unavailablefor 408 and 429, and.invalidResponsefor the other listed responses. Keep the session-retention assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: a675d111-b20e-483c-8170-7f6e28db2fb0
📒 Files selected for processing (3)
apps/ios/SetupKit/Sources/SetupKit/Account.swiftapps/ios/SetupKit/Sources/SetupKit/AuthKitClient.swiftapps/ios/SetupKit/Tests/SetupKitTests/AccountTests.swift
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Register the WorkOS callback scheme. · Info.plist:5-6
apps/ios/Origin89/Info.plist:5-6
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRegister the WorkOS callback scheme.
When a configured signed-out user taps Sign in,
AccountViewpassescom.origin89.apps.iostoASWebAuthenticationSession. The app target does not register this scheme inInfo.plistor its build settings. iOS therefore cannot route the WorkOS redirect back to the app, so sign-in cannot continue to the code exchange.🐛 Suggested fix
<dict> + <key>CFBundleURLTypes</key> + <array> + <dict> + <key>CFBundleURLSchemes</key> + <array> + <string>com.origin89.apps.ios</string> + </array> + </dict> + </array> <key>WorkOSClientID</key>
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: 6e9a0c19-2b6c-42d9-9e24-d9205814ecfc
📒 Files selected for processing (1)
apps/ios/SetupKit/Tests/SetupKitTests/AccountTests.swift
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
|
@coderabbitai re Register the WorkOS callback scheme ( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline apps/ios/Origin89/AccountView.swift --items all
rg -n -C4 'ASWebAuthenticationSession|callbackURLScheme|CFBundleURLTypes|WorkOSClientID' apps/ios/Origin89/AccountView.swift apps/ios/Origin89/Info.plist apps/ios/SetupKit/Sources/SetupKit/AuthKitClient.swiftLength of output: 2125 🤖 web_search executed:
Length of output: 5580 🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline apps/ios/SetupKit/Sources/SetupKit/Account.swift --match 'signIn|Account' --view expanded
ast-grep outline apps/ios/SetupKit/Sources/SetupKit/AuthKitClient.swift --match 'AuthKitConfiguration|callbackScheme|redirectURI' --view expanded
rg -n -C5 'callbackScheme|redirectURI|authenticate\(' apps/ios/SetupKit/Sources/SetupKit/Account.swift apps/ios/SetupKit/Sources/SetupKit/AuthKitClient.swiftLength of output: 10630
✏️ Learnings added
You are interacting with an AI system. |
Change
Part of #33. Sign-in is optional and never gates setup. The account button on the setup screen opens AuthKit in
ASWebAuthenticationSession(ephemeral). The app is a public client with PKCE (S256) and checksstate; no secret ships. The redirectcom.origin89.apps.ios://auth/callbackis a constant, independent of the bundle identifier.Account.xcconfigsets the client ID: Debug uses staging, Release uses production.WhenUnlockedThisDeviceOnly, not synchronizable).Account.accessToken()refreshes within 60 s ofexp, runs one refresh at a time because WorkOS rotates the refresh token, and keeps the rotated token. Only aninvalid_grantanswer ends the session, and only once the Keychain no longer holds it; the person is told. A timeout, rate limit, other client error or unreachable WorkOS keeps the session. A refresh that finishes after sign-out is dropped.com.origin89.setup.enrolment/<user id>) and show only while that account is signed in. Sign-out and switching accounts delete or change no enrolment. The app suspends the current flow, closing its connection, and starts one for the new owner, each with its own last controller. Forgetting while signed in removes the account's pairing and the signed-out one for that controller; other accounts' pairings stay.On #33's open question: WorkOS documents Apple sign-in only through the AuthKit web sheet (
provider=authkitorAppleOAuth). There is no way to exchange a token from Apple's native button, so this uses the sheet, which satisfies guideline 4.8.Linking controllers to a site (#35) and in-app account deletion (#36) wait for origin89hq/cloud#4 to deploy. Linking also needs the core to expose the enrolment epoch. Sign-out removes the local session only; it does not revoke the WorkOS session server-side.
Validation
just checkpasses: swift-format, Rust fmt/clippy/tests, 167 SetupKit tests, the simulator app build and the bench build. New tests:state; anerrorredirect; a code WorkOS refuses; unreadable JSON, 503 and 429; a Keychain that refuses the save; a build without a client ID. PKCE is checked against the RFC 7636 appendix B vector.invalid_grant(session ended, Keychain cleared);invalid_grantwith a Keychain that keeps the session (still signed in); 408, 429,invalid_request,invalid_clientand a bare 403 (session kept); offline and 502 (session kept); concurrent callers (one request); sign-out during a refresh.Checked against WorkOS: the authorize URL the app builds, with the staging and production client IDs, redirects to each environment's AuthKit
bootstrap; a wrong redirect goes toredirect-uri-invalid. Not done yet: a sign-in on a device against staging (#33's acceptance). The Keychain stores have no unit tests, like the existingKeychainEnrolmentStore; the tests use in-memory stores in their place.