Merge pull request #758 from quickfix-j/defaultapplverid-range

Correctly handle unknown `DefaultApplVerID` on `Logon`
quickfix-j · Feb 14, 2024 · 24d836b · 24d836b
2 parents b18003e + 0276061
commit 24d836b
Show file tree

Hide file tree

Showing 10 changed files with 91 additions and 20 deletions.
diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml
@@ -28,7 +28,7 @@ jobs:
           java-version: ${{ matrix.java }}
           cache: 'maven'
       - name: Test with Maven
-        run: ./mvnw install -B -V -Pminimal-fix-latest -D"java.util.logging.config.file"="logging.properties" -D"http.keepAlive"="false" -D"maven.wagon.http.pool"="false" -D"maven.wagon.httpconnectionManager.ttlSeconds"="120"
+        run: ./mvnw install -B -V -Pminimal-fix-latest -D"java.util.logging.config.file"="./quickfixj-core/src/test/resources/logging.properties" -D"http.keepAlive"="false" -D"maven.wagon.http.pool"="false" -D"maven.wagon.httpconnectionManager.ttlSeconds"="120"
 
   test-windows:
     runs-on: ${{ matrix.os }}
@@ -55,4 +55,4 @@ jobs:
           java-version: ${{ matrix.java }}
           cache: 'maven'
       - name: Test with Maven on Windows
