T 1551265: Support public OAuth2 clients without a client secret

[vishal-mb]

• I've read, understood, and done my best to follow the CONTRIBUTING guidelines.

This pull request includes (pick all that apply):

• Bugfixes
• New features
• Breaking changes
• Documentation updates
• Unit tests
• Other

Summary

Support public OAuth2 clients, i.e. clients that hold no client secret (RFC 6749 §2.1). A public client identifies itself with client_id in the token request body instead of an Authorization: Basic header. Confidential clients are unchanged. AB#1551265

Implementation

• OAuth2ClientConfiguration gains init(publicClientIdentifier:environment:guestUsername:guestPassword:) and a read-only isPublicClient flag. The flag can only be set through that initializer, so an existing configuration cannot drift into public mode.
• buildStandardTokenGrantRequest is the single chokepoint for the five in-tree grant strategies (authorization_code, refresh_token, extension grants, password, client_credentials). For a public client it adds client_id to the body and skips the Basic header; a grant-supplied client_id still wins.
• A new internal basicToken accessor returns nil for a public client, so a public client never emits a Basic header, even if a secret is assigned later. A confidential client is byte-identical to 1.4.6, including one built with an empty secret.
• Client-level authorization for a public client through OAuth2RequestPipelineMiddleware requires guest credentials (password grant). Client-level .basic has no secret to send, and client-level .bearer without guest credentials would need the client_credentials grant, which is reserved for confidential clients (RFC 6749 §4.4), so the middleware fails both with OAuth2Error.internalFailure before any request is built. Constructing OAuth2ClientCredentialsTokenGrantStrategy directly is not guarded. internalFailure is used rather than clientFailure so a misconfiguration is distinguishable from a server-side auth rejection.
• The token-store key ignores both the secret and the flag, so switching a client from confidential to public resolves the same stored token. Whether the server still accepts that token is the server's policy.

Test Plan

• OAuth2PublicClientTokenGrantTests (new): public vs confidential request shape for all five grant types; a public client with an assigned secret still sends no Basic header; a grant-supplied client_id wins.
• OAuth2ClientConfigurationTests: confidential by default; public initializer holds an empty secret and passes guest credentials through; public and confidential configurations with the same identifier are not equal.
• OAuth2MiddlewareClientLevelAuthorizationTests (new): confidential client-level basic applies the header; public client-level basic and public client-level bearer without guest credentials both fail with internalFailure.
• OAuth2TokenStoreTests: token identifier is unchanged across secret and public flag.
• Full suite green locally (swift test). SwiftLint run locally: no warnings in changed files (CI runs swift test only).

Security Impact

• No security impact
• I have linked to a document that considers security impact or described the security impact below.

Enables clients to operate without an embedded client secret, which removes a secret from the client binary where this configuration is adopted. A public client sends only its client_id, which identifies the client but does not authenticate it (RFC 6749 §3.2.1). Confidential clients keep sending Authorization: Basic exactly as before. No stored-token format or key changes.

For a public client the client secret no longer binds an authorization code to the client, so a public client using authorization_code must use PKCE and the authorization server must enforce it (RFC 8252 §6). Conduit already carries code_challenge through OAuth2AuthorizationRequest.additionalParameters and code_verifier through tokenGrantRequestAdditionalBodyParameters; first-class PKCE support is a follow-up. Client-level bearer for a public client through the middleware requires guest credentials; the middleware never sends an unauthenticated client_credentials grant.

[claude]

Review — deep pass

Performed at head f17772c1a59fbc76fb3f981bfc2ac33cb1ffe690, against base main @ a2e9773 — computed as the true merge base locally rather than taken from the API, and it is exactly tag 1.4.6.

Verdict

No blocker. Two MAJORs, eight MINORs, four questions. Nothing here should stop this from merging on its own terms.

The change is additive and not source-breaking: both initializers are explicit so the memberwise init was never part of the API, isPublicClient defaults to false, and no confidential client's request changes by a single byte. Nothing in this PR changes behavior for an existing Conduit 1.4.6 consumer — every behavioral finding below is reachable only by a caller that adopts init(publicClientIdentifier:). Conduit has no feature flags, so that adoption boundary is the only gating there is. I've restated it per-finding where it matters.

Both MAJORs sit on the RFC-conformance and security-guidance axis; neither is a defect in what the diff does mechanically:

1. A public client with no guest credentials reaches an unauthenticated client_credentials grant, which RFC 6749 §4.4 restricts to confidential clients. The sharpest form is an asymmetry inside this PR: it hard-fails client-level .basic for a public client because there is no secret to send, then lets client-level .bearer — in the identical situation — build the grant and fail at the server. At 1.4.6 that same branch was conformant, so the missing credential is new here.
2. The Security Impact section omits PKCE, and authorization_code — implemented in-tree over SFSafariViewController with a private-use scheme redirect — is the case that needs it. This one is documentation, not code: PKCE should not be built in this PR, and PKCE is already achievable by a consumer through the existing extension points. What's missing is the sentence saying so.

