Fix certificate field in IDP export-import with json #1042
Conversation
WalkthroughAdds feature-flagged support for exporting/importing Identity Providers with multiple certificates: new model Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as ServerIdpManagementService
participant Model as IdentityProviderExportResponse
participant Util as CertificateUtil
participant JSON as Gson
Note over Client,Service: Export flow (feature flag ON)
Client->>Service: exportIDP(idp)
Service->>Service: check feature flag
Service->>Model: createIDPExportResponse(idp)
Model->>Util: convertCertificateJsonString(certificate)
Util-->>Model: certificate JSON + thumbprints
Service->>JSON: serialize Model with CertificateSkippingExclusionStrategy
JSON-->>Client: exported file content
Note over Client,Service: Import flow (feature flag ON)
Client->>Service: importIDP(fileContent)
Service->>Service: check feature flag
Service->>JSON: deserialize to IdentityProviderExportResponse
Service->>Service: createIDPImportRequest(deserializedModel)
Service->>Util: parse certificate JSON -> Certificate objects
Util-->>Service: Certificate list
Service->>Service: persist transformed IdentityProvider
Service-->>Client: import result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | ||
| identityProvider = identityProviderManager.addIdPWithResourceId( |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | |
| identityProvider = identityProviderManager.addIdPWithResourceId( | |
| String tenantDomain = ContextLoader.getTenantDomainFromContext(); | |
| log.info("Importing IDP to tenant domain: " + tenantDomain); | |
| identityProvider = identityProviderManager.addIdPWithResourceId( |
...in/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java
Show resolved
Hide resolved
...in/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java
Show resolved
Hide resolved
|
|
||
| /** | ||
| * Utility class for Certificate related operations. | ||
| */ |
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
| */ | |
| public final class CertificateUtil { | |
| private static final Log log = LogFactory.getLog(CertificateUtil.class); |
...er.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java
Show resolved
Hide resolved
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java (2)
26-33: Consider enhancing the class documentation.The JavaDoc could benefit from additional details about:
- When this class is used (export operations with the feature flag enabled)
- The relationship between the certificate field and the legacy certificate format
- The feature flag that controls this behavior
Example enhancement:
/** * Identity Provider Export Response model. + * <p> + * This class is used when exporting Identity Providers with the + * support_multiple_certificate_export feature flag enabled. + * It extends the base IdentityProvider model with a structured + * certificate field that replaces the legacy stringified array format. */
47-58: Add identity check optimization.The equals method can be optimized by checking if the compared object is the same instance before performing more expensive checks.
Apply this diff:
@Override public boolean equals(Object o) { + if (this == o) { + return true; + } if (o == null || getClass() != o.getClass()) { return false; } if (!super.equals(o)) { return false; } IdentityProviderExportResponse that = (IdentityProviderExportResponse) o; return Objects.equals(certificate, that.certificate); }components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java (1)
37-42: Add a private constructor to prevent instantiation.As a utility class with only static methods, it should have a private constructor to prevent instantiation and clearly signal its intent as a utility class.
Apply this diff to add a private constructor:
public final class CertificateUtil { private static final ObjectMapper MAPPER = new ObjectMapper(); private static final Pattern PEM_INNER_BASE64 = Pattern.compile("-----BEGIN CERTIFICATE-----(.*?)-----END CERTIFICATE-----", Pattern.DOTALL); + + private CertificateUtil() { + // Prevent instantiation + }components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java (1)
3825-3837: Consider adding fallback handling for legacy JSON format.When the feature flag is enabled, the code deserializes JSON files to
IdentityProviderExportResponse. If a user imports a JSON file that was exported with the old format (wherecertificateis a stringified array), the deserialization may fail or produce unexpected results because theCertificateobject's structure differs from the legacy string format.Consider adding fallback handling to support both formats during import for better backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java(11 hunks)components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java(1 hunks)components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java (2)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java (1)
IdentityProviderExportResponse(29-65)components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java (1)
CertificateUtil(37-124)
🔇 Additional comments (4)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java (1)
41-45: LGTM!The setter method and
hashCode()implementation follow proper conventions:
- Setter naming matches the field name
hashCode()correctly includes both the superclass hash and the certificate field- Implementation is consistent with the
equals()methodAlso applies to: 60-64
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java (2)
100-112: PEM extraction logic looks correct.The regex pattern correctly matches standard PEM certificate format, and removing whitespace from the inner base64 content is appropriate. The method returns an empty array when PEM format is not detected, which allows the caller to handle the fallback.
Note: The silent return of an empty array when parsing fails is consistent with the method's contract, though as mentioned in a previous comment, the caller's fallback behavior should be reconsidered.
114-123: LGTM: SHA-256 thumbprint computation is correct.The thumbprint computation follows the standard approach: computing SHA-256 digest and converting to lowercase hex string. While
NoSuchAlgorithmExceptionis unlikely for SHA-256 (a required algorithm in the JCA), it's appropriately declared and handled by the caller.components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java (1)
3690-3693: Inconsistent certificate handling across export formats.The TODO comment indicates XML (and similarly YAML at line 3729) export doesn't support the new multiple certificate format. This creates an inconsistency where JSON exports use the structured
Certificateobject format, but XML/YAML exports continue using the legacy format. Users might encounter data inconsistencies when converting between formats.Consider documenting this limitation in the API documentation or release notes.
...rc/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java
Show resolved
Hide resolved
| IdentityProviderProperty[] idpProperties = identityProvider.getIdpProperties(); | ||
| for (IdentityProviderProperty property : idpProperties) { | ||
| if (Constants.JWKS_URI.equals(property.getName())) { | ||
| certificate = new Certificate().jwksUri(property.getValue()); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential NullPointerException when iterating over IDP properties.
identityProvider.getIdpProperties() may return null, which would cause an NPE in the for-each loop.
Certificate certificate = null;
IdentityProviderProperty[] idpProperties = identityProvider.getIdpProperties();
- for (IdentityProviderProperty property : idpProperties) {
- if (Constants.JWKS_URI.equals(property.getName())) {
- certificate = new Certificate().jwksUri(property.getValue());
- break;
+ if (idpProperties != null) {
+ for (IdentityProviderProperty property : idpProperties) {
+ if (Constants.JWKS_URI.equals(property.getName())) {
+ certificate = new Certificate().jwksUri(property.getValue());
+ break;
+ }
}
}🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java
around lines 495 to 501, guard against a potential NullPointerException by
checking whether identityProvider.getIdpProperties() is null before iterating;
if null, skip the loop or treat as empty array/list, otherwise iterate normally
and set certificate when a property with name Constants.JWKS_URI is found;
ensure the null check is concise and maintain existing behavior (break after
setting certificate).
...rc/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java
Show resolved
Hide resolved
| @JsonProperty("certificate") | ||
| public Certificate getCertificates() { | ||
|
|
||
| return certificate; | ||
| } |
There was a problem hiding this comment.
Fix getter naming to match JavaBean conventions.
The getter method is named getCertificates() (plural) while the field is certificate (singular) and the setter is setCertificate() (singular). This violates JavaBean naming conventions and creates confusion about whether the method returns a single object or a collection.
Apply this diff to align the naming:
@JsonProperty("certificate")
-public Certificate getCertificates() {
+public Certificate getCertificate() {
return certificate;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @JsonProperty("certificate") | |
| public Certificate getCertificates() { | |
| return certificate; | |
| } | |
| @JsonProperty("certificate") | |
| public Certificate getCertificate() { | |
| return certificate; | |
| } |
🤖 Prompt for AI Agents
In
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java
around lines 35-39, the getter is named getCertificates() while the field is
named certificate and the setter is setCertificate(); rename the getter to
getCertificate() to follow JavaBean conventions, update any references/usages in
the class or tests to the new method name, and ensure the
@JsonProperty("certificate") remains on the renamed getter so serialization
behavior is preserved.
| byte[] outerDecoded; | ||
| try { | ||
| outerDecoded = Base64.getDecoder().decode(certValue); | ||
| } catch (IllegalArgumentException iae) { | ||
| outerDecoded = certValue.getBytes(StandardCharsets.UTF_8); | ||
| } |
There was a problem hiding this comment.
Clarify the base64 decoding fallback behavior.
The fallback to UTF-8 bytes when base64 decoding fails could lead to processing non-certificate data and computing incorrect thumbprints. If certificates can legitimately arrive in multiple formats (base64-encoded vs. raw), this should be explicitly documented and validated. Otherwise, the silent fallback may hide data quality issues.
Consider one of the following approaches:
- If only base64-encoded certificates are expected, remove the fallback and propagate the exception:
- byte[] outerDecoded;
- try {
- outerDecoded = Base64.getDecoder().decode(certValue);
- } catch (IllegalArgumentException iae) {
- outerDecoded = certValue.getBytes(StandardCharsets.UTF_8);
- }
+ byte[] outerDecoded;
+ try {
+ outerDecoded = Base64.getDecoder().decode(certValue);
+ } catch (IllegalArgumentException e) {
+ throw new IdentityProviderManagementClientException(
+ "Invalid base64-encoded certificate value", e);
+ }- If both formats are valid, add explicit format detection and logging:
byte[] outerDecoded;
try {
outerDecoded = Base64.getDecoder().decode(certValue);
} catch (IllegalArgumentException iae) {
+ // Log or document: treating as raw certificate data
outerDecoded = certValue.getBytes(StandardCharsets.UTF_8);
}🤖 Prompt for AI Agents
In components/org.wso2.carbon.identity.api.server.idp/.../CertificateUtil.java
around lines 73-78, the code silently falls back to treating certValue as UTF-8
bytes when Base64 decoding throws, which can produce incorrect thumbprints or
mask bad input; either (A) if only Base64 is expected, remove the fallback and
rethrow or propagate the IllegalArgumentException so callers can handle/report
invalid input, or (B) if both Base64 and raw binary are supported, implement
explicit detection (e.g., try Base64 and if it fails log a warning with the
input identifier, validate the resulting bytes as a DER/PEM certificate before
using them, and continue only if validation passes), and add descriptive logging
for either branch so malformed data isn’t silently accepted.
| String pemText = new String(outerDecoded, StandardCharsets.UTF_8); | ||
| byte[] derBytes = getDerBytesFromPem(pemText); | ||
| if (derBytes.length == 0) { | ||
| derBytes = outerDecoded; | ||
| } |
There was a problem hiding this comment.
Validate certificate format before computing thumbprints.
When PEM parsing fails (returns empty array), the code falls back to using outerDecoded directly for thumbprint computation. If outerDecoded is not valid DER-encoded certificate data (e.g., it's PEM text with malformed delimiters or other non-certificate data), the computed thumbprint will not be a valid certificate thumbprint. This could lead to incorrect certificate identification.
Consider adding explicit validation or error handling:
String pemText = new String(outerDecoded, StandardCharsets.UTF_8);
byte[] derBytes = getDerBytesFromPem(pemText);
if (derBytes.length == 0) {
- derBytes = outerDecoded;
+ // Attempt to use outerDecoded as raw DER, but validate first
+ // or throw an exception if certificate format cannot be determined
+ if (!isValidDerCertificate(outerDecoded)) {
+ throw new IdentityProviderManagementClientException(
+ "Cannot extract DER bytes from certificate value. " +
+ "Expected PEM format or raw DER encoding.");
+ }
+ derBytes = outerDecoded;
}Alternatively, if the fallback is intentional to support raw DER-encoded certificates, document this behavior clearly in the method's Javadoc.
Committable suggestion skipped: line range outside the PR's diff.
mpmadhavig
force-pushed
the
idp-export
branch
from
1df9812 to
82d971c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java (1)
38-45: Fix getter naming to match JavaBean conventions.The getter method
getCertificates()(plural) doesn't match the field namecertificate(singular) and settersetCertificate(). This violates JavaBean conventions and may cause serialization/deserialization issues with frameworks that rely on consistent naming.@JsonProperty("certificate") -public Certificate getCertificates() { +public Certificate getCertificate() { if (log.isDebugEnabled()) { log.debug("Retrieving certificate from IdentityProviderExportResponse"); } return certificate; }components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java (2)
80-85: Clarify the base64 decoding fallback behavior.The silent fallback to UTF-8 bytes when base64 decoding fails could hide data quality issues and produce incorrect thumbprints for malformed input.
87-91: Validate certificate format before computing thumbprints.When PEM parsing fails, the code falls back to using
outerDecodeddirectly without validation, which could produce incorrect thumbprints for non-certificate data.components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java (2)
463-479: Update comment to accurately describe the exclusion strategy behavior.The comment states it skips "the 'certificate' field derived from the IdentityProvider class," but the code checks for
IdentityProviderExportResponse.class. The strategy actually skips thecertificatefield declared inIdentityProviderExportResponseto avoid type conflicts during Gson serialization/deserialization.
494-501: Potential NullPointerException when iterating over IDP properties.
identityProvider.getIdpProperties()may returnnull, which would cause an NPE in the for-each loop.Certificate certificate = null; IdentityProviderProperty[] idpProperties = identityProvider.getIdpProperties(); - for (IdentityProviderProperty property : idpProperties) { - if (Constants.JWKS_URI.equals(property.getName())) { - certificate = new Certificate().jwksUri(property.getValue()); - break; + if (idpProperties != null) { + for (IdentityProviderProperty property : idpProperties) { + if (Constants.JWKS_URI.equals(property.getName())) { + certificate = new Certificate().jwksUri(property.getValue()); + break; + } } }
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java (2)
39-44: Add private constructor to prevent instantiation.Utility classes should have a private constructor to prevent instantiation. This enforces the static-only usage pattern.
public final class CertificateUtil { private static final ObjectMapper MAPPER = new ObjectMapper(); private static final Pattern PEM_INNER_BASE64 = Pattern.compile("-----BEGIN CERTIFICATE-----(.*?)-----END CERTIFICATE-----", Pattern.DOTALL); private static final Log log = LogFactory.getLog(CertificateUtil.class); + + private CertificateUtil() { + // Utility class - prevent instantiation + }
113-116: Consider logging decode failures for troubleshooting.The
IllegalArgumentExceptionis silently ignored, which could make troubleshooting difficult when certificates have malformed base64 content within valid PEM markers.try { return Base64.getDecoder().decode(innerBase64); - } catch (IllegalArgumentException ignored) { + } catch (IllegalArgumentException e) { + if (log.isDebugEnabled()) { + log.debug("Failed to decode base64 content from PEM certificate", e); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/pr-builder.yml(1 hunks)components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java(11 hunks)components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java(1 hunks)components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr-builder.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mpmadhavig
Repo: wso2/identity-api-server PR: 1042
File: components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java:515-534
Timestamp: 2025-12-10T02:37:06.312Z
Learning: In the file `components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java`, the `createIDPImportRequest` method intentionally supports only JSON format for the new certificate export/import structure when the `SupportMultipleCertificateExport` feature flag is enabled. XML and YAML format support for the new certificate structure is deferred as future work.
📚 Learning: 2025-12-10T02:37:06.312Z
Learnt from: mpmadhavig
Repo: wso2/identity-api-server PR: 1042
File: components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java:515-534
Timestamp: 2025-12-10T02:37:06.312Z
Learning: In the file `components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java`, the `createIDPImportRequest` method intentionally supports only JSON format for the new certificate export/import structure when the `SupportMultipleCertificateExport` feature flag is enabled. XML and YAML format support for the new certificate structure is deferred as future work.
Applied to files:
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.javacomponents/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.javacomponents/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java
🧬 Code graph analysis (1)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java (3)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java (1)
IdentityProviderExportResponse(31-74)components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java (1)
CertificateUtil(39-131)components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.common/src/main/java/org/wso2/carbon/identity/api/server/idp/common/Constants.java (1)
Constants(26-323)
🔇 Additional comments (7)
components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/model/IdentityProviderExportResponse.java (1)
56-73: LGTM!The
equals()andhashCode()implementations correctly delegate to the superclass and include thecertificatefield using null-safe utilities.components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/util/CertificateUtil.java (1)
121-130: LGTM!The
sha256Heximplementation correctly computes the SHA-256 digest and converts it to a lowercase hexadecimal string.components/org.wso2.carbon.identity.api.server.idp/org.wso2.carbon.identity.api.server.idp.v1/src/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java (5)
3690-3693: LGTM!Good practice to document the TODO for future XML support of
MultipleCertificateExport. This aligns with the deferred work mentioned in the learnings.
3825-3837: LGTM!The conditional deserialization correctly routes JSON imports to
IdentityProviderExportResponsewhen theSupportMultipleCertificateExportfeature flag is enabled, enabling the new certificate structure handling.
406-410: LGTM!The integration of
createIDPExportResponsecleanly transforms the IDP for export, enabling the new certificate structure when the feature flag is active while maintaining backward compatibility when disabled.
431-432: LGTM!The
createIDPImportRequestwrapper correctly transforms imported IDP data to handle the new certificate structure.
515-534: Acknowledged: JSON-only support is intentional.The
createIDPImportRequestmethod intentionally supports only JSON format for the new certificate structure when the feature flag is enabled, as confirmed in the codebase. XML and YAML format support is deferred as future work.However, the unchecked cast to
IdentityProviderExportResponseat lines 529-530 will cause aClassCastExceptionif XML/YAML files are imported with the flag enabled, sinceparseIdpFromXml()andparseIdpFromYaml()always return baseIdentityProviderobjects, notIdentityProviderExportResponse. Consider adding aninstanceofcheck or validation to provide a clearer error message for non-JSON imports with the feature flag enabled.
1 participant
Purpose
Add the below config to disable the fix:
export API change:
Old
New
Related Issues:
Tested flow
Fix applied
Fix disabled
Summary by CodeRabbit
New Features
Bug Fixes / Data Handling
Chores
✏️ Tip: You can customize this high-level summary in your review settings.