refactor: add more errors and reset authentication state on authentication error

felixauringer · felixauringer · commit 2387ebef8352 · 2025-10-26T13:41:11.000+01:00
While trying to get the MPT tests for managesieve running, I realized that a client
is trapped in the state "AuthenticationInProgress" when using continuation. This
made longer test scripts impossible.
To consistently reset the session, I introduced more errors that are handled in one
place instead of returning a "NO" answer directly during authentication.

Those changes also required some small changes to the MPT and the normal tests.

Additionally, there were a parsing bug when sending space-separated username and
password in the authentication continuation which is fixed now.
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/authenticate.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/authenticate.test
@@ -18,20 +18,25 @@
 ################################################################
 
 C: AUTHENTICATE
-S: NO ManageSieve syntax is incorrect : You must specify a SASL mechanism as an argument of AUTHENTICATE command
+S: NO "ManageSieve syntax is incorrect: quoted SASL mechanism must be supplied"
 
 C: AUTHENTICATE "UNKNOWN"
-S: NO Unknown SASL mechanism UNKNOWN
+S: NO "Unknown SASL mechanism UNKNOWN"
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
+S: ""
+S: OK
 C: GETSCRIPT toto.sieve
-S: NO ManageSieve syntax is incorrect : You must supply a password for the authentication mechanism. Formal syntax : <NULL>username<NULL>password
+S: NO "ManageSieve syntax is incorrect: quoted authentication data must be supplied"
 
-C:  tin password
-S: NO authentication failed
+C: AUTHENTICATE "PLAIN"
+S: ""
+S: OK
+C: "tin password"
+S: NO "Authentication failed with: Verification of credentials failed"
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
-S: OK
+S: ""
+S: OK
+C: "user password"
+S: OK
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/authenticateBase64.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/authenticateBase64.test
@@ -18,4 +18,4 @@
 ################################################################
 
 C: AUTHENTICATE "PLAIN" "AHVzZXIAcGFzc3dvcmQ="
-S: OK
+S: OK
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/capability.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/capability.test
@@ -38,8 +38,9 @@ S: "VERSION" "1.0"
 S: OK
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: CAPABILITY
@@ -51,4 +52,4 @@ S: "STARTTLS"
 S: "IMPLEMENTATION" "Apache ManageSieve v1.0"
 S: "VERSION" "1.0"
 }
-S: OK
+S: OK
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/checkscript.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/checkscript.test
@@ -34,8 +34,9 @@ C:
 S: NO
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: CHECKSCRIPT {99+}
@@ -51,4 +52,3 @@ C: #comment
 C: InvalidSieveCommand
 C:
 S: NO "Syntax Error: org.apache.jsieve.parser.generated.ParseException: Encountered "<EOF>" at line 2, column 21.
-
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/deletescript.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/deletescript.test
@@ -24,8 +24,9 @@ C: DELETESCRIPT "foo"
 S: NO
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: DELETESCRIPT "foo"
@@ -77,4 +78,4 @@ C: SETACTIVE "mysievescript"
 S: OK
 
 C: DELETESCRIPT "mysievescript"
-S: NO \(ACTIVE\) "You may not delete an active script"
+S: NO \(ACTIVE\) "You may not delete an active script"
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/getscript.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/getscript.test
@@ -24,8 +24,9 @@ C: GETSCRIPT "foo"
 S: NO
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: GETSCRIPT "foo"
@@ -48,5 +49,3 @@ S:   fileinto "INBOX.sent";
 S: \}
 S:
 S: OK
-
-
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/havespace.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/havespace.test
@@ -27,12 +27,13 @@ C: HAVESPACE "scriptname" 49
 S: NO
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: HAVESPACE "scriptname" 49
 S: OK
 
 C: HAVESPACE "scriptname" 51
-S: NO \(QUOTA/MAXSIZE\) "Quota exceeded"
+S: NO \(QUOTA/MAXSIZE\) "Quota exceeded"
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/listscripts.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/listscripts.test
@@ -21,8 +21,9 @@ C: LISTSCRIPTS
 S: NO
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: LISTSCRIPTS
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/logout.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/logout.test
@@ -18,4 +18,4 @@
 ################################################################
 
 C: LOGOUT
-S: OK channel is closing
+S: OK channel is closing
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/noop.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/noop.test
@@ -25,4 +25,4 @@ S: OK \(TAG \{16\}
 S: STARTTLS-SYNC-42\) "DONE"
 
 C: NooP
