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> |
| String getHostAddress() throws UnknownHostException; | ||
|
|
||
| /** | ||
| * Returns the port that the connection uses. | ||
| * | ||
| * @return the port that the connection uses. | ||
| */ | ||
| default int getHostPort() { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
The Javadoc for getHostPort says "Returns the port that the connection uses" but does not document what the default implementation returning 0 means. Consider clarifying that 0 indicates that the port is unknown/unavailable, to match the Connection#getRemotePort contract and avoid callers treating 0 as a real port.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
I'm afraid that copilot cannot apply changes (even if it says it can) in this repository. You'll have to do that manually.
|
@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.
| String getHostAddress() throws UnknownHostException; | ||
|
|
||
| /** | ||
| * Returns the port that the connection uses. | ||
| * | ||
| * @return the port that the connection uses. | ||
| */ | ||
| default int getHostPort() { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
I'm afraid that copilot cannot apply changes (even if it says it can) in this repository. You'll have to do that manually.
|
sure, Guus |
…r-server-to-server-connection
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