fix: refactor DN parsing #12935
Merged
fix: refactor DN parsing #12935
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
Contributor
📝 WalkthroughWalkthroughReplaces fragile split-based DN parsing with a dedicated RFC 2253-aware helper in the cert-authn plugin, adds LDAP SDK import and error handling; updates documentation (procedural wording, URLs, paths, PEM/JSON wording) and removes trailing whitespace in the plugin POM. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
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 |
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mo-auto
added
area-documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/casa/plugins/cert-authn.mdjans-casa/plugins/cert-authn/pom.xmljans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/apache/certauthn_vhost_tls1.3.conf:0-0
Timestamp: 2025-12-31T16:10:36.685Z
Learning: For the cert-authn plugin in jans-casa, Apache configuration templates (such as certauthn_vhost_tls1.3.conf) are documented in the plugin documentation page rather than with inline comments in the configuration files themselves.
📚 Learning: 2025-12-31T16:22:54.513Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/pom.xml:99-109
Timestamp: 2025-12-31T16:22:54.513Z
Learning: For the cert-authn plugin (jans-casa/plugins/cert-authn/pom.xml), the jans-scim-model dependency is intentionally scoped as compile to bundle those classes into the plugin JAR. Use wildcard exclusions to prevent transitive dependencies from being included. This is a one-file, file-specific guideline; ensure this intention is well-documented in comments or a developer note. If this pattern should apply to more modules, consider converting to a broader pattern (e.g., a Maven configuration guideline) and include a rationale for bundling versus provided scope.
Applied to files:
jans-casa/plugins/cert-authn/pom.xml
📚 Learning: 2025-12-31T16:10:36.685Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/apache/certauthn_vhost_tls1.3.conf:0-0
Timestamp: 2025-12-31T16:10:36.685Z
Learning: For the cert-authn plugin in jans-casa, Apache configuration templates (such as certauthn_vhost_tls1.3.conf) are documented in the plugin documentation page rather than with inline comments in the configuration files themselves.
Applied to files:
jans-casa/plugins/cert-authn/pom.xmldocs/casa/plugins/cert-authn.md
📚 Learning: 2025-12-31T16:12:54.538Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/model/Reference.java:3-29
Timestamp: 2025-12-31T16:12:54.538Z
Learning: In jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/model/Reference.java, the Reference class does not require equals() and hashCode() implementations because instances are not used in collections or for value-based equality checks.
Applied to files:
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
📚 Learning: 2025-12-31T16:11:45.351Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/apache/certauthn_vhost_tls1.3.conf:23-25
Timestamp: 2025-12-31T16:11:45.351Z
Learning: In the cert-authn plugin for jans-casa, certificate validation is performed at the Java application level (via CertService and PathCertificateVerifier), not at the Apache level. The Apache configuration uses `SSLVerifyClient optional_no_ca` intentionally to accept any client certificate and forward it to the Java application via the X-ClientCert header, where proper PKIX-based validation is performed.
Applied to files:
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.javadocs/casa/plugins/cert-authn.md
📚 Learning: 2025-12-31T16:14:49.605Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java:186-191
Timestamp: 2025-12-31T16:14:49.605Z
Learning: In CertService.java (jans-casa plugins cert-authn), do not guard the result of getJansExtUid() with null checks when using the returned value on BasePerson, IdentityPerson, or CertPerson. Since getJansExtUid() never returns null, you can safely call methods on the returned List without null checks, reducing boilerplate and avoiding false negatives in reviews.
Applied to files:
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
📚 Learning: 2025-12-31T16:16:31.777Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/src/main/resources/assets/cbasic.zul:7-7
Timestamp: 2025-12-31T16:16:31.777Z
Learning: In the cert-authn plugin for jans-casa (file cbasic.zul), the base href intentionally hardcodes "https://" rather than using a dynamic protocol, because client certificate authentication inherently requires HTTPS/TLS and is not meant to work over plain HTTP.
Applied to files:
docs/casa/plugins/cert-authn.md
📚 Learning: 2025-12-31T16:23:03.002Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/pom.xml:99-109
Timestamp: 2025-12-31T16:23:03.002Z
Learning: In the cert-authn plugin for jans-casa (file jans-casa/plugins/cert-authn/pom.xml), the jans-scim-model dependency intentionally uses compile scope (rather than provided scope like other dependencies) to bundle the jans-scim-model classes into the plugin JAR, while using wildcard exclusions to prevent any transitive dependencies from being included.
Applied to files:
docs/casa/plugins/cert-authn.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: sonar scan (jans-link)
- GitHub Check: sonar scan (jans-orm)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: sonar scan (agama)
- GitHub Check: lint_docs
🔇 Additional comments (5)
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java (2)
4-4: Wildcard import is acceptable here.The wildcard import for
com.unboundid.ldap.sdk.*is reasonable given that multiple classes (DN,RDN,Attribute) are used from this package.
234-235: Good refactor to centralize DN parsing.Delegating to
getDNAttributes()for RFC 2253-compliant parsing addresses the original issue where escaped characters (e.g.,CN=Doe\, John) caused incorrect splitting.docs/casa/plugins/cert-authn.md (2)
91-95: Good addition: P12 packaging step.Adding the explicit step to package the certificate and key into a
.p12file improves the documentation flow and makes the process clearer for users who need to import certificates into browsers.
138-138: URL change correctly distinguishes Agama project artifact from plugin JAR.The URL path change from
cert-authn/tocert-authn-agama/is correct and aligns with the artifact ID defined injans-casa/plugins/cert-authn/agama/pom.xml. Thecert-authn-agamamodule generates a-project.ziparchive via the maven-assembly-plugin, matching the documentation's expected filename. This naming pattern is consistent with other Casa plugin Agama projects in the codebase (e.g.,email_2fa-agama,acct-linking-agama).jans-casa/plugins/cert-authn/pom.xml (1)
80-110: The UnboundID LDAP SDK dependency is not missing. WhileCertService.javadoes importcom.unboundid.ldap.sdk.*classes, theunboundid-ldapsdkdependency is already available throughcasa-shared(which cert-authn depends on with provided scope) and throughjans-scim-model(which cert-authn depends on with compile scope). No additional dependency needs to be added.Likely an incorrect or invalid review comment.
...asa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
Show resolved
Hide resolved
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java (1)
292-318: Good RFC 2253-compliant implementation; add defensive check for empty values.The case sensitivity issue from the previous review is correctly addressed at line 306. However,
attr.getValues()[0]could throwArrayIndexOutOfBoundsExceptionif the values array is empty. While unlikely for valid RDNs, a defensive check improves robustness.Proposed defensive check
for (Attribute attr : attrs) { String name = attr.getName().toLowerCase(); - if (!map.containsKey(name)) { - map.put(name, attr.getValues()[0]); + String[] values = attr.getValues(); + if (!map.containsKey(name) && values.length > 0) { + map.put(name, values[0]); } }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/apache/certauthn_vhost_tls1.3.conf:0-0
Timestamp: 2025-12-31T16:10:36.685Z
Learning: For the cert-authn plugin in jans-casa, Apache configuration templates (such as certauthn_vhost_tls1.3.conf) are documented in the plugin documentation page rather than with inline comments in the configuration files themselves.
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/src/main/resources/assets/cbasic.zul:7-7
Timestamp: 2025-12-31T16:16:31.777Z
Learning: In the cert-authn plugin for jans-casa (file cbasic.zul), the base href intentionally hardcodes "https://" rather than using a dynamic protocol, because client certificate authentication inherently requires HTTPS/TLS and is not meant to work over plain HTTP.
📚 Learning: 2025-12-31T16:23:03.002Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/pom.xml:99-109
Timestamp: 2025-12-31T16:23:03.002Z
Learning: In the cert-authn plugin for jans-casa (file jans-casa/plugins/cert-authn/pom.xml), the jans-scim-model dependency intentionally uses compile scope (rather than provided scope like other dependencies) to bundle the jans-scim-model classes into the plugin JAR, while using wildcard exclusions to prevent any transitive dependencies from being included.
Applied to files:
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
📚 Learning: 2025-12-31T16:11:45.351Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/apache/certauthn_vhost_tls1.3.conf:23-25
Timestamp: 2025-12-31T16:11:45.351Z
Learning: In the cert-authn plugin for jans-casa, certificate validation is performed at the Java application level (via CertService and PathCertificateVerifier), not at the Apache level. The Apache configuration uses `SSLVerifyClient optional_no_ca` intentionally to accept any client certificate and forward it to the Java application via the X-ClientCert header, where proper PKIX-based validation is performed.
Applied to files:
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
📚 Learning: 2025-12-31T16:12:54.538Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/model/Reference.java:3-29
Timestamp: 2025-12-31T16:12:54.538Z
Learning: In jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/model/Reference.java, the Reference class does not require equals() and hashCode() implementations because instances are not used in collections or for value-based equality checks.
Applied to files:
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
📚 Learning: 2025-12-31T16:14:49.605Z
Learnt from: jgomer2001
Repo: JanssenProject/jans PR: 12927
File: jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java:186-191
Timestamp: 2025-12-31T16:14:49.605Z
Learning: In CertService.java (jans-casa plugins cert-authn), do not guard the result of getJansExtUid() with null checks when using the returned value on BasePerson, IdentityPerson, or CertPerson. Since getJansExtUid() never returns null, you can safely call methods on the returned List without null checks, reducing boilerplate and avoiding false negatives in reviews.
Applied to files:
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sonar scan (jans-core)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: sonar scan (jans-keycloak-integration)
🔇 Additional comments (1)
jans-casa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java (1)
235-235: LGTM!Clean delegation to the new RFC 2253-compliant parsing helper. This properly addresses the issue with escaped characters in DNs.
...asa/plugins/cert-authn/src/main/java/io/jans/casa/plugins/certauthn/service/CertService.java
Show resolved
Hide resolved
moabu
approved these changes
Jan 2, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prepare
Description
Target issue
closes #12929
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.