-S: OK "NOOP completed"
+S: OK "NOOP completed"
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/putscript.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/putscript.test
@@ -39,8 +39,9 @@ C:
 S: NO
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: PUTSCRIPT "mysievescript" {97+}
@@ -56,4 +57,3 @@ C: #comment
 C: InvalidSieveCommand
 C:
 S: NO "Syntax Error: org.apache.jsieve.parser.generated.ParseException: Encountered "<EOF>" at line 2, column 21.
-
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/renamescript.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/renamescript.test
@@ -27,8 +27,9 @@ C: RENAMESCRIPT "foo" "bar"
 S: NO
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: PUTSCRIPT "mysievescript" {99+}
@@ -75,5 +76,3 @@ S: OK
 
 C: RENAMESCRIPT "mysievescript" "mysievescriptbis"
 S: NO \(ALREADYEXISTS\) "A script with that name already exists"
-
-
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/setactive.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/setactive.test
@@ -24,8 +24,9 @@ C: SETACTIVE "foo"
 S: NO
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: SETACTIVE "foo"
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/starttls.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/starttls.test
@@ -24,11 +24,12 @@ C: STARTTLS
 S: NO You can't enable two time SSL encryption
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: STARTTLS
 S: NO command STARTTLS is issued in the wrong state. It must be issued as you are unauthenticated
 
-C: LOGOUT
+C: LOGOUT
diff --git a/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/unauthenticate.test b/mpt/impl/managesieve/core/src/main/resources/org/apache/james/managesieve/scripts/unauthenticate.test
@@ -27,8 +27,9 @@ C: UNAUTHENTICATE
 S: NO UNAUTHENTICATE command must be issued in authenticated state
 
 C: AUTHENTICATE "PLAIN"
-S: \+ ""
-C:  user password
+S: ""
+S: OK
+C: "user password"
 S: OK
 
 C: GETSCRIPT any
@@ -38,4 +39,4 @@ C: UNAUTHENTICATE
 S: OK
 
 C: GETSCRIPT any