What was verified rather than assumed

• The diff was read against the true merge base a2e9773, not the API's reported base.
• All five in-tree grant strategies were traced to the single chokepoint — OAuth2AuthorizationCodeTokenGrantStrategy.swift:52, OAuth2RefreshTokenGrantStrategy.swift:43, OAuth2ExtensionTokenGrantStrategy.swift:38, OAuth2PasswordTokenGrantStrategy.swift:47, OAuth2ClientCredentialsTokenGrantStrategy.swift:35. The body's "single chokepoint" claim holds as enumerated (with one narrowing, in the questions below).
• Every claim in the PR body was checked — 45 in total. One is false as worded (posted inline); four are overstated.
• Every file:line in this review was re-verified against f17772c1 immediately before posting.

Test credit, honestly

This is a genuinely well-tested PR by this channel's standards, and the structure is the reason. All ten grant-strategy tests in OAuth2PublicClientTokenGrantTests.swift are real public/confidential pairs covering all five grant types. The public halves each kill the behavioral mutants — drop the client_id injection, restore the unconditional Basic header, never set isPublicClient. The confidential halves are deliberate conditionality guards rather than padding: they are what catch the over-broad mutants ("always send client_id", "never send Basic", "default to public"), which are precisely the ways a diff like this could have broken every existing 1.4.6 consumer. testAppliesBasicHeaderForConfidentialClientLevelBasicAuthorization (OAuth2RequestPipelineMiddlewareTests.swift:357) adds coverage of a configuration no test at 1.4.6 ever constructed. No new test is vacuous outright, and every changed production line has at least one assertion that observes at least one mutation of it.

The exception is two surviving mutants, both posted inline: the guard !isPublicClient at OAuth2ClientConfiguration.swift:80 — the PR's one documented invariant, and the reason clientSecret staying a public var is acceptable — and the guest-credential pass-through at :71-72, which guards the first MAJOR's only conformant escape hatch.

Folded in here, because there is no diff line to anchor it

MINOR — "no SwiftLint warnings in changed files" isn't something this repo can substantiate. .swiftlint.yml exists, but no CI job invokes swiftlint: .github/workflows/ci.yml is fifteen lines whose only step is swift test on macos-latest (:11, :15). The one SwiftLint invocation in the repo, Dangerfile:16-17, is dead config — there is no Danger step, no Gemfile, no fastlane/, no Makefile.

To be even-handed about it: the test claim in the same sentence is substantiated by that check — 232 func test declarations at base, 247 at head, with one macOS-excluded behind a targetEnvironment guard at KeychainHybridKeyProviderTests.swift:212, which gives exactly the stated 246. And we could not run SwiftLint here either. So this is an observation about what CI enforces, not an accusation that the claim is wrong. The mechanical subset that could be checked by hand — line length against line_length: 160, trailing whitespace, single_test_class — is clean on every changed file.

Questions

1. Should init(publicClientIdentifier:) require guestUsername/guestPassword? That is the only configuration in which client-level bearer works for a public client — the design question behind the first MAJOR. Related and not answerable from here: for the client being onboarded, is client-level bearer in use at all, and is the authorization server expected to accept client_credentials with only a client_id? If it enforces §4.4 the path fails permanently against a config that looks fine at the call site; if it doesn't, it will mint a client-level token for anyone who can read the client_id out of the app binary.
2. "Single chokepoint for every grant type" holds for all five in-tree strategies, but buildStandardTokenGrantRequest is internal (OAuth2TokenGrantStrategy.swift:30) while the protocol at :12 is public and requires only issueToken (:18, :24) — and refreshStrategyFactory is a public var (OAuth2RequestPipelineMiddleware.swift:20). So an out-of-module conformer builds its own request, never sees isPublicClient, and keeps sending an empty-secret Basic header. Does anything in SharedLibraries-iOS implement its own grant strategy or refresh factory? We can't see that repo from here, so this is a question rather than an assertion.
3. The "a grant-supplied client_id still wins" ordering is structurally true — :44-46 runs before the merge at :48-50, and the comment says so — but nothing tests it. Intentional, or worth pinning?
4. CHANGELOG.md:7-11 is correct and in the right section. The hard-wrapped multi-line bullet is the only one in the file — worth matching the surrounding single-line style?

