Repository navigation
Release 1.0.0 - #9
Merged
Merged
Conversation
Four files each carried their own Duration-to-seconds or Duration-to-milliseconds helper, and DeviceAuthorization converted with Double(components.seconds), which drops the fractional part of the polling limit and of a fractional expires_in. One package-level pair in Core replaces them all. The three HTTP-date formats get names and examples so the parser reads as RFC 9110 §5.6.7, with the reason Date.HTTPFormatStyle is not enough.
Six call sites tested 2xx with a hand-written range and three spelled out "429 or 5xx". HTTPResponse.Status already knows its kind, so 2xx becomes kind == .successful and the transient check becomes one isTransient property that cites RFC 6585 §4 and RFC 9110 §15.6. Behaviour is unchanged.
The documentation promised that Authorization is never indexed in HPACK or QPACK. The library does mark the field, but URLSessionTransport converts the request to URLRequest, which has no indexing strategy, so the marking never reaches URLSession. The docs now say the marking survives only through a transport that keeps the indexing strategy of HTTPFields. Keeping the marking is still right for such custom transports.
RFC 9110 §11.2 allows optional whitespace around "=" in an auth-param, and no corpus row exercised it. The decision table also lacked the case where insufficient_scope comes from a DPoP challenge next to a bare Bearer one: the Bearer challenge carries no error, so the request retries once like any bare Bearer 401.
Only invalid_grant may end a session, but no test failed a refresh with a 503, a 429 or a timeout and checked what is left. The new test checks for each that the error says retry later, no session-ending event is emitted, the stored refresh token is unchanged, and the next call refreshes again.
An app that obtains a refresh token and never adopts it as a session still owes the server a revocation, but the throwing revoke has no time limit, so apps wrapped it in their own timer and copied the five seconds the token manager uses. The new overload runs the request under the internal time limit on the client's injected clock and reports the outcome as SignOutResult.Revocation, the type sign-out already returns. The token manager's own revocation now uses it, and the five seconds become OAuthClient.defaultRevocationTimeLimit. The constant is public because the overload is selected by its timeLimit label: a call without it resolves to the throwing variant, so a default value for the limit could never be used.
signOut(revoke:) returned only after the store delete and the revocation finished. Apps that needed the moment the session was gone, for example to sign in again at once, started signOut in a task and waited for the .signedOut event, which works only while sign-outs never overlap and relies on when the manager first suspends. beginSignOut(revoke:) does the part of sign-out before that suspension, synchronously on the actor, and returns a PendingSignOut whose result() awaits the delete and the revocation. Every ordering guarantee of sign-out holds at that point: the event is emitted, the revocation has its turn on the ended lane and the delete is queued ahead of any later save. signOut is now beginSignOut followed by result(). The revocation's time limit and log line move into a task the manager owns, so they no longer depend on who waits; a cancelled waiter still gets .cancelled. ADR 0014 records the decision.
SwiftKeychain replaces an existing item by deleting it and adding it again. A failure between the two steps (device locked, denied prompt) or a process that dies there loses the stored credential, and with refresh token rotation the replaced token is already invalid, so the user has to sign in again. Stores racing on one item also failed with errSecDuplicateItem. saveInKeychain(_:) accepts any KeychainServiceType, so the store now passes its own: insert calls SecItemUpdate with only kSecValueData to change, adds only on errSecItemNotFound, and updates again when another store added the item in between. A missing payload, which SwiftKeychain produces when archiving fails, is refused instead of stored. No upstream change is needed; ADR 0009 records the resolution and the race test of eight stores is a regular test again.
The shared Duration conversion keeps fractional seconds, so a device authorization with a fractional expires_in no longer has its expiry truncated. That is visible through DeviceAuthorization.expiresAt.
Building HTTPResponse.Status from the statusCode of a PassportError made the public initializers trap for numbers such as -1 or 1000, because Status(code:) has a precondition on its range. Classification now goes through an Int helper that treats such numbers as not transient, as the hand-written check did before.
The update query of the save carried kSecAttrAccessible, as SwiftKeychain's fetch query does. The data protection keychain matches queries on it, so after an app changed its accessibility class every save failed with errSecItemNotFound followed by a duplicate add, and load returned nil. Every Keychain call of the store now matches the item by class, service, account, synchronizable and access group, and a save changes both the value and the accessibility class. Without an access group the query names none, so on iOS it matches the item in any group the app can reach. Restricting it to the default group would need a probe write to learn that group's name, so the initializer documents the behaviour instead.
When exchangeRefreshToken was superseded after the server answered, the manager discarded the response like that of a refresh. The issued token is for another audience and was never session state, so it was lost while it stayed valid at the server, and the manager cannot revoke it: RFC 7009 §2.1 lets only the client it was issued to do that, and that client need not be part of this configuration. The response is now returned. Its rotated subject is not persisted and still goes to the ended session's revocation, so the current session is untouched. An exchange superseded before sending, and a failure that arrives late, still throw notAuthenticated. ADR 0015 records it and amends ADR 0007.
Before the API is frozen: the revocation outcome is shared by sign-out and the best-effort revoke, so it moves out of SignOutResult to the top-level RevocationOutcome. This breaks code that names SignOutResult.Revocation. The best-effort revoke gets a name of its own, revokeBestEffort(_:typeHint:timeLimit:), which lets its limit default to OAuthClient.revocationTimeLimit (the limit the token manager applies on sign-out) without being shadowed by the throwing revoke(_:typeHint:). PendingSignOut.result becomes an async property shaped like Task.value; per-reader cancellation is unchanged. The new tests check that a timeout or a cancellation of the best-effort revoke also cancels the request.
A cancelled reader of PendingSignOut.result must not change what the other readers get, and the revocation outcome must be logged when nobody reads the result at all; neither was tested. The suites of the testing module and the URLSession transport await clocks or sockets but had no time limit, so a regression would hang instead of failing.
The dependency table said which product stack already used the package. The repository is vendor-neutral, so the row now says what the package is instead.
SessionEvent, SignOutReason, RetryDecision, Recovery and RevocationOutcome said whether they may gain cases; the other public enumerations did not, so after 1.0 adding a case to any of them would have been a silent source break. Verdicts and JSON value types are complete and frozen. Events, discovery options, Keychain classes and the testing module's scripting enums may grow.
The rotation test delayed the in-flight request by ten seconds and advanced the clock to the next sleeper. Since the sign-out's time limit started in a task of its own, that sleeper could be the five-second limit: the revocation then timed out and, for an exchange, the test waited forever on a request whose answer was never delivered. The delay is now two seconds and the clock advances by exactly that.
load() read the store directly. After a sign-out, or a session ended by invalid_grant, whose delete was still running, a load read the old credential, restored the session and emitted .signedIn; the delete then removed the credential from the store while the restored session lived on in memory. load() now waits for the newest queued store change before it reads, the same chain saves and deletes already follow.
exchangeRefreshToken returned the server's response as it came, so its refreshToken was the session's rotated root refresh token. A caller that handed it to another manager or client would present one refresh token from two places and could make the server revoke the whole grant under reuse detection. The manager already saves the rotation, so the returned response now has refreshToken set to nil. The documentation also says what a cancelled exchange costs: once the request was sent, the issued token reaches nobody and stays valid until it expires. ADR 0015, spec invariants 6 and 7, the architecture note and the article are corrected to match.
The docs blamed SwiftKeychain for the store using the file-based login keychain on macOS. The store makes its own Keychain calls now, so the choice is its own: the data protection keychain on macOS needs a keychain access group entitlement of the hosting process, which plug-ins loaded into another app, command-line tools and swift test do not have. ADR 0009, the store, the security model, the spec and the roadmap now say so, and that an opt-in can come later without breaking the API. The synchronizable and identity-query wording and the STO-3 row match the code.
LoopbackUserAgent installed its state filter only when it started waiting, after the browser had been opened. A request that reached the listener in between passed the default filter and became the callback. The client's own state check still rejected it, but the flow failed and the real redirect was refused. The filter is now installed before the browser opens.
Many callers of beginSignOut only need the session cleared and never read the result. Without @discardableResult each of them had to write _ = to silence the warning.
Credential.additionalFields was documented as application data stored with the credential, but no TokenManager call could set it, and the library never read it. Shipping it in 1.0 would freeze an extension point that does not work. Records written by earlier versions that carry the field still decode: synthesized decoding ignores the key, and a test pins that.
detail could only be set inside the library, because it is sanitized on the way in, while code, recovery, statusCode, errorURI and underlying were freely settable. A client that changed code without recovery could build an error the classification table never produces. All members are now public internal(set); nothing in the tests, the testing module or the known clients assigns them.
The loopback listener and user agent called their wait limit timeout, while every other API that gives up after a duration on the injected clock says timeLimit. 1.0 is the last chance to align them. ManualClock keeps waitForSleeper(timeout:): that bound is real time, not the injected clock.
OpenID Connect Core §3.1.2.1 forbids combining prompt=none with any other value. The documentation said so but the request was built and sent anyway, and the server's answer to such a request is not defined. beginAuthorization now throws invalidConfiguration before anything is sent; AZC-15 records it.
The concurrency note on TokenManager counted three rules and listed four, and linked an outdated spendRefreshToken signature. The article topics lacked PendingSignOut. The AZC-3 and AZC-11 rows pointed at files that do not hold the redirect URI checks, which live in OAuthClient+AuthorizationCode.swift and LoopbackHost.swift. The changelog had a stray blank line and an over-long line.
RFC 9110 §5.6.7 says a two-digit RFC 850 year that would be more than 50 years in the future is the most recent past year with those digits, so anything up to 50 years ahead is in the future. DateFormatter's default window read years 46 to 76 as 1946 to 1976, turning a future Retry-After into a zero delay. The window now starts 50 years before the injected wall clock. A test adds the asctime form with a space-padded day, the RFC's own example shape. The transient classification of 429 and 5xx had no matrix row; ERR-6 and the RFC 6585 entry now trace it.
RFC 8693 §2.2.1 lets a server return a refresh token for the issued token's context, while exchangeRefreshToken, since ADR 0005, reads a refresh_token in the response as the rotated subject. ADR 0015 and the spec now say so and point servers of the other kind at OAuthClient.exchange, which returns the response untouched.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af592f49cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let changes: KeychainAttributes = [String(kSecValueData): data, String(kSecAttrAccessible): accessibility] | ||
| var status = SecItemUpdate(identity as CFDictionary, changes as CFDictionary) | ||
| if status == errSecItemNotFound { | ||
| status = SecItemAdd(attributes as CFDictionary, nil) |
There was a problem hiding this comment.
When KeychainCredentialStore is initialized with a non-nil accessGroup and no item exists yet, this passes the original CredentialItem.attributes to SecItemAdd, but that dictionary omits kSecAttrAccessGroup. On iOS the item is therefore created in the app's default group, while all subsequent fetch, update, and delete operations use identity, which requires the configured group; the save can report success even though the store can never load or delete that credential. Merge the identity attributes into the add dictionary (or add the access group to CredentialItem.attributes).
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Prepares the first stable release. The changes come from a line-by-line review of the library and of the first app built on it, plus two independent pre-release reviews (RFC conformance, security, concurrency, docs):
TokenManager.beginSignOut(revoke:)returningPendingSignOut,OAuthClient.revokeBestEffort(_:typeHint:timeLimit:)returning the top-levelRevocationOutcome(ADR 0014).exchangeRefreshTokenreturns an answered response even after its session ended, without the session's rotated refresh token (ADR 0015).load()no longer restores a session whose delete is pending; the Keychain item is updated in place and matched by identity, not accessibility; the loopbackstatefilter is installed before the browser opens; two-digit RFC 850 years; a status outside 0...999 no longer traps.timeLimitnaming, read-onlyPassportErrormembers,Credential.additionalFieldsremoved,prompt=nonecombinations refused, every public enum states whether it is frozen.Durationconversions andHTTPResponse.Statuschecks; docs, ADR 0009 and the RFC matrix brought in line with the code.All breaking changes are listed in
CHANGELOG.mdunder 1.0.0.Standards
RFC 7009 §2 (revocation), RFC 8693 §2.2.1 (exchange response), RFC 9110 §5.6.7 (HTTP-date), RFC 6585 §4 and RFC 9110 §15.6 (transient statuses), RFC 8252 §8.9 (loopback), OpenID Connect Core §3.1.2.1 (
prompt=none). ADR 0014 and ADR 0015 are new; ADR 0007 and ADR 0009 are amended.Verification
make check: all checks passedmake docs: okmake integration: 16 tests in 5 suites passedswift package diagnose-api-breaking-changes 0.3.2: 14 breakages, all listed in the changelogChecklist
make checkpassesdocs/rfc-matrix.mdanddocs/spec.mdupdated for protocol or API changesmake docspasses if a public symbol or a DocC article changedCHANGELOG.mdupdated for user-visible changes