-        run: ./mvnw.cmd install -B -V  -D"maven.javadoc.skip"="true" -P"skipBundlePlugin,minimal-fix-latest" -D"java.util.logging.config.file"="logging.properties"  -D"http.keepAlive"="false" -D"maven.wagon.http.pool"="false" -D"maven.wagon.httpconnectionManager.ttlSeconds"="120"
+        run: ./mvnw.cmd install -B -V  -D"maven.javadoc.skip"="true" -P"skipBundlePlugin,minimal-fix-latest" -D"java.util.logging.config.file"="./quickfixj-core/src/test/resources/logging.properties"  -D"http.keepAlive"="false" -D"maven.wagon.http.pool"="false" -D"maven.wagon.httpconnectionManager.ttlSeconds"="120"
diff --git a/quickfixj-base/src/main/java/quickfix/FixVersions.java b/quickfixj-base/src/main/java/quickfix/FixVersions.java
@@ -30,7 +30,7 @@ public interface FixVersions {
     String BEGINSTRING_FIX44 = "FIX.4.4";
 
     /*
-     * FIX 5.0+ does not have a begin string.
+     * FIX 5.0+ does not have a BeginString.
      */
     String FIX50 = "FIX.5.0";
     String FIX50SP1 = "FIX.5.0SP1";

diff --git a/quickfixj-base/src/main/java/quickfix/InvalidMessage.java b/quickfixj-base/src/main/java/quickfix/InvalidMessage.java
@@ -19,8 +19,8 @@
 
 package quickfix;
 
-/*
- * An exception when a message is not valid according to the
+/**
+ * Thrown when a message is not valid according to the
  * basic message validation or the data dictionary.
  */
 public class InvalidMessage extends Exception {

diff --git a/quickfixj-base/src/main/java/quickfix/MessageUtils.java b/quickfixj-base/src/main/java/quickfix/MessageUtils.java
@@ -101,6 +101,7 @@ public static Message parse(MessageFactory messageFactory, DataDictionary dataDi
      * @param messageFactory
      * @param dataDictionary
      * @param messageString
+     * @param validateChecksum
      * @return the parsed message
      * @throws InvalidMessage
      */
@@ -129,6 +130,10 @@ public static boolean isLogon(String message) {
         return isMessageType(message, MsgType.LOGON);
     }
 
+    public static boolean isLogonMsgType(String msgType) {
+        return MsgType.LOGON.equals(msgType);
+    }
+
     private static boolean isMessageType(String message, String msgType) {
         try {
             return msgType.equals(getMessageType(message));
@@ -208,10 +213,10 @@ public static String getStringField(String messageString, int tag) {
     };
 
     /**
-     * Convert an ApplVerID to a "begin string"
+     * Convert an ApplVerID to a BeginString.
      *
      * @param applVerID
-     * @return the begin string for the specified ApplVerID.
+     * @return the BeginString for the specified ApplVerID.
      * @throws QFJException if conversion fails.
      * @see ApplVerID
      */
@@ -239,10 +244,10 @@ public static String toBeginString(ApplVerID applVerID) throws QFJException {
     };
 
     /**
-     * Convert a begin string to an ApplVerID
+     * Convert a BeginString to an ApplVerID.
      *
      * @param beginString
-     * @return the ApplVerID for the specified begin string.
+     * @return the ApplVerID for the specified BeginString.
      * @throws QFJException if conversion fails.
      * @see FixVersions
      */

diff --git a/quickfixj-core/src/main/java/quickfix/MessageSessionUtils.java b/quickfixj-core/src/main/java/quickfix/MessageSessionUtils.java
@@ -36,6 +36,7 @@ public class MessageSessionUtils {
     public static Message parse(Session session, String messageString) throws InvalidMessage {
         final String beginString = MessageUtils.getStringField(messageString, BeginString.FIELD);
         final String msgType = MessageUtils.getMessageType(messageString);
+        final boolean isLogon = MessageUtils.isLogonMsgType(msgType);
         final MessageFactory messageFactory = session.getMessageFactory();
         final DataDictionaryProvider ddProvider = session.getDataDictionaryProvider();
         final ApplVerID applVerID;
@@ -44,17 +45,20 @@ public static Message parse(Session session, String messageString) throws Invali
         final quickfix.Message message;
         final DataDictionary payloadDictionary;
 
-        if (!MessageUtils.isAdminMessage(msgType) || MessageUtils.isLogon(messageString)) {
+        if (!MessageUtils.isAdminMessage(msgType) || isLogon) {
             if (FixVersions.BEGINSTRING_FIXT11.equals(beginString)) {
-                applVerID = getApplVerID(session, messageString);
+                applVerID = getApplVerID(session, messageString, isLogon);
             } else {
                 applVerID = MessageUtils.toApplVerID(beginString);
             }
-            final DataDictionary applicationDataDictionary = ddProvider == null ? null : ddProvider
-                    .getApplicationDataDictionary(applVerID);
-            payloadDictionary = MessageUtils.isAdminMessage(msgType)
-                    ? sessionDataDictionary
-                    : applicationDataDictionary;
+
+            if (isLogon) {
+                payloadDictionary = sessionDataDictionary;
+            } else {    // we got an app message
+                final DataDictionary applicationDataDictionary = ddProvider == null ? null : ddProvider
+                        .getApplicationDataDictionary(applVerID);
+                payloadDictionary = applicationDataDictionary;
+            }
         } else {
             applVerID = null;
             payloadDictionary = sessionDataDictionary;
@@ -70,7 +74,7 @@ public static Message parse(Session session, String messageString) throws Invali
         return message;
     }
 
-    private static ApplVerID getApplVerID(Session session, String messageString)
+    private static ApplVerID getApplVerID(Session session, String messageString, boolean isLogon)
             throws InvalidMessage {
         ApplVerID applVerID = null;
 
@@ -83,7 +87,7 @@ private static ApplVerID getApplVerID(Session session, String messageString)
             applVerID = session.getTargetDefaultApplicationVersionID();
         }
 
-        if (applVerID == null && MessageUtils.isLogon(messageString)) {
+        if (applVerID == null && isLogon) {
             final String defaultApplVerIdString = MessageUtils.getStringField(messageString,
                     DefaultApplVerID.FIELD);
             if (defaultApplVerIdString != null) {

diff --git a/quickfixj-core/src/main/java/quickfix/Session.java b/quickfixj-core/src/main/java/quickfix/Session.java
@@ -64,6 +64,7 @@
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
@@ -2162,6 +2163,19 @@ private void nextLogon(Message logon) throws FieldNotFound, RejectLogon, Incorre
             throw new RejectLogon("Logon attempt not within session time");
         }
 
+        if (sessionID.isFIXT()) {
+            final DataDictionary dictionary = dataDictionaryProvider
+                    .getSessionDataDictionary(sessionID.getBeginString());
+            if (dictionary != null) {
+                Optional<String> defaultApplVerID = logon.getOptionalString(DefaultApplVerID.FIELD);
+                if (defaultApplVerID.isPresent()) {
+                    if (!dictionary.isFieldValue(ApplVerID.FIELD, defaultApplVerID.get())) {
+                        throw new RejectLogon("Invalid DefaultApplVerID=" + defaultApplVerID.get());
+                    }
+                }
+            }
+        }
+
         // QFJ-926 - reset session before accepting Logon
         resetIfSessionNotCurrent(sessionID, SystemTime.currentTimeMillis());
 

diff --git a/quickfixj-core/src/test/java/quickfix/mina/acceptor/AcceptorIoHandlerTest.java b/quickfixj-core/src/test/java/quickfix/mina/acceptor/AcceptorIoHandlerTest.java
@@ -23,6 +23,7 @@
 import org.junit.Test;
 import quickfix.FixVersions;
 import quickfix.Message;
+import quickfix.MessageUtils;
 import quickfix.Responder;
 import quickfix.Session;
 import quickfix.SessionFactoryTestSupport;
@@ -33,6 +34,7 @@
 import quickfix.field.ApplVerID;
 import quickfix.field.DefaultApplVerID;
 import quickfix.field.EncryptMethod;
+import quickfix.field.Headline;
 import quickfix.field.HeartBtInt;
 import quickfix.field.MsgSeqNum;
 import quickfix.field.MsgType;
@@ -42,6 +44,7 @@
 import quickfix.field.Text;
 import quickfix.fix44.Logout;
 import quickfix.fixt11.Logon;
+import quickfix.fix50.News;
 import quickfix.mina.EventHandlingStrategy;
 import quickfix.mina.NetworkingOptions;
 import quickfix.mina.SessionConnector;
@@ -56,6 +59,7 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -101,6 +105,42 @@ public void testFIXTLogonAndApplVerID() throws Exception {
         }
     }
 
+    @Test
+    public void testFIXTLogonAndUnknownApplVerID() throws Exception {
+        EventHandlingStrategy mockEventHandlingStrategy = mock(EventHandlingStrategy.class);
+        IoSession mockIoSession = mock(IoSession.class);
+        SessionSettings settings = mock(SessionSettings.class);
+
+        final SessionID sessionID = new SessionID(FixVersions.BEGINSTRING_FIXT11, "SENDER",
+                "TARGET");
+        final UnitTestApplication unitTestApplication = new UnitTestApplication();
+        try (Session session = SessionFactoryTestSupport.createSession(sessionID, unitTestApplication, false, false, true, true, new DefaultApplVerID(DefaultApplVerID.FIX50SP2))) {
+            when(mockIoSession.getAttribute("QF_SESSION")).thenReturn(null);    // to create a new Session
+
+            final HashMap<SessionID, Session> acceptorSessions = new HashMap<>();
+            acceptorSessions.put(sessionID, session);
+            final StaticAcceptorSessionProvider sessionProvider = createSessionProvider(acceptorSessions);
+
+            final AcceptorIoHandler handler = new AcceptorIoHandler(sessionProvider,
+                    settings, new NetworkingOptions(new Properties()), mockEventHandlingStrategy);
+
+            final DefaultApplVerID defaultApplVerID = new DefaultApplVerID("33");
+            final Logon message = new Logon(new EncryptMethod(EncryptMethod.NONE_OTHER),
+                    new HeartBtInt(30), defaultApplVerID);
+            message.getHeader().setString(TargetCompID.FIELD, sessionID.getSenderCompID());
+            message.getHeader().setString(SenderCompID.FIELD, sessionID.getTargetCompID());
+            message.getHeader().setField(new SendingTime(LocalDateTime.now()));
+            message.getHeader().setInt(MsgSeqNum.FIELD, 1);
+
+            handler.messageReceived(mockIoSession, message.toString());
+            session.next(message);
+
+            Message lastToAdminMessage = unitTestApplication.lastToAdminMessage();
+            assertEquals(MsgType.LOGOUT, MessageUtils.getMessageType(lastToAdminMessage.toString()));
+            assertTrue(lastToAdminMessage.getString(Text.FIELD).contains("Invalid DefaultApplVerID=33"));
+        }
+    }
+
     @Test
     public void testMessageBeforeLogon() throws Exception {
         IoSession mockIoSession = mock(IoSession.class);

diff --git a/quickfixj-core/src/test/resources/logging.properties b/quickfixj-core/src/test/resources/logging.properties
@@ -1,5 +1,5 @@
 handlers=java.util.logging.ConsoleHandler
-java.util.logging.ConsoleHandler.level=ALL
+java.util.logging.ConsoleHandler.level=FINE
 java.util.logging.ConsoleHandler.formatter=java.util.logging.SimpleFormatter
 
-.level=ALL
+.level=FINE
diff --git a/...es/quickfix/test/acceptance/definitions/server/fixLatest/LogonUnknownDefaultApplVerID.def b/...es/quickfix/test/acceptance/definitions/server/fixLatest/LogonUnknownDefaultApplVerID.def
@@ -0,0 +1,7 @@
+# Send Logout on unknown DefaultApplVerID
+
+iCONNECT
+I8=FIXT.1.135=A34=149=TW52=<TIME>56=ISLD98=0108=301137=33
+# we expect Logout due to 1137=33
+E8=FIXT.1.19=4935=534=149=ISLD52=00000000-00:00:00.00056=TW58=Invalid DefaultApplVerID=3310=0
+eDISCONNECT
diff --git a/quickfixj-messages/quickfixj-messages-fixt11/src/main/resources/FIXT11.xml b/quickfixj-messages/quickfixj-messages-fixt11/src/main/resources/FIXT11.xml
@@ -353,6 +353,7 @@
       <value enum="7" description="FIX50"/>
       <value enum="8" description="FIX50SP1"/>
       <value enum="9" description="FIX50SP2"/>
+      <value enum="10" description="FIXLatest"/>
     </field>
     <field number="1129" name="CstmApplVerID" type="STRING"/>
     <field number="1130" name="RefApplVerID" type="STRING"/>