Also worth deciding: eight tests in this area are DISABLED_ (six in OAuth2RequestPipelineMiddlewareTests.swift, two in AuthMigratorTests.swift), and two of them — :194 and :230 — are exactly the ones covering the grant selection that the first MAJOR turns on. They appear to need a live localhost:5000. Is re-enabling them in scope here, or worth a follow-up ticket?

Evidence limits

There is no Swift toolchain in this review environment, so nothing was compiled or executed. The mutation analysis is coverage-by-construction from exhaustive reading and grep, not an executed mutation run. The two RFC-conformance findings describe server-side outcomes that were not exercised: whether the authorization server enforces §4.4, whether it requires PKCE, whether it returns 400 or 401 on a refresh with absent client authentication, and whether the same client_id is being re-registered as public or a new one issued — none of that is observable from this repo, and each would change the real-world severity. SharedLibraries-iOS and the consumer app are likewise not reachable from here.

---

Generated by Claude Code

[claude]

Follow-up — re-verified at d23c0e3

Re-verified all ten findings against the code at this head. Both MAJORs and all eight MINORs are genuinely fixed, and each new test kills the mutant it targets rather than merely passing. Two worth naming:

• testPublicClientNeverSendsBasicHeaderEvenWithAnAssignedSecret assigns a secret to a public configuration and asserts no Basic token — the first reference to basicToken from the test suite at all. Restore the unconditional accessor and it fails.
• testPublicClientLevelBearerWithoutGuestCredentialsFailsWithInternalFailure would fail on revert for a non-obvious reason: the reverted path reaches the grant call against localhost:5000, and connection-refused maps through OAuth2TokenGrantManager.errorFrom to networkFailure, not internalFailure. It is not a test that passes either way.

CI is green on this head — 251 tests, 0 failures — which does confirm the new tests pass. It is not a mutation run, so the kill analysis above is by reading, as before.

The answer on the skipped PostFetch hook is reasonable: no token fetch is attempted, so there is nothing for a migrator to observe. Failing at request time rather than requiring guest credentials at construction is one of the two options offered, so that settles the design question.

Still open:

1. Does anything in SharedLibraries-iOS implement its own OAuth2TokenGrantStrategy or set refreshStrategyFactory? Out-of-module conformers build their own request and never see isPublicClient, so the public-client flag doesn't reach them. We still can't see that repo from here.
2. Will the authorization server accept or reject what the client now sends?
3. The eight DISABLED_ tests remain a decision rather than a finding — two of them, OAuth2RequestPipelineMiddlewareTests.swift:193 and :229, are still the only coverage of grant selection.

One new MINOR and a folded nit inline. Both are documentation; the code is right.

---

Generated by Claude Code

[vishal-mb]

Answers to the three open questions from the follow-up review:

1. SharedLibraries-iOS custom strategies. One exists: StravaTokenRefreshStrategyFactory under Sources/StravaAPI/Strategies. It builds its own refresh request from a confidential Strava configuration and never reads isPublicClient, so it is unaffected. IntegrationHelpers/TokenRetriever constructs confidential configurations only. There is no other OAuth2TokenGrantStrategy conformer and no other refreshStrategyFactory assignment in that repo.
2. Server acceptance. Verified against the production authorization server the consuming apps use, with the exact public shape (client_id in the body, no Authorization header): refresh_token, delegation, and authorization_code all return invalid_grant for a bogus grant, meaning the client is accepted and only the grant is rejected. Omitting client_id returns invalid_request. A live end-to-end delegation exchange from a consuming app built against this branch also succeeded today.
3. DISABLED_ tests. They need a live token server on localhost:5000 and predate this change. Re-enabling them against a stub server is a follow-up outside this PR.

[claude]

Closing out from our side: everything from both reviews is addressed at e3a03e6, and the latest commit is comment-only — every other file in the PR is byte-identical to d23c0e3 and the Swift file differs only in /// lines — so nothing blocking from us.

The two declined nits are reasonable as declined: an added OAuth2Error case would be source-breaking for consumers switching exhaustively, and nothing enforces the SwiftLint headroom either way since no job runs it. The two answers — the single out-of-module conformer being confidential-only, and the production authorization server accepting the new public wire shape — aren't verifiable from outside this repo, so both are taken on the author's word.

All 11 inline threads are still unresolved by anyone, but the author replied on every one; open threads here should not be read as open findings.

Two things left, both explicitly non-blocking: the CHANGELOG bullet is still the only hard-wrapped one in the file, and the eight DISABLED_ tests are untouched, two of which remain the only coverage of middleware grant selection. One correction as well — the Test Plan still attributes the three client-level tests to OAuth2RequestPipelineMiddlewareTests, but they now live in OAuth2MiddlewareClientLevelAuthorizationTests.swift; it went stale when that file was split out.

---

Generated by Claude Code