Repository navigation
JAXB3 Migration - #1023
JAXB3 Migration#1023maximthomas wants to merge 35 commits into
Conversation
… after JAXB3 migration getIDPSSOConfigOrSPSSOConfig() now returns List<JAXBElement<BaseConfigType>>, but removeFromEntityConfig still cast elements directly to BaseConfigType / AttributeType. Unwrap via getValue(), mirroring addToEntityConfig.
…B3 migration getSingleLogoutService() returns List<SingleLogoutServiceElement> (JAXBElement wrappers); unwrap to List<EndpointType> before LogoutUtil.doLogout().
Unwrap the SingleLogoutService wrapper list to List<EndpointType> before LogoutUtil.doLogout(). The wrapper-expecting getSLOServiceLocation() path is left unchanged.
…r JAXB3 migration convertAttributes() added the AttributeValueElement wrapper instead of its content to the list passed to setAttributeValueString(), which casts each element to String. Add the unwrapped value.
…ws after JAXB3 migration getIDPSSOConfigOrSPSSOConfigOrAuthnAuthorityConfig() returns List<JAXBElement<BaseConfigType>>; unwrap via getValue() before use.
…ter JAXB3 migration getUse() returns null when the optional use attribute is absent (one key for both signing and encryption); guard before calling value().
… migration SingleLogoutService / NameIDMappingService / AuthnQueryService / AssertionIDRequestService endpoints were created with createAttributeServiceType(), which injects a spurious xsi:type="AttributeServiceType" on marshal. Use createEndpointType().
…DFFCOTUtils after JAXB3 migration getAffiliationDescriptorConfig() is null for ordinary SP/IDP entities; guard the wrapper before getValue() in add/remove EntityConfig.
…ements after JAXB3 migration Optional JAXB elements are now possibly-null JAXBElement wrappers. Add a null-safe jaxbValue() helper in IDPPBaseContainer and use it across the IDPP container read-back methods.
…nt after JAXB3 migration getIDPSSODescriptor() can return null for an unknown issuer; reject with InvalidGrantException instead of dereferencing the null wrapper.
AttributeValueElement.getValue() returns the scalar content (not a List) for string-valued attributes, so the "instanceof List" guard silently skipped context matching. Normalize scalar and list content to restore the previous getContent() behaviour.
vharseko
left a comment
There was a problem hiding this comment.
PR #1023 "JAXB3 Migration" — fix registry
Branch tip after follow-up work: f168117cc6 (11 commits on top of 63ff4448ae).
PR: #1023
Root cause (common to all 11 fixes)
The migration moves XML binding from JAXB 1.0 / javax.xml.bind to JAXB 3.0 / jakarta.xml.bind and stops checking generated classes into the repo — they are now generated at build time (jaxb-maven-plugin). In doing so the shape of the generated API changes, which breaks consumer code in three ways that compile but fail at runtime:
-
Global XML elements are now
JAXBElement<T>wrappers.
A getter that used to return the concrete type now returns a wrapper, so callers must call.getValue().- Any remaining raw cast
(Type) iter.next()or enhanced-forfor (Type x : rawList)over a list of wrappers → ClassCastException. It only compiles because the list is declared as a rawList.
- Any remaining raw cast
-
Optional elements are now nullable wrappers.
AgetXxx().getValue()chain on an absent (optional) element → NullPointerException, and the existingif (x != null)guard below becomes dead code. -
ObjectFactory.createXxx()factory methods now take a value argument.
Passing the subtypeAttributeServiceTypeinto a factory whose declared value type isEndpointTypemakes JAXB emit a spuriousxsi:type="AttributeServiceType"on marshal → invalid SAML2 metadata.
Commit registry
| # | Commit | File(s) | Type | Essence |
|---|---|---|---|---|
| 1 | dac13f3c06 |
WSFederationCOTUtils.java |
CCE | remove path not migrated |
| 2 | 9e8cad2dd5 |
SAML2SingleLogoutHandler.java |
CCE | wrappers into doLogout(List<EndpointType>) |
| 3 | ea473f7fc0 |
SPSingleLogout.java |
CCE | same in SP SLO |
| 4 | 6a4d81419b |
AttributeQueryUtil.java |
CCE | wrapper added instead of value |
| 5 | 3435063154 |
GetCircleOfTrusts.java, ImportSAML2MetaData.java |
CCE | raw cast to BaseConfigType |
| 6 | 271ad11235 |
SAMLv2ModelImpl.java |
NPE | getUse() == null |
| 7 | faf0be2e7e |
SAMLv2ModelImpl.java |
XML | spurious xsi:type on endpoints |
| 8 | d9d4b8e13e |
IDFFCOTUtils.java |
NPE | no affiliation config |
| 9 | 5aa4d1c5e3 |
7× IDPP* containers |
NPE | optional Liberty PP elements |
| 10 | f77dbb76bd |
Saml2GrantTypeHandler.java |
NPE | unknown issuer |
| 11 | f168117cc6 |
SAML2IDPProxyFRImpl.java |
logic | silently disabled context matching |
Per-fix detail
1. dac13f3c06 — WSFederationCOTUtils.removeFromEntityConfig (CCE)
File: openam-federation-library/.../wsfederation/meta/WSFederationCOTUtils.java
Problem: getIDPSSOConfigOrSPSSOConfig() returns List<JAXBElement<BaseConfigType>>. addToEntityConfig was migrated to .getValue(), but the sibling removeFromEntityConfig was not: it still did (BaseConfigType) iter.next() and (AttributeType) iter2.next(). Removing a WS-Fed entity from a circle of trust → CCE.
Fix: generics + .getValue(), mirroring the add method.
2. 9e8cad2dd5 — Multi-protocol SOAP single logout (CCE)
File: .../multiprotocol/SAML2SingleLogoutHandler.java
Problem: getSingleLogoutService() → List<SingleLogoutServiceElement> (which are JAXBElement<EndpointType>). The list is passed to LogoutUtil.doLogout(List<EndpointType>), whose for (EndpointType e : list) → CCE. Sibling paths (IDPSessionListener/SPSessionListener) were fixed; this one was missed.
Fix: .stream().map(JAXBElement::getValue).collect(...) before doLogout.
3. ea473f7fc0 — SP single logout (CCE)
File: .../saml2/profile/SPSingleLogout.java
Problem: same wrapper list passed into doLogout(List<EndpointType>).
Fix: unwrap to List<EndpointType>. The second slosList (feeds getSLOServiceLocation, which expects the wrappers) is intentionally left unchanged.
4. 6a4d81419b — SAML2 attribute query conversion (CCE)
File: .../saml2/profile/AttributeQueryUtil.java
Problem: convertAttributes() added the wrapper AttributeValueElement instead of its content. The list goes to setAttributeValueString(), which does (String) iter.next() → CCE. The computed content was unused.
Fix: add content (the unwrapped value).
5. 3435063154 — OpenFM workflows (CCE)
Files: OpenFM/.../workflow/GetCircleOfTrusts.java, ImportSAML2MetaData.java
Problem: getIDPSSOConfigOrSPSSOConfigOrAuthnAuthorityConfig() → List<JAXBElement<BaseConfigType>>, but the code did (BaseConfigType) config.iterator().next() → CCE when resolving the realm for a hosted entity.
Fix: generics + .getValue().
6. 271ad11235 — KeyDescriptor without use (NPE)
File: openam-console/.../federation/model/SAMLv2ModelImpl.java
Problem: keyOne.getValue().getUse().value() — getUse() is null when a <KeyDescriptor> omits the optional use attribute (one key for both signing and encryption). The next line if (type == null ...) was written for exactly that null case but is now unreachable. NPE when viewing the entity in the console.
Fix: null guard before .value().
7. faf0be2e7e — invalid metadata endpoints (XML)
File: openam-console/.../federation/model/SAMLv2ModelImpl.java
Problem: SingleLogoutService / NameIDMappingService / AuthnQueryService / AssertionIDRequestService endpoints were built with createAttributeServiceType(). Since AttributeServiceType extends EndpointType, marshalling injects a spurious xsi:type="AttributeServiceType" → off-schema metadata. 8 sites.
Fix: createEndpointType(). The two legitimate createAttributeServiceElement(createAttributeServiceType()) sites are left untouched.
8. d9d4b8e13e — IDFFCOTUtils without affiliation config (NPE)
File: .../federation/meta/IDFFCOTUtils.java
Problem: getAffiliationDescriptorConfig().getValue() — for ordinary SP/IDP entities the getter is null, so .getValue() fails before the dead if (affiConfig != null) check. 2 sites (add/remove EntityConfig).
Fix: null-check the wrapper before .getValue().
9. 5aa4d1c5e3 — Liberty Personal Profile containers (NPE)
Files: IDPPBaseContainer.java + IDPPFacade, IDPPDemographics, IDPPLegalIdentity, IDPPEmploymentIdentity, IDPPCommonName, IDPPAddressCard
Problem: read-back methods (getXxxMap/createAddressCard) do obj.getXxx().getValue() on optional PP elements; for absent ones (common, e.g. middle name) → NPE. The old code stored the nullable value and downstream getAttributeMap handled null fine.
Fix: add protected static <T> T jaxbValue(JAXBElement<T>) to the base class and apply it at every read site.
10. f77dbb76bd — OAuth2 SAML2 bearer, unknown issuer (NPE)
File: openam-oauth2-saml2/.../core/Saml2GrantTypeHandler.java
Problem: metaManager.getIDPSSODescriptor(...) can return null for an unknown issuer; idpSsoDescriptor.getValue() → NPE. Previously null was handled gracefully (clean rejection). This turns a rejection into an uncaught exception.
Fix: null-check the descriptor → InvalidGrantException("Unknown assertion issuer").
11. f168117cc6 — IDP proxy: authn-context matching (logic)
File: .../saml2/plugins/SAML2IDPProxyFRImpl.java
Problem: AttributeValueElement.getValue() returns a scalar (String) for string-valued attributes, not a List, so the instanceof List guard silently skipped the whole proxy-IDP authn-context selection block (a silent regression, no error).
Fix: normalize scalar and list content to List, restoring the previous getContent() behaviour.
Verification
- Build (JDK 26 / Maven 3.9.16) of the affected modules —
BUILD SUCCESS:
Federation Library, OpenFM, Admin Console, OAuth2-SAML2. - Schema modules (saml2/liberty/wsfederation/xacml3) build and generate classes correctly.
- The cumulative diff of all 11 commits is byte-identical to the patch that was built and verified.
- Push to the PR branch was a fast-forward (
63ff4448ae..f168117cc6).
Still to verify manually (not covered by compilation)
- JSPs (not compile-checked):
SA_IDP.jsp,SA_SP.jsp,spSingleLogoutInit.jsp,realmSelection.jsp. - Functionally: SAML2 SLO (SOAP + multi-protocol), attribute query, COT add/remove (SAML2 / WS-Fed / IDFF), Liberty Personal Profile, OAuth2 SAML2 bearer.
- RestAuthNameValueOutputCallbackHandler.java: keep correct NameValueOutputCallback Javadoc - ResourceOwnerSessionValidator.java: merge copyright attributions
vharseko
left a comment
There was a problem hiding this comment.
Review: JAXB 1.x → JAXB 3 migration
Deep review of the full diff vs merge base 0ee32ac4 (2,787 files, +4,327/−541,024). The build/codegen architecture was verified empirically (regenerated all three schema modules offline with the PR's plugin config and bindings, plus an unmarshal/marshal runtime probe with jaxb-runtime 3.0.2 on the PR's own test resources). The remaining findings are call-site bugs of the same classes already fixed in the follow-up commits on this branch.
What checks out (verified, not just read)
- Package parity confirmed: codegen reproduces every deleted JAXB1 package exactly; every class-bearing package gets an
ObjectFactory, so allJAXBContext.newInstance(contextPath)call sites resolve. Metadata + entity-config round-trip probe passes. - All 31
jaxb.propertiescorrectly rewritten toorg.glassfish.jaxb.runtime.v2.ContextFactory(the deprecatedjakarta.xml.bind.context.factorykey is accepted by the 3.0ContextFinder). xs:dateTimekept onjava.util.CalendarandanyURIonStringviaglobalBindings— avoids theXMLGregorianCalendarbreak;*Elementclasses recreated viajxb:classasJAXBElement<T>subclasses; the curation from ~127 to ~35 element classes in saml2 leaves zero dangling references.- Prefix-mapper key change to
org.glassfish.jaxb.namespacePrefixMapperis correct and required (the oldcom.sun.xml.bind.*key throwsPropertyExceptionon the new runtime — verified). - No
javax.xml.bindreferences remain anywhere in main sources. No hand-fixes were lost in the deleted generated code (checked full git history of those files). - Nice bonus fixes: the pre-existing
iter/iterVbug inAttributeQueryUtil.convertAttributes, null check inSaml2GrantTypeHandler; newSAML2MetaUtilsTest+ Playwright REST-STS e2e test.
Blocking bugs
CI is green because it compiles and runs unit tests, but these break at runtime ("needs testing" label is accurate).
WS-Federation — broken entirely
WSFederationMetaUtils.getAttributes(~line 231): casts members ofList<AttributeElement>toAttributeType→ guaranteedClassCastException. Every WSFed extended-config read funnels through this helper (RPSigninRequest/RPSigninResponse, account/attribute mappers,WSFederationUtils.createSAML11Token).WSFederationMetaUtils.getAttribute(~line 296):avp.getName()now resolves toJAXBElement.getName()returning a QName;equals(String key)is always false → every lookup (SIGNING_CERT_ALIAS,AUTH_URL,ASSERTION_TIME_SKEW,ACTIVE_REQUESTOR_PROFILE_ENABLED,ENDPOINT_BASE_URL, NameID settings) silently returns null. Needsavp.getValue().getName().
IDFF — broken entirely
IDFFMetaUtils.getAttributes(lines 297–306): same(AttributeType)cast on aList<AttributeElement>→ CCE. This is the central helper for the whole IDFF stack (FSAssertionManager, FSAuthnDecisionHandler, attribute mappers, IDP proxy, …).IDFFCOTUtils.updateCOTAttrInConfig(line 226) /removeCOTNameFromConfig(line 263):(BaseConfigType)and(AttributeType)casts on element lists, and a rawAttributeTypeadded into aList<AttributeElement>. The create path in the same file was migrated; the update helpers were missed — adding/removing an IDFF entity to/from a COT fails whenever the entity has an entity config.federation/key/KeyUtil.getKeyDescriptor(~295–318):kd.getValue().getUse().value()NPEs whenuseis absent (the followinguse == nullhandling is now dead), and the no-match branch returnsnoUsageKD.getValue()→ NPE where the old code returned null to null-tolerant callers.IDFFProviderManager(~128/188/214),LibertyManager(~1276/2025),EncryptedNameIdentifier(~225):.getValue()chained onto nullablegetSPDescriptorConfig(...)before the null check → NPE for IDP-only entities; the SP→IDP fallback logic is now dead code.
SAML2 — optional-Boolean unboxing NPEs (highest-impact class)
JAXB1 generated primitive boolean for optional XSD attributes (absent → false); XJC 3 generates nullable Boolean (attributes are use="optional" with no default in the XSDs). Direct unboxing now NPEs whenever the attribute is absent — which is schema-legal and common in third-party metadata:
IDPSSOUtil.java:1865, 1953, 2026—acs.getValue().isIsDefault()looping over all ACS entries of the remote SP;getDefaultACSurlruns on the common no-ACS-in-request path → IDP-side SSO fails for typical third-party SP metadata with any ACS entry lackingisDefault.SPSSOFederate.java:377, 448, 1000;SAML2Utils.java:495(verifyResponse);IDPArtifactResolution.java:472;IDPSSOUtil.java:3017;UtilProxySAMLAuthenticator.java:161;IDPProxyUtil.java:222;SPACSUtils.java:511;QueryClient.java:811;QueryHandlerServlet.java:421—isWantAssertionsSigned()/isAuthnRequestsSigned()/isWantAuthnRequestsSigned().isHosted()— same class:SAML2MetaManager.java:1237/1408/1494/1532,SAML2MetaSecurityUtils.java:182/475,IDFFMetaManager.java:837/871,WSFederationMetaManager.java:833/896/982,ImportMetaData.java:192/264/324, OpenFM workflow tasks, consoleTaskModelImpl. OpenAM always writes the attribute, but hand-imported extended configs may omit it.- Systematic fix:
Boolean.TRUE.equals(...)at call sites (the PR already does exactly this forisPassive()inUtilProxySAMLAuthenticator), or XSD defaults, or an xjb customization. Note the added test resources set every optional boolean (hosted="1",isDefault="1", …), which is why tests don't catch this class — please add a metadata fixture with the optional attributes omitted. - (IDFF metadata side is not affected:
lib-arch-metadata.xsdhasdefault="false"— checked.)
SAML2 — other
AttributeQueryUtil.isSameAttribute:988—desired.getName().equals(supported.getName())comparesStringtoQName(always false) → every AttributeQuery naming specific attributes is rejected with "Attribute name not supported". Needssupported.getValue().getName().SAML2MetaSecurityUtils.setExtendedAttributeValue:565—(AttributeType)cast on anAttributeElementlist → CCE; breaksssoadm update-entity-keyinfo(the add at line 575 was fixed, the removal loop wasn't).SAML2MetaSecurityUtils.updateProviderKeyInformation:483-495—.getValue()NPEs before the intendedentityNotIDP/entityNotSPerrors (now dead code).SAML2ProviderManager(lines ~96, ~167, ~191, ~216) —getSPSSOConfig(...).getValue()NPEs for IDP-only providers; the IDP fallback in all four methods is dead; the catch handlesSAML2MetaExceptiononly.
Liberty PP / Discovery / WSS Policy
WSSPolicyManager(lines 120, 131, 249–250, 302–303):createExactlyOneElement(createPolicyElement())substitutes the anonymousPolicytype whereOperatorContentTypeis expected — the runtime reportsSUBSTITUTED_BY_ANONYMOUS_TYPEand marshalling aborts →getPolicy()/getInputPolicy()/getOutputPolicy()always throw. UsecreateOperatorContentType().IDPPEncryptKey/IDPPSignKey(getContainerObject): bareX509DataTypeadded intoKeyInfocontent (an@XmlElementRefslist) →MarshalException: missing an @XmlRootElement annotation. Should beof.createX509DataElement(x509DataType)(the read paths in the same files already expectX509DataElement).IDPPLegalIdentity:489—(AltIDType)cast afterinstanceof AltIDElement→ CCE (AltIDElement extends JAXBElement<AltIDType>now);:105—createAltIDElement(getAltID(userMap))is never null, so an empty<AltID/>is always added where the old code added nothing.IDPPDemographics:281→IDPPBaseContainer.getAttributeMap:List<LanguageElement>members match neitherinstanceof DSTStringnorDSTURI→ PPLanguagevalues silently dropped on read and potentially wiped on modify.DiscoveryService(~221, ~362):getEncryptedResourceID().getValue()NPEs when both ResourceID variants are absent.
Console
ImportEntityModelImpl(~145createSAMLv2Entity, ~316createWSFedEntity):(BaseConfigType) config.iterator().next()onList<JAXBElement<BaseConfigType>>→ CCE when importing hosted entities with extended metadata via the console (the identical pattern was fixed in OpenFMImportSAML2MetaData/GetCircleOfTrusts; the console counterpart was missed).SAMLv2ModelImpl:2035, 2096—(AttributeType)casts → CCE opening the XACML PEP/PDP extended-metadata pages (lines 1605/1676 in the same file show the correct pattern).SAMLv2ModelImpl:1733-1737—getClass().getName().contains("KeySizeImpl")matches nothing under JAXB3 (no*Implclasses) → encryption key size silently never read. Should beinstanceof EncryptionMethodType.KeySize.IDFFModelImpl:497, 565, 1247, 1306, 1494;SAMLv2ModelImpl:2321, 2380—.getValue()before the null check: first-time access to an IDFF entity's IDP/SP tab now NPEs instead of auto-creating the default extended config;invalid.entity.nameerrors degraded to NPE.SMDiscoEntryData(~212–267): directives unwrapped to bareDirectiveTypeand added to an@XmlAnyElementlist →MarshalException+ element identity (which directive) lost; the read path checksinstanceof AuthenticateRequesterElementetc., so the round-trip is broken. Add theJAXBElementwrappers, not their values.
Build / packaging
openam-clientsdk/pom.xml(~146): shade includes the non-existent coordinatejakarta.xml.bind:jaxb-api(correct:jakarta.xml.bind:jakarta.xml.bind-api) and omits jaxb-runtime's transitive deps (org.glassfish.jaxb:jaxb-core,com.sun.istack:istack-commons-runtime,org.glassfish.jaxb:txw2,jakarta.activation:jakarta.activation-api) → standalone clientsdk consumers getNoClassDefFoundErroron first JAXB use.openam-wsfederation-schema/pom.xml:85-88: leftovercom.sun.xml.bind:jaxb1-impldependency (removed from saml2/liberty poms in this PR) — drags the dead JAXB1 runtime into every WAR. Related:openam-auth-saml2 SAML2ProxyTest.java:26still importscom.sun.xml.bind.StringInputStream(exists only in jaxb1-impl).
Risks / cleanups (non-blocking)
- Sweep the
.getValue()-before-null-check pattern — beyond the cases above there are ~a dozen more (e.g.SAML2Utils.java:556/2194/3556,SPACSUtils.java:392,IDPSessionListener.java:159,NameIDMapping.java:206,DoManageNameID.java:291,AssertionIDRequestUtil.java:405, multiprotocolIDFFSingleLogoutHandler:226,SingleLogoutManager:689,ValidRelayStateExtractor,DefaultAttributeMapper,ValidWReplyExtractor,ConfigureGoogleApps:94). Formerly handled null-metadata cases now surface as NPEs. BaseConfigTypeflippedabstract="true"→"false"in three XSDs — necessary for JAXB3 instantiation and only used as codegen input, but worth recording as intentional in the PR description (which is currently empty).- Split package:
org.w3._2001.xmlschema.Adapter1is now generated into both the liberty and wsfederation jars (same bytecode; duplicate-class checkers/JPMS will complain). saml2 avoids this via itsschema.xsdpackage-mapping trick — the same could be applied to wsfed. FAMClassLoader(OpenFM): lists now internally inconsistent —classesToBeLoadedsaysjakarta.xml.bind.butmaskedResoucesstill masksjavax/xml/bind/, andcom.sun.xml.bind.*(JAXB1 RI) entries remain.- Dead JAXB1 cruft to delete with this PR: ~25
bgm.sergrammar caches still packaged in the schema jars; JAXB1 entries inTHIRD-PARTY.properties;com.sun.xml.bind.util.ListImpl/ProxyListImplwhitelist entries inIOUtilsandserverdefaults.properties:191; root pom<jaxb1.version>property; orphanjaxb.propertiesin packages with no classes (.../common/jaxb/xmlsec/, parentcommon/jaxb/). PPRequestHandler: stray junk importorg.glassfish.jaxb.core.v2.model.core.ID;getMarshaller().setProperty(...); getMarshaller().marshal(...)uses two different marshaller instances so the prefix mapper never applies (pre-existing).SLOLocationTest:createSingleLogoutServiceElement(of.createAttributeServiceType())works only becauseAttributeServiceType extends EndpointType— usecreateEndpointType().- Upgrade note: previously serialized blobs containing JAXB1
com.sun.xml.bind.util.ListImplinstances can no longer deserialize after upgrade (class gone from classpath).
Suggested order of fixes
WSFederationMetaUtils.getAttributes/getAttribute,IDFFMetaUtils.getAttributes,IDFFCOTUtilsupdate helpers (restores WSFed + IDFF).- Systematic optional-
Booleansweep (Boolean.TRUE.equals(...)or XSD defaults) + a test metadata fixture with optional attributes omitted. .getValue()-before-null-check sweep.WSSPolicyManager, PP containers,SMDiscoEntryData, console CCEs (ImportEntityModelImpl,SAMLv2ModelImpl).- clientsdk shade config + jaxb1-impl removal.
Resolves the runtime defects found reviewing the JAXB1->JAXB3 migration: - WSFederation/IDFF metadata helpers: unwrap AttributeElement/JAXBElement lists (ClassCastException) and compare element value names, not QNames - SAML2/WSFed/IDFF: guard optional Boolean metadata attributes with Boolean.TRUE.equals(...) (absent attribute is false, not an NPE) on the core SSO, artifact, logout and query paths - KeyUtil/SAML2MetaSecurityUtils/SAML2ProviderManager and ~12 other sites: null-check JAXB elements before getValue() so designed null/fallback paths run instead of throwing - Liberty PP, WS-Policy and Disco: marshal element wrappers (not bare types) so KeyInfo/Policy/Directive content serialises - Console: fix entity-import CCEs, XACML PEP/PDP casts, key-size read and extended-config auto-creation - clientsdk shade: correct jakarta.xml.bind-api coordinate and add the jaxb-runtime transitive stack - Build cleanup: drop leftover jaxb1-impl, dead bgm.ser/jaxb.properties, jaxb1 THIRD-PARTY entries and jaxb1.version; move wsfed Adapter1 out of the split org.w3._2001.xmlschema package - Add sp-metadata-no-optional-booleans.xml + SAML2MetaUtilsTest coverage pinning the nullable-Boolean generated shape
Fixes pushed —
|
Optional config/metadata getters now return JAXBElement wrappers; several call sites unwrapped them with a bare .getValue(), throwing NPE on entities that lack an extended/role config where the pre-migration code degraded gracefully (null flowed through to a null-tolerant consumer). Add a null-safe unwrap() helper to SAML2MetaUtils / IDFFMetaUtils and route the affected sites through it, or guard inline. HIGH: - IDPPBaseContainer: unwrap the JAXBElement in the scalar attribute branch (mirroring the list branch) so Personal Profile scalar values are no longer silently cleared on Modify. - DiscoveryService.getResourceID: accept and marshal the EncryptedResourceIDElement (a JAXBElement) instead of the non-root EncryptedResourceIDType, which failed to marshal and misrouted encrypted resource-ID lookups to the implied-resource (B2E) path. - IDFFMetaUtils.getExtendedConfig, SAML2MetaManager (PEP branch guard to match the PDP branch), AssertionIDRequestUtil, SPSessionListener, AuthnQueryUtil. MED (unguarded .getValue() on a nullable config getter whose consumer tolerates null; NPE otherwise escaped an IDFFMetaException-only catch): - IDPSingleLogout, IDPSessionListener, SPSingleLogout (idpConfig is null for all non-SOAP bindings), QueryClient, FSLoginHelper, FSServiceManager, FSAttributeStatementHelper, FSPreLogin, FSReturnLogoutServlet, FSTerminationReturnServlet, FSPostLogin, FSIDPFinderService, FSAssertionManager, FSSingleLogoutHandler. Cleanup: - Cache DatatypeFactory in a static field in Time.java. - Drop the orphaned jaxb.version property and the redundant JAXB version pins / commented-out JAXB2 dependency blocks in openam-core.
…ert lookup, PP EncryptedResourceID and null-guard regressions - LogoutUtil: accept both SingleLogoutServiceElement wrappers and unwrapped EndpointType items in getSLOServiceLocation/getSLOResponseServiceLocation (fixes session-sync SLO ClassCastException in IDP/SP session listeners) - WSFederationMetaUtils.findTokenSigningCertificate: unwrap X509DataElement in lax any content so remote WS-Fed IdP token signature verification works - DSTRequestHandler: marshal EncryptedResourceIDType via its element wrapper (raw type has no @XmlRootElement) — fixes PP Query/Modify with encrypted resource IDs - SAML2Utils.getAttributeValueString: extract text from anyType-bound AttributeValue payloads (DOM nodes after JAXB3); use in SAML2IDPProxyFRImpl and AttributeQueryUtil - IDPPBaseContainer/IDPPAddressCard: unwrap JAXBElement for PP elements without generated element classes (AnalyzedName, address-card leaves) - Null-guard absent extended entity config: SPSessionListener, SAML2SingleLogoutHandler, FSServiceManager, FSAssertionManager, FSIDPFinderService, FSAccountManager, NameIDMapping - WSFedPropertiesModelImpl: mutate TokenIssuerName value in place to keep extensible attributes - FAMClassLoader: restore javax/com.sun.xml.bind masks (provided by bundled Metro jars); let jakarta JAXB delegate to the webapp classloader - IDPPBaseContainer.getDSTMonthDay: accept legacy MM-DD directory values - Tests: SLO location lookups for both list shapes, WS-Fed token certificate round-trip, AttributeValue text extraction, EncryptedResourceID marshalling
Fix report for the review findingsAll findings from the review above — high, medium and low — are addressed in d020a36 (22 files, +388/−78). High severity
Medium severity
Low severity
Tests added
Verification
One note for anyone building this branch locally: Maven's |
…te-2026-05-19 # Conflicts: # openam-federation/openam-federation-library/src/main/java/com/sun/identity/liberty/ws/interaction/WSPRedirectHandlerServlet.java
JAXB 3 keeps whitespace-only text in @XmlMixed content lists, so casting index 0 of EncryptionMethod or KeyInfo content threw ClassCastException for indented metadata, which OpenAM's own templates write. - saml2 KeyUtil: read KeySize by type in getEncInfo and getPEPEncInfo - ID-FF KeyUtil.getCert: find X509Data by type in KeyInfo content - console SAMLv2ModelImpl: cast the matched KeySize, not index 0 - document the JAXB 1 to JAXB 3 API break in the upgrade guide - fix stale RequestedServiceType Javadoc in DiscoEntryHandler and impls - parse gMonthDay values with the shared DatatypeFactory in Time - extract DSTRequestHandler.toDocument so its test reaches the fix - add tests for KeySize/X509Data lookup, absent isDefault ACS fallback, WS-Fed getAttributes and the gMonthDay prefix normalisation - drop the duplicate copyright line in WSSPolicyManager
getUserDiscoEntries drops every stored entry it cannot unmarshal and asks the caller to store the rest, so a regression in the JAXB 3 unwrapping would silently delete a user's discovery entries. Cover read-back with and without an entryID, a malformed entry, and a write/read round trip through setUserDiscoEntries.
…on metadata JAXB 3 does not bind the root element's namespace to the default namespace and declares every namespace on the root element. String post-processing of SAML2 exports relied on the JAXB 1 layout: - workaroundAbstractRoleDescriptor wrote the RoleDescriptor of an attribute query descriptor in no namespace, so import dropped it - CreateFedlet.flipHostedParameter no longer found "<EntityConfig " and left the remote IdP marked as hosted in the Fedlet bundle - the import parses each RoleDescriptor on its own, and its xsi:type prefix was declared only on the root, so the import failed SAML2MetaUtils now marshals with the root namespace as the default namespace, and the export declares the query prefix on the RoleDescriptor again. ExportMetaData uses the library workaround instead of its own copy. The JAXB 3 unmarshaller also recovers from errors by dropping an unexpected element, where JAXB 1 rejected the document. SAML2MetaUtils, IDFFMetaUtils and WSFederationMetaUtils now fail on the first error, so importing metadata with, for example, a KeyDescriptor in the wrong namespace fails instead of losing the certificate. - console SAMLv2ModelImpl reads the key size through KeyUtil.getKeySize - upgrade guide: list the xs:anyType and mixed-content differences - make the indented ID-FF test fixture schema-valid - add tests for the export namespace and re-import, the EntityConfig root, rejection of unexpected elements and KeyUtil.getKeySize
JAXB 3 keeps the whitespace-only text of mixed-content types, and the formatted marshaller indents around it again, so every console save or export and import of SAML2, ID-FF and WS-Fed metadata added blank lines inside KeyInfo, EncryptionMethod and the other key and method types. The three metadata utilities now drop that text on unmarshal through MixedContentWhitespaceListener. Types whose text can be content are not registered. An md:RoleDescriptor with an xsi:type JAXB 3 does not know, such as the WS-Federation roles of AD FS metadata, failed the import because RoleDescriptorType is abstract. The import now removes every RoleDescriptor other than an attribute query descriptor before unmarshalling. It also copies the live NodeList first, so a second attribute query descriptor is no longer skipped. When the JAXB package list adds an element in no namespace, JAXB 3 binds the default namespace to "" and prefixes the metadata namespace. The exported RoleDescriptor now declares the metadata namespace itself, and CreateFedlet.flipHostedParameter matches a prefixed EntityConfig root. - KeyUtil tests put the whitespace back themselves, so they still pin the lookup by type - pin that the Liberty ID-WSF unmarshallers stay lenient - upgrade guide: say which unmarshallers drop mixed-content whitespace
…te-2026-05-19 # Conflicts: # openam-documentation/openam-doc-source/src/main/asciidoc/upgrade-guide/chap-upgrade-servers.adoc
- SAML2IDPProxyFRImpl: the three extension/attribute guards used `x != null || !x.isEmpty()`, which dereferences x exactly when it is null. Use `&&` (CodeQL java/dereferenced-value-is-always-null). - IDFFModelImpl: drop the private static supportedSSOProfileList field and its initializer; nothing read it, the other uses in the file are local variables of the same name (java/unused-container). - WSFedPropertiesModelImpl: pass the unwrapped baseConfig to updateBaseConfig, as the SP branch already does (java/local-variable-is-never-read). All three were already flagged on master; the migration rewrote the lines, so CodeQL reported them again on the pull request.
…lder (#1186) Since `ubuntu-latest` moved to the `ubuntu-26.04` runner image, `build-maven` fails in `openam-core-rest` on every Ubuntu leg (JDK 11, 17, 21, 25, 27). It was first seen in run [39217607024](https://github.com/OpenIdentityPlatform/OpenAM/actions/runs/39217607024) on #1023, whose changes do not touch this module: `openam-core-rest` there is identical to `master` (d75bdb8). The same tests were green on `ubuntu-24.04` three days earlier, and are green on macOS and Windows in the failing run. ## Cause `InjectorHolder` builds its injector once per JVM, from the module loader `InjectorConfiguration` holds at that moment. The modules on this test classpath cannot be built (`No implementation for NotificationBroker was bound`). The first test that touches `InjectorHolder` with the default loader therefore leaves the class unusable for the rest of the fork. `SessionResourceV2Test.setUp` constructs a real `AMIdentity`. Its constructor asks `InjectorHolder` for a `RepoAuditorFactory` and swallows the failure (`AMIdentity.java:190`). `IdentityResourceNotificationTest` and `SessionResourceTest` installed an empty loader, but in `@BeforeClass`, which only helped when they ran before `SessionResourceV2Test`. Surefire runs the classes in file-system order, and on `ubuntu-26.04` that order puts `SessionResourceV2Test` first: - `IdentityResourceNotificationTest.setupMocks:155` fails with `NoClassDefFoundError: Could not initialize class org.forgerock.guice.core.InjectorHolder`, through `IdentityServicesImpl.<clinit>`. - That `@BeforeMethod` fails after `RealmTestHelper.setupRealmClass()`, so the `@AfterMethod` teardown never runs. As a result, `OathDevicesResourceTest`, `PushDevicesResourceTest`, `TrustedDevicesResourceTest` and `SmsResourceProviderTest` fail with `Previously setup Realm class - forgot to call teardown`. ## Change Test code only. - `IdentityResourceNotificationTest` installs the empty loader from `@BeforeSuite(alwaysRun = true)` instead of `@BeforeClass`. `openam-core-rest` sets no `forkCount`, `parallel` or `suiteXmlFiles`, so surefire runs all its classes in one TestNG suite, and every `@BeforeSuite` method runs before the first test, whatever order the classes are in. The tests get the same empty injector they got on the green runs. Only the moment the loader is installed changes. - `SessionResourceTest` and `SessionResourceV2Test` no longer build a real `AMIdentity`, whose constructor reached `InjectorHolder`. They return a mock with `getName()` and `getRealm()` stubbed, the only two getters the session code reads, and they no longer install a loader. - The `IdentityResourceNotificationTest` javadoc states the limits of the hook: TestNG instantiates every test class before any `@BeforeSuite` method runs, and the hook covers the other classes only while the module runs as one TestNG suite in one JVM. ## Tests `mvn -o -pl openam-core-rest test -Dsurefire.runOrder=<order>`, run locally on macOS: - `reversealphabetical` at `master` puts `session.SessionResourceV2Test` ahead of `IdentityResourceNotificationTest`, as on `ubuntu-26.04`. Result: `IdentityResourceNotificationTest.setupMocks` fails with the CI error, caused at `SessionResourceV2Test.setUp:112`. The default order is green at `master`. - With the first version of this change (5f9ef67, a `@BeforeSuite` hook in all three classes), `reversealphabetical`, `alphabetical`, `filesystem` and `random` with seeds 1, 42 and 20261019 all pass: 446 passed, 0 failed, 0 skipped each, including `IdentityResourceNotificationTest` 8/8, `SessionResourceTest` 33/33 and `SessionResourceV2Test` 26/26. - Each of the three classes alone (`-Dtest=<class>`) passes. - At 0c41e0a (after review): `reversealphabetical`, the default order and `-DreuseForks=false` (53 classes, each in its own JVM) all pass, 446/0/0. With the `IdentityResourceNotificationTest` hook put back on `@BeforeClass`, `reversealphabetical` still passes (446/0), so neither session class reaches `InjectorHolder`. With `SessionResourceV2Test` building a real `AMIdentity` again, the same run fails as on `ubuntu-26.04`. Locally, the `ubuntu-26.04` order is only approximated by `reversealphabetical`. This PR's own CI run checks the real image.
…te-2026-05-19 Conflicts with the GHSA series on master: - SAML2Utils.isSPProfileBindingSupported: keep master's null check on the SP descriptor and the JAXB 3 endpoint list type. - SPACSUtils: keep master's GHSA-3j2f comment and the getValue() unwrap. - ValidRelayStateExtractor (SAML2), ValidWReplyExtractor (WS-Fed): keep master's role resolution through the getSPSSOConfig/getIDPSSOConfig seams, and unwrap the JAXB 3 config element inside the seams so they still return BaseConfigType. - LibertyManager, SAML2UtilsTest: keep the imports and tests of both sides. - chap-upgrade-servers.adoc: keep master's PKCE and SAML 1.x notes and the JAXB 3 note as separate blocks. - Copyright header lines: take master's. Master code that merged cleanly but was written against JAXB 1: - SPACSUtils.getResponseFromArtifact: unwrap the SP descriptor and read the nullable WantAssertionsSigned with Boolean.TRUE.equals. - ID-FF ValidRelayStateExtractor: unwrap SP/IDP descriptor configs. - Tests (SAML2UtilsTest, FSPostLoginTest, HostedProviderFixture, LibertyManagerTest, the three relay state / wreply extractor tests): build the config and descriptor wrappers the way JAXB 3 returns them, with entity config attributes as AttributeElement.
|
@maximthomas Merged current Conflicts
Master code that merged without conflicts but was written against JAXB 1
The build does not compile JSPs, so I compared their imports by hand after the merge: none were lost, and the JSP changes from master do not touch JAXB types. Verification
One leftover, not changed in the merge: |
vharseko
left a comment
There was a problem hiding this comment.
Reviewed head 564e8ff383ec6d5c76bffea14f7d78486656d047 against merge base 4d5cca0e5d52a4cad9633f1a042a92a40865cc01, including the complete discussion history and linked issues #980 and #1185.
No new actionable code defects found in the areas reviewed: JAXB element/value conversions, optional metadata values, mixed-content handling, namespace round trips, federation call sites, dependency changes, and the changed server JSPs. Previously resolved findings and the explicitly tracked client SDK limitation in #1185 are not being raised again.
Validation: a fresh local reactor build regenerated the schema classes and passed 62 targeted federation tests on JDK 27 / Maven 3.10.0, with zero failures, errors, or skips. These cover SAML2/ID-FF/WS-Fed metadata, key extraction, SLO endpoint lookup, ACS defaults, attribute values, encrypted resource IDs, and Personal Profile dates. The nine Maven CI matrix jobs and Docker build are also green for this head.
CodeQL remains red. Its critical annotation points to the unchanged deserialization call in IOUtils; this PR only removes the obsolete JAXB 1 classes from that file's whitelist. I did not identify a new deserialization defect introduced by this change.
Integration is currently blocked by a conflict with origin/master (a13a83cb78) in openam-documentation/openam-doc-source/src/main/asciidoc/upgrade-guide/chap-upgrade-servers.adoc. GitHub reports CONFLICTING, and a local merge-tree check confirms the conflict. Validation above applies to the current PR head, not a future conflict resolution.
Limits: this review did not repeat deployed end-to-end or mixed-version upgrade testing. The legacy multiprotocol JSP samples still contain JAXB 1 accessor calls, but I could not establish that the current Maven distribution packages them, so I am recording this as a follow-up caveat rather than a blocking finding.
…te-2026-05-19 # Conflicts: # openam-documentation/openam-doc-source/src/main/asciidoc/upgrade-guide/chap-upgrade-servers.adoc
The Fedlet index.jsp and fedletSampleApp.jsp include header.jspf, which still called getSingleSignOnService(), getAssertionConsumerService(), getBinding() and getLocation() on the IDPSSODescriptorElement, SPSSODescriptorElement, SingleSignOnServiceElement and AssertionConsumerServiceElement wrappers. Under JAXB 3 these are JAXBElement subclasses without those methods, so Jasper failed to compile both pages. The default build does not precompile JSPs, so only a request at runtime showed it. Read the content through getValue(), as SA_IDP.jsp already does.
|
Addressed in 756605d and 6af59de:
|
vharseko
left a comment
There was a problem hiding this comment.
Reviewed head 6af59de75ac8c986c3dfc014981f806583b06d6e, focusing on the changes since the previous review at 564e8ff383ec6d5c76bffea14f7d78486656d047: the merge of current master (756605d8dd) and the Fedlet JSP fix (6af59de75a). I read the complete PR discussion, including earlier reviews and author replies, and linked issues #980 and #1185.
No new actionable defects found in this follow-up review.
- The merge conflict is resolved correctly: all three new upgrade notes from master and the JAXB 3 migration note are preserved as separate blocks. The current master (
a13a83cb78) is an ancestor of this head, and GitHub reports MERGEABLE. - The Fedlet fix correctly unwraps the descriptor and endpoint elements. I compiled both complete consuming pages,
index.jspandfedletSampleApp.jsp, with Tomcat Jasper 10.1.41 against the freshly built JAXB 3 classes: both passed and produced servlet class files. Repeating the same compilation with the pre-fixheader.jspffailed on the six obsolete accessor calls in each page, confirming that the fix removes the compilation regression. - A fresh Maven reactor build regenerated the federation schemas and passed all 62 selected federation tests on JDK 27 / Maven 3.10.0, with zero failures, errors, or skips.
- The Maven packaging configuration does not include the legacy
OpenFM/src/main/samplestree. The earlier sample-JSP caveat and the separately tracked client SDK limitation in #1185 are not being raised as new findings.
Validation limits: this was a follow-up review of the two new commits and their integration, not a repeat of every earlier migration review. Deployed SSO/SLO, end-to-end flows, and mixed-version upgrades were not exercised. CI for this exact head is still queued or in progress, so its final result remains outstanding.
Summary
Moves the generated federation schema classes from JAXB 1 to JAXB 3 (
jakarta.xml.bind), which removes the last JAXB 1 runtime from OpenAM. It then fixes the call sites that relied on JAXB 1 shapes.Build and dependencies
openam-saml2-schema,openam-liberty-schemaandopenam-wsfederation-schemano longer commit JAXB 1 xjc output. That removes 2,443 generated Java files and thebgm.sergrammar caches. The classes are now generated at build time fromsrc/main/xsdbyorg.jvnet.jaxb:jaxb-maven-plugin3.0.2.jakarta.xml.bind:jakarta.xml.bind-api3.0.1 andorg.glassfish.jaxb:jaxb-runtime3.0.2. The PR removesjaxb1-impl,javax.xml.bind:jaxb-api,com.sun.xml.bind:jaxb-coreandcom.sun.xml.bind:jaxb-impl, and the client SDK shading now includes the JAXB 3 artifacts. The shaded runtime does not yet serve the federationJAXBContexts in the client SDK, which was already the case with JAXB 1; see Client SDK: shaded JAXB runtime is not found through jaxb.properties #1185.jaxb.propertiesfiles now point atorg.glassfish.jaxb.runtime.v2.ContextFactory, and three are deleted.com.sun.identity.entitlement.xacml3.core) move fromjavax.xml.bindtojakarta.xml.bind.com.sun.xml.bind.util.ListImplandProxyListImplare removed from the defaultopenam.deserialisation.classes.whitelist, because those classes no longer ship.Deliberate schema change
BaseConfigTypechanges fromabstract="true"toabstract="false"in three XSDs: the SAML2 and WS-Fedentity-config-schema.xsdandidff-entity-config-schema.xsd. JAXB 3 modelsIDPSSOConfig,SPSSOConfigand the other config elements asJAXBElement<BaseConfigType>instead of generating a subtype for each, so the code has to instantiateBaseConfigTypedirectly. These XSDs are only used for code generation. Nothing validates stored configuration against them at runtime.Call-site migration
JAXB 3 differs from JAXB 1 in six ways that affect existing code, and the call sites were updated for each:
*Elementclasses are nowJAXBElementsubclasses, not subtypes of their content type. Lists of elements are unwrapped withgetValue(), and bare types without@XmlRootElementare marshalled throughObjectFactory.create*Element(...). This fixes ClassCastExceptions and NullPointerExceptions in single logout, attribute query, WS-Fed and ID-FF circle-of-trust updates, OpenFM workflows and console metadata import, and marshalling failures in Liberty IDPP and Discovery. The Fedlet'sheader.jspf, included by itsindex.jspandfedletSampleApp.jsp, now reads the SSO and ACS endpoints throughgetValue()as well; before that, Jasper failed to compile both pages.Boolean. The SSO, ACS and metadata guards read them throughBoolean.TRUE.equals(...), so an absent attribute still meansfalse.xs:anyTypeattribute values unmarshal to DOM nodes instead of strings.SAML2Utils.getAttributeValueStringreturns their text, which restores attribute query value matching and IDP proxy authn-context matching.KeyInfo,EncryptionMethod) keep whitespace text.KeySizeandX509Dataare now looked up by type instead of by index.SAML2MetaUtilsnow asks for the JAXB 1 layout. Exported metadata therefore keeps the unprefixedRoleDescriptorof an attribute query descriptor in the metadata namespace, andCreateFedletfinds the<EntityConfigit edits. The export also declares thequeryprefix on thatRoleDescriptoragain, because the import, in this release and in earlier ones, parses the element on its own.SAML2MetaUtils,IDFFMetaUtilsandWSFederationMetaUtilsnow fail on the first error. Importing metadata that has, for example, aKeyDescriptorin the wrong namespace fails again instead of losing the certificate. JAXB 3 still does not report a missing required element, which JAXB 1 did. The Liberty ID-WSF unmarshallers (soapbinding.Utils, Discovery, IDPP) stay lenient.Upgrade impact
This breaks binary compatibility for custom code compiled against the generated federation types,
KeyUtil.getKeyDescriptor(s), or theDiscoEntryHandlerplugin interface. The upgrade guide documents the change inchap-upgrade-servers.adoc.Tests
KeyUtilTest(SAML2 and ID-FF):KeySizeandX509Datalookup in indented metadata, includingKeyUtil.getKeySize, which the console now uses.IDPSSOUtilTest: ACS fallback whenisDefaultis absent.SAML2MetaUtilsTest: unmarshal round-trips, export and re-import of an attribute query descriptor, the unprefixedEntityConfigroot, and rejection of unexpected elements.IDFFMetaUtilsTest,WSFederationMetaUtilsTest: rejection of unexpected elements.WSFederationMetaUtilsTest: token-signing certificate and entity-config attributes.SLOLocationTest,SAML2UtilsTest.DSTRequestHandlerTest:EncryptedResourceIDmarshalling.IDPPBaseContainerTest:gMonthDayvalues.DiscoEntryHandlerImplUtilsTest: reading back stored discovery entries.e2e/sts/rest-sts-test.spec.mjspublishes a REST STS instance and translates an OpenAM token into a SAML2 assertion in the built WAR.Not covered
header.jspfwas checked by compiling its declarations against the generated classes, not in a deployed Fedlet. The sample JSPs underopenam-federation/OpenFM/src/main/samplesstill use JAXB 1 accessors; no build packages them.DiscoEntrywritten by JAXB 1 read back unchanged under JAXB 3, and the other way round. No automated test covers it.