fix: Updated discriminator to take application name along with sign o…#469
fix: Updated discriminator to take application name along with sign o…#469sagar-okta wants to merge 3 commits intomasterfrom
Conversation
|
Issue: During serialization the params were getting stripped which do not have signon mode as SAML type. This resulted in error during create application for application types other than signon mode SAML Fix: Added application type as discriminator along with signon mode. So that the params are not stripped. |
aniket-okta
left a comment
There was a problem hiding this comment.
Review: fix: Updated discriminator to take application name along with sign on mode
The root cause analysis is correct — OIN apps with signOnMode: SAML_2_0 were resolving to SamlApplication instead of their concrete subtype, causing app-specific fields to be stripped before serialization and producing the Okta API domain validation error. The name-based secondary discriminator is the right approach.
However there are 3 blocking issues and several significant concerns that need to be addressed before this can merge. See inline comments for details.
Blocking
ZoomUsApplicationis missing from both thetypeMapandoinAppMappings— same bug will remain for ZoomUs appsfixGenerated.cjstargetsObjectSerializer.tsbut the runtime file is.js— the patch will silently skip on next codegen, causing the bug to silently reappear- No tests added for the bug fix; the existing type-safety test was weakened instead of updated
Significant
- Name-based discriminator check runs before the OAS spec-defined
mappingcheck — ordering should be inverted - Spurious
/// <reference types="node" />line added tohttp.d.tswith no justification - Fragile regex in
fixGenerated.cjswill silently no-op if generator whitespace changes
| 'salesforce': 'SalesforceApplication', | ||
| 'slack': 'SlackApplication', | ||
| 'trendmicroapexoneservice': 'TrendMicroApexOneServiceApplication', | ||
| 'zscaler_byz': 'ZscalerbyzApplication' |
There was a problem hiding this comment.
[Blocking] ZoomUsApplication is missing from oinAppMappings.
ZoomUsApplication.js receives the id/lastUpdated/_links attribute additions in this PR, but 'zoomus' is never added here. Users creating a ZoomUs application with signOnMode: 'SAML_2_0' will hit the exact same stripping bug.
Add the missing entry:
'zoomus': 'ZoomUsApplication'| }; | ||
|
|
||
| // Read the ObjectSerializer.ts file | ||
| const objSerializerPath = 'src/generated/models/ObjectSerializer.ts'; |
There was a problem hiding this comment.
[Blocking] Wrong file extension — this fix will silently no-op on the next code regeneration.
The target path is ObjectSerializer.ts, but the generated runtime file in this SDK is ObjectSerializer.js. The fs.existsSync guard on line 265 will evaluate to false after a fresh codegen run, log a warning that is easy to miss, and return { modified: false, skipped: true } — silently leaving the bug in place for every future consumer who regenerates the SDK.
Change to:
const objSerializerPath = 'src/generated/models/ObjectSerializer.js';| } | ||
|
|
||
| // 2. Modify findCorrectType to check 'name' property for Application types with SAML signOnMode | ||
| const findCorrectTypeRegex = /(if \(mapping != undefined && mapping\[discriminatorType\]\) \{\s+return mapping\[discriminatorType\];[^\}]+\})/; |
There was a problem hiding this comment.
[Significant] This regex is fragile and will silently fail on generator whitespace changes.
The [^\}]+ atom does not account for nested braces inside the matched block. If the code generator changes indentation, adds a log statement, or nests another conditional inside the target if block, this regex will silently fail to match. Because modifications is only incremented on a successful match, a failed patch won't throw — it will just leave the discriminator logic unpatched with no visible error.
Consider either:
- Anchoring on a more stable, unique surrounding string (e.g. the comment block you own), or
- Throwing explicitly if
findCorrectTypeMatchisnull:
if (!findCorrectTypeMatch) {
throw new Error('fixApplicationDiscriminator: could not find findCorrectType target — ObjectSerializer structure may have changed');
}| '__salesforce': SalesforceApplication_1.SalesforceApplication, | ||
| '__slack': SlackApplication_1.SlackApplication, | ||
| '__trendmicroapexoneservice': TrendMicroApexOneServiceApplication_1.TrendMicroApexOneServiceApplication, | ||
| '__zscaler_byz': ZscalerbyzApplication_1.ZscalerbyzApplication, |
There was a problem hiding this comment.
[Blocking] ZoomUsApplication is missing from the typeMap.
ZoomUsApplication.js is updated in this PR but '__zoomus' is never registered in the typeMap. Without this entry, ObjectSerializer cannot resolve zoomus apps to ZoomUsApplication and will fall back to SamlApplication, preserving the original bug.
Add after this line:
'__zoomus': ZoomUsApplication_1.ZoomUsApplication,| const prefixedDiscriminatorType = `__${discriminatorType}`; | ||
| const manuallyDiscriminatedType = typeMap[prefixedTypeDiscriminatorType] || typeMap[prefixedDiscriminatorType]; | ||
| // For Application type, also check 'name' property for OIN apps | ||
| if (expectedType === 'Application' && data['name']) { |
There was a problem hiding this comment.
[Significant] The name-based check runs before the OAS spec-defined discriminator mapping, inverting the intended priority.
As written, any Application whose name field matches a key in typeMap will always resolve via name — even if the spec's authoritative mapping[discriminatorType] provides a more specific or different answer. The name check should act as a tiebreaker for the ambiguous SAML_2_0 case only, not unconditionally override the spec mapping.
Consider restructuring:
// Check OAS spec mapping first (authoritative)
if (mapping != undefined && mapping[discriminatorType]) {
return mapping[discriminatorType];
}
// Fall back to name-based lookup for OIN apps where signOnMode is ambiguous
if (expectedType === 'Application' && data['name']) {
const nameMapping = typeMap[`__${data['name']}`];
if (nameMapping) {
return `__${data['name']}`;
}
}| expect(app).to.be.instanceOf(Application); | ||
| // Application objects can be either base Application type or specific OIN app types | ||
| // (GoogleApplication, Office365Application, etc.) which have their own schemas | ||
| expect(app).to.exist; |
There was a problem hiding this comment.
[Blocking] The instanceof Application assertion was removed instead of updated, weakening type-safety coverage.
The original expect(app).to.be.instanceOf(Application) was catching deserialization regressions (e.g. plain objects returned instead of typed instances). The replacement (expect(app).to.exist) only checks for truthiness — it would pass even if the serializer returned a raw {} object.
The correct fix is to assert against the expected concrete type. Since OIN apps now resolve to their subclasses, use a union check:
const knownAppTypes = [
Application, GoogleApplication, Office365Application,
Org2OrgApplication, SalesforceApplication, SlackApplication,
TrendMicroApexOneServiceApplication, ZoomUsApplication, ZscalerbyzApplication
];
expect(knownAppTypes.some(T => app instanceof T)).to.be.true;This preserves the original intent while accommodating the new subtype resolution.
| /// <reference types="node" /> | ||
| /// <reference types="node" /> | ||
| /// <reference types="node" /> | ||
| /// <reference types="node" /> |
There was a problem hiding this comment.
[Minor] Spurious duplicate /// <reference types="node" /> directive.
There are now 5 identical triple-slash directives where 3 existed before. This looks like a code-generation artifact that was accidentally committed. It has no functional impact but inflates the diff, making it harder to review genuine changes. Please revert this file to its pre-PR state.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: https://oktainc.atlassian.net/browse/OKTA-1100279
What is the new behavior?
In the current implementation the discriminator uses only the sign on mode, however for some cases we need to consider name of the application as well for discriminator. This change enables it to take the application name as discriminator.
Does this PR introduce a breaking change?
Other information
Reviewers