Add IdentityContextUtil methods to resolve client IP and UA from Request #7632
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
|
|
||
| if (request != null) { | ||
| resolveHeaders(request); | ||
| this.ipAddress = request.getRemoteAddr(); | ||
| } |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| if (request != null) { | |
| resolveHeaders(request); | |
| this.ipAddress = request.getRemoteAddr(); | |
| } | |
| if (request != null) { | |
| resolveHeaders(request); | |
| this.ipAddress = request.getRemoteAddr(); | |
| } | |
| log.debug("Request builder initialized with IP address: {}", this.ipAddress); |
| */ | ||
| public static String getClientIpAddress(IdentityContext identityContext) { | ||
|
|
||
| Request request = getRequestFromContext(identityContext); | ||
| return request != null ? getClientIpAddress(request) : null; |
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
| */ | |
| public static String getClientIpAddress(IdentityContext identityContext) { | |
| Request request = getRequestFromContext(identityContext); | |
| return request != null ? getClientIpAddress(request) : null; | |
| public static String getClientIpAddress(IdentityContext identityContext) { | |
| Request request = getRequestFromContext(identityContext); | |
| if (request != null) { | |
| LOG.debug("Resolving client IP address from IdentityContext."); | |
| return getClientIpAddress(request); | |
| } | |
| LOG.debug("Request is null in IdentityContext. Cannot resolve client IP address."); | |
| return null; |
| } | ||
| String ip = values.get(0); | ||
| if (StringUtils.isNotEmpty(ip) && !IdentityConstants.UNKNOWN.equalsIgnoreCase(ip)) { | ||
| return IdentityUtil.getFirstIP(ip); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
| } | |
| String ip = values.get(0); | |
| if (StringUtils.isNotEmpty(ip) && !IdentityConstants.UNKNOWN.equalsIgnoreCase(ip)) { | |
| return IdentityUtil.getFirstIP(ip); | |
| String ip = values.get(0); | |
| if (StringUtils.isNotEmpty(ip) && !IdentityConstants.UNKNOWN.equalsIgnoreCase(ip)) { | |
| String resolvedIp = IdentityUtil.getFirstIP(ip); | |
| if (LOG.isDebugEnabled()) { | |
| LOG.debug("Client IP address resolved from header " + ipHeader + ": " + resolvedIp); | |
| } | |
| return resolvedIp; |
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 | ||
| #### Log Improvement Suggestion No: 3 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds two User-Agent header constants, extends Request with an ipAddress field populated from HttpServletRequest.getRemoteAddr(), and introduces IdentityContextUtil with static helpers to resolve client IP and User-Agent from IdentityContext/Request headers with fallbacks and logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant IC as IdentityContext (thread-local)
participant Util as IdentityContextUtil
participant Req as Request
participant H as Request.headers
Caller->>Util: getClientIpAddress()
Util->>IC: read thread-local IdentityContext
IC-->>Util: Request
Util->>Req: access headers map and ipAddress
Req->>H: provide header entries
H-->>Util: X-Forwarded-For / other IP headers
alt header contains valid IP
Util-->>Caller: first valid IP (from headers)
else no valid header
Util-->>Caller: Request.ipAddress (remoteAddr) or null
end
par user-agent resolution
Caller->>Util: getClientUserAgent()
Util->>Req: read headers
H-->>Util: X-Forwarded-User-Agent and/or User-Agent
alt X-Forwarded-User-Agent present
Util-->>Caller: first X-Forwarded-User-Agent value
else if User-Agent present
Util-->>Caller: first User-Agent value
else
Util-->>Caller: null
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (5)**/*.{py,java,ts,tsx,js,jsx,cs,go,rb,php}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{py,java,ts,tsx,js,jsx,cs,go,rb,php,c,cpp,h,hpp}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.java📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{java,ts,tsx,js,jsx,py,cs,go,php}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{java,ts,tsx,js,jsx,py,cs,go,php,yml,yaml,json,env,properties,conf}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-11-11T00:57:09.814ZApplied to files:
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Request.java (1)
35-41: ipAddress field wiring looks correct; consider documenting nullability
ipAddressis correctly wired from the builder and exposed viagetIpAddress(). Since it can legitimately benullwhen the builder is not initialized from anHttpServletRequest, it would be helpful to document that in the Javadoc ofRequestorgetIpAddress()so callers don’t assume it’s always non-null.components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/util/IdentityContextUtil.java (2)
59-80: Guard against null Request in public utility methodsBoth
getClientIpAddress(Request request)andgetClientUserAgent(Request request)assumerequestis non-null and will throwNullPointerExceptionif called defensively with a null value. Since these are public utilities, it’s safer either to:
- Explicitly guard against null and return
null(with a debug log), or- Enforce non-null via an early
Objects.requireNonNull(request, "...")and document it.Example (if you prefer graceful handling):
- public static String getClientIpAddress(Request request) { - - Map<String, List<String>> headerMap = request.getHeaders().stream() + public static String getClientIpAddress(Request request) { + if (request == null) { + if (LOG.isDebugEnabled()) { + LOG.debug("Request is null. Cannot resolve client IP address."); + } + return null; + } + + Map<String, List<String>> headerMap = request.getHeaders().stream() .collect(Collectors.toMap(Header::getName, Header::getValue));Apply the same pattern (or a documented non-null contract) for
getClientUserAgent(Request request)for consistency.Also applies to: 100-123
59-76: Header lookup is case-sensitive; consider normalizing for robustness
headerMapkeys are taken directly fromHeader::getNameand then looked up using constants like"X-Forwarded-For"and"User-Agent". If a container or intermediary normalizes header names to a different case (e.g.,x-forwarded-for), these lookups will fail.Consider normalizing header names (e.g., to lower case) both when building the map and in the constants you use for lookup, to make the resolution logic case-insensitive. This is an optional hardening step but can avoid subtle interoperability issues.
Also applies to: 102-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/identity-core/org.wso2.carbon.identity.base/src/main/java/org/wso2/carbon/identity/base/IdentityConstants.java(1 hunks)components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Request.java(1 hunks)components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/util/IdentityContextUtil.java(1 hunks)
🔇 Additional comments (3)
components/identity-core/org.wso2.carbon.identity.base/src/main/java/org/wso2/carbon/identity/base/IdentityConstants.java (1)
157-160: User-Agent header constants are appropriate and consistentThe added
USER_AGENTandX_FORWARDED_USER_AGENTconstants match standard header names and integrate cleanly with the new utility methods.components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Request.java (2)
48-51: Accessor behavior aligns with the new field
getIpAddress()is a simple accessor over the captured servletremoteAddr, which is exactly what the new utility methods expect. No functional issues here.
61-66: fromHttpRequest correctly captures servlet remote addressPopulating
ipAddressfromrequest.getRemoteAddr()infromHttpRequestis consistent with the goal of preserving the original servlet remote address and lets higher-level utilities apply their own proxy/header logic. This looks good as-is.
...ntity.core/src/main/java/org/wso2/carbon/identity/core/context/util/IdentityContextUtil.java
Outdated
Show resolved
Hide resolved
ZiyamSanthosh
force-pushed
the
master-add-util-identity-context
branch
from
2c719bf to
63df921
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces the IdentityContextUtil utility class with methods to extract client IP addresses and User-Agent information from IdentityContext and Request objects. It also enhances the Request model to include a first-class ipAddress attribute populated from HttpServletRequest.getRemoteAddr().
- Added
IdentityContextUtilclass with IP and User-Agent resolution methods - Enhanced
Requestclass withipAddressfield and getter method - Added User-Agent related constants to
IdentityConstants
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
IdentityContextUtil.java |
New utility class providing methods to resolve client IP address and User-Agent from IdentityContext and Request objects |
Request.java |
Added ipAddress field to preserve remote address from servlet request |
IdentityConstants.java |
Added User-Agent header constants (USER_AGENT and X_FORWARDED_USER_AGENT) |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| return Collections.unmodifiableList(headers); | ||
| } | ||
|
|
||
| public String getIpAddress() { |
There was a problem hiding this comment.
Missing docstring for public method. All public methods should have a docstring. Consider adding:
/**
* Gets the IP address of the request.
*
* @return The IP address.
*/
public String getIpAddress() {| Map<String, List<String>> headerMap = request.getHeaders().stream() | ||
| .collect(Collectors.toMap(Header::getName, Header::getValue)); |
There was a problem hiding this comment.
The stream collection to a map may throw IllegalStateException if there are duplicate header names with different cases (e.g., "User-Agent" and "user-agent"). HTTP headers are case-insensitive, but the current code treats them as case-sensitive keys. Consider using Collectors.toMap() with a merge function to handle duplicates, or use a case-insensitive map structure like TreeMap with String.CASE_INSENSITIVE_ORDER.
| Map<String, List<String>> headerMap = request.getHeaders().stream() | ||
| .collect(Collectors.toMap(Header::getName, Header::getValue)); |
There was a problem hiding this comment.
The stream collection to a map may throw IllegalStateException if there are duplicate header names with different cases (e.g., "User-Agent" and "user-agent"). HTTP headers are case-insensitive, but the current code treats them as case-sensitive keys. Consider using Collectors.toMap() with a merge function to handle duplicates, or use a case-insensitive map structure like TreeMap with String.CASE_INSENSITIVE_ORDER.
| public static String getClientUserAgent(Request request) { | ||
|
|
||
| Map<String, List<String>> headerMap = request.getHeaders().stream() | ||
| .collect(Collectors.toMap(Header::getName, Header::getValue)); | ||
|
|
||
| List<String> userAgent = headerMap.get(IdentityConstants.USER_AGENT); | ||
| List<String> forwardedUserAgent = headerMap.get(IdentityConstants.X_FORWARDED_USER_AGENT); | ||
| if (forwardedUserAgent != null && !forwardedUserAgent.isEmpty()) { | ||
| if (LOG.isDebugEnabled() && forwardedUserAgent.size() > 1) { | ||
| LOG.debug("Multiple values found for header: " + IdentityConstants.X_FORWARDED_USER_AGENT + | ||
| ". Using the first value as the User-Agent."); | ||
| } | ||
| return forwardedUserAgent.get(0); | ||
| } else if (userAgent != null && !userAgent.isEmpty()) { | ||
| if (LOG.isDebugEnabled() && userAgent.size() > 1) { | ||
| LOG.debug("Multiple values found for header: " + IdentityConstants.USER_AGENT + | ||
| ". Using the first value as the User-Agent."); | ||
| } | ||
| return userAgent.get(0); |
There was a problem hiding this comment.
Creating a map from the headers stream on every call is inefficient. If getClientUserAgent is called frequently, consider either:
- Caching the header map in the Request object
- Iterating through the headers list directly instead of creating an intermediate map
- Adding a method to Request class to retrieve headers by name
For example:
for (Header header : request.getHeaders()) {
String name = header.getName();
if (IdentityConstants.X_FORWARDED_USER_AGENT.equalsIgnoreCase(name)) {
return header.getValue().isEmpty() ? null : header.getValue().get(0);
}
}
// ... continue with USER_AGENT check| public static String getClientUserAgent(Request request) { | |
| Map<String, List<String>> headerMap = request.getHeaders().stream() | |
| .collect(Collectors.toMap(Header::getName, Header::getValue)); | |
| List<String> userAgent = headerMap.get(IdentityConstants.USER_AGENT); | |
| List<String> forwardedUserAgent = headerMap.get(IdentityConstants.X_FORWARDED_USER_AGENT); | |
| if (forwardedUserAgent != null && !forwardedUserAgent.isEmpty()) { | |
| if (LOG.isDebugEnabled() && forwardedUserAgent.size() > 1) { | |
| LOG.debug("Multiple values found for header: " + IdentityConstants.X_FORWARDED_USER_AGENT + | |
| ". Using the first value as the User-Agent."); | |
| } | |
| return forwardedUserAgent.get(0); | |
| } else if (userAgent != null && !userAgent.isEmpty()) { | |
| if (LOG.isDebugEnabled() && userAgent.size() > 1) { | |
| LOG.debug("Multiple values found for header: " + IdentityConstants.USER_AGENT + | |
| ". Using the first value as the User-Agent."); | |
| } | |
| return userAgent.get(0); | |
| /** | |
| * Resolves the client User-Agent from the given Request. | |
| * | |
| * @param request The Request object. | |
| * @return The client User-Agent if found, else null. | |
| */ | |
| public static String getClientUserAgent(Request request) { | |
| // Iterate through headers to find X_FORWARDED_USER_AGENT or USER_AGENT. | |
| List<Header> headers = request.getHeaders(); | |
| for (Header header : headers) { | |
| String name = header.getName(); | |
| List<String> values = header.getValue(); | |
| if (IdentityConstants.X_FORWARDED_USER_AGENT.equalsIgnoreCase(name)) { | |
| if (values != null && !values.isEmpty()) { | |
| if (LOG.isDebugEnabled() && values.size() > 1) { | |
| LOG.debug("Multiple values found for header: " + IdentityConstants.X_FORWARDED_USER_AGENT + | |
| ". Using the first value as the User-Agent."); | |
| } | |
| return values.get(0); | |
| } | |
| } | |
| } | |
| for (Header header : headers) { | |
| String name = header.getName(); | |
| List<String> values = header.getValue(); | |
| if (IdentityConstants.USER_AGENT.equalsIgnoreCase(name)) { | |
| if (values != null && !values.isEmpty()) { | |
| if (LOG.isDebugEnabled() && values.size() > 1) { | |
| LOG.debug("Multiple values found for header: " + IdentityConstants.USER_AGENT + | |
| ". Using the first value as the User-Agent."); | |
| } | |
| return values.get(0); | |
| } | |
| } |
| if (LOG.isDebugEnabled() && forwardedUserAgent.size() > 1) { | ||
| LOG.debug("Multiple values found for header: " + IdentityConstants.X_FORWARDED_USER_AGENT + | ||
| ". Using the first value as the User-Agent."); |
There was a problem hiding this comment.
Debug logging should be wrapped in an if (LOG.isDebugEnabled()) check when performing string concatenation. The current condition if (LOG.isDebugEnabled() && forwardedUserAgent.size() > 1) still performs the string concatenation in the LOG.debug() call even when the size check fails but debug is enabled. Consider restructuring:
if (LOG.isDebugEnabled()) {
if (forwardedUserAgent.size() > 1) {
LOG.debug("Multiple values found for header: " + IdentityConstants.X_FORWARDED_USER_AGENT +
". Using the first value as the User-Agent.");
}
}| if (LOG.isDebugEnabled() && userAgent.size() > 1) { | ||
| LOG.debug("Multiple values found for header: " + IdentityConstants.USER_AGENT + | ||
| ". Using the first value as the User-Agent."); |
There was a problem hiding this comment.
Debug logging should be wrapped in an if (LOG.isDebugEnabled()) check when performing string concatenation. The current condition if (LOG.isDebugEnabled() && userAgent.size() > 1) still performs the string concatenation in the LOG.debug() call even when the size check fails but debug is enabled. Consider restructuring:
if (LOG.isDebugEnabled()) {
if (userAgent.size() > 1) {
LOG.debug("Multiple values found for header: " + IdentityConstants.USER_AGENT +
". Using the first value as the User-Agent.");
}
}| if (LOG.isDebugEnabled() && values.size() > 1) { | ||
| LOG.debug("Multiple values found for header: " + ipHeader + | ||
| ". Using the first value as the client IP address."); |
There was a problem hiding this comment.
Debug logging should be wrapped in an if (LOG.isDebugEnabled()) check when performing string concatenation. The current condition if (LOG.isDebugEnabled() && values.size() > 1) still performs the string concatenation in the LOG.debug() call even when the size check fails but debug is enabled. Consider restructuring:
if (LOG.isDebugEnabled()) {
if (values.size() > 1) {
LOG.debug("Multiple values found for header: " + ipHeader +
". Using the first value as the client IP address.");
}
}| for (String ipHeader : IdentityConstants.HEADERS_WITH_IP) { | ||
| List<String> values = headerMap.get(ipHeader); | ||
| if (values != null && !values.isEmpty()) { | ||
| if (LOG.isDebugEnabled() && values.size() > 1) { | ||
| LOG.debug("Multiple values found for header: " + ipHeader + | ||
| ". Using the first value as the client IP address."); | ||
| } | ||
| String ip = values.get(0); | ||
| if (StringUtils.isNotEmpty(ip) && !IdentityConstants.UNKNOWN.equalsIgnoreCase(ip)) { | ||
| return IdentityUtil.getFirstIP(ip); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Vulnerability: IP Spoofing via Header Manipulation
Risk: The code trusts IP addresses from HTTP headers (X-Forwarded-For, Proxy-Client-IP, etc.) which can be easily spoofed by malicious clients. This could lead to incorrect IP-based authentication, authorization bypass, or audit log tampering.
Suggestion: Ensure that:
- This method is only used behind a trusted reverse proxy that properly sanitizes these headers
- Add documentation warning about the security implications
- Consider validating IP format and rejecting obviously invalid IPs
- If used for security-sensitive operations (authentication, rate limiting, audit), add additional verification mechanisms
| Map<String, List<String>> headerMap = request.getHeaders().stream() | ||
| .collect(Collectors.toMap(Header::getName, Header::getValue)); | ||
|
|
||
| for (String ipHeader : IdentityConstants.HEADERS_WITH_IP) { | ||
| List<String> values = headerMap.get(ipHeader); | ||
| if (values != null && !values.isEmpty()) { | ||
| if (LOG.isDebugEnabled() && values.size() > 1) { | ||
| LOG.debug("Multiple values found for header: " + ipHeader + | ||
| ". Using the first value as the client IP address."); | ||
| } | ||
| String ip = values.get(0); | ||
| if (StringUtils.isNotEmpty(ip) && !IdentityConstants.UNKNOWN.equalsIgnoreCase(ip)) { | ||
| return IdentityUtil.getFirstIP(ip); | ||
| } |
There was a problem hiding this comment.
Creating a map from the headers stream on every call is inefficient. If getClientIpAddress is called frequently, consider either:
- Caching the header map in the Request object
- Iterating through the headers list directly instead of creating an intermediate map
- Adding a method to Request class to retrieve headers by name
For example:
for (String ipHeader : IdentityConstants.HEADERS_WITH_IP) {
for (Header header : request.getHeaders()) {
if (ipHeader.equalsIgnoreCase(header.getName())) {
List<String> values = header.getValue();
// ... rest of logic
}
}
}| Map<String, List<String>> headerMap = request.getHeaders().stream() | |
| .collect(Collectors.toMap(Header::getName, Header::getValue)); | |
| for (String ipHeader : IdentityConstants.HEADERS_WITH_IP) { | |
| List<String> values = headerMap.get(ipHeader); | |
| if (values != null && !values.isEmpty()) { | |
| if (LOG.isDebugEnabled() && values.size() > 1) { | |
| LOG.debug("Multiple values found for header: " + ipHeader + | |
| ". Using the first value as the client IP address."); | |
| } | |
| String ip = values.get(0); | |
| if (StringUtils.isNotEmpty(ip) && !IdentityConstants.UNKNOWN.equalsIgnoreCase(ip)) { | |
| return IdentityUtil.getFirstIP(ip); | |
| } | |
| // Iterate through the headers list directly to avoid creating an intermediate map. | |
| for (String ipHeader : IdentityConstants.HEADERS_WITH_IP) { | |
| for (Header header : request.getHeaders()) { | |
| if (ipHeader.equalsIgnoreCase(header.getName())) { | |
| List<String> values = header.getValue(); | |
| if (values != null && !values.isEmpty()) { | |
| if (LOG.isDebugEnabled() && values.size() > 1) { | |
| LOG.debug("Multiple values found for header: " + ipHeader + | |
| ". Using the first value as the client IP address."); | |
| } | |
| String ip = values.get(0); | |
| if (StringUtils.isNotEmpty(ip) && !IdentityConstants.UNKNOWN.equalsIgnoreCase(ip)) { | |
| return IdentityUtil.getFirstIP(ip); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Request.java (1)
48-51: Document that this getter can return null.Since
HttpServletRequest.getRemoteAddr()may return null, and the ipAddress field is nullable, consider adding javadoc to document this behavior for callers.Apply this diff to add documentation:
+ /** + * Gets the IP address of the client. + * + * @return the client IP address, or null if not available + */ public String getIpAddress() { return ipAddress; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/identity-core/org.wso2.carbon.identity.base/src/main/java/org/wso2/carbon/identity/base/IdentityConstants.java(1 hunks)components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Request.java(1 hunks)components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/util/IdentityContextUtil.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/identity-core/org.wso2.carbon.identity.base/src/main/java/org/wso2/carbon/identity/base/IdentityConstants.java
- components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/util/IdentityContextUtil.java
🔇 Additional comments (1)
components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Request.java (1)
35-35: LGTM: IP address field properly implemented.The ipAddress field follows the correct immutability pattern—final in the Request class and mutable in the Builder. The constructor properly copies the value from the builder.
Also applies to: 40-40, 59-59
|
|
||
| if (request != null) { | ||
| resolveHeaders(request); | ||
| this.ipAddress = request.getRemoteAddr(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify null handling in IdentityContextUtil.
The request.getRemoteAddr() call can return null according to the Servlet API specification. Ensure that IdentityContextUtil properly handles the case where ipAddress is null.
Run the following script to verify how IdentityContextUtil handles null IP addresses:
🏁 Script executed:
#!/bin/bash
# Description: Verify that IdentityContextUtil handles null ipAddress properly
# Search for getClientIpAddress implementations and check null handling
ast-grep --pattern $'getClientIpAddress($$$) {
$$$
}'
# Also check for direct usage of Request.getIpAddress()
rg -nP -A 5 'getIpAddress\(\)' --type=javaLength of output: 1824
🏁 Script executed:
#!/bin/bash
# Get full context around IdentityContextUtil line 79 and find getClientIpAddress method
echo "=== Context around line 79 in IdentityContextUtil ==="
rg -nP -B 10 -A 10 'request\.getIpAddress\(\)' \
components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/util/IdentityContextUtil.java
echo -e "\n=== Looking for getClientIpAddress method ==="
rg -nP 'getClientIpAddress|ClientIpAddress' \
components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/util/IdentityContextUtil.java
echo -e "\n=== Full IdentityContextUtil file to understand structure ==="
cat components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/util/IdentityContextUtil.javaLength of output: 6846
Handle potential null return from IP address fallback path.
Verification confirms that request.getRemoteAddr() can return null per the Servlet API specification. When IdentityContextUtil.getClientIpAddress(Request) falls back to request.getIpAddress() (line 79), this can return null without explicit handling or documentation. Consider either:
- Documenting that the method can return null and require callers to handle it, or
- Providing a fallback/default value when both header-based resolution and
getRemoteAddr()return null
🤖 Prompt for AI Agents
In
components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Request.java
around line 65, request.getRemoteAddr() can return null; update the assignment
to handle null by providing a safe fallback (e.g., set ipAddress to an explicit
default like "unknown" or empty string) and add a brief Javadoc or inline
comment noting that the field may be a default when no client IP is available so
callers know it can be non-specific.
| * @param identityContext The IdentityContext object. | ||
| * @return The client IP address if found, else falls back to the request IP address. | ||
| */ | ||
| public static String getClientIpAddress(IdentityContext identityContext) { |
There was a problem hiding this comment.
Do we need to pass the identity context to this method ?
Can't we directly access the identity context inside the method ?
|
|
||
| for (String ipHeader : IdentityConstants.HEADERS_WITH_IP) { | ||
| List<String> values = headerMap.get(ipHeader); | ||
| if (values != null && !values.isEmpty()) { |
There was a problem hiding this comment.
| if (values != null && !values.isEmpty()) { | |
| if (!CollectionUtils.isEmpty(values)) { |
| * @param request The Request object. | ||
| * @return The client IP address if found, else falls back to the request IP address. | ||
| */ | ||
| public static String getClientIpAddress(Request request) { |
There was a problem hiding this comment.
Do we need to make this public ?
|
|
||
| List<String> userAgent = headerMap.get(IdentityConstants.USER_AGENT); | ||
| List<String> forwardedUserAgent = headerMap.get(IdentityConstants.X_FORWARDED_USER_AGENT); | ||
| if (forwardedUserAgent != null && !forwardedUserAgent.isEmpty()) { |
There was a problem hiding this comment.
| if (forwardedUserAgent != null && !forwardedUserAgent.isEmpty()) { | |
| if (!CollectionUtils.isEmpty(forwardedUserAgent)) { |
| ". Using the first value as the User-Agent."); | ||
| } | ||
| return forwardedUserAgent.get(0); | ||
| } else if (userAgent != null && !userAgent.isEmpty()) { |
There was a problem hiding this comment.
| } else if (userAgent != null && !userAgent.isEmpty()) { | |
| } else if (!CollectionUtils.isEmpty(userAgent)) { |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #7632 +/- ##
============================================
- Coverage 50.67% 50.46% -0.22%
+ Complexity 19876 18939 -937
============================================
Files 2121 2122 +1
Lines 130029 128055 -1974
Branches 27000 26601 -399
============================================
- Hits 65894 64618 -1276
+ Misses 55787 55173 -614
+ Partials 8348 8264 -84
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ZiyamSanthosh
force-pushed
the
master-add-util-identity-context
branch
from
63df921 to
cf0ed03
Compare
|
2 participants
This PR introduces IdentityContextUtil class to have util methods related to IdentityContext. Currently, it has been added with methods to resolve client IP address and User Agent from the request retrieved from the IdentityContext.
Also, the existing Request object is introduced with a first class ipAddress attribute to preserve remoteAddress value from the Servlet request object.
Related PR:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.