[PR #34] [MERGED] feat(auth): add ComfyAuthKit — default ASWebAuth presenter + Keychain token store (BE-2452) #38

New issue

Closed

opened 2026-10-26 22:34:14 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:34:14 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-swift-sdk/pull/34
Author: @mattmillerai
Created: 7/7/2026
Status: ✅ Merged
Merged: 7/7/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-2452-comfyauthkit-defaults

📝 Commits (1)

a02216c feat(auth): add ComfyAuthKit — default ASWebAuth presenter + Keychain token store (BE-2452)

📊 Changes

6 files changed (+727 additions, -2 deletions)

View changed files

📝 Package.swift (+11 -2)
📝 README.md (+35 -0)
➕ Sources/ComfyAuthKit/ASWebAuthPresenter.swift (+153 -0)
➕ Sources/ComfyAuthKit/KeychainTokenStore.swift (+269 -0)
➕ Tests/ComfyAuthKitTests/ASWebAuthPresenterTests.swift (+97 -0)
➕ Tests/ComfyAuthKitTests/KeychainTokenStoreTests.swift (+162 -0)

📄 Description

ELI-5

The SDK on its own knows the whole "Sign in with Comfy" dance except two app-shaped chores: popping the browser sheet and stashing the tokens in the Keychain. Today an app has to hand-write both. This adds a small second package, ComfyAuthKit, that ships the obvious defaults for those two chores — a ready-made browser presenter and a Keychain token store — so an app can sign in with one call instead of a page of boilerplate. Apps that want full control still just use ComfySwiftSDK and plug in their own. The defaults live in a separate target so they can import AuthenticationServices/Security without breaking the core SDK's Foundation-only rule.

What & why

Implements BE-2452. Two-tier auth story:

ComfySwiftSDK (core) — Foundation-only, inject your own ComfyWebAuthPresenter / ComfyTokenStore.
ComfyAuthKit (new) — batteries-included defaults, allowed to import AuthenticationServices + Security.

Changes

Package.swift — add .library/.target ComfyAuthKit (depends on ComfySwiftSDK) + ComfyAuthKitTests. Platforms unchanged (iOS 17+/macOS 14+).
Sources/ComfyAuthKit/ASWebAuthPresenter.swift — @MainActor public final class ASWebAuthPresenter: NSObject, ComfyWebAuthPresenter, ASWebAuthenticationPresentationContextProviding. Ports the present-and-retain + cancellation mapping from Comfy-iOS OAuthCoordinator, narrowed to the ComfyWebAuthPresenter.authenticate(url:callbackURLScheme:) contract (returns the callback URL unchanged; the SDK does state verification/code extraction). prefersEphemeralWebBrowserSession = false; .canceledLogin → ComfyError.authCancelled; optional anchor: override, else ASPresentationAnchor() (iOS 17+ auto-locates the key window). Cancellation mapping is extracted to a nonisolated static mapCallback(url:error:) so it's unit-testable without a real session.
Sources/ComfyAuthKit/KeychainTokenStore.swift — public struct KeychainTokenStore: ComfyTokenStore. Ports the 3 OAuth slots + SecItem* helpers from Comfy-iOS KeychainStore, using kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly (background reattach reads while locked). Configurable service for test isolation; load()/save()/clear() map to the slots; never logs tokens; load() is all-or-nothing (a partial triple → nil).
Tests — presenter conformance/compile + cancellation-mapping matrix; KeychainTokenStore round-trip under a UUID-keyed service.
README.md — documents the two tiers.

Stacked PR

Based on matt/be-2451-signin-orchestrator (#33), which itself stacks on #32 → #31. Those three children define the ComfyWebAuthPresenter / ComfyTokenStore / ComfyStoredTokens protocols and ComfyError.authCancelled that this PR builds on. Per the always-stack policy I built on the stack tip rather than waiting. Review/merge order: #31 → #32 → #33 → this. GitHub will retarget this PR to main as the children merge.

Testing

swift build + swift test green: 224 tests, 0 failures. The untouched ImportBoundaryTests still passes — confirming ComfyAuthKit's AuthenticationServices/Security imports do not trip the core-target scan (the whole point of the separate product).

Judgment calls

Keychain round-trip tests are gated .enabled(if: keychainAvailable) — a headless/unsigned test host (e.g. a plain swift test with no usable login keychain) can't add generic-password items, so those tests skip rather than fail; the conformance/compile + value-isolation coverage runs everywhere, and the round-trip runs for real on a provisioned host (simulator / CI with an unlocked keychain). This is what keeps CI green without weakening the store.
"Configurable service/account" — implemented as a configurable service. The three OAuth slots use fixed account keys (namespaced constants), so a UUID-keyed service is the isolation seam; a separate per-instance account would be redundant for a 3-slot store and isn't ported from the reference (that param was for the legacy API-key slot, which this store does not include).
Error-takes-precedence in mapCallback — when a callback arrives with both a URL and an error, the error wins (matches the reference OAuthCoordinator), so a failed session can't be mistaken for success. Documented with a test.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-swift-sdk/pull/34 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/7/2026 **Status:** ✅ Merged **Merged:** 7/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-2452-comfyauthkit-defaults` --- ### 📝 Commits (1) - [`a02216c`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/a02216c5aecc856164237ce589264900c7f25080) feat(auth): add ComfyAuthKit — default ASWebAuth presenter + Keychain token store (BE-2452) ### 📊 Changes **6 files changed** (+727 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `Package.swift` (+11 -2) 📝 `README.md` (+35 -0) ➕ `Sources/ComfyAuthKit/ASWebAuthPresenter.swift` (+153 -0) ➕ `Sources/ComfyAuthKit/KeychainTokenStore.swift` (+269 -0) ➕ `Tests/ComfyAuthKitTests/ASWebAuthPresenterTests.swift` (+97 -0) ➕ `Tests/ComfyAuthKitTests/KeychainTokenStoreTests.swift` (+162 -0) </details> ### 📄 Description ## ELI-5 The SDK on its own knows the whole "Sign in with Comfy" dance *except* two app-shaped chores: popping the browser sheet and stashing the tokens in the Keychain. Today an app has to hand-write both. This adds a small second package, **`ComfyAuthKit`**, that ships the obvious defaults for those two chores — a ready-made browser presenter and a Keychain token store — so an app can sign in with one call instead of a page of boilerplate. Apps that want full control still just use `ComfySwiftSDK` and plug in their own. The defaults live in a separate target so they can `import AuthenticationServices`/`Security` without breaking the core SDK's Foundation-only rule. ## What & why Implements **BE-2452**. Two-tier auth story: - **`ComfySwiftSDK`** (core) — Foundation-only, inject your own `ComfyWebAuthPresenter` / `ComfyTokenStore`. - **`ComfyAuthKit`** (new) — batteries-included defaults, allowed to import `AuthenticationServices` + `Security`. ### Changes - **`Package.swift`** — add `.library`/`.target` `ComfyAuthKit` (depends on `ComfySwiftSDK`) + `ComfyAuthKitTests`. Platforms unchanged (iOS 17+/macOS 14+). - **`Sources/ComfyAuthKit/ASWebAuthPresenter.swift`** — `@MainActor public final class ASWebAuthPresenter: NSObject, ComfyWebAuthPresenter, ASWebAuthenticationPresentationContextProviding`. Ports the present-and-retain + cancellation mapping from Comfy-iOS `OAuthCoordinator`, narrowed to the `ComfyWebAuthPresenter.authenticate(url:callbackURLScheme:)` contract (returns the callback URL unchanged; the SDK does `state` verification/code extraction). `prefersEphemeralWebBrowserSession = false`; `.canceledLogin` → `ComfyError.authCancelled`; optional `anchor:` override, else `ASPresentationAnchor()` (iOS 17+ auto-locates the key window). Cancellation mapping is extracted to a `nonisolated static mapCallback(url:error:)` so it's unit-testable without a real session. - **`Sources/ComfyAuthKit/KeychainTokenStore.swift`** — `public struct KeychainTokenStore: ComfyTokenStore`. Ports the 3 OAuth slots + `SecItem*` helpers from Comfy-iOS `KeychainStore`, using `kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly` (background reattach reads while locked). Configurable `service` for test isolation; `load()`/`save()`/`clear()` map to the slots; never logs tokens; `load()` is all-or-nothing (a partial triple → `nil`). - **Tests** — presenter conformance/compile + cancellation-mapping matrix; `KeychainTokenStore` round-trip under a UUID-keyed service. - **`README.md`** — documents the two tiers. ## Stacked PR Based on **`matt/be-2451-signin-orchestrator`** (#33), which itself stacks on #32 → #31. Those three children define the `ComfyWebAuthPresenter` / `ComfyTokenStore` / `ComfyStoredTokens` protocols and `ComfyError.authCancelled` that this PR builds on. Per the always-stack policy I built on the stack tip rather than waiting. **Review/merge order: #31 → #32 → #33 → this.** GitHub will retarget this PR to `main` as the children merge. ## Testing `swift build` + `swift test` green: **224 tests, 0 failures.** The untouched `ImportBoundaryTests` still passes — confirming ComfyAuthKit's `AuthenticationServices`/`Security` imports do **not** trip the core-target scan (the whole point of the separate product). ## Judgment calls - **Keychain round-trip tests are gated `.enabled(if: keychainAvailable)`** — a headless/unsigned test host (e.g. a plain `swift test` with no usable login keychain) can't add generic-password items, so those tests **skip** rather than fail; the conformance/compile + value-isolation coverage runs everywhere, and the round-trip runs for real on a provisioned host (simulator / CI with an unlocked keychain). This is what keeps CI green without weakening the store. - **"Configurable service/account"** — implemented as a configurable `service`. The three OAuth slots use fixed account keys (namespaced constants), so a UUID-keyed `service` is the isolation seam; a separate per-instance `account` would be redundant for a 3-slot store and isn't ported from the reference (that param was for the legacy API-key slot, which this store does not include). - **Error-takes-precedence in `mapCallback`** — when a callback arrives with both a URL and an error, the error wins (matches the reference `OAuthCoordinator`), so a failed session can't be mistaken for success. Documented with a test. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:34:14 +00:00