-S: NO
+S: NO
diff --git a/protocols/managesieve/src/main/java/org/apache/james/managesieve/core/CoreProcessor.java b/protocols/managesieve/src/main/java/org/apache/james/managesieve/core/CoreProcessor.java
@@ -210,25 +210,26 @@ public String noop(String tag) {
 
     @Override
     public String chooseMechanism(Session session, String mechanism) {
-        if (session.isAuthenticated()) {
-            return "NO \"already authenticated\"";
-        }
         try {
             if (Strings.isNullOrEmpty(mechanism)) {
-                return "NO ManageSieve syntax is incorrect : You must specify a SASL mechanism as an argument of AUTHENTICATE command";
+                throw new SyntaxException("quoted SASL mechanism must be supplied");
             }
-            String unquotedMechanism = ParserUtils.unquoteFirst(mechanism);
-            SupportedMechanism supportedMechanism = SupportedMechanism.retrieveMechanism(unquotedMechanism);
+
+            SupportedMechanism supportedMechanism = SupportedMechanism.retrieveMechanism(mechanism);
             if (!this.authenticationProcessorMap.containsKey(supportedMechanism)) {
-                throw new UnknownSaslMechanism("SASL mechanism disabled: " + unquotedMechanism);
+                throw new UnknownSaslMechanism("SASL mechanism disabled: " + mechanism);
             }
 
             session.setChoosedAuthenticationMechanism(supportedMechanism);
             session.setState(Session.State.AUTHENTICATION_IN_PROGRESS);
             AuthenticationProcessor authenticationProcessor = authenticationProcessorMap.get(supportedMechanism);
             return authenticationProcessor.initialServerResponse(session);
-        } catch (UnknownSaslMechanism unknownSaslMechanism) {
-            return "NO " + unknownSaslMechanism.getMessage();
+        } catch (UnknownSaslMechanism e) {
+            resetSession(session);
+            return "NO \"" + e.getMessage() + "\"";
+        } catch (SyntaxException e) {
+            resetSession(session);
+            return "NO \"ManageSieve syntax is incorrect: " + e.getMessage() + "\"";
         }
     }
 
@@ -237,41 +238,46 @@ public String authenticate(Session session, String suppliedData) {
         try {
             SupportedMechanism currentAuthenticationMechanism = session.getChoosedAuthenticationMechanism();
             AuthenticationProcessor authenticationProcessor = authenticationProcessorMap.get(currentAuthenticationMechanism);
-            String unquotedSuppliedData = ParserUtils.unquoteFirst(suppliedData);
-            if (unquotedSuppliedData == null) {
-                return "NO \"authentication failed\"";
+            if (Strings.isNullOrEmpty(suppliedData)) {
+                throw new SyntaxException("quoted authentication data must be supplied");
             }
-            if (unquotedSuppliedData.equals("*")) {
-                return "NO \"authentication aborted\"";
+            if (suppliedData.equals("*")) {
+                throw new AuthenticationException("authentication aborted by client");
             }
-            Username authenticatedUsername = authenticationProcessor.isAuthenticationSuccesfull(session, unquotedSuppliedData);
+            Username authenticatedUsername = authenticationProcessor.isAuthenticationSuccesfull(session, suppliedData);
             if (authenticatedUsername != null) {
                 session.setUser(authenticatedUsername);
                 session.setState(Session.State.AUTHENTICATED);
                 return "OK";
             } else {
-                session.setState(Session.State.UNAUTHENTICATED);
-                session.setUser(null);
-                return "NO authentication failed";
+                resetSession(session);
+                return "NO \"authentication failed\"";
             }
         } catch (AuthenticationException e) {
-            return "NO Authentication failed with: " + e.getMessage();
+            resetSession(session);
+            return "NO \"Authentication failed with: " + e.getMessage() + "\"";
         } catch (SyntaxException e) {
-            return "NO ManageSieve syntax is incorrect : " + e.getMessage();
+            resetSession(session);
+            return "NO \"ManageSieve syntax is incorrect: " + e.getMessage() + "\"";
         }
     }
 
     @Override
     public String unauthenticate(Session session) {
         if (session.isAuthenticated()) {
-            session.setState(Session.State.UNAUTHENTICATED);
-            session.setUser(null);
+            resetSession(session);
             return "OK";
         } else {
             return "NO UNAUTHENTICATE command must be issued in authenticated state";
         }
     }
 
+    private static void resetSession(Session session) {
+        session.setState(Session.State.UNAUTHENTICATED);
+        session.setUser(null);
+        session.setChoosedAuthenticationMechanism(null);
+    }
+
     @Override
     public void logout() throws SessionTerminatedException {
         throw new SessionTerminatedException();
diff --git a/protocols/managesieve/src/main/java/org/apache/james/managesieve/core/PlainAuthenticationProcessor.java b/protocols/managesieve/src/main/java/org/apache/james/managesieve/core/PlainAuthenticationProcessor.java
@@ -72,11 +72,11 @@ public Username isAuthenticationSuccesfull(Session session, String suppliedClien
     private Username authenticateWithSeparator(Session session, String suppliedClientData, char c) throws SyntaxException, AuthenticationException {
         Iterator<String> it = Splitter.on(c).omitEmptyStrings().split(suppliedClientData).iterator();
         if (!it.hasNext()) {
-            throw new SyntaxException("You must supply a username for the authentication mechanism. Formal syntax : <NULL>username<NULL>password");
+            throw new SyntaxException("You must supply a username for the authentication mechanism. Formal syntax: <NULL>username<NULL>password");
         }
         Username userName = Username.of(it.next());
         if (!it.hasNext()) {
-            throw new SyntaxException("You must supply a password for the authentication mechanism. Formal syntax : <NULL>username<NULL>password");
+            throw new SyntaxException("You must supply a password for the authentication mechanism. Formal syntax: <NULL>username<NULL>password");
         }
         String password = it.next();
         session.setUser(userName);
@@ -85,7 +85,7 @@ private Username authenticateWithSeparator(Session session, String suppliedClien
             if (user != null && user.verifyPassword(password)) {
                 return user.getUserName();
             } else {
-                return null;
+                throw new AuthenticationException("Verification of credentials failed");
             }
         } catch (UsersRepositoryException e) {
             throw new AuthenticationException(e.getMessage());
diff --git a/protocols/managesieve/src/main/java/org/apache/james/managesieve/transcode/ManageSieveProcessor.java b/protocols/managesieve/src/main/java/org/apache/james/managesieve/transcode/ManageSieveProcessor.java
diff --git a/protocols/managesieve/src/main/java/org/apache/james/managesieve/util/ParserUtils.java b/protocols/managesieve/src/main/java/org/apache/james/managesieve/util/ParserUtils.java
diff --git a/server/protocols/protocols-managesieve/src/test/java/org/apache/james/managesieveserver/AuthenticateTest.java b/server/protocols/protocols-managesieve/src/test/java/org/apache/james/managesieveserver/AuthenticateTest.java
diff --git a/server/protocols/protocols-managesieve/src/test/java/org/apache/james/managesieveserver/OIDCTest.java b/server/protocols/protocols-managesieve/src/test/java/org/apache/james/managesieveserver/OIDCTest.java
