OF-1927: Show TCP port for server-to-server (S2S) connections in admin console#3233
Conversation
|
hey @guusdk manual test won't happen because to build connection btw servers, need hosting that's why i am not able to check |
There was a problem hiding this comment.
Pull request overview
This PR enhances Openfire’s admin console “Remote Server Connections Details” page by exposing and displaying the TCP port used for S2S connections, enabling admins to distinguish standard S2S vs direct TLS connections.
Changes:
- Adds port accessors (
Connection#getRemotePort()andSession#getHostPort()) with implementations for Socket- and Netty-based connections. - Extends clustered session plumbing (RemoteSession/RemoteSessionTask) to retrieve the port from remote cluster nodes.
- Updates
server-session-details.jspto display a new “Port” column for incoming and outgoing server sessions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| xmppserver/src/main/webapp/server-session-details.jsp | Adds a Port column and renders ${session.hostPort} in incoming/outgoing S2S tables. |
| xmppserver/src/main/java/org/jivesoftware/openfire/session/Session.java | Introduces default getHostPort() API. |
| xmppserver/src/main/java/org/jivesoftware/openfire/session/LocalSession.java | Implements getHostPort() by delegating to the underlying connection. |
| xmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSession.java | Caches and fetches host port via cluster task. |
| xmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSessionTask.java | Adds a new clustered operation to fetch host port. |
| xmppserver/src/main/java/org/jivesoftware/openfire/session/IncomingServerSessionTask.java | Adds a default case to the operation switch. |
| xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.java | Implements getRemotePort() using Netty channel remote address. |
| xmppserver/src/main/java/org/jivesoftware/openfire/net/SocketConnection.java | Implements getRemotePort() by delegating to existing getPort(). |
| xmppserver/src/main/java/org/jivesoftware/openfire/Connection.java | Introduces default getRemotePort() API. |
Comments suppressed due to low confidence (1)
xmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSessionTask.java:226
- RemoteSessionTask serializes the operation using operation.ordinal() and deserializes using Operation.values()[...]. Adding the new enum constant
getHostPortin the middle of the enum changes the ordinal values of all subsequent constants (e.g., validate/removeDetached/markNonResumable), which can cause nodes on different versions (or persisted/queued tasks) to execute the wrong operation. To keep cluster serialization stable, append new enum constants to the end (or switch serialization to a stable identifier like the enum name with backwards-compatible parsing).
public void writeExternal(ObjectOutput out) throws IOException {
ExternalizableUtil.getInstance().writeBoolean(out, operation != null);
if (operation != null) {
ExternalizableUtil.getInstance().writeInt(out, operation.ordinal());
}
}
public void readExternal(ObjectInput in) throws IOException, ClassNotFoundException {
if (ExternalizableUtil.getInstance().readBoolean(in)) {
operation = Operation.values()[ExternalizableUtil.getInstance().readInt(in)];
}
}
public enum Operation {
/**
* Basic session operations
*/
getStreamID,
getServerName,
getCreationDate,
getLastActiveDate,
getNumClientPackets,
getNumServerPackets,
getTLSProtocolName,
getCipherSuiteName,
getPeerCertificates,
getSoftwareVersion,
close,
isClosed,
isDetached,
isEncrypted,
getHostAddress,
getHostName,
getHostPort,
validate,
removeDetached,
markNonResumable,
/**
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </c:choose> | ||
| <td><c:out value="${session.TLSProtocolName}"/></td> | ||
| <td><c:out value="${session.cipherSuiteName}"/></td> | ||
| <td><c:out value="${session.hostPort}"/></td> |
There was a problem hiding this comment.
The UI will render the integer returned by session.hostPort even when the backend uses 0 to mean "not available" (e.g., detached session / unknown remote address). This will show a confusing port value of 0 in the table. Consider rendering an empty value or placeholder when hostPort <= 0 to avoid implying a real TCP port.
| <td><c:out value="${session.hostPort}"/></td> | |
| <td> | |
| <c:choose> | |
| <c:when test="${session.hostPort gt 0}"> | |
| <c:out value="${session.hostPort}"/> | |
| </c:when> | |
| <c:otherwise> </c:otherwise> | |
| </c:choose> | |
| </td> |
|
@MilanTyagi2004 thanks for doing this! Copilot found some small but good things to improve. If this relates to a JIRA issue, can you please mention the JIRA number in the commit message? That way, automation will link the ticket to the code change. |
MilanTyagi2004
changed the title
|
@guusdk changes are done. |
guusdk
left a comment
guusdk
left a comment
There was a problem hiding this comment.
This looks pretty good, apart from the Copilot comment that you can address, I'd also like you to see if you can settle on either remotePort or hostPort to identify the port used by the peer (I think I prefer 'remote' as it clearly signals that we're talking about the post used by the other party). Having two different names for basically the same thing is a bit confusing.
|
sure, Guus |
|
Hi @MilanTyagi2004 - am I right to assume that you are still planning to add improvements to this PR? The changes look promising, it would be good to be able to finish this! |
|
sure i will look into this and finish this. |
|
@coderabbitai |
|
✅ Actions performedReview triggered.
|
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR exposes remote TCP peer port information end-to-end. Connection and Session interfaces gain a default getRemotePort(); SocketConnection and NettyConnection implement it. LocalSession delegates to its Connection; RemoteSession lazily queries and caches the port via a new RemoteSessionTask.Operation.getRemotePort remote operation. IncomingServerSessionTask adds a defensive default switch case. The admin UI adds a ports.column to incoming and outgoing server session tables and conditionally displays session.remotePort. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSessionTask.java`:
- Around line 221-224: The enum ordering change (in Operation inside
RemoteSessionTask) breaks clustered serialization because code uses
operation.ordinal() and Operation.values()[ordinal]; to fix, preserve existing
ordinals by not inserting new enum constants in the middle—move the newly added
getHostPort constant to the end of the Operation enum (after markNonResumable)
so existing values like getHostName, validate, removeDetached, markNonResumable
keep their original ordinals; update only the enum declaration in
RemoteSessionTask and do not change places that use operation.ordinal() or
Operation.values()[ordinal].
In `@xmppserver/src/main/webapp/server-session-details.jsp`:
- Line 391: Replace the direct output of session.hostPort (the <c:out
value="${session.hostPort}"/> tag) with a conditional that emits a blank when
the port is 0 to match the incoming-table behavior; e.g. render an empty string
if session.hostPort == 0 otherwise the port (use EL ternary or a
<c:choose>/<c:when> around session.hostPort) so unknown outgoing ports are shown
as blank instead of "0".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4abb7bea-518f-417b-8999-839bce523146
📒 Files selected for processing (9)
xmppserver/src/main/java/org/jivesoftware/openfire/Connection.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/SocketConnection.javaxmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/IncomingServerSessionTask.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/LocalSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSessionTask.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/Session.javaxmppserver/src/main/webapp/server-session-details.jsp
…n console Display the remote TCP port in the incoming and outgoing S2S session tables on the admin console, allowing administrators to distinguish standard S2S (port 5269) from direct TLS (port 5270) connections. - Added Session#getRemotePort() delegating to Connection#getRemotePort() - Implemented in LocalSession, RemoteSession, and RemoteSessionTask - Appended new enum constant at the end of RemoteSessionTask.Operation to preserve ordinal compatibility in mixed-version clusters - Standardized naming from hostPort to remotePort across all layers - Added port column to S2S session detail tables (reusing ports.port i18n key) - Render blank instead of 0 when port is unavailable - Improved Javadocs to document the 0-when-unavailable contract
MilanTyagi2004
force-pushed
the
OF-1927-Show-TCP-port-used-for-server-to-server-connection
branch
from
6a06fd0 to
0f6f185
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSession.java`:
- Around line 186-191: The code currently converts a null cluster lookup result
into 0 and caches it in the remotePort field, preventing future retries; change
the logic in RemoteSession (the remotePort initialization block that calls
getRemoteSessionTask and doSynchronousClusterTask) so that you only assign to
remotePort when result != null (e.g., remotePort = (Integer) result); if result
is null leave remotePort as the original sentinel (e.g., -1) so subsequent calls
will retry the cluster lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e333a952-0ec4-489c-be2a-fa61ceae08df
📒 Files selected for processing (9)
xmppserver/src/main/java/org/jivesoftware/openfire/Connection.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/SocketConnection.javaxmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/IncomingServerSessionTask.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/LocalSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSessionTask.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/Session.javaxmppserver/src/main/webapp/server-session-details.jsp
✅ Files skipped from review due to trivial changes (2)
- xmppserver/src/main/java/org/jivesoftware/openfire/net/SocketConnection.java
- xmppserver/src/main/java/org/jivesoftware/openfire/session/LocalSession.java
🚧 Files skipped from review as they are similar to previous changes (4)
- xmppserver/src/main/webapp/server-session-details.jsp
- xmppserver/src/main/java/org/jivesoftware/openfire/Connection.java
- xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.java
- xmppserver/src/main/java/org/jivesoftware/openfire/session/IncomingServerSessionTask.java
| if (remotePort == -1) { | ||
| ClusterTask<Object> task = getRemoteSessionTask(RemoteSessionTask.Operation.getRemotePort); | ||
| Object result = doSynchronousClusterTask(task); | ||
| remotePort = result == null ? 0 : (Integer) result; | ||
| } | ||
| return remotePort; |
There was a problem hiding this comment.
Don’t permanently cache an unavailable remote port.
At Line 189, null is converted to 0 and cached forever. If the lookup fails transiently (for example while detached), later calls won’t retry and can keep showing no port even when it becomes available.
Proposed minimal fix
`@Override`
public int getRemotePort() {
if (remotePort == -1) {
ClusterTask<Object> task = getRemoteSessionTask(RemoteSessionTask.Operation.getRemotePort);
Object result = doSynchronousClusterTask(task);
- remotePort = result == null ? 0 : (Integer) result;
+ if (result != null) {
+ remotePort = (Integer) result;
+ }
}
- return remotePort;
+ return remotePort == -1 ? 0 : remotePort;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/session/RemoteSession.java`
around lines 186 - 191, The code currently converts a null cluster lookup result
into 0 and caches it in the remotePort field, preventing future retries; change
the logic in RemoteSession (the remotePort initialization block that calls
getRemoteSessionTask and doSynchronousClusterTask) so that you only assign to
remotePort when result != null (e.g., remotePort = (Integer) result); if result
is null leave remotePort as the original sentinel (e.g., -1) so subsequent calls
will retry the cluster lookup.
There was a problem hiding this comment.
@MilanTyagi2004 I suggest that you apply the proposed minimal fix.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
hi @guusdk, |
guusdk
left a comment
guusdk
left a comment
There was a problem hiding this comment.
Thanks for this Milan! I have a couple of requests, which I left as review comments. Also, don't forget to update the year in the copyright header in every file that you change.
| if (remotePort == -1) { | ||
| ClusterTask<Object> task = getRemoteSessionTask(RemoteSessionTask.Operation.getRemotePort); | ||
| Object result = doSynchronousClusterTask(task); | ||
| remotePort = result == null ? 0 : (Integer) result; | ||
| } | ||
| return remotePort; |
There was a problem hiding this comment.
@MilanTyagi2004 I suggest that you apply the proposed minimal fix.
| * @return the remote port, or 0 when unavailable. | ||
| */ | ||
| default int getRemotePort() { | ||
| return 0; |
There was a problem hiding this comment.
Throughout the code, there are two different values used to represent a 'unknown' remote port: 0 and -1. Unless it is important to have different values (I cannot immediately see a reason) I would use the same value everywhere.
| result = ((IncomingServerSession) getSession()).getValidatedDomains(); | ||
| break; | ||
| default: | ||
| break; |
There was a problem hiding this comment.
Why was this default added? I'm not saying it is wrong, I'm just surprised to see it.
| * | ||
| * @return the port that the connection uses. | ||
| * @return the remote port, or 0 when unavailable. | ||
| */ |
There was a problem hiding this comment.
What you're doing here is basically replacing getPort() (which is specific to this class) with getRemotePort() (which you have now defined for all Connections.
Ideally, we do not keep two duplicate methods around forever, but we cannot simply remove the old method either: some plugins (maybe even created by other people) may be using it. Removing it will immediately break those plugins.
What we typically do, is first send a 'signal' to warn people that this method is going to be removed. Then, in a future version, we remove the method. That gives people some time to adjust.
This 'signal' is performed by marking the method as being deprecated. You can do that like this:
/**
* Returns the remote port used by the connection.
*
* @return the remote port, or 0 when unavailable.
* @deprecated This method is replaced by {@link #getRemotePort()}
*/
@Deprecated(forRemoval = true, since = "5.1.0") // Remove in or after Openfire 5.2.0.
public int getPort() {
return getRemotePort();
}
@Override
public int getRemotePort() {
return socket.getPort();
}After doing that, you should make sure that all code in Openfire itself that uses the deprecated method is modified to use the new method instead.
|
Hey @guusdk, No code in Openfire calls .getPort() on a SocketConnection instance, the only usages are new SocketConnection(...), SocketConnection.getInstances(), SocketConnection.TLSPolicy, etc. The getPort() method was only called internally by getRemotePort() within the same class, which I've already reversed. No additional callers need updating. The deprecation is purely for external plugin consumers who might be using SocketConnection.getPort() directly. All Openfire internal code already uses getRemotePort() (via the Connection or Session interface), not the concrete SocketConnection.getPort(). |
|
could you please review these new changes. |
|
once you review and approve i will squash the commits into one |
Fishbowler
left a comment
Fishbowler
left a comment
There was a problem hiding this comment.
This looks reasonable to me. You haven't been able to test this at all?
Summary
Enhances the Openfire administrative console by displaying the TCP port used for server-to-server (S2S) connections on the "Remote Server Connections Details" page. This allows administrators to distinguish between standard S2S connections (typically port 5269) and direct TLS connections (typically port 5270).
Changes
Backend (Connection & Session Layers)
getRemotePort()method to the Connection interfacegetRemotePort()in:getPort()method)getHostPort()method to Session interfaceAdministrative UI
server-session-details.jspto include a new "Port" columnports.portfor column headerVerification
Build
mvn clean install -DskipTests -pl xmppserver
Manual Testing
Functional Validation