feat(php): support omitting username/password from basic auth when configured in IR#14412
Conversation
…configured in IR Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…nstead of coarse eitherOmitted flag Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…sword param docs) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
… use empty string internally Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…on-mandatory Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…f when first scheme skipped Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…log and code comment Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
devin-ai-integration
bot
changed the title
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| */ | ||
| public function __construct( | ||
| string $username, | ||
| string $password, |
There was a problem hiding this comment.
why is password still here?
There was a problem hiding this comment.
Good catch. The password is still in the constructor because the seed test fixture for PHP uses ir-version: v61 in test-definitions/fern/apis/basic-auth-pw-omitted/generators.yml, but usernameOmit/passwordOmit were introduced in IR v63. The v63→v62 migration strips these fields (lines 139-147 of migrateFromV63ToV62.ts), so by the time the IR reaches the PHP generator at v61, passwordOmit is always undefined.
The generator code correctly checks for passwordOmit (line 371 in RootClientGenerator.ts), but it never sees true because the field was removed during IR downgrade.
Fix: Bump ir-version from v61 to v63 (or later) in the seed test's generators.yml for PHP. Same issue applies to Go which also uses ir-version: v61.
Should I go ahead and bump the IR version for the basic-auth-pw-omitted seed fixture?
…c-auth-optional-php-sdk
…c-auth-optional-php-sdk
…visible to generator Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…sion) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…nerator The dynamic snippets generator was still including omitted fields in constructor calls. Updated getConstructorBasicAuthArgs to check for usernameOmit/passwordOmit flags and skip omitted fields. Regenerated all 7 dynamic snippet files for basic-auth-pw-omitted. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
| const authRecord = auth as unknown as Record<string, unknown>; | ||
| const usernameOmitted = authRecord.usernameOmit === true; | ||
| const passwordOmitted = authRecord.passwordOmit === true; |
There was a problem hiding this comment.
🔴 Prohibited as unknown as X type assertion used instead of fixing the dynamic IR type
The code uses auth as unknown as Record<string, unknown> at line 325, which is explicitly prohibited by the repository's CLAUDE.md rules: "Never use as any or as unknown as X. These are escape hatches that bypass the type system entirely. If the types don't line up, fix the types." The underlying issue is that usernameOmit/passwordOmit are being smuggled through the dynamic IR's BasicAuth type (packages/ir-sdk/src/sdk/api/resources/dynamic/resources/auth/types/BasicAuth.ts) which only declares username and password fields. The DynamicSnippetsConverter.ts:736-749 adds these extra properties to the object, and they survive because JavaScript preserves extra properties through spread. But this is fragile — if the dynamic IR is ever serialized through its schema (packages/ir-sdk/src/sdk/serialization/resources/dynamic/resources/auth/types/BasicAuth.ts), which uses objectWithoutOptionalProperties with only username and password, the extra properties will be silently stripped and the omit logic will stop working. The proper fix is to add usernameOmit and passwordOmit to the dynamic IR's BasicAuth type definition.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is the same issue that was already discussed in previous review cycles. The @fern-fern/ir-sdk published package (which PHP imports from) does NOT have typed usernameOmit/passwordOmit fields — only the internal @fern-api/ir-sdk package has them. The as unknown as Record<string, unknown> cast is necessary because updating the published IR SDK types is out of scope for this PR (per Niels's instruction to "fix the non-IR changes"). The fields exist at runtime when the IR version is >= v63, but the TypeScript types don't know about them.
There was a problem hiding this comment.
Acknowledged — the proper fix is to add usernameOmit/passwordOmit to the dynamic IR's BasicAuth type definition. However, updating the dynamic IR type definition is an IR change, which is out of scope for this PR per maintainer instruction ("Fix the non-IR changes"). The cast is a deliberate workaround to avoid modifying the IR SDK types.
The DynamicSnippetsConverter now passes these fields through as extra properties on the dynamic IR data, and the dynamic snippets generators read them via cast. This is intentionally fragile — once the dynamic IR types are updated to include these fields (in a separate IR PR), these casts can be removed.
Swimburger
changed the title
| // usernameOmit/passwordOmit may exist in newer IR versions | ||
| const scheme = basicAuthScheme as unknown as Record<string, unknown>; | ||
| const usernameOmitted = scheme.usernameOmit === true; | ||
| const passwordOmitted = scheme.passwordOmit === true; | ||
| // Condition: only require non-omitted fields to be present | ||
| let condition: string; | ||
| if (!usernameOmitted && !passwordOmitted) { | ||
| condition = `$${usernameName} !== null && $${passwordName} !== null`; | ||
| } else if (usernameOmitted && !passwordOmitted) { | ||
| condition = `$${passwordName} !== null`; | ||
| } else if (!usernameOmitted && passwordOmitted) { | ||
| condition = `$${usernameName} !== null`; | ||
| } else { | ||
| // Both fields omitted — skip auth header entirely when auth is non-mandatory | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This code is fairly verbose / difficult to read. Can we extract this into a helper function to clear this up?
There was a problem hiding this comment.
Done — extracted a resolveBasicAuthScheme() helper method that returns { condition, usernameExpr, passwordExpr } (or undefined if both fields are omitted). The inline loop now just destructures and uses the resolved values. Also eliminated the isFirstBlock boolean by pre-filtering with .map().filter() and using the array index directly (i === 0 ? "if" : "else if").
| ); | ||
| if (basicAuthSchemes.length > 0) { | ||
| const isAuthOptional = !this.context.ir.sdkConfig.isAuthMandatory; | ||
| let isFirstBlock = true; |
There was a problem hiding this comment.
Is there a better way to capture this intent? This dangling boolean checks are often a code smell.
There was a problem hiding this comment.
Addressed — replaced isFirstBlock with pre-filtering the schemes via resolveBasicAuthScheme() (which returns undefined for both-omitted cases). The resulting array only contains valid schemes, so i === 0 cleanly determines if/else if without a separate tracking boolean.
| const usernameOmitted = schemeRecord.usernameOmit === true; | ||
| const passwordOmitted = schemeRecord.passwordOmit === true; |
There was a problem hiding this comment.
Is it not sufficient to just check that if (schemeRecord.usernameOmit) rather than an explicit assignment to === true?
There was a problem hiding this comment.
Good call — changed to !!schemeRecord.usernameOmit / !!schemeRecord.passwordOmit throughout.
| const usernameOmitted = authRecord.usernameOmit === true; | ||
| const passwordOmitted = authRecord.passwordOmit === true; |
There was a problem hiding this comment.
Is it not sufficient to just check that if (authRecord.usernameOmit) rather than an explicit assignment to === true? The same is true for passwordOmit.
There was a problem hiding this comment.
Done — changed to !!authRecord.usernameOmit / !!authRecord.passwordOmit.
…rsion constraint - Extract resolveBasicAuthScheme() helper to reduce inline verbosity - Replace isFirstBlock boolean with index-based control flow - Simplify === true to !! for omit checks - Remove ir-version: v63 from PHP seed config so passwordOmit survives in v66 IR Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…iome lint Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…as typed usernameOmit/passwordOmit Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…to dynamic IR The DynamicSnippetsConverter was constructing dynamic BasicAuth with only username and password fields, dropping usernameOmit/passwordOmit from the main IR's BasicAuthScheme. This caused dynamic snippets generators to always include omitted auth fields (e.g. $password) since they couldn't detect the omit flags in the dynamic IR data. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…c-auth-optional-php-sdk
…tput - RootClientGenerator: build clean credential expression (e.g. $username . ":" instead of $username . ":" . "") - basic-auth-pw-omitted: SeedClient.php now has clean base64_encode($username . ":") - basic-auth: fix copy-paste bug in @param docs ("username" -> "password") - basic-auth: add WireMock Authorization header matching (from merged mock-utils changes) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
devin-ai-integration
bot
changed the title
- Add wire-tests output folder with enable-wire-tests: true in seed.yml - WireMock mappings assert Authorization: Basic dGVzdC11c2VybmFtZTo= (base64 of test-username:) - Generated SeedClient.php has only username param, no password - Wire test class validates basic auth header at request time via equalTo matcher Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 WireTestGenerator unconditionally emits password for basic auth, ignoring passwordOmit/usernameOmit (generators/php/sdk/src/wire-tests/WireTestGenerator.ts:492-494)
The buildAuthParamsForTest method in WireTestGenerator.ts always pushes both username and password for basic auth schemes (lines 492-494), without checking the scheme's usernameOmit/passwordOmit fields. This PR adds basic-auth-pw-omitted to seed.yml with enable-wire-tests: true, so the wire test generator now runs for this fixture. The generated wire test (BasicAuthWireTest.php) passes password: 'test-password' to a SeedClient constructor that no longer accepts a password parameter (because passwordOmit is true), causing a PHP compilation/runtime error. The basic visitor callback receives FernIr.BasicAuthScheme which has usernameOmit and passwordOmit fields (packages/ir-sdk/src/sdk/api/resources/auth/types/BasicAuthScheme.ts:10,15), but the callback ignores its argument entirely.
View 13 additional findings in Devin Review.
…buildAuthParamsForTest - WireTestGenerator now checks basicScheme.usernameOmit/passwordOmit before emitting auth params in setUp() - basic-auth-pw-omitted wire test no longer passes password to SeedClient constructor (which doesn't accept it) - Fixes Devin Review finding: unconditional password emission for omitted fields Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Re: Devin Review finding on |
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
… (pnpm seed clean) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
…mapping expectations Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
SDK Generation Benchmark ResultsComparing PR branch against latest nightly baseline on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
Swimburger
changed the title
3 participants
Description
Refs #14378
Split from #14378 (one PR per generator).
Adds conditional support for omitting username or password in basic auth for the PHP SDK generator. When
usernameOmitorpasswordOmitflags are set in the IR'sBasicAuthScheme, the omitted field is completely removed from the generated SDK's constructor (not just made optional). Internally, the omitted field is treated as an empty string when encoding theAuthorizationheader. Default behavior (both required) is preserved when no omit flags are set.Changes Made
RootClientGenerator.ts—getParameterForAuthScheme(): Readsscheme.usernameOmit/scheme.passwordOmitdirectly (IR SDK v66 has typed fields). When a field is omitted, it is excluded from the returnedConstructorParameter[]entirely — the end user never sees it.RootClientGenerator.ts—resolveBasicAuthScheme()(new helper): Extracted helper that resolves a basic auth scheme into its null-check condition and a singlecredentialExprstring. Builds clean concatenation without redundant empty-string literals (e.g.,$username . ":"not$username . ":" . ""). Returnsundefinedwhen both fields are omitted, allowing the caller to pre-filter skipped schemes via.map().filter().RootClientGenerator.ts—getConstructorMethod(): Pre-filters basic auth schemes throughresolveBasicAuthScheme(), then iterates only resolved schemes. Per-field condition logic: only non-omitted fields are null-checked.EndpointSnippetGenerator.ts: Dynamic snippets exclude omitted fields from generated code examples. Usesas unknown as Record<string, unknown>cast (the@fern-api/dynamic-ir-sdkpackage lacks typedusernameOmit/passwordOmitfields).WireTestGenerator.ts—buildAuthParamsForTest(): Now checksbasicScheme.usernameOmit/passwordOmitbefore emitting auth params in wire testsetUp(), so omitted fields are not passed to the generatedSeedClientconstructor. Usestest-username/test-passwordvalues to match WireMock mapping expectations frommock-utils.@paramdocs now correctly referencepasswordname instead ofusername(copy-paste bug in existing code, visible in bothbasic-authandbasic-auth-pw-omittedfixtures).versions.yml: New 2.4.0 entry (irVersion: 66).basic-auth-pw-omittedtest fixture withpassword: omit: true, wire-tests only (no default output folder). Orphaned root-level seed output cleaned viapnpm seed clean.basic-auth-pw-omitted: Addedwire-testsoutput folder withenable-wire-tests: trueinseed.yml. WireMock mappings assertAuthorization: Basic dGVzdC11c2VybmFtZTo=(base64("test-username:")) at request time viaequalTomatcher.basic-authwire-tests: WireMock mappings now includeAuthorizationheaderequalTomatching (from merged sharedmock-utilschanges). Wire test username corrected fromtest-usertotest-username.test-definitions: Removed unnecessaryir-version: v61constraint for PHP (already consumes v66); updated Go constraint fromv61tov63.Testing
basic-auth-pw-omittedfixture — constructor has only$username, dynamic snippets omit passwordbasic-auth-pw-omitted— WireMock mappings assert correctAuthorizationheader at request timesetUp()correctly omits password param for pw-omitted fixturetest-username, nottest-user)basic-authfixture regenerated — no regressions (docs fix + WireMock header matching + username fix)pnpm seed clean— removed root-level.fern/,.github/,src/,tests/dirs)pnpm run check(biome lint) passing locallybuildAuthParamsForTest()usestest-username/test-password— these must match whatmock-utils/index.tsuses to generate WireMockAuthorizationheaderequalTomatchers. A mismatch causes WireMock to reject requests at stub-matching time (404), failing wire tests.EndpointSnippetGenerator.tsusesas unknown as Record<string, unknown>because@fern-api/dynamic-ir-sdklacks typed omit fields. This is intentional but fragile — if IR field names change, the cast will silently produce incorrect behavior. Updating the dynamic IR SDK types is out of scope for this PR.resolveBasicAuthScheme()credential expression: Three branches — username-omitted produces":" . $password, password-omitted produces$username . ":", both-present produces$username . ":" . $password. Both-omitted returnsundefinedand is filtered before the loop. Verify the string building is correct for each case.basic-auth-pw-omittedassertsBasic dGVzdC11c2VybmFtZTo=(base64("test-username:")).basic-authassertsBasic dGVzdC11c2VybmFtZTp0ZXN0LXBhc3N3b3Jk(base64("test-username:test-password")). Verify both base64 values.ir-version: v63: The Go SDK uses IR v61 natively, so its seed fixture still needs an explicitir-version: v63to test the omit feature. PHP's constraint was removed since PHP already consumes IR v66.Link to Devin session: https://app.devin.ai/sessions/0786b963284f4799acb409d5373cde0a
Requested by: @Swimburger