[PR #33] [MERGED] feat(auth): ComfyWebAuthPresenter + one-call ComfyAuth.signIn #35

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-2451-signin-orchestrator

📝 Commits (3)

f744342 feat(auth): add ComfyWebAuthPresenter + ComfyAuth.signIn one-call orchestrator (BE-2451)
9fbf3ba fix(auth): address Cursor review — session lifetime, main-actor presenter, empty-token guard
0b35e38 docs(auth): harden the WebAuthPresenter sample against a start() hang

📊 Changes

5 files changed (+588 additions, -4 deletions)

View changed files

📝 README.md (+66 -4)
➕ Sources/ComfySwiftSDK/Public/ComfyAuthSignIn.swift (+96 -0)
📝 Sources/ComfySwiftSDK/Public/ComfyCloudClient.swift (+7 -0)
➕ Sources/ComfySwiftSDK/Public/ComfyWebAuthPresenter.swift (+46 -0)
➕ Tests/ComfySwiftSDKTests/ComfyAuthSignInTests.swift (+373 -0)

📄 Description

ELI-5

Signing into Comfy Cloud is a little dance: open a browser to log in, catch the redirect, swap the code for tokens, save them, and hand back a ready client. Every app was choreographing that dance by hand. This PR turns it into one call — ComfyAuth.signIn(presenter:store:) — while keeping the one step the SDK can't own (popping the browser) app-side through a tiny protocol, so the SDK still never imports AuthenticationServices.

What & why

Adds the one-call interactive sign-in that composes the primitives from the two preceding auth PRs (config + authorize/exchange, and the token-store/restore-client) into the full flow, mirroring Comfy-iOS's OAuthCoordinator.signIn() with the browser step lifted out behind an injected protocol.

New public surface (Foundation-only, DocC'd):

ComfyWebAuthPresenter — a Sendable protocol with authenticate(url:callbackURLScheme:) async throws -> URL. An app conforms a thin adapter over its ASWebAuthenticationSession; conformers throw ComfyError.authCancelled on user dismissal. This is the seam that keeps AuthenticationServices out of the SDK's import boundary.
ComfyAuth.signIn(presenter:store:config:) — build → present → verify state + extract code → exchange → persist → return a ready .oauthRefreshable client. The security-critical callback validation (extractCode(fromCallback:), which enforces the state match) is done by the SDK, never the presenter. .authCancelled, .authStateMismatch, and exchange ComfyErrors propagate unchanged.

Internal:

The returned client is constructed directly from makeRefreshableCredential (the exact path restoreClient uses), seeding the expiry cache from the just-persisted expiresAt — no redundant store re-load, and no restoreClient(...)! force-unwrap (which would crash on a degenerate empty-token response; the direct build instead degrades to .authInvalid on first use).
ComfyCloudClient.credential is exposed internally (not public) so tests can assert the returned client's credential mode.

Testing

swift build clean and the full swift test suite green (212 tests), including ImportBoundaryTests (Foundation-only enforced). New ComfyAuthSignInTests uses a fake ComfyWebAuthPresenter (valid / cancelled / missing-code / tampered-state) + an in-memory ComfyTokenStore, over an injected exchange seam (the real static exchangeAuthorizationCode builds its own URLSession, so it isn't interceptable by the test URL protocol). Asserts: refreshable client returned, tokens persisted, code+verifier+config threaded into the exchange, and every error branch propagates without touching the store.

Notes / judgment calls

Stacked PR. Base is matt/be-2450-token-store-restore-client (#32), which itself stacks on #31. Review shows only this PR's net diff; GitHub retargets to main as the foundations merge.
store.save failures propagate as their raw error (not remapped to .unknown). The spec calls for unchanged propagation of .authCancelled / .authStateMismatch / exchange errors only; surfacing a native persistence error (e.g. a Keychain write failure) at interactive sign-in time — where the caller has full context — read as the better behavior than obscuring it. Easy to change if a reviewer prefers the .unknown wrap that the ongoing-refresh closure uses.
"No secrets logged" holds structurally: the signIn path contains zero log calls, and ComfyStoredTokens already redacts its description/debugDescription (covered by the existing LoggingPrivacy suite).

_{🔄 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/33 **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-2451-signin-orchestrator` --- ### 📝 Commits (3) - [`f744342`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/f74434234cf73ccbc1f3b11fb93d057f2a8402a2) feat(auth): add ComfyWebAuthPresenter + ComfyAuth.signIn one-call orchestrator (BE-2451) - [`9fbf3ba`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/9fbf3ba7edee38f23eabcfc9eebf940ea00e2620) fix(auth): address Cursor review — session lifetime, main-actor presenter, empty-token guard - [`0b35e38`](https://github.com/Comfy-Org/comfy-swift-sdk/commit/0b35e38853e4fe5354a45639f14bd3ab11e4217d) docs(auth): harden the WebAuthPresenter sample against a start() hang ### 📊 Changes **5 files changed** (+588 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `README.md` (+66 -4) ➕ `Sources/ComfySwiftSDK/Public/ComfyAuthSignIn.swift` (+96 -0) 📝 `Sources/ComfySwiftSDK/Public/ComfyCloudClient.swift` (+7 -0) ➕ `Sources/ComfySwiftSDK/Public/ComfyWebAuthPresenter.swift` (+46 -0) ➕ `Tests/ComfySwiftSDKTests/ComfyAuthSignInTests.swift` (+373 -0) </details> ### 📄 Description ## ELI-5 Signing into Comfy Cloud is a little dance: open a browser to log in, catch the redirect, swap the code for tokens, save them, and hand back a ready client. Every app was choreographing that dance by hand. This PR turns it into **one call** — `ComfyAuth.signIn(presenter:store:)` — while keeping the one step the SDK can't own (popping the browser) app-side through a tiny protocol, so the SDK still never imports `AuthenticationServices`. ## What & why Adds the one-call interactive sign-in that composes the primitives from the two preceding auth PRs (config + authorize/exchange, and the token-store/restore-client) into the full flow, mirroring Comfy-iOS's `OAuthCoordinator.signIn()` with the browser step lifted out behind an injected protocol. New public surface (Foundation-only, DocC'd): - **`ComfyWebAuthPresenter`** — a `Sendable` protocol with `authenticate(url:callbackURLScheme:) async throws -> URL`. An app conforms a thin adapter over its `ASWebAuthenticationSession`; conformers throw `ComfyError.authCancelled` on user dismissal. This is the seam that keeps `AuthenticationServices` out of the SDK's import boundary. - **`ComfyAuth.signIn(presenter:store:config:)`** — build → present → verify `state` + extract code → exchange → persist → return a ready `.oauthRefreshable` client. The security-critical callback validation (`extractCode(fromCallback:)`, which enforces the `state` match) is done by the SDK, never the presenter. `.authCancelled`, `.authStateMismatch`, and exchange `ComfyError`s propagate unchanged. Internal: - The returned client is constructed directly from `makeRefreshableCredential` (the exact path `restoreClient` uses), seeding the expiry cache from the just-persisted `expiresAt` — no redundant store re-load, and no `restoreClient(...)!` force-unwrap (which would crash on a degenerate empty-token response; the direct build instead degrades to `.authInvalid` on first use). - `ComfyCloudClient.credential` is exposed **internally** (not public) so tests can assert the returned client's credential mode. ## Testing `swift build` clean and the full `swift test` suite green (212 tests), including `ImportBoundaryTests` (Foundation-only enforced). New `ComfyAuthSignInTests` uses a fake `ComfyWebAuthPresenter` (valid / cancelled / missing-code / tampered-state) + an in-memory `ComfyTokenStore`, over an injected exchange seam (the real static `exchangeAuthorizationCode` builds its own `URLSession`, so it isn't interceptable by the test URL protocol). Asserts: refreshable client returned, tokens persisted, code+verifier+config threaded into the exchange, and every error branch propagates without touching the store. ## Notes / judgment calls - **Stacked PR.** Base is `matt/be-2450-token-store-restore-client` (#32), which itself stacks on #31. Review shows only this PR's net diff; GitHub retargets to `main` as the foundations merge. - **`store.save` failures propagate as their raw error** (not remapped to `.unknown`). The spec calls for unchanged propagation of `.authCancelled` / `.authStateMismatch` / exchange errors only; surfacing a native persistence error (e.g. a Keychain write failure) at interactive sign-in time — where the caller has full context — read as the better behavior than obscuring it. Easy to change if a reviewer prefers the `.unknown` wrap that the ongoing-refresh closure uses. - **"No secrets logged"** holds structurally: the `signIn` path contains zero log calls, and `ComfyStoredTokens` already redacts its `description`/`debugDescription` (covered by the existing `LoggingPrivacy` suite). --- <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:11 